-
Notifications
You must be signed in to change notification settings - Fork 63
fix casing issue when sql table is case insensitive #235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
{ | ||
// ToDo: add check for duplicate primary keys once we find a way to get primary keys. | ||
// JObjects ignore serializer settings (https://web.archive.org/web/20171005181503/http://json.codeplex.com/workitem/23853) | ||
// so we have to manually convert property names to lower case before inserting into the query in that case |
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.
What about case sensitive collations?
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.
(even if this works, clarifying that it works in a comment would be helpful to readers)
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.
Case sensitive has always been up to the caller to get right - we have no way of knowing what the expected case is. Case insensitive is just always converted to lower case for comparisons for convenience (since the casing still has to be the same, but we can control that for case insensitive)
https://github.com/Azure/azure-functions-sql-extension/blob/main/src/SqlAsyncCollector.cs#L526
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.
(This behavior should be documented somewhere but I don't think this is the right place to get into that fine of detail. I'll see if I can find a better place to do that)
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.
Thanks @Charles-Gagnon
* add lowercasing of jobject properties * add comments * check for c# type * Refactor * remove unused * fix Co-authored-by: chgagnon <chgagnon@microsoft.com>
Fixes #232 where the column names are case insensitive, we are building the sql query converting the columns to lowercase but the javascript object keys are being passed as defined and the sql query is expecting lowercased values.
Fix: Added an extra step to convert the JObjects keys to lower case and passed that to the sql query.
Tried using the JsonSerializerSettings and passing the NamingStrategy to convert property names to lowercase but that was not working and still returned keys as is. Alternatively manually updated the properties to lower case by iterating through them.