-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[SMALL] Fix to #30330 - Json: updating property with conversion from string to other type fails on sql server #30380
Conversation
Hello @maumar! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
…o other type fails on sql server when producing update statement, when deciding whether we need CAST around the result of JSON_VALUE, we need to look if the property has converter, rather than just look at it's clr type. If the clr type is string, but it gets converted to int/bool etc we still need a CAST. Fixes #30330
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
1 similar comment
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
@@ -154,8 +154,10 @@ protected virtual int MergeIntoMinimumThreshold | |||
|
|||
if (columnModification.Property != null) | |||
{ | |||
var needsTypeConversion = columnModification.Property.ClrType.IsNumeric() | |||
|| columnModification.Property.ClrType == typeof(bool); | |||
var propertyClrType = columnModification.Property.GetTypeMapping().Converter?.ProviderClrType |
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.
Don't we have a simply shortcut to just get the provider CLR type of a property? If not, we probably should...
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 was the method suggested by @AndriySvyryd, so I guess it's the best one we've got ;)
@@ -558,7 +558,7 @@ public override async Task Edit_single_property_char() | |||
|
|||
SET IMPLICIT_TRANSACTIONS OFF; | |||
SET NOCOUNT ON; | |||
UPDATE [JsonEntitiesAllTypes] SET [Reference] = JSON_MODIFY([Reference], 'strict $.TestCharacter', CAST(JSON_VALUE(@p0, '$[0]') AS nvarchar(1))) | |||
UPDATE [JsonEntitiesAllTypes] SET [Reference] = JSON_MODIFY([Reference], 'strict $.TestCharacter', JSON_VALUE(@p0, '$[0]')) |
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.
Unrelated to this PR :)
I noticed that we we send a single-element JSON array wrapping the value, and then use JSON_VALUE server-side to unwrap the value. What's the reason for having to do this, i.e. why not just send the value directly and just do JSON_MODIFY([Reference], 'strict $.TestCharacter', @p0)
? The examples for JSON_VALUE just seem to show the simple, direct way...
If we did that, wouldn't we be able to just get rid of the CAST around the JSON_VALUE? I'm assuming the cast is needed because JSON_VALUE always returns nvarchar (but we need e.g. an int). But if we just embedded the value directly, it would be typed correctly and could just go directly into the document?
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.
When passing values directly in parameter I hit issues with some data types (DateTime, Guid). e.g. if stored directly as parameter DateTime would be stored as "1\/1\/3000 12:34:56 PM"
(and that wouldn't round trip), whereas what we want is "3000-01-01T12:34:56"
. By doing the array trick we always get values in a form that is appropriate to store as json for a given data type. Perhaps we could do special casing and simplify for types that work (which is the majority). Or better yet, when/if we have dedicated type mapping for json, we would always be able to render the parameter values correctly.
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.
filed #30410 to track this
WHERE [Id] = @p1; | ||
""", | ||
// | ||
""" |
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.
Need to see if this can be enforced e.g. with .editorconfig
""" | |
""" |
WHERE [Id] = @p1; | ||
""", | ||
// | ||
""" |
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.
""" | |
""" |
(@maumar just a ping to make sure you saw the comments above) |
when producing update statement, when deciding whether we need CAST around the result of JSON_VALUE, we need to look if the property has converter, rather than just look at it's clr type. If the clr type is string, but it gets converted to int/bool etc we still need a CAST.
Fixes #30330