Getting batched events #205

mhoeger · 2021-03-02T18:21:58Z

Resolves #127
Resolves #115

SDK Changes

For inputs, try to set "dataType" if we know the requested payload is string or byte[]
For trigger attributes that implement IBatchedOutput, convert IsBatched property to add "cardinality": "many" or "cardinality": "one".
- Fail if we detect "cardinality": "many" but the requested payload DOES NOT implement IEnumerable
- Set "dataType" if we know that the requested payload inside of the collection is string or byte[]
Note that not all IEnumerables will be converted properly by .NET worker side (example: "Queue"). SDK shouldn't need to be changed though for future converter additions.

.NET Worker Changes

Enable capability to receive TypedData of:
- collection bytes
- collection string
- collection double
- collection long
New converter which converts RepeatedCollection<string>, RepeatedCollection<byte[]>, RepeatedCollection<ReadOnlyMemory<byte>>, RepeatedCollection<double>, and RepeatedCollection<long> (from above capability) to corresponding Array types.
- RepeatedCollection implements IEnumerable, IList, and ICollection

E2E App Changes

Added some EventHubs functions I was using to test - not hooked up to actual tests yet since we can't emulate and the PR is already big...

fabiocav · 2021-03-03T18:53:36Z

Was this replaced by the other PR? Should it be closed?

extensions/Worker.Extensions.EventHubs/EventHubTriggerAttribute.cs

sdk/Sdk/FunctionMetadataGenerator.cs

fabiocav

This looks great! Awesome set of tests.

Leaving an initial set of comments, but some things may require a chat

extensions/Worker.Extensions.EventHubs/EventHubTriggerAttribute.cs

extensions/Worker.Extensions.Abstractions/IBatchedInput.cs

extensions/Worker.Extensions.EventHubs/EventHubTriggerAttribute.cs

sdk/Sdk/DataTypeEnum.cs

sdk/Sdk/FunctionMetadataGenerator.cs

src/DotNetWorker/Context/Features/GrpcFunctionBindingsFeature.cs

src/DotNetWorker/Converters/EnumerableConverter.cs

fabiocav · 2021-03-04T07:40:01Z

src/DotNetWorker/Converters/EnumerableConverter.cs

