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

Check arity consistency when constructing SqlFunctionExpressions #33815

Merged
merged 3 commits into from
May 29, 2024

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented May 26, 2024

It is currently possible to construct SqlFunctionExpressions in which the size of arguments does not match the size of argumentsPropagateNullability.

While this is definitely a problem of mis-using the API, it is both easy to trip in it (in fact there is an occurrence of such problem in the repo 😅 ) and easy for EFCore to provide better feedback when this happens.

Discovered while working on #33814

@ranma42
Copy link
Contributor Author

ranma42 commented May 26, 2024

I believe that the commit fixing the translation of random() in sqlite is likely uncontroversial, while the one adding checks probably needs some discussion:

  • is this considered a breaking change? (previously it was possible to create more SqlFunctionExpressions; EFCore would have been unable to handle them properly, but someone might have been creating them with the inconsistency and later on transforming them with another library before passing them to EFCore)
  • I used ArgumentException as that seemed the most appropriate for this case; for a similar problem SqlUnaryExpression and SqlBinaryExpression use InvalidOperationException, which is meant to be "thrown when a method call is invalid for the object's current state" source; is it better to use InvalidOperationException for this case as well?
  • should we do this check at all?

@cincuranet
Copy link
Contributor

is this considered a breaking change? (previously it was possible to create more SqlFunctionExpressions; EFCore would have been unable to handle them properly, but someone might have been creating them with the inconsistency and later on transforming them with another library before passing them to EFCore)

I don't think that creating them inconsistently would be considered valid case. It is a breaking change, but with pretty limited blast radius.

I used ArgumentException as that seemed the most appropriate for this case; for a similar problem SqlUnaryExpression and SqlBinaryExpression use InvalidOperationException, which is meant to be "thrown when a method call is invalid for the object's current state" source; is it better to use InvalidOperationException for this case as well?

InvalidOperationException seems to be better fit.

should we do this check at all?

Seems valid to me.

@maumar
Copy link
Contributor

maumar commented May 28, 2024

It is a breaking change. While we already rely on the assumption that arguments and propagates nullability information tables are in sync, those were only being used when processing IS NULL/IS NOT NULL expressions. With this change (and nullability computation work in #33814) more people get affected. However, I agree with @cincuranet that the impact is relatively small.

One thing to note, argument propagates nullability is somewhat complicated concept - some users may not fully understand it and as such leave the array empty (and hey, it happened to have worked! ;) ) - Perhaps it's beneficial for the exception message to provide some guidance - as in, if not sure use "false", or (in the future) link to a documentation section, once we have it.

@maumar
Copy link
Contributor

maumar commented May 28, 2024

wrt exception type, I also like InvalidOperationException better, EF tends to favor it for almost all errors we issue.

@ranma42 ranma42 force-pushed the abs-1-argument branch 2 times, most recently from b0429e2 to 739bde5 Compare May 28, 2024 22:11
ranma42 added 2 commits May 29, 2024 00:12
The Sqlite translation of `random()` was creating an expression in which a
`SqlFunction` was defined to have 1 argument, but had an empty
`argumentsPropagateNullability` parameter.
@ranma42
Copy link
Contributor Author

ranma42 commented May 28, 2024

I switched the exception type to InvalidOperationException and moved the strings to RelationalStrings for consistency with Sql{Bi,U}naryExpression.

@ranma42 ranma42 marked this pull request as ready for review May 28, 2024 22:28
@cincuranet
Copy link
Contributor

Looks like we have 2 (strongly related :)) failures:

Microsoft.EntityFrameworkCore.Query.SpatialQuerySqliteTest.GeometryType(async: ...)
System.InvalidOperationException : 'rtrim' was constructed with 2 arguments, but the nullability was defined for 1 arguments.
Microsoft.EntityFrameworkCore.Query.SpatialQuerySqliteTest.OgcGeometryType(async: ...)
System.InvalidOperationException : 'rtrim' was constructed with 2 arguments, but the nullability was defined for 1 arguments.

@ranma42
Copy link
Contributor Author

ranma42 commented May 29, 2024

ouch, sorry, I did not notice that locally I was not running with SpatiaLite.
I am running on linux so unfortunately it requires customizing the csprojs (as explained here https://learn.microsoft.com/en-us/ef/core/providers/sqlite/spatial ).
I enabled it for some runs, but I probably forgot to apply those changes while rewriting the baselines in this case.
I will fix ASAP 😇

If its 2-arguments form is being used, the `argumentsPropagateNullability` should have 2 elements as well.
@ranma42
Copy link
Contributor Author

ranma42 commented May 29, 2024

side note: in addition to the arity mismatches there seem to be some inconsistency in the nullability configuration in the codebase.
I might eventually be able to check that as well, but I believe that can be done in a separate PR.

@cincuranet
Copy link
Contributor

but I believe that can be done in a separate PR

Agreed.

@cincuranet cincuranet merged commit c594d31 into dotnet:main May 29, 2024
7 checks passed
@cincuranet
Copy link
Contributor

Thanks for the contribution.

@ranma42
Copy link
Contributor Author

ranma42 commented May 29, 2024

thank you for your patience and for guiding me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check arity consistency when constructing SqlFunctionExpressions
4 participants