Skip to content
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

Query: converter from bool used in predicate without comparison fails on sqlite (and other non-sql server backends?) #30326

Closed
maumar opened this issue Feb 23, 2023 · 3 comments · Fixed by #30666
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@maumar
Copy link
Contributor

maumar commented Feb 23, 2023

query:

ss.Set<JsonEntityConverters>().Where(x => x.Reference.BoolConvertedToStringYN)

produces

SELECT "j"."Id", "j"."Reference"
FROM "JsonEntitiesConverters" AS "j"
WHERE json_extract("j"."Reference",'$.BoolConvertedToStringYN')

which gives incorrect results

if we construct the predicate manually, like so:

ss.Set<JsonEntityConverters>().Where(x => x.Reference.BoolConvertedToStringYN == true)

we produce the correct sql and results

SELECT "j"."Id", "j"."Reference"
FROM "JsonEntitiesConverters" AS "j"
WHERE json_extract("j"."Reference",'$.BoolConvertedToStringYN') = 'Y'

sql server is not affected because search condition visitor always adds the comparison in the predicate, but sqlite can execute query without it. We should be adding comparisons always when predicate has bool value with a converter.

@ajcvickers ajcvickers modified the milestones: Backlog, 8.0.0 Mar 2, 2023
maumar added a commit that referenced this issue Apr 4, 2023
…comparison fails on sqlite (and other non-sql server backends?)

SqlServer doesn't have the problem because search condition replacing visitor already converts all bool values in the predicate into conditions.
Fix is to re-purpose this visitor to be used by other providers also and convert bool values to conditions when they have a converter.

Fixes #30326
maumar added a commit that referenced this issue Apr 12, 2023
…comparison fails on sqlite (and other non-sql server backends?)

SqlServer doesn't have the problem because search condition replacing visitor already converts all bool values in the predicate into conditions.
Fix is to re-purpose this visitor to be used by other providers also and convert bool values to conditions when they have a converter.

Fixes #30326
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Apr 12, 2023
maumar added a commit that referenced this issue Apr 12, 2023
…comparison fails on sqlite (and other non-sql server backends?)

RelationalValueConverterCompensatingExpressionVisitor was not compensating for bool values with converters for JsonScalarExpression.

Fixes #30326
maumar added a commit that referenced this issue Apr 13, 2023
…comparison fails on sqlite (and other non-sql server backends?)

RelationalValueConverterCompensatingExpressionVisitor was not compensating for bool values with converters for JsonScalarExpression.

Fixes #30326
maumar added a commit that referenced this issue Apr 13, 2023
…comparison fails on sqlite (and other non-sql server backends?)

RelationalValueConverterCompensatingExpressionVisitor was not compensating for bool values with converters for JsonScalarExpression.

Fixes #30326
maumar added a commit that referenced this issue Apr 13, 2023
…comparison fails on sqlite (and other non-sql server backends?)

RelationalValueConverterCompensatingExpressionVisitor was not compensating for bool values with converters for JsonScalarExpression.

Fixes #30326
maumar added a commit that referenced this issue Apr 14, 2023
…comparison fails on sqlite (and other non-sql server backends?) (#30666)

RelationalValueConverterCompensatingExpressionVisitor was not compensating for bool values with converters for JsonScalarExpression.

Fixes #30326
@maumar maumar reopened this Apr 14, 2023
@maumar
Copy link
Contributor Author

maumar commented Apr 14, 2023

reopening for potential servicing. Minimal risk, causes data corruption but it was not a customer report - thoughts @ajcvickers ?

@maumar maumar removed this from the 8.0.0 milestone Apr 14, 2023
@ajcvickers ajcvickers added this to the 7.0.x milestone Apr 20, 2023
@ajcvickers
Copy link
Member

Note from triage: bring to Tactics for patch.

@maumar maumar removed this from the 7.0.x milestone Apr 28, 2023
@maumar
Copy link
Contributor Author

maumar commented Apr 28, 2023

now that I think about it, patching this doesn't make much sense - we don't support JSON on non-sql server providers, and SQL Server is already covered by search condition visitor

@ajcvickers ajcvickers modified the milestones: 8.0.0-preview4, 8.0.0 Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment