Repository navigation
Fix placeholder bugs by refactoring to make them impossible - #7
Conversation
jgaskins
left a comment
jgaskins
left a comment
There was a problem hiding this comment.
I'm still working my way through this. I've looked at all the commits individually and they make sense on their own. I'm currently looking at the full diff and trying to wrap my mind around the big picture.
One thing that kept coming to mind for me in reviewing this PR so far is that it's getting more and more flexible, which I do feel like we need, but I'm wondering if that flexibility needs to be captured in Interro specifically. Last year I wrote a SQL parser/builder specifically for Postgres for unrelated reasons1 but I've been toying with the idea of bringing it into Interro to solve some of the same problems you're solving in this PR. The string concatenation I've been doing here clearly has some brittle edges.
I added a quick example of a SQL query builder (not like an Interro::QueryBuilder, just a generic one to get the idea across) using that shard here and I'd love to get your thoughts on the idea as someone who's been working to fix some of these edge cases.
Footnotes
-
I wanted to instrument SQL queries in an app that runs a nontrivial amount of of hand-written SQL and some of those queries have literals in them, so I needed to sanitize the query to group identical query plans together. ↩
f57fea2 to
35182b0
Compare
|
Thanks for reviewing, sorry it's taken me a while to get back to this...
Interesting stuff!
I haven't really had time to give this proper consideration. My gut feel is that if one wanted to do more complex symbolic manipulation of the SQL query then yes, absolutely, but as a way to build fairly simple queries it's a lot of added code and complexity. I'm hoping that with this change the placeholder interpolation bugs should at least be gone! One observation is that your AST approach still has manual numbering ( |
35182b0 to
57b0b70
Compare
|
Rebased on master. |
|
Looks like you'll need to merge or rebase again in order to fix CI. There was a bug in the |
In particular this adds test coverage for: - Queries combining LIMIT and OFFSET - Queries combining DISTINCT ON with ORDER BY (including the parameterized case)
The code was previously taking the 2nd character of the matched string, instead of the first match group. It's the same fix in lots of places; just adding one spec since this code is simplified later in the branch by removing the need to renumber entirely and refactoring parsing into one place.
Limit and offset args were not being provided in the following cases, resulting in errors like 'bind message supplies 1 parameters, but prepared statement requires 2': - the iterator form of each (e.g. `query.first(5).each`) - both sides of a CompoundQuery The abstracted `QueryBuilder#bind_args` is removed entirely later in this branch when this bug becomes impossible, so I didn't worry too much about its naming. This fix revealed that limited/offset sides of a compound query generated invalid SQL (due to missing parentheses), so that's also fixed here, and is a change which survives the later refactor.
`QueryBuilder#none?` only renders the WHERE clause, but args could contain ORDER BY values. This bug also becomes impossible later in this branch.
This is required by the next commit, which moves placeholder numbering to render time and so needs ORDER BY keys to be expression objects rather than pre-numbered SQL text. Rendering is byte-identical, since QueryExpression#to_sql still just writes the stored text. The one nuance is that Hash key equality now compares values as well as SQL text, so two ORDER BY entries with identical text but different values no longer deduplicate — but any query that could hit that already produced a broken query.
Essentially this relied on two changes: 1. Internal representation of `QueryExpression` changes to `Array(Part)` where `Part = String | Any` — raw SQL text (`String`) interleaved with the values to bind (`Any`). Raw fragments from user code (where/order_by/distinct) are parsed on init, with `$n` resolving to the fragment's own values. After that, fragments carry no numbers at all, and placeholder numbers are only assigned at the point of rendering the SQL. 2. `QueryBuilder` no longer stores an args array alongside the SQL. Keeping the two in sync by hand was the shared cause of most of the bugs fixed here — it was easy to emit a placeholder and forget its value, or bind a value the rendered clause never referenced. Now every read path builds SQL and args in one pass, and `QueryExpression#to_sql(io, args)` appends to both in one place. The specs added in interro_spec.cr describe cases that previously failed on master. The trick is `QueryExpression#to_sql(io : IO, args : Array(Any)) : Nil`; this is passed an io and args to mutate, and appends to both on consecutive lines (`args << part` & `io << '$' << args.size`), making it impossible to change one without the other. Every read path now builds SQL and args in that one pass: `each`, `to_a`, `first?` and `to_sql` via the shared `QueryBuilder#render`, with `scalar`, `none?` and `CompoundQuery` threading their own args array through the same method. That removes the need for `bind_args` and the `gsub`-based renumbering of placeholders, and is a nice code simplification. One annoying complexity which had to be handled: Since Postgres enforces that an expression appearing in both the DISTINCT ON subclause and ORDER BY has to render identically in both (including placeholder numbers), these are rendered via a `render_once` cache to they get the same placeholder ids and emit a single arg. Behaviour notes: - A $n repeated within one fragment renders as one placeholder per reference, each bound to the same value – different SQL text, identical semantics. - A $n referencing a missing value raises ArgumentError on init, rather than surfacing as a Postgres bind error at execution (an improvement). - A $n in a DISTINCT ON expression now raises, since such an expression carries no values of its own. A parameterized ORDER BY expression doesn't need repeating there to satisfy Postgres, as its key is added to the DISTINCT ON subclause anyway. So that parsing raw SQL is explicit, the constructor taking SQL text and values is gone, and internals must call `QueryExpression.parse` directly. QueryValue is refactored in the next commit to remove the remaining four-argument constructor.
Separated from the last commit for clarity; `QueryValue` now constructs `QueryExpression` parts directly rather than parsing a numbered expression. By removing the now unused `QueryExpression.new(lhs, comparator, rhs, values)` constructor, all SQL expression parsing always goes through `QueryExpression.parse` (making it easy to spot), and is now only used for raw fragments from user code.
Before this branch, a $n inside a SQL string literal risked being silently renumbered along with the real placeholders. Nothing failed visibly, because the renumbering only rewrote text and left the number of arguments sent to Postgres unchanged — the only damage was to the contents of the string. After the core refactor the same mistake becomes visible, which is arguably an improvement: either parsing raises, because the misread placeholder has no value to resolve to, or the value it swallows is bound to a placeholder Postgres cannot see inside the quotes, and Postgres rejects the statement. With this fix a $n inside an ordinary single-quoted literal is left alone, and ArgumentError is raised only where it should be: when a $n references a value that was not provided. Note that only ordinary single-quoted literals are recognised. A $n inside an E'' string, a dollar-quoted string, or a quoted identifier is still taken for a placeholder. Covering those means matching every form of literal Postgres accepts, which is a much larger pattern, and left till later.
Simplifies callers by avoiding the need to upcast to `Array(Any)` first, and by defaulting to empty values. Includes adding missing spec coverage for ORDER BY on a raw expression with no args.
57b0b70 to
75930a3
Compare
|
@jgaskins done (also tweaked a handful of comments and commit descriptions that I felt were unclear/unnecessary on re-reading, no other changes). |
jgaskins
left a comment
jgaskins
left a comment
There was a problem hiding this comment.
This is very, very good. I really like that you were able to remove so much of the duplication around arg parsing. I ran into so many bugs around that. This should make it so much easier to add arg parsing to other areas like JOIN clauses.
| it "continues numbering across fragments rendered into one args array" do | ||
| first = QueryExpression.parse("a = $1", [1]) | ||
| second = QueryExpression.parse("b = $1", [2]) | ||
| args = [] of Any | ||
|
|
||
| sql = String.build do |str| | ||
| first.to_sql str, args | ||
| str << " AND " | ||
| second.to_sql str, args | ||
| end | ||
|
|
||
| sql.should eq "a = $1 AND b = $2" | ||
| args.should eq [Any.new(1), Any.new(2)] | ||
| end |
There was a problem hiding this comment.
This feels like pure sorcery even though it's quite simple. Unbelievable that this works.
Sorry about the size of this PR!
The commits are intended to be read in order: they form a narrative that isolates the complex change to a single core commit, with small fixes and refactors on either side. Every commit passes the full spec suite, and of the specs this PR adds, 10 fail on unmodified master (7 failures + 3 errors), demonstrating the bugs being fixed.
For motivation, this bug class has a history of fixes: e.g. 213a8ab and 99529ac. This PR handles read-path bugs; the write-path numbering has similar issues, and is handled by a (thankfully simpler!) follow-up PR.
Core commit: "Fix placeholder bugs and make impossible by numbering at render time"
Essentially this relied on two changes:
Internal representation of
QueryExpressionchanges toArray(Part)wherePart = String | Any— raw SQL text (String) interleaved with the values to bind (Any).Raw fragments from user code (where/order_by/distinct) are parsed on init, with
$nresolving to the fragment's own values.After that, fragments carry no numbers at all, and placeholder numbers are only assigned at the point of rendering the SQL.
QueryBuilderno longer stores an args array alongside the SQL.Keeping the two in sync by hand was the shared cause of most of the bugs fixed here — it was easy to emit a placeholder and forget its value, or bind a value the rendered clause never referenced.
Now every read path builds SQL and args in one pass, and
QueryExpression#to_sql(io, args)appends to both in one place.The commit message 3e448c8 has the full detail.
Narrative
First, "Add missing spec coverage" b4fe85b adds specs that pass throughout, and "Refactor subquery embedding onto Subquery" efa9720 moves some duplication out of the way.
Several of the bugs fixed by the core commit were simple enough to extract as pre-commits, avoiding landing everything in one massive change, so these come next:
Two refactors are really part of the core commit, but were extracted to reduce its size: "Refactor to key OrderBy Hash by QueryExpression" 0f3d100 immediately before it, and "Refactor to remove placeholder numbering from QueryValue internals" ffa14e2 immediately after.
Finally, "Fix renumbering of $n inside string literals" c04a011 fixes a remaining parsing bug which would have been hard to fix while the code for this was scattered in several places. Finally 75930a3 is a simplification I noticed late in the day, and could be folded into the core commit.Visible behaviour changes
Nothing here is intended to change the results of a query that already worked, but a few things are visible:
$nin a raw fragment referencing no value raisesArgumentErrorwhen the query is built, rather than a Postgres bind error at execution.Same for a
$nin adistinct(on:)expression, which has no values of its own; a parameterized ORDER BY expression doesn't need repeating there, since its key is added to the DISTINCT ON subclause automatically.$nrepeated within one fragment renders as one placeholder per reference:where("a = $1 OR b = $1", [v])becomesa = $1 OR b = $2. Identical semantics, different SQL text.CompoundQuerywraps each side in parentheses, required once a side can carry its own LIMIT/OFFSET.QueryExpressionis now constructed from parts, or from raw SQL viaQueryExpression.parse(fragment, values)– theexpression/valuesconstructor is gone, though#valuesremains (now derived).QueryValue.newno longer takes a placeholder index,Interro::OrderByisHash(QueryExpression, String), andQueryBuilder'sargsproperty is gone.QueryExpression.parseonly recognises ordinary single-quoted literals (dbe202d1e), so a$ninside anE''string, a dollar-quoted string, or a quoted identifier is still taken for a placeholder and will raise (since the number of args will not match) – previously this would work if no args preceded, but the placeholder would be (incorrectly) renumbered it if any did. The pattern that recognises every literal form Postgres accepts is big enough to deserve its own review, so I have that as a follow-up – happy to bring it into this PR if you'd prefer.