fix: stop silently dropping input that a dialect cannot honour - #18
Open
abnegate wants to merge 2 commits into
Open
fix: stop silently dropping input that a dialect cannot honour#18abnegate wants to merge 2 commits into
abnegate wants to merge 2 commits into
Conversation
An audit for the pattern behind the previous commits turned up its quieter
half. Rather than gating with a throw, these paths accepted input and
discarded it, which is harder to notice and in one case unsafe.
The MongoDB builder dropped Hook\Filter. Hook\Filter::filter() returns a
Condition -- a raw SQL expression plus bindings -- and an operation
document has nowhere to put one, so addHook() stored the hook and nothing
ever read it. A Hook\Filter\Tenant that correctly emits
`WHERE tenant_id IN (?)` on MySQL produced an unscoped MongoDB query
returning every tenant's documents. MongoDB::addHook() now rejects
Hook\Filter and Hook\Join\Filter; Hook\Attribute and Hook\Write are
dialect-neutral and still apply. No test covered this, which is why it
survived; there are four now, in SecurityRegressionTest.
Column::collation() was dead on all five dialects: the setter stored a
value, no compiler ever read it, and the only test asserted the property
round-tripped rather than that any DDL changed. The one COLLATE emission
in the codebase is for index collations, a separate path. It is now
emitted for MySQL, MariaDB, PostgreSQL and SQLite -- PostgreSQL quotes the
name via a quoteCollation() hook -- and removed from ClickHouse (which
collates in ORDER BY) and MongoDB (per collection).
Three more modifiers moved to the dialects that emit them, verified by
diffing DDL with and without each one rather than by reading the source:
unique() MySQL, MariaDB, PostgreSQL, SQLite
after() MySQL, MariaDB, SQLite (PostgreSQL cannot order columns)
comment() all but PostgreSQL, which needs a separate COMMENT ON --
commentOnColumn() already emits it, and a test asserting the
inline form produced nothing becomes a type-level assertion
Schema::compileCreate() branched on `$this instanceof Feature\Partitioning`
to decide whether to append a partitioning clause -- the same gate, one
level up. It now calls a protected compileCreateSuffix() hook that returns
'' by default and is overridden by Trait\Partitioning, so the base class
no longer enumerates the features that might exist. MySQL and PostgreSQL
emit identical partitioning DDL.
Left alone and documented as known gaps, because the library's own column
factories set them internally and unpicking that is a separate change:
unsigned() renders nothing on PostgreSQL and SQLite, and srid(),
autoIncrement() and vector()'s $dimensions are accepted on dialects that
cannot express them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📊 Coverage
Full per-file breakdown in the job summary. |
Contributor
Greptile SummaryThis PR prevents dialect-specific schema and builder inputs from being silently discarded.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "fix(schema): drop after() from SQLite, w..." | Re-trigger Greptile |
Exposing after() on Column\SQLite carried forward a pre-existing bug: base Schema::compileAlter() emits `AFTER <col>`, SQLite uses that base path, and SQLite's ALTER TABLE ADD COLUMN has no AFTER clause, so the statement was a syntax error at execution: ALTER TABLE `t` ADD COLUMN `b` INTEGER NOT NULL AFTER `a` -> SQLSTATE[HY000]: General error: 1 near "AFTER": syntax error after() is now on MySQL and MariaDB only, the two dialects that accept it. My audit missed this because the detector compared generated output with and without each modifier and treated "the string changed" as honoured, which says nothing about whether the result is valid DDL. Two tests close that gap by executing the emitted statement instead of matching it: one runs the SQLite ALTER against an in-memory database and checks the column list via pragma_table_info, the other runs the SQLite COLLATE CREATE. For the same reason the new collation() emission now has integration coverage on MySQL and PostgreSQL, asserting COLLATION_NAME/collation_name from information_schema rather than trusting the emitted string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Stacked on #17 → #16 → #15. This is the audit you asked for: a sweep for more of the pattern from #16, in every form.
Method
Not grep — grep gave me false positives and false negatives (some properties are read in
compileColumnType(), which a per-method scan misses). Instead I diffed generated output with and without each modifier, per dialect. If the DDL is byte-identical, the input was discarded.🔴 Data-isolation bug: MongoDB dropped
Hook\FilterHook\Filter::filter()returns aCondition— a raw SQL expression plus bindings. A MongoDB operation document has nowhere to put one, soaddHook()stored the hook and nothing ever read it:A multi-tenant app on the MongoDB builder got every tenant's documents. Adding a real filter doesn't help — only that filter appears;
tenant_idis nowhere.MongoDB::addHook()now rejectsHook\FilterandHook\Join\Filter.Hook\AttributeandHook\Writeare dialect-neutral and still apply (verified). No test covered this, which is why it survived — there are four now, inSecurityRegressionTest.The deeper fix is to make
Hook\FilterreturnQueryobjects instead of a SQLCondition, which would let MongoDB honour it. That's a public-contract change and belongs in its own PR — flagging it rather than doing it here.Dead API:
collation()did nothing, anywhereThe setter stored a value, no compiler read it, and the test asserted only that the property round-tripped:
The single
COLLATEemission in the codebase is for index collations — an unrelated path. It's now emitted where it's supported, and gone where it isn't:Modifiers moved to the dialects that emit them
collation()unique()after()comment()PostgreSQL's inline
comment()had a pre-existing test namedtestCreateTableNoInlineCommentasserting the output contained noCOMMENT— the drop was known.commentOnColumn()is the supported path, so the test becomes a type-level assertion plus a check thatCOMMENT ON COLUMNreally is emitted.The
instanceofself-gateSchema::compileCreate()did this:The base class enumerating which features its subclasses might have is the same gate one level up. It now calls a protected
compileCreateSuffix()returning'', overridden byTrait\Partitioning. MySQL and PostgreSQL emit identical partitioning DDL (verified, includingPARTITIONS 4).Deliberately left alone
unsigned()renders nothing on PostgreSQL and SQLite — but via an explicit overridablecompileUnsigned()hook returning''. That's an intentional dialect mapping, not an accident, so I left it and documented it.srid(),autoIncrement(),vector()'s$dimensionsare accepted on dialects that can't express them, but the library's own factories (Table::point(),Table::id(),Trait\Serial, the threevector()methods) call them internally. Moving them needs those factories restructured — separate change.cursorAfter()/cursorBefore(),fetch()andwindow()are silently dropped.cursorAfteris the concerning one — pagination silently doesn't advance. Each needs an implement-or-reject decision I'd rather you weigh in on than pick unilaterally.All four are now in a "Known gaps" note in the README.
Test plan
composer test— 5318 tests, 12364 assertions, all pass (was 5311)composer check— PHPStan level max, no errorscomposer lint— passIntegration tests need Docker and were not run locally; CI covers them.
🤖 Generated with Claude Code