DefaultQuerySqlGenerator not flexible enough for provider override #9143
Comments
|
@roji Thanks for reporting this.
Our general approach is intentionally to avoid overdesigning the provider model and wait for feedback like yours to help us triangulate where the extensibility and additional flexibility is needed. This obviously works much better if we have time to react to feedback like this... So the second point from my list of regrets at #9126 (comment) applies here too. Fortunately in this case I believe we can take changes like the ones you suggest later, e.g. in 2.1, without them being breaking changes. So it would be great if you can send a PR. I also hope you can either do something that works (even if it is suboptimal) for 2.0 or wait until 2.1 to add the new Like variant. |
|
|
Thanks (as always) for your reactions and for caring :) The good news on this is that the lacking extensibility in DefaultSqlQueryGenerator doesn't have a big on me at the moment - the only tangible problem is the extra @smitpatel no problem, I'll let you guys take a look at the two issues and fix them as you see fit. If you prefer to receive a PR from me on one of them (or both) let me know. |
When a custom binary operator uses a standard comparison operator (=, <>, <, >, <=, >=)
EF Core will generate a pattern that causes a syntax error in PostgreSQL.
Example SQL:
WHERE a < b = TRUE
This patch adds logic to check if the custom binary operator is one of the standard
comparison operators. If so, it wraps the expression in parenthesis.
While it seems like this could be avoided by having the custom translator construct
a standard Expression.LessThan(Expression,Expression), this causes an error to be
thrown...because the binary operator isn't defined.
See the related links for more information.
Example stack trace:
System.InvalidOperationException : The binary operator LessThanOrEqual is not defined
for the types 'System.Net.IPAddress' and 'System.Net.IPAddress'.
at System.Linq.Expressions.Expression.GetUserDefinedBinaryOperatorOrThrow(...)
at System.Linq.Expressions.Expression.GetComparisonOperator(...)
at System.Linq.Expressions.Expression.LessThanOrEqual(...)
Related:
- dotnet/efcore#9143
- npgsql#323 (review)
When a custom binary operator uses a standard comparison operator (`=`, `<>`, `<`, `>`, `<=`, `>=`) EF Core will generate a pattern that causes a syntax error in PostgreSQL. Example SQL: ```sql WHERE x."Inet" < @__inet_1 = TRUE ``` This patch adds logic to check if the custom binary operator is one of the standard comparison operators. If so, it wraps the expression in parenthesis. Example SQL (patched): ```sql WHERE (x."Inet" < @__inet_1) = TRUE ``` While it seems like this could be avoided by having the custom translator construct a standard Expression.LessThan(Expression,Expression), this causes an error to be thrown...because the binary operator isn't defined. Example stack trace: ``` System.InvalidOperationException : The binary operator LessThanOrEqual is not defined for the types 'System.Net.IPAddress' and 'System.Net.IPAddress'. at System.Linq.Expressions.Expression.GetUserDefinedBinaryOperatorOrThrow(...) at System.Linq.Expressions.Expression.GetComparisonOperator(...) at System.Linq.Expressions.Expression.LessThanOrEqual(...) ``` Related: - [](dotnet/efcore#9143) - [](npgsql#323 (review))
When a custom binary operator uses a standard comparison operator (`=`, `<>`, `<`, `>`, `<=`, `>=`) EF Core will generate a pattern that causes a syntax error in PostgreSQL. Example SQL: ```sql WHERE x."Inet" < @__inet_1 = TRUE ``` This patch adds logic to check if the custom binary operator is one of the standard comparison operators. If so, it wraps the expression in parenthesis. Example SQL (patched): ```sql WHERE (x."Inet" < @__inet_1) = TRUE ``` While it seems like this could be avoided by having the custom translator construct a standard `Expression.LessThan(Expression,Expression)`, this causes an error to be thrown...because the binary operator isn't defined. Example stack trace: ``` System.InvalidOperationException : The binary operator LessThanOrEqual is not defined for the types 'System.Net.IPAddress' and 'System.Net.IPAddress'. at System.Linq.Expressions.Expression.GetUserDefinedBinaryOperatorOrThrow(...) at System.Linq.Expressions.Expression.GetComparisonOperator(...) at System.Linq.Expressions.Expression.LessThanOrEqual(...) ``` Related: - dotnet/efcore#9143 - npgsql#323 (review)
When a custom binary operator uses a standard comparison operator (`=`, `<>`, `<`, `>`, `<=`, `>=`) EF Core will generate a pattern that causes a syntax error in PostgreSQL. Example SQL: ```sql WHERE x."Inet" < @__inet_1 = TRUE ``` This patch adds logic to check if the custom binary operator is one of the standard comparison operators. If so, it wraps the expression in parenthesis. Example SQL (patched): ```sql WHERE (x."Inet" < @__inet_1) = TRUE ``` While it seems like this could be avoided by having the custom translator construct a standard `Expression.LessThan(Expression,Expression)`, this causes an error to be thrown...because the binary operator isn't defined. Example stack trace: ``` System.InvalidOperationException : The binary operator LessThanOrEqual is not defined for the types 'System.Net.IPAddress' and 'System.Net.IPAddress'. at System.Linq.Expressions.Expression.GetUserDefinedBinaryOperatorOrThrow(...) at System.Linq.Expressions.Expression.GetComparisonOperator(...) at System.Linq.Expressions.Expression.LessThanOrEqual(...) ``` Related: - dotnet/efcore#9143 - #323 (review)
|
Superseded by #10513 |
As part of #9126, I set about adding a PostgreSQL-specific ILikeExpression and modifying NpgsqlQuerySqlGenerator to render it correctly. I hit several limitations with DefaultQuerySqlGenerator not exposing enough functionality to subclasses:
VisitLike()does)= TRUEis rendered out, and may have other consequences I'm not aware of as well.I can submit PRs for these two specific problems, but I've run into similar issues previously, so you guys may want to do a proper review of DefaultQuerySqlGenerator keeping providers in mind.
The text was updated successfully, but these errors were encountered: