Improving Table filter parameter validation #984

mathewc · 2017-01-23T22:25:20Z

No description provided.

mathewc · 2017-01-24T03:09:30Z

src/Microsoft.Azure.WebJobs.Host/Tables/TableAttributeBindingProvider.cs

@@ -79,7 +80,24 @@ public static IBindingProvider Build(INameResolver nameResolver, IConverterManag
            var bindAsyncCollector = bindingFactory.BindToAsyncCollector<TableAttribute, ITableEntity>(original.BuildFromTableAttribute, null, original.CollectAttributeInfo);

            var bindToJobject = bindingFactory.BindToExactAsyncType<TableAttribute, JObject>(original.BuildJObject, null, original.CollectAttributeInfo);
-            var bindToJArray = bindingFactory.BindToExactAsyncType<TableAttribute, JArray>(original.BuildJArray, null, original.CollectAttributeInfo);
+
+            BindingTemplate bindingTemplate = null;


Need to figure out state management. There is 1 failing test and it's because I think this local is leaked incorrectly. The delegate model for adding hooks makes it hard to figure out how to do state management like this. How can I capture the BindingTemplate once in my "post resolve hook" and make that available in all further invocations of my "pre bind hook"?

mathewc · 2017-01-24T03:12:30Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/ExactTypeBindingProvider.cs

+
+            protected override Task<IValueProvider> BuildAsync(TAttribute attrResolved, BindingContext context)
+            {
+                _preBindHook?.Invoke(attrResolved, context);


I've only plumbed this through ExactBinding, not the other binding types. If we agree on expanding the hooks like this (I think we need to anyways) we need to do it a cross the board.

Loose delegates like this as an extensibility model make it cumbersome for binding implementations to hook in. As the number of hook points increases, I think it'll get worse. They really need to be specifying a custom type defining the contract, allowing them to override only the bits they need.

What if you had the BindingContext passed into the BindToExactType delegate? Would you need the post-resolve/pre-bind hooks?

I don't like these hooks either -- I've been asking Mike if we can reduce them to keep the concept count lower. I think
with his newest changes we'll be closer to the 'custom type' model that you're proposing.

Not sure I understand - BindingContext is only available at invoke time, whereas up to this point all of the other binding logic (other than converters) was index time.

Doesn't resolving parameters like {id} happen at invoke time? You're doing it inside the pre-bind hook right now. I'm trying to think through the pattern for people that want to do custom binding and not use [AutoResolve] -- which we'll need to do for the DocDB SqlQuery feature as well.

You brought it up elsewhere already -- you want to construct the template during index time, which explains the need for a 'post-resolve hook'. But I'm curious whether you'd need a 'pre-bind hook' if we just passed the BindingContext directly to the 'rule' delegate.

Ah, I see what you mean by the "rule" delegate - I think you're right - if I just change the signature there that would work.

MikeStall · 2017-01-25T18:04:25Z

src/Microsoft.Azure.WebJobs.Host/Tables/TableFilterFormatter.cs

+                string value = null;
+                if (convertedBindingData.TryGetValue(parameterName, out value))
+                {
+                    if (isStringLiteral)


This should also escape DateTimes. (https://docs.microsoft.com/en-us/rest/api/storageservices/fileservices/formatting-datetime-property-values )

Formatting DateTimes when we have the type info as ISO is a good idea. I've added formatting for DateTime and DateTimeOffset. See new if block below.

brettsam · 2017-01-26T15:05:54Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/ExactTypeBindingProvider.cs

+
+            protected override Task<IValueProvider> BuildAsync(TAttribute attrResolved, BindingContext context)
+            {
+                _preBindHook?.Invoke(attrResolved, context);


What if you had the BindingContext passed into the BindToExactType delegate? Would you need the post-resolve/pre-bind hooks?

I don't like these hooks either -- I've been asking Mike if we can reduce them to keep the concept count lower. I think
with his newest changes we'll be closer to the 'custom type' model that you're proposing.

brettsam · 2017-01-26T15:07:38Z

src/Microsoft.Azure.WebJobs.Host/Tables/TableAttributeBindingProvider.cs

+                if (!string.IsNullOrEmpty(attribute.Filter))
+                {
+                    bindingTemplate = BindingTemplate.FromString(attribute.Filter);
+                }


Could the bindingTemplate creation be in the pre-bind hook? You've got the attribute there, which it looks like all you need. That would keep the bindingTemplate logic in one place rather than splitting it up between these two hooks.

The parsing and validation of a binding template needs to be an index time operation - you want to get errors early. All existing bindings not using BindingFactory do this when the binding is created, not on the invoke path. That's what I need to replicate. The logic is split intentionally - validate and parse, and on the invoke path use/reuse.

That makes sense, but the API doesn't make that very clear. I brought this up in another review with two other concepts: "Filter" and "Validator". One runs at indexing time and the other runs per-invocation. It's not clear which is which. I think we do need to reconcile all of this to have some cohesive naming so it's clear where you do certain things. That's doesn't really apply to this PR, though -- this just helps illustrate the point :-)

MikeStall · 2017-01-26T17:42:49Z

src/Microsoft.Azure.WebJobs.Host/Tables/TableFilterFormatter.cs

+                object originalValue;
+                if (bindingData.TryGetValue(parameterName, out originalValue))
+                {
+                    if (originalValue is DateTime)


if (originalValue is DateTime) [](start = 20, length = 30)

originalValue's type may often be a string - especially if this is coming in from an untyped or query string parameter. The way we know to format as a date time is from the Odata string context. Ie, "Prop eq datetime'{x}'" is a datetime, but "Prop eq '{x}'" is a string literal.

we have this same issue with guid and binary - although those are likely rarer cases.

In reply to: 98048689 [](ancestors = 98048689)

No, guid is already handled. Guid formatting is just ToString on the guid (that's what Azure SDK does). We already do this. For Binary, that is too complicated, and I think we should leave that to the user. I.e., if they have a byte[] property, we make them do the base 64. I don't think we should concern ourselves with that right now.

Also, for this type formatting, we're only helping when we have the type info. That will always be the case for core SDK. For dynamic scenarios like Azure Functions non-C#, yes these will be strings, so for now we expect users to format those values. That story can improve in the future with an AF type model for trigger values.

MikeStall · 2017-01-26T17:47:58Z

src/Microsoft.Azure.WebJobs.Host/Tables/TableAttributeBindingProvider.cs

@@ -79,7 +80,24 @@ public static IBindingProvider Build(INameResolver nameResolver, IConverterManag
            var bindAsyncCollector = bindingFactory.BindToAsyncCollector<TableAttribute, ITableEntity>(original.BuildFromTableAttribute, null, original.CollectAttributeInfo);

            var bindToJobject = bindingFactory.BindToExactAsyncType<TableAttribute, JObject>(original.BuildJObject, null, original.CollectAttributeInfo);
-            var bindToJArray = bindingFactory.BindToExactAsyncType<TableAttribute, JArray>(original.BuildJArray, null, original.CollectAttributeInfo);
+
+            BindingTemplate bindingTemplate = null;


bindingTemplate [](start = 28, length = 15)

I don't think we should have any changes in this file. This is controlling { } resolution, so it feels like the change should be in attribute cloner. There shouldn't be any hooks needed. I'll get you a specific proposal.

brettsam · 2017-01-26T18:35:48Z

src/Microsoft.Azure.WebJobs.Host/Bindings/AttributeCloner.cs

+            // Expand on [AutoResolve] property to make this pluggable, 
+            public static ResolutionPolicy GetPolicy(PropertyInfo propInfo)
+            {
+                if (propInfo.DeclaringType == typeof(TableAttribute) && propInfo.Name == "Filter")


I assume the plan is that we open this up when I need it for DocDB?

Yes. This is a a highly tactical change to get Table.Filter in quickly. Note that we could revert a bunch of files around bindings, bindng provider, flow the delegates, etc - those changes are more complex and not necessary to fix table.filter.

In reply to: 98059901 [](ancestors = 98059901)

In this PR I think we should just put an attribute in place allowing this to be specified rather than be hardcoded like this. Kind of like what I did for ParameterResolver. We can still iterate on it, but it would be good to put what we think we want in place now.

brettsam · 2017-01-26T18:40:24Z

src/Microsoft.Azure.WebJobs.Host/Bindings/AttributeCloner.cs

+                var filter = TableFilterFormatter.Format(template, bindingData);
+                return filter;
+            }
+        }


What happened to the 'Properties' bag? I'm still going to need that for DocDB to pass data from the template binding to my binding rules.

All that should be done in a separate work item I think. You'll be most of the way there with this work.

MikeStall · 2017-01-26T18:40:55Z

test/Microsoft.Azure.WebJobs.Host.UnitTests/Tables/TableFilterFormatterTests.cs

+            // datetime filter range
+            bindingData = new Dictionary<string, object>
+                {
+                    { "D1", "2000-01-10" },


"2000-01-10" [](start = 28, length = 13)

One of these should be in a different date format just to exercise that we're actually doing the date formatting and just passing the strings through directly.

For string literals, we're not doing any formatting. We only do that when we have type info.

mathewc · 2017-01-26T23:07:21Z

Merged. Any additional feedback or requested changes we can deal with in follow up PRs as needed.

dnfclas added the cla-already-signed label Jan 23, 2017

ahmelsayed added the in progress label Jan 23, 2017

mathewc force-pushed the table_filter branch from 50967cb to 9891edc Compare January 24, 2017 02:12

mathewc commented Jan 24, 2017

View reviewed changes

MikeStall reviewed Jan 25, 2017

View reviewed changes

mathewc force-pushed the table_filter branch from 9891edc to 6abb094 Compare January 26, 2017 03:11

brettsam reviewed Jan 26, 2017

View reviewed changes

MikeStall reviewed Jan 26, 2017

View reviewed changes

brettsam reviewed Jan 26, 2017

View reviewed changes

MikeStall reviewed Jan 26, 2017

View reviewed changes

mathewc and others added 2 commits January 26, 2017 14:47

Improving Table Filter parameter validation

67dca7e

Use AttributeCloner to template resolution

5399037

mathewc force-pushed the table_filter branch from d77e0be to 5399037 Compare January 26, 2017 23:01

mathewc merged commit 5399037 into dev Jan 26, 2017

ahmelsayed removed the in progress label Jan 26, 2017

MikeStall deleted the table_filter branch July 20, 2017 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving Table filter parameter validation #984

Improving Table filter parameter validation #984

mathewc commented Jan 23, 2017

mathewc Jan 24, 2017 •

edited

Loading

mathewc Jan 24, 2017 •

edited

Loading

brettsam Jan 26, 2017

mathewc Jan 26, 2017

brettsam Jan 26, 2017

mathewc Jan 26, 2017

MikeStall Jan 25, 2017

mathewc Jan 26, 2017 •

edited

Loading

brettsam Jan 26, 2017

brettsam Jan 26, 2017

mathewc Jan 26, 2017

brettsam Jan 26, 2017

MikeStall Jan 26, 2017

MikeStall Jan 26, 2017

mathewc Jan 26, 2017

mathewc Jan 26, 2017

MikeStall Jan 26, 2017

brettsam Jan 26, 2017

MikeStall Jan 26, 2017

mathewc Jan 26, 2017 •

edited

Loading

brettsam Jan 26, 2017

mathewc Jan 26, 2017 •

edited

Loading

MikeStall Jan 26, 2017

mathewc Jan 26, 2017

mathewc commented Jan 26, 2017

Improving Table filter parameter validation #984

Improving Table filter parameter validation #984

Conversation

mathewc commented Jan 23, 2017

mathewc Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

mathewc Jan 24, 2017 • 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

Choose a reason for hiding this comment

mathewc Jan 26, 2017 • 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

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 Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc commented Jan 26, 2017

mathewc Jan 24, 2017 •

edited

Loading

mathewc Jan 24, 2017 •

edited

Loading

mathewc Jan 26, 2017 •

edited

Loading

mathewc Jan 26, 2017 •

edited

Loading

mathewc Jan 26, 2017 •

edited

Loading