Improving Table filter parameter validation #984
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using Microsoft.Azure.WebJobs.Host.Bindings; | ||
| using Microsoft.Azure.WebJobs.Host.Bindings.Path; | ||
|
|
||
| namespace Microsoft.Azure.WebJobs.Host.Tables | ||
| { | ||
| internal static class TableFilterFormatter | ||
| { | ||
| public static string Format(BindingTemplate template, IReadOnlyDictionary<string, object> bindingData) | ||
| { | ||
| if (!template.ParameterNames.Any()) | ||
| { | ||
| return template.Pattern; | ||
| } | ||
|
|
||
| if (template.ParameterNames.Count() == 1) | ||
| { | ||
| // Special case where the entire filter expression | ||
| // is a single parameter. We let this go through as is | ||
| string parameterName = template.ParameterNames.Single(); | ||
| if (template.Pattern == $"{{{parameterName}}}") | ||
| { | ||
| return template.Bind(bindingData); | ||
| } | ||
| } | ||
|
|
||
| // each distinct parameter can occur one or more times in the template | ||
| // so group by parameter name | ||
| var parameterGroups = template.ParameterNames.GroupBy(p => p); | ||
|
|
||
| // for each parameter, classify it as a string literal or other | ||
| // and perform value validation | ||
| var convertedBindingData = BindingDataPathHelper.ConvertParameters(bindingData); | ||
| foreach (var parameterGroup in parameterGroups) | ||
| { | ||
| // to classify as a string literal, ALL occurrences in the template | ||
| // must be string literals (e.g. of the form '{p}') | ||
| // note that this will also capture OData expressions of the form | ||
| // datetime'{p}', guid'{p}', X'{p}' which is fine, because single quotes | ||
| // aren't valid for those values anyways. | ||
| bool isStringLiteral = true; | ||
| string parameterName = parameterGroup.Key; | ||
| string stringParameterFormat = $"'{{{parameterName}}}'"; | ||
| int count = 0, idx = 0; | ||
| while (idx >= 0 && idx < template.Pattern.Length && count++ < parameterGroup.Count()) | ||
| { | ||
| idx = template.Pattern.IndexOf(stringParameterFormat, idx, StringComparison.OrdinalIgnoreCase); | ||
| if (idx < 0) | ||
| { | ||
| isStringLiteral = false; | ||
| break; | ||
| } | ||
| idx++; | ||
| } | ||
|
|
||
| // validate and format the value based on its classification | ||
| string value = null; | ||
| if (convertedBindingData.TryGetValue(parameterName, out value)) | ||
| { | ||
| if (isStringLiteral) | ||
|
There was a problem hiding this comment. This should also escape DateTimes. (https://docs.microsoft.com/en-us/rest/api/storageservices/fileservices/formatting-datetime-property-values ) There was a problem hiding this comment. 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. |
||
| { | ||
| convertedBindingData[parameterName] = value.Replace("'", "''"); | ||
| } | ||
| else if (!TryValidateNonStringLiteral(value)) | ||
| { | ||
| throw new InvalidOperationException($"An invalid parameter value was specified for filter parameter '{parameterName}'."); | ||
| } | ||
| } | ||
|
|
||
| // perform any OData specific formatting on the values | ||
| object originalValue; | ||
| if (bindingData.TryGetValue(parameterName, out originalValue)) | ||
| { | ||
| if (originalValue is DateTime) | ||
|
There was a problem hiding this comment.
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. There was a problem hiding this comment. we have this same issue with guid and binary - although those are likely rarer cases. In reply to: 98048689 [](ancestors = 98048689) There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. |
||
| { | ||
| // OData DateTime literals should be ISO 8601 formatted (e.g. 2009-03-18T04:25:03Z) | ||
| convertedBindingData[parameterName] = ((DateTime)originalValue).ToUniversalTime().ToString("o"); | ||
| } | ||
| else if (originalValue is DateTimeOffset) | ||
| { | ||
| convertedBindingData[parameterName] = ((DateTimeOffset)originalValue).UtcDateTime.ToString("o"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return template.Bind(convertedBindingData); | ||
| } | ||
|
|
||
| internal static bool TryValidateNonStringLiteral(string value) | ||
| { | ||
| // value must be one of the odata supported non string literal types: | ||
| // bool, int, long, double | ||
| bool boolValue; | ||
| long longValue; | ||
| double doubleValue; | ||
| if (bool.TryParse(value, out boolValue) || | ||
| long.TryParse(value, out longValue) || | ||
| double.TryParse(value, out doubleValue)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the plan is that we open this up when I need it for DocDB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.