Refactor binding pipeline to allow type info to flow through #1002

mathewc · 2016-11-29T01:39:31Z

This builds on the work/investigations that @mamaso did in #871 to address #814.

I wanted to make this a deeper fix across bindings, allowing ExpandoObjects to flow into the binding layer (preserving type info).

I'm going to add a bunch of new unit tests for some of the new logic - working on those now.

mathewc · 2016-11-29T01:41:50Z

src/WebJobs.Script/Binding/FunctionBinding.cs

+        {
+            // TODO: we should optimize this to remove the double
+            // serialization/deserialization
+            string json = ToJson(value);


Not sure of an easier out of the box way to recursively convert from an ExpandoObject to a JObject. No worse that what we had before, but we have room for improvement.

This seems simple enough. For some reason, though, I just noticed we have a custom converter for ExpandoObject do we have a good reason for that? There's an out of the box converter with Json.NET : http://www.newtonsoft.com/json/help/html/t_newtonsoft_json_converters_expandoobjectconverter.htm

I wasn't aware of that converter. I'll remove our custom converter :)

mathewc · 2016-11-29T01:42:51Z

src/WebJobs.Script/Binding/HttpBinding.cs

+            return Task.CompletedTask;
+        }
+
+        internal static HttpResponseMessage CreateResponse(HttpRequestMessage request, object content)


Making this logic unit testable - I'm going to add a bunch of tests

mathewc · 2016-11-29T01:44:26Z

test/WebJobs.Script.Tests/NodeEndToEndTests.cs

@@ -1035,6 +1035,36 @@ public void ExcludedFunction_NotAddedToHost()
            Assert.True(false, "There was no log found for duplicate calls to done");
        }

+        [Fact]
+        public async Task HttpTrigger_Scenarios_Buffer()


Stole this test verbatim (and supporting function) from @mamaso's PR :)

fabiocav

A couple of small comments.

fabiocav · 2016-11-29T01:47:08Z

src/WebJobs.Script/Binding/FunctionBinding.cs

+        {
+            // TODO: we should optimize this to remove the double
+            // serialization/deserialization
+            string json = ToJson(value);


This seems simple enough. For some reason, though, I just noticed we have a custom converter for ExpandoObject do we have a good reason for that? There's an out of the box converter with Json.NET : http://www.newtonsoft.com/json/help/html/t_newtonsoft_json_converters_expandoobjectconverter.htm

fabiocav · 2016-11-29T02:06:40Z

src/WebJobs.Script/Binding/HttpBinding.cs

-                            statusCode = (HttpStatusCode)(int)value;
-                        }
+                    if ((responseObject.TryGetValue("status", out value) && value is int) ||
+                        (responseObject.TryGetValue("statusCode", out value) && value is int))


This logic could be simplified to:
(responseObject.TryGetValue("status", out value) || responseObject.TryGetValue("statusCode", out value)) && value is int

fabiocav · 2016-11-29T02:08:20Z

src/WebJobs.Script/Binding/HttpBinding.cs

-                        {
-                            isRawResponse = (bool)value;
-                        }
+                    if (responseObject.TryGetValue("isRaw", out value) && value is bool)


It's tempting to add a generic extension method to avoid those && value is <type> checks....

fabiocav · 2016-11-29T02:09:43Z

src/WebJobs.Script/Binding/HttpBinding.cs

@@ -248,7 +265,7 @@ private static void AddResponseHeader(HttpResponseMessage response, KeyValuePair
                        }
                        break;
                    case "content-md5":
-                        response.Content.Headers.ContentMD5 = header.Value.Value<byte[]>();
+                        response.Content.Headers.ContentMD5 = header.Value as byte[];


A string here would be valid as well. (I believe I had a similar comment on @mamaso's PR)

Good catch - missed that in the other review. Fixed.

fabiocav · 2016-11-29T02:10:46Z

src/WebJobs.Script/Description/Node/NodeFunctionInvoker.cs

@@ -240,23 +236,6 @@ private static void EnsureInitialized()
            }
        }

-        /// <summary>


🎉 👍 😄

fabiocav · 2016-11-29T02:12:28Z

src/WebJobs.Script/Extensions/IDictionaryExtensions.cs

+        public static IDictionary<string, TValue> ToCaseInsensitive<TValue>(this IDictionary<string, TValue> dictionary)
+        {
+            return new Dictionary<string, TValue>(dictionary, StringComparer.OrdinalIgnoreCase);
+        }


Oh, just think about how nice it would be to have a TryGetValue here... 😄

Done. Also added missing test suites + tests for new and some existing code

mamaso

awesome stuff!

mamaso · 2016-11-29T04:24:30Z

src/WebJobs.Script/Extensions/IDictionaryExtensions.cs

+            return new Dictionary<string, TValue>(dictionary, StringComparer.OrdinalIgnoreCase);
+        }
+
+        public static bool TryGetValue<TValue>(this IDictionary<string, object> obj, string name, out TValue value)


I like it, type safe TryGetValue

mamaso · 2016-11-29T04:26:13Z

src/WebJobs.Script/Extensions/IDictionaryExtensions.cs

+
+        public static IDictionary<string, TValue> AsCaseInsensitive<TValue>(this IDictionary<string, TValue> dictionary)
+        {
+            return new Dictionary<string, TValue>(dictionary, StringComparer.OrdinalIgnoreCase);


Does this throw if there are dupe keys in the input dictionary? ie 'key' and 'KEY'. Not sure if we'd prefer to throw or just overwrite one of the props in that case

Hmmm, not sure. I'll add a test and figure out the right behavior.

Good point indeed. This will surely throw an exception complaining about duplicate keys being added. I think throwing here is the right behavior, we just need to make sure this is properly handled and we provide a meaningful, detailed message from the calling code.

Removed the whole case insensitive thing. Modified TryGetValue helper to support case insensitive queries, which matches the logic we had before with JObject

mamaso · 2016-11-29T04:28:41Z

src/WebJobs.Script/Binding/HttpBinding.cs

-                            statusCode = (HttpStatusCode)(int)value;
-                        }
+                    int statusValue;
+                    if (responseObject.TryGetValue<int>("status", out statusValue) || 


Pretty sure we need to support string for this as well :/ I remember some tests breaking when I treated status as an integer.

Seems like that cast is supported

All tests are passing with these changes. What is the repro you're thinking of?

Just tried out this in prod and it gives a 400, think it will fail with this new logic.

context.res = { body: true, status: "400" }; context.done();

I've added support for both integers and strings

mamaso · 2016-11-29T04:52:42Z

src/WebJobs.Script/Binding/HttpBinding.cs

-                            isRawResponse = (bool)value;
-                        }
+                    bool isRawValue;
+                    if (responseObject.TryGetValue<bool>("isRaw", out isRawValue))


potentially same cast issue

The previous code had a check that verified the JToken was of type Boolean, so I don't think strings were supported. However, I don't think we want to support strings for this. I could put in logic to support string values, but I don't think it's worth it.

Yeah, since isRaw is so new and mainly internal I think we're good on this one

mamaso

Just the one comment, tests look good too

mamaso · 2016-11-29T21:39:33Z

src/WebJobs.Script/Extensions/IDictionaryExtensions.cs

+
+            if (!found && ignoreCase)
+            {
+                // try a case insensitive match - first match wins


As a small optimization we could consider special casing for first letter, i.e. TryGetValue 'body' and 'Body', I think that would catch 99% of these with no linear scan

I agree that would catch most cases, but I don't think this is a big perf issue really. I'd rather not get in the business of checking a reduced set of permutations (e.g. Key, key, KEY, Key)

totally fair :)

mathewc · 2016-11-29T22:00:52Z

Merged. I'll address any additional feedback people have.

mathewc · 2016-11-29T22:34:25Z

Merged in commit 6b4c50b

dnfclas added the cla-already-signed label Nov 29, 2016

ahmelsayed added the in progress label Nov 29, 2016

mathewc commented Nov 29, 2016

View reviewed changes

mathewc assigned fabiocav and mamaso Nov 29, 2016

fabiocav reviewed Nov 29, 2016

View reviewed changes

mathewc force-pushed the buffer-fix branch from 4390a19 to c7287e1 Compare November 29, 2016 03:45

mathewc changed the title ~~Refactor binding pipeline to allow Node type info to flow through~~ Refactor binding pipeline to allow type info to flow through Nov 29, 2016

mathewc force-pushed the buffer-fix branch from c7287e1 to b653d31 Compare November 29, 2016 03:56

mamaso reviewed Nov 29, 2016

View reviewed changes

mathewc force-pushed the buffer-fix branch from b653d31 to 98caa6f Compare November 29, 2016 21:28

mamaso approved these changes Nov 29, 2016

View reviewed changes

mathewc closed this Nov 29, 2016

mathewc force-pushed the buffer-fix branch from 98caa6f to 6b4c50b Compare November 29, 2016 22:00

ahmelsayed removed the in progress label Nov 29, 2016

mathewc mentioned this pull request Nov 29, 2016

Added buffer support to Node #871

Closed

mathewc deleted the buffer-fix branch December 14, 2016 19:15

Refactor binding pipeline to allow type info to flow through #1002

Refactor binding pipeline to allow type info to flow through #1002

Conversation

mathewc commented Nov 29, 2016 • edited Loading

mathewc Nov 29, 2016 • edited Loading

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

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

mathewc Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

mamaso 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

mamaso Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamaso Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamaso 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

mathewc commented Nov 29, 2016

mathewc commented Nov 29, 2016

mathewc commented Nov 29, 2016 •

edited

Loading

mathewc Nov 29, 2016 •

edited

Loading

mathewc Nov 29, 2016 •

edited

Loading

mamaso Nov 29, 2016 •

edited

Loading

mamaso Nov 29, 2016 •

edited

Loading

mathewc Nov 29, 2016 •

edited

Loading