Repository navigation
fix(sqlite): keep parameter in LIKE ... ESCAPE ? - #4675
Open
manuelschoene wants to merge 2 commits into
Open
manuelschoene wants to merge 2 commits into
manuelschoene wants to merge 2 commits into
Conversation
Added a escape field to the AST expression struct. This field now stores the node for the ESCAPE clause and gets walked. The SQLite converter fills in the field. Both the default and core analyzer got a new case for recording the ESCAPE parameters.
Added end-to-end tests with different combinations of parameters inside the ESCAPE clause. like_escape uses the default analyzer while like_escape_core uses the core analyzer. Both tests contain the same test cases but use the different analyzers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4668
Problem
With the SQLite engine, a parameter in the
ESCAPEclause ofLIKEis dropped from the generated code. The query text keeps every placeholder, but the generated method passes one argument too few:What happens at runtime depends on how the parameters are written:
?placeholders: the query fails withsql: expected 2 arguments, got 1.@pattern ESCAPE @esc,sqlc.arg(...)): these are rewritten to?1/?2, and the driver does not count arguments for those.?2is bound to NULL,LIKE … ESCAPE NULLevaluates to NULL, and the query silently returns no rows.… ESCAPE ? AND score > ?): the arguments shift, soscoreis bound in theESCAPEposition.Both the default analyzer and the core analyzer (
SQLCEXPERIMENT=coreanalyzer) are affected. An escape given as a literal (ESCAPE '!') already worked.Cause
The SQLite parser reads the escape into
LikeExpr.Escape, butconvertLikeExpr(internal/engine/sqlite/convert.go) only converted the two operands.ast.A_Exprhad no field for an escape, so the parameter was gone before parameter discovery ran.Changes
internal/sql/ast/a_expr.go: addA_Expr.Escape, and printESCAPE <expr>for the pattern-match operators inFormat. The PostgreSQL engine never sets the field, because pg_query folds the escape into the pattern aslike_escape(p, e).internal/engine/sqlite/convert.go: convertn.Escapeinto the new field.internal/sql/astutils/walk.go,rewrite.go: visitEscape, so parameter discovery and named-parameter rewriting reach it.internal/compiler/find_params.go,resolve.go(default analyzer): record an escape parameter with its own parent type. Otherwise it would be typed and named after the matched column. It is now alwaystext, namedescapeunless the user names it.internal/core/analyzer/expr.go(core analyzer): type the escape operand the same way (text, namedescapeby default), and count it towards the nullability of the result.After the fix:
Tests
Two new end-to-end cases share the same schema and queries:
internal/endtoend/testdata/like_escape/sqlite/covers the default analyzer.internal/endtoend/testdata/like_escape_core/sqlite/covers the core analyzer; itsexec.jsonsetsSQLCEXPERIMENT=coreanalyzer.Both paths generate identical code. The cases cover:
?parameters;@pattern ESCAPE @esc);NOT LIKE;ESCAPEposition;… ESCAPE ? AND score > ?).I ran the test suite without databases (
go test ./...), and it passes. I did not run the tests that need a live PostgreSQL or MySQL server (themanaged-dbcontext and the examples). The change only affects the SQLite converter and the shared AST code, and the PostgreSQL and MySQL cases still generate unchanged underbase.