fix(sql): wrong results, invalid statements and injection points in the SQL layer - #1
Merged
Merged
Conversation
This was referenced Jul 21, 2026
compile_where concatenated every condition into one flat list. Since AND
binds tighter than OR, any query mixing them put Norm's own predicates on
the wrong side of the user's OR:
where("title","a"):or_where("views",">",5)
-> WHERE title = ? OR views > ? AND deleted_at IS NULL
so a title match alone returned soft-deleted rows, and the same state
drives update() and delete(). The correlation predicate of where_has and
the parent key set of include() were defeated the same way, making both
match every row.
Conditions Norm injects itself are now flagged `system`. compile_where
splits the list into contiguous runs, parenthesises a user run containing
an OR, and always joins system predicates with AND. Statement order is
unchanged when no OR is involved.
Operators and sort directions were concatenated into the statement
verbatim while only the column was quoted, so both were injection points
on values an application typically takes from user input:
order("id", "DESC, (SELECT 1)") -> ORDER BY `id` DESC, (SELECT 1)
where("views", "> 0 OR 1=1 -- ", 5)
safe_op and safe_dir now validate them and raise on anything outside the
supported set. They cover where, join ON, having and order.
Two ways a nil reached the binder and corrupted the statement:
where(col, op, value) decided it was the two-arg form by testing
`value == nil`, so where("views", ">", nil) compiled to `views = ?`
bound to the string ">". The form is now chosen from the argument
count, and an explicit nil raises.
In BETWEEN and IN, `params[#params + 1] = nil` is a no-op while the
placeholder is still emitted, so every later binding shifted by one and
the next value silently became a bound. Both now assert.
having() had the same three-arg problem and is fixed the same way.
sql.count and sql.aggregate rebuilt the statement from the table and the
wheres only, and paginate() passed an explicitly stripped state. Any
query filtering on a joined table produced SQL referencing a table it
never joined:
join("users",...):where("users.admin", true):count()
-> SELECT COUNT(*) FROM `posts` WHERE `users`.`admin` = ?
which MySQL rejects with "Unknown column". Pagination broke the same way
as soon as the filter touched the joined table, while the data query of
the same call was correct. The join rendering is now shared by select,
count and aggregate.
These three used d.quote on the whole reference while where, order and
join ON used quote_ref, so a table-qualified column became one quoted
identifier:
select("posts.id") -> SELECT `posts.id`
group_by("users.name") -> GROUP BY `users.name`
sum("posts.views") -> SUM(`posts.views`)
All three are rejected as unknown columns, even though the join
documentation encourages exactly that form.
first() wrote limit into the shared builder state, so the restriction outlived the call: a builder kept around as a base query returned a single row from every later all(), and an explicit limit(5) set before it was overwritten for good. The limit now applies to the state built for that one statement.
sync() produced three statements stock MySQL 8 rejects: `label` VARCHAR -- error 1064, no width `notes` TEXT DEFAULT 'none' -- error 1101, literal default on TEXT/JSON CREATE INDEX IF NOT EXISTS ... -- unsupported clause The dialect now carries index_if_not_exists and defaults_on_text. MySQL falls back to VARCHAR(255) when no length is given, skips a literal DEFAULT on TEXT/JSON columns with a warning (raw() still works), and drops the IF NOT EXISTS guard. Since dropping the guard also drops the idempotency sync() relied on, index statements are marked optional on those dialects so a second sync() logs and continues instead of failing. The index tests only ran on SQLite, which supports all three, so none of this was caught.
JustGodWork
force-pushed
the
pr/sql-and-query-builder
branch
2 times, most recently
from
July 22, 2026 01:00
f772b66 to
9eb89f5
Compare
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
1 participant
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.
Seven fixes in the SQL builder and the query builder. Each commit is standalone and carries its own regression tests.
Wrong results
OR/AND precedence.
compile_wherebuilt one flat condition list, so AND binding tighter than OR put Norm's own predicates on the wrong side of a user OR:A title match alone returned soft-deleted rows, in reads and in the
update()/delete()that share the same state.where_haslost its correlation the same way, so the EXISTS was true for every row, andinclude()lost its parent key set. No public API could group conditions, so there was no way around it.Conditions Norm injects itself are now flagged
system: a user run containing an OR is parenthesised, system predicates always join with AND. Statement order is unchanged when no OR is involved.nil bindings.
params[#params + 1] = nilis a no-op while the placeholder is still emitted, sowhere_between("views", 10, nil)produced three placeholders for two params and the following value silently became a bound.where(col, op, nil)picked the two-arg form and compared the column to the operator string. Both now raise.first() pinned LIMIT 1 on the shared builder state, so a later
all()on the same builder returned a single row, and an explicitlimit(5)set beforehand was lost.Invalid statements
count, aggregates and paginate dropped the joins, leaving the WHERE referencing tables the statement never joined. Filtering a paginated query on a joined table failed while the data query of the same call was correct.
select, group_by and aggregates quoted a dotted reference as a single identifier:
`posts.id`instead of`posts`.`id`, even though the join documentation encourages that form.MySQL DDL. Three statements stock MySQL 8 rejects:
VARCHARwith no width (1064), a literal DEFAULT on TEXT/JSON (1101), andCREATE INDEX IF NOT EXISTS.sync()could not run on MySQL for a model with an unsized string column or any index at all. The index tests only ran on SQLite, which accepts all three.Injection
Operators and ORDER BY directions were concatenated verbatim while only the column was quoted, on values an application typically takes from user input:
Both now go through a whitelist, covering where, join ON, having and order.
Note on index idempotency
Dropping
IF NOT EXISTSalso drops the idempotencysync()relied on, so index statements are marked optional on dialects without it: a secondsync()logs and continues instead of failing the whole schema step. The alternatives would be probinginformation_schemaor leaving indexes to migrations.Suite: 300 passing, 260 before this branch.