Fix Cosmos DB exception when using headers and query binding expressions #354
Conversation
| { | ||
| string sqlToken = GetSqlParameterName(fullToken); | ||
| paramCollection.Add(new SqlParameter(sqlToken, item.Value)); | ||
| tokens.Add(item.Key, sqlToken); |
There was a problem hiding this comment.
This logic seems duplicated from the other above, could they be encapsulated in a helper method to avoid some future break by an edit that forgets the update both?
There was a problem hiding this comment.
Refactored out the duplicate logic. Hope I didn't make it worse. 😄
| object tokenObject = bindingData[token]; | ||
| if (tokenObject is string tokenString) | ||
| { | ||
| string sqlToken = GetSqlParameterName(token); |
There was a problem hiding this comment.
This case seems like a subset of the other in terms of logic, just thinking out loud, but what if we had a single logic that created the replacements and the difference is that when the param is a string we are sending an input Dictionary of 1 item? Probably @brettsam has a more valuable feedback 😄
|
@brettsam I added you since you have probably more insight on the bindings |
|
Hey @MikeStall -- I'm taking a look at this as well, but do we have some re-usable logic for parsing out nested property values? Edit -- n/m -- I think it's up to the ResolutionPolicy to handle it all itself here, which makes sense. |
| private string GetSqlParameterName(string name) | ||
| { | ||
| const string safeSeparator = "_"; | ||
| return "@" + name.Replace(".", safeSeparator).Replace("-", safeSeparator); |
There was a problem hiding this comment.
Are these not allowed in SqlParameter names? Is the list larger than this, or is it just these two characters? When doing replacements like this, I worry that we could stomp over another matching token. Even if it's rare, I wonder if there's another way to handle it.
There was a problem hiding this comment.
@ealsur do you know the complete naming rules for the parameters?
Good point. We can generate something like a GUID for the name to avoid clobbering an existing token. If we do that, as long as it fits under the length limit for a parameter name, it should work.
There was a problem hiding this comment.
@anthonychu Sorry, I missed this for some reason, never saw the notification :( Regarding the rules, I'm not really familiar with all of them but this one looks complete.
There was a problem hiding this comment.
Thanks. So I guess Cosmos DB follows SQL rules for param names? Perhaps switching to a GUID is a safer way to go. I'll see if I can do that.
| @@ -52,5 +52,30 @@ public void TemplateBind_DuplicateParameters() | |||
| Assert.Single(resolvedAttribute.SqlQueryParameters, p => p.Name == "@foo" && p.Value.ToString() == "1234"); | |||
| Assert.Equal("SELECT * FROM c WHERE c.id = @foo AND c.value = @foo", result); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void TemplateBind_NestedParameter() | |||
There was a problem hiding this comment.
I'd like to see a test that includes both a nested and a non-nested parameter in the same SqlQuery. You should be able to add something here to check that as well.
|
@brettsam @MikeStall I think this binding illustrates the need for a bit more information from the |
|
Sorry this fell off my radar! I don't think this is going to tackle all the scenarios that it should, due to the lack of a good API for resolving the template tokens. This works great for headers, but from some of our other code I see that we also support things like JObject which this won't support. Let me follow up offline to see what we can do to reduce some of the code that we need here. |
|
I'm going to go ahead and close this until we can come back with a fix that works for all dotted-notation. See Azure/azure-webjobs-sdk#1726 for the issue being tracked. |
Currently this exception occurs when trying to use binding expressions that access headers and query from the HTTP trigger in Azure Functions:
"sqlQuery": "select * from c where c.id = {headers.foo}"