+namespace Microsoft.Azure.Functions.Worker.Converters
+{
+    // Converting IEnumerable<> to Array
+    internal class EnumerableConverter : IConverter


Adding some suggestions to the class, but given the fact this is pretty specific to gRPC, I'm wondering if this shouldn't be a converted injected by the gRPC integration (mostly thinking about when we break it off) and operating on the gRPC types, which would make this a bit easier and rely less on type checking. We can chat, but wanted to share this to get your thoughts.

sdk/Sdk/FunctionMetadataGenerator.cs

extensions/Worker.Extensions.EventHubs/EventHubTriggerAttribute.cs

src/DotNetWorker/Converters/EnumerableConverter.cs

mhoeger · 2021-03-04T22:34:06Z

src/DotNetWorker/Converters/EnumerableConverter.cs

+            EnumerableTargetType? targetType = null;
+            target = null;
+            // Array
+            if (context.Parameter.Type.IsArray)


Use ElementType or GetElementType API

Add cache on function id and parameter (or add a TODO)

sdk/Sdk/FunctionMetadataGenerator.cs

…s collections event hubs e2e initial updates temp add to solution too update to new model rebase to latest Improved logic for resolving generics remove webjobs reference Fix IEnumerable some enumerable converter tests Clean up test Cleanup reverting local settings json change change localsettings typo added datatype string to some tests fix generator test

…le future

mhoeger · 2021-03-05T02:30:17Z

extensions/Worker.Extensions.Abstractions/DefaultValueAttribute.cs

+namespace Microsoft.Azure.Functions.Worker
+{
+    [AttributeUsage(AttributeTargets.Property)]
+    public class DefaultValueAttribute : Attribute


This is temporary to set default while we do not load attributes.

nit:No need to change for right now, but I wonder if we should move this to a different namespace so it's not discoverable and polluting?

mhoeger · 2021-03-05T02:31:56Z

sdk/Sdk/CustomAttributeExtensions.cs

            LoadConstructorArguments(properties, attribute);
            LoadDefinedProperties(properties, attribute);

            return properties;
        }

-        private static void LoadConstructorArguments(IDictionary<string, object> properties, CustomAttribute attribute)
+        private static IEnumerable<(string, CustomAttributeArgument?)> GetDefaultValues(this CustomAttribute attribute)


this is all should be removed later... after above todo is done: avoid needing to instantiate any types, assume that the constructor argument names are equal to property names.

mhoeger · 2021-03-05T02:32:51Z

sdk/Sdk/FunctionMetadataGenerator.cs

+                    // Note that this logic doesn't dictate what we can/can't do, and
+                    // we can be more restrictive in the future because today some 
+                    // scenarios result in runtime failures.
+                    if (IsIterableCollection(parameterType, out DataType dataType))


Note that this should be changed, but likely for a future iteration it will be a runtime failure anyways, so no worries about introducing breaking behavior (it'll just fail faster)

Actually - something that is derived from IEnumerable<Poco> (ex: List<Poco>) is valid for cardinality: many and does have an appropriate converter. Would not want to throw in these cases.

The only failure cases are when element type is string, byte[], ReadOnlyMemory<byte> int, double, long AND collection type is not equal to IEnumerable, IList, or ICollection (or corresponding generic).

We could go one step further and say if not derived from IEnumerable assume it's not batched... in this case we want to know if it was set explicitly or if it's just from the user... for event hubs we definitely want to discourage non-batched anyways

mhoeger · 2021-03-05T02:34:17Z

src/DotNetWorker/Converters/ArrayConverter.cs

+                        || elementType.IsAssignableFrom(typeof(byte[]))
+                        || elementType.IsAssignableFrom(typeof(ReadOnlyMemory<byte>))
+                        || elementType.IsAssignableFrom(typeof(long))
+                        || elementType.IsAssignableFrom(typeof(double)))


Note: should improve efficiency here... if anyone has quick suggestions they would be very welcome but otherwise i think i'll flag this as a future improvement

fabiocav

Added some comments, but nothing major. We should be able to get this in as I don't see major blockers, but some of the comments would be good to address.

fabiocav · 2021-03-05T05:08:58Z

sdk/Sdk/FunctionMetadataGenerator.cs

+            }
+
+            // IEnumerable and not string or dictionary
+            bool isEnumerableOfT = IsOrDerivedFrom(type, Constants.IEnumerableOfT);


nit: Since this may not be used until you reach the last check, could we defer this check?

is there a fancy .net thing i can do to assign while being evaluated (like if (bool t = ShouldContinue())? I use isEnumerableOfT a bit earlier in line 473 too

src/DotNetWorker/Converters/ArrayConverter.cs

test/DotNetWorkerTests/Converters/ArrayConverterTests.cs

ankitkumarr · 2021-03-05T02:41:35Z

extensions/Worker.Extensions.Abstractions/DefaultValueAttribute.cs

+namespace Microsoft.Azure.Functions.Worker
+{
+    [AttributeUsage(AttributeTargets.Property)]
+    public class DefaultValueAttribute : Attribute


nit:No need to change for right now, but I wonder if we should move this to a different namespace so it's not discoverable and polluting?

sdk/Sdk/CustomAttributeExtensions.cs

…en't passed over by host yet apparently

…e) less discoverable

mhoeger · 2021-03-05T07:10:35Z

Looks like long[] and double[] aren't used by host - updated release notes to reflect this and filed issue: Azure/azure-functions-host#7209

ankitkumarr

Looks great! All these tests make it so much easier. Thank you!!

I added very minor final nit comments, that don't need to be addressed in this PR. The only thing I will leave it to you is to change the namespace Microsoft.Azure.Functions.Worker.Annotations

ankitkumarr · 2021-03-05T06:59:36Z

sdk/Sdk/FunctionMetadataGenerator.cs

+            return IsDerivedFrom(baseType, interfaceFullName);
+        }
+
+        private static string? ResolveIEnumerableOfTType(TypeReference type, Dictionary<string, string> foundMapping)


For later -- If I am reading it right, this finds IEnumerable<T> through recursion, while IsOrDerivedFrom(..., Constants.IEnumerableOfT checks if there's any. They both seem to go through all interfaces directly or indirectly implemented through baseType. Is there a reason to do both? Could you combine the two to be like TryGetIEnumerableOfTType? That way we don't have to recurse the same path twice?

I understand that you may still want IsOrDerivedFrom to be around for other IEnumerable types.

Definitely not required right now. Was just curious and wanted to make sure I am understanding it right.

Definitely a good call!!

We should flag some of the logic using mono cecil for review. I noticed a few other things we may want to reevaluate, but these are much simpler to walk through with the tests.

ankitkumarr · 2021-03-05T07:10:55Z

sdk/Sdk/FunctionMetadataGenerator.cs

+            TypeDefinition definition = type.Resolve();
+            if (definition.HasGenericParameters && type is GenericInstanceType genericType)
+            {
+                for (int i = 0; i < genericType.GenericArguments.Count(); i++)


For later -- Would it ever make sense to just look at the first argument, as we know IEnumerable<T> would always only have at most one generic arg?

Also this can be made foreach. As apparently, ElementAt() could go over the whole ienumerable every lookup depending on the implementation it says. Not that it matters much here :)

^This was done because you might have some Foo<K, V> : IEnumerable<V>

So in this case the recursive search would want to match V from Foo (the second generic) with V from IEnumerable (the first and only generic)

Let's follow up on this case.

extensions/Worker.Extensions.Abstractions/DefaultValueAttribute.cs

sdk/Sdk/CustomAttributeExtensions.cs

fabiocav · 2021-03-05T09:54:13Z

src/DotNetWorker/Converters/ArrayConverter.cs

+
+        private static object? GetBinaryData(IEnumerable<ReadOnlyMemory<byte>> source, Type targetType)
+        {
+            if (targetType.IsAssignableFrom(typeof(ReadOnlyMemory<byte>)))


Had left an earlier comment about the comparison using IsAssignableFrom, but can't find it. This check here isn't required, are any of the tests specifically testing this code path? Aside from ReadOnlyMemory<byte> this shouldn't evaluate to true, in practice.

mhoeger requested review from fabiocav, ankitkumarr and brettsam March 2, 2021 18:32

mhoeger force-pushed the mhoeger/cardinalityManyChange branch from 143254f to b1e7aa4 Compare March 2, 2021 20:11

mhoeger mentioned this pull request Mar 3, 2021

Cardinality many from array #209

Closed

mhoeger force-pushed the mhoeger/cardinalityManyChange branch from 7900f7d to 5c6d9d7 Compare March 4, 2021 04:13

mhoeger commented Mar 4, 2021

View reviewed changes

extensions/Worker.Extensions.EventHubs/EventHubTriggerAttribute.cs Outdated Show resolved Hide resolved

mhoeger commented Mar 4, 2021

View reviewed changes

sdk/Sdk/FunctionMetadataGenerator.cs Outdated Show resolved Hide resolved

mhoeger commented Mar 4, 2021

View reviewed changes

sdk/Sdk/FunctionMetadataGenerator.cs Outdated Show resolved Hide resolved

mhoeger changed the title ~~"Cardinality": "Many" and getting collections~~ Getting batched events Mar 4, 2021

fabiocav reviewed Mar 4, 2021

View reviewed changes

ankitkumarr reviewed Mar 4, 2021

View reviewed changes

sdk/Sdk/FunctionMetadataGenerator.cs Outdated Show resolved Hide resolved

sdk/Sdk/FunctionMetadataGenerator.cs Outdated Show resolved Hide resolved

sdk/Sdk/FunctionMetadataGenerator.cs Outdated Show resolved Hide resolved

mhoeger commented Mar 4, 2021

View reviewed changes

extensions/Worker.Extensions.EventHubs/EventHubTriggerAttribute.cs Outdated Show resolved Hide resolved

mhoeger commented Mar 4, 2021

View reviewed changes

src/DotNetWorker/Converters/EnumerableConverter.cs Outdated Show resolved Hide resolved

mhoeger commented Mar 4, 2021

View reviewed changes

src/DotNetWorker/Converters/EnumerableConverter.cs Outdated Show resolved Hide resolved

mhoeger commented Mar 4, 2021

View reviewed changes

sdk/Sdk/FunctionMetadataGenerator.cs Outdated Show resolved Hide resolved

mhoeger added 12 commits March 4, 2021 18:22

update sdk e2e test for datatype

b88044b

move cardinality and datatype stuff to later

992201a

resolve typeof's

2d527aa

cardinality through IBatchedInput - tests need changes

dbfd447

re-add removed stuff

c51705d

fix spacing

e97c883

add comments

c178301

Update tests and make SDK generation exceptions surface as build error

e9d95d2

fixing low hanging cr changes

6815331

minor updates from cr

c5f1185

change where we get default value

b54d254

mhoeger added 4 commits March 4, 2021 18:22

casing

8ce8fce

fix docs

182e9a4

Modify batched output to be ISupportCardinality - set up for extensib…

8876297

…le future

array converter only

8d0d9a1

mhoeger force-pushed the mhoeger/cardinalityManyChange branch from 2f627df to 8d0d9a1 Compare March 5, 2021 02:25

mhoeger commented Mar 5, 2021

View reviewed changes

mhoeger added 2 commits March 4, 2021 18:53

test fixes

4f59720

release notes

d0db91d

fabiocav reviewed Mar 5, 2021

View reviewed changes

update array converter logic

b734b7f

ankitkumarr reviewed Mar 5, 2021

View reviewed changes

mhoeger added 2 commits March 4, 2021 23:02

remove nested type search and update readme because double and int ar…

775d70e

…en't passed over by host yet apparently

address comment to make default value attribute (a temporary attribut…

8fb62e2

…e) less discoverable

nit: add example to readme

37de7a8

ankitkumarr approved these changes Mar 5, 2021

View reviewed changes

mhoeger added 2 commits March 4, 2021 23:28

update namespace

ea58fa3

namespace reference

13594cd

mhoeger merged commit 16856c4 into main Mar 5, 2021

mhoeger deleted the mhoeger/cardinalityManyChange branch March 5, 2021 07:55

fabiocav reviewed Mar 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Getting batched events #205

Getting batched events #205

mhoeger commented Mar 2, 2021 •

edited

Loading

fabiocav commented Mar 3, 2021

fabiocav left a comment

fabiocav Mar 4, 2021

mhoeger Mar 4, 2021

mhoeger Mar 4, 2021

mhoeger Mar 5, 2021

ankitkumarr Mar 5, 2021

mhoeger Mar 5, 2021

mhoeger Mar 5, 2021

mhoeger Mar 5, 2021

mhoeger Mar 5, 2021

fabiocav left a comment

fabiocav Mar 5, 2021

mhoeger Mar 5, 2021 •

edited

Loading

ankitkumarr Mar 5, 2021

mhoeger commented Mar 5, 2021

ankitkumarr left a comment

ankitkumarr Mar 5, 2021

mhoeger Mar 5, 2021

fabiocav Mar 5, 2021

ankitkumarr Mar 5, 2021

mhoeger Mar 5, 2021 •

edited

Loading

fabiocav Mar 5, 2021

fabiocav Mar 5, 2021

Getting batched events #205

Getting batched events #205

Conversation

mhoeger commented Mar 2, 2021 • edited Loading

SDK Changes

.NET Worker Changes

E2E App Changes

fabiocav commented Mar 3, 2021

fabiocav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiocav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhoeger Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhoeger commented Mar 5, 2021

ankitkumarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhoeger Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhoeger commented Mar 2, 2021 •

edited

Loading

mhoeger Mar 5, 2021 •

edited

Loading

mhoeger Mar 5, 2021 •

edited

Loading