Fix Eloquent collision recovery and query timeout placement - #16
Closed
binaryfire wants to merge 7 commits into
Closed
Fix Eloquent collision recovery and query timeout placement#16binaryfire wants to merge 7 commits into
binaryfire wants to merge 7 commits into
Conversation
Route create-or-first collision fallback reads through the write PDO so a configured replica cannot hide the winning row. Verify BelongsToMany pivot collisions against the exact parent, related model, pivot constraints, and morph discriminator before reporting attachment success. Rethrow the original attach violation when that membership cannot be proven. Correct return annotations for create, save, and create-or-retrieve paths that attach pivot rows without hydrating a pivot model. Add focused unit coverage, a SQLite read/write split regression, and shared four-engine collision coverage.
Split raw select assembly from complete-statement decoration so MySQL and MariaDB apply query timeouts exactly once at the executed statement root. Cover unions, exists queries, grouped pagination, locking reads, and retained fragments while preserving each child builder's grammar and table prefix. Normalize Relation and Eloquent subqueries to one Query Builder snapshot before timeout checks, SQL compilation, bindings, or cross-database qualification. Reject timed embedded queries and timed EXPLAIN statements with clear diagnostics, and keep opaque or tableless sources safe during qualification. Align Query Builder's supported queryable input types, remove the obsolete SQLite group-limit fallback, preserve raw PostgreSQL and SQLite DML subselects, and add extensive grammar, builder, and real MySQL/MariaDB enforcement coverage.
Widen Eloquent forwarding methods to the Query, Eloquent, and Relation inputs their Query Builder callees already support, preserving Laravel-style application-facing query composition. Validate relationship constraint timeouts after scope and relation constraints are merged but before the child query is stored or its bindings are added. Apply one relationship-specific diagnostic across withAggregate, withExists, and both whereHas execution strategies. Add family-level coverage for the widened forwarding APIs and regressions for aggregate, exists, and count constraint timeout rejection.
Document query timeouts as outer select-statement limits on MySQL and MariaDB, including the rejection of timed embedded queries and EXPLAIN statements. Explain the caller-owned transaction retry required when repeatable-read snapshots cannot observe a concurrent create-or-first winner. Rename the transaction retry guidance around the broader concurrency errors it actually detects while making clear that unique violations are not retried automatically.
Capture the final reviewed design for truthful Eloquent collision recovery, exact pivot membership proof, statement-owned MySQL and MariaDB timeouts, retained-fragment grammar ownership, and queryable Relation parity. Record the supported transaction boundaries, performance constraints, driver-specific verification matrix, regression coverage, and the rule that opaque cross-database expressions must qualify their own database-owned references.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request corrects Eloquent collision recovery, applies MySQL and MariaDB timeouts to complete select statements, normalizes relations as queryable inputs, updates related type declarations, and adds documentation plus unit and integration coverage. ChangesEloquent collision recovery
Queryable relation normalization
Statement-scoped query timeouts
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant QueryBuilder
participant QueryGrammar
participant MySQLOrMariaDB
QueryBuilder->>QueryGrammar: compile the complete outer select
QueryGrammar->>QueryGrammar: leave embedded selects undecorated
QueryGrammar->>MySQLOrMariaDB: execute the driver-specific timeout statement
MySQLOrMariaDB-->>QueryBuilder: return results or a timeout error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@coderabbitai review |
|
Greptile SummaryThe PR makes Eloquent collision recovery read through the writer and verify exact pivot membership, while moving MySQL and MariaDB timeouts to the executed statement root.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/database/src/Eloquent/Relations/BelongsToMany.php | Adds writer-routed collision reads and exact pivot-membership verification before swallowing attachment uniqueness errors. |
| src/database/src/Eloquent/Relations/HasOneOrManyThrough.php | Routes create-or-first collision recovery through the write connection. |
| src/database/src/Query/Builder.php | Normalizes retained subqueries, rejects child-owned timeouts, and transfers grouped-pagination timeouts to the outer count statement. |
| src/database/src/Query/Grammars/Grammar.php | Separates raw select assembly from complete-statement timeout decoration and uses raw compilation for embedded fragments. |
| src/database/src/Query/Grammars/MySqlGrammar.php | Places the execution-time optimizer hint after the first statement-level SELECT. |
| src/database/src/Query/Grammars/MariaDbGrammar.php | Wraps complete selects with MariaDB’s per-statement timeout syntax. |
| src/database/src/Eloquent/Concerns/QueriesRelationships.php | Rejects relationship constraints carrying their own timeout before embedding or mutating the outer selection. |
| src/database/src/Concerns/ExplainsQueries.php | Rejects timed EXPLAIN calls before compiling or executing invalid statement combinations. |
Reviews (3): Last reviewed commit: "Refine relationship timeout regression c..." | Re-trigger Greptile
Update BelongsToMany type fixtures for create, save, attach, and create-or-retrieve paths that return plain related models without hydrating a pivot property. Keep pivot intersections on models loaded through the relation and narrow mixed paths to their truthful common Role type. This matches the corrected production PHPDoc and prevents static analysis from approving unsafe pivot access.
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/database/src/Query/Builder.php (1)
1756-1757: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Relationis missing from thewhereExistsandunionparameter unions. This PR widenedwhere,whereBetween,orderBy,from,insertUsing, and the join helpers to acceptRelation, andisQueryable()already returnstrueforRelation. The exists and union families were not widened, so they reject relations with aTypeErroreven thoughRelation::toBase()returns the exactBuilderthese methods need.
src/database/src/Query/Builder.php#L1756-L1757: addRelationto the$callbackunion and docblock forwhereExists,orWhereExists,whereNotExists, andorWhereNotExists, or state in the docblock that relations are not supported here.src/database/src/Query/Builder.php#L2708-L2722: addRelationto the$queryunion forunionandunionAll, and normalize it in the existingtoBase()branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/src/Query/Builder.php` around lines 1756 - 1757, Add Relation to the whereExists family callback type unions and docblocks in src/database/src/Query/Builder.php at lines 1756-1757, covering whereExists, orWhereExists, whereNotExists, and orWhereNotExists. Also add Relation to the union and unionAll query unions at src/database/src/Query/Builder.php lines 2708-2722, and pass relations through the existing toBase() normalization branch.tests/Database/DatabaseEloquentBuilderTest.php (1)
1827-1848: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated timeout-rejection assertion into a helper.
Four tests repeat the same snapshot,
try/catch, message assertion, and post-state assertions.tests/Database/DatabaseQueryBuilderTest.phpalready usesassertTimedEmbeddingRejectedBeforeQueryIsEmbeddedfor the same shape. A matching helper here would keep the two suites consistent.♻️ Suggested helper
protected function assertConstraintTimeoutRejected(Builder $builder, Closure $apply): void { $sql = $builder->toSql(); $bindings = $builder->getBindings(); try { $apply(); $this->fail('Expected the relationship constraint timeout to be rejected.'); } catch (InvalidArgumentException $exception) { $this->assertSame( 'A relationship constraint cannot define its own query timeout. Apply the timeout to the outer query instead.', $exception->getMessage() ); } $this->assertSame($sql, $builder->toSql()); $this->assertSame($bindings, $builder->getBindings()); }Also applies to: 2004-2048
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Database/DatabaseEloquentBuilderTest.php` around lines 1827 - 1848, Extract the repeated timeout-rejection setup, exception/message assertion, and builder post-state checks from the affected tests into a shared assertConstraintTimeoutRejected helper, matching the existing assertTimedEmbeddingRejectedBeforeQueryIsEmbedded pattern. Define the helper with the appropriate Builder and Closure parameters, then update testWithCountRejectsConstraintTimeoutBeforeEmbeddingTheConstraint and the other three affected tests to pass their builder and timeout-applying callback through it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/database/src/Query/Builder.php`:
- Around line 1756-1757: Add Relation to the whereExists family callback type
unions and docblocks in src/database/src/Query/Builder.php at lines 1756-1757,
covering whereExists, orWhereExists, whereNotExists, and orWhereNotExists. Also
add Relation to the union and unionAll query unions at
src/database/src/Query/Builder.php lines 2708-2722, and pass relations through
the existing toBase() normalization branch.
In `@tests/Database/DatabaseEloquentBuilderTest.php`:
- Around line 1827-1848: Extract the repeated timeout-rejection setup,
exception/message assertion, and builder post-state checks from the affected
tests into a shared assertConstraintTimeoutRejected helper, matching the
existing assertTimedEmbeddingRejectedBeforeQueryIsEmbedded pattern. Define the
helper with the appropriate Builder and Closure parameters, then update
testWithCountRejectsConstraintTimeoutBeforeEmbeddingTheConstraint and the other
three affected tests to pass their builder and timeout-applying callback through
it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fe62ec8-70d2-4ba8-93de-707abd35f05e
📒 Files selected for processing (27)
docs/plans/2026-08-12-1351-database-eloquent-collision-and-query-timeout-correctness.mdsrc/database/src/Concerns/ExplainsQueries.phpsrc/database/src/Eloquent/Builder.phpsrc/database/src/Eloquent/Concerns/QueriesRelationships.phpsrc/database/src/Eloquent/Relations/BelongsToMany.phpsrc/database/src/Eloquent/Relations/HasOneOrManyThrough.phpsrc/database/src/Query/Builder.phpsrc/database/src/Query/Grammars/Grammar.phpsrc/database/src/Query/Grammars/MariaDbGrammar.phpsrc/database/src/Query/Grammars/MySqlGrammar.phpsrc/database/src/Query/Grammars/PostgresGrammar.phpsrc/database/src/Query/Grammars/SQLiteGrammar.phpsrc/docs/database.mdsrc/docs/eloquent.mdsrc/docs/queries.mdtests/Database/DatabaseEloquentBelongsToManyCreateOrFirstTest.phptests/Database/DatabaseEloquentBuilderTest.phptests/Database/DatabaseEloquentHasManyThroughCreateOrFirstTest.phptests/Database/DatabaseMariaDbQueryGrammarTest.phptests/Database/DatabaseMySqlQueryGrammarTest.phptests/Database/DatabaseQueryBuilderTest.phptests/Integration/Database/EloquentBelongsToManyCreateOrFirstCollisionTest.phptests/Integration/Database/MariaDb/QueryTimeoutTest.phptests/Integration/Database/MySql/QueryTimeoutTest.phptests/Integration/Database/QueryTimeoutTestCase.phptests/Integration/Database/Sqlite/EloquentCreateOrFirstReadWriteTest.phptypes/Database/Eloquent/Relations.php
Consolidate the four relationship timeout rejection tests behind one focused helper while preserving each public API path and the unchanged outer-query assertions.\n\nReplace the plan's implementation-only Relation exclusion rationale with the behavioral reason: an instance-bound relation would produce an uncorrelated exists subquery, while whereHas provides the correlated relationship API. Keep union inputs aligned with Laravel's declared builder contract.
Collaborator
Author
|
@coderabbitai Addressed the review:
The targeted Eloquent builder tests and PHP CS Fixer check pass. |
|
Tip For best results, initiate chat on the files or code changes.
The extracted helper keeps the relationship-timeout tests focused on each API call. The ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
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.
Summary
This change fixes two database correctness issues:
It also aligns Query and Eloquent Builder subquery types with the inputs their implementations already support.
Eloquent collision recovery
Collision fallback reads now use the write PDO so they can observe the row that won a concurrent insert.
BelongsToManyno longer treats every pivot unique violation as proof that the requested attachment already exists. It checks the exact parent and related keys through the existing pivot query, including configured pivot predicates and morph discriminators. If that membership is not visible, the original attach exception is rethrown.The related return annotations now distinguish models loaded through a relation, which carry a hydrated pivot, from models that were created or attached directly and do not.
Statement-owned query timeouts
Select compilation now separates raw select assembly from complete-statement timeout decoration. Only the statement sent to the database receives the timeout:
MAX_EXECUTION_TIMEafter the first statement-levelSELECT.SET STATEMENT max_statement_time=... FORaround the complete select.exists(), grouped pagination counts, and locking selects receive one timeout at the statement root.Timed embedded builders and timed
EXPLAINcalls are rejected with direct diagnostics. Grouped pagination transfers the timeout from the derived-table clone to the outer count statement.Subquery consistency
Relation and Eloquent subqueries are normalized once to the Query Builder snapshot that is actually retained. This avoids repeated global-scope application and ensures timeout inspection, SQL, and bindings all come from the same snapshot.
Retained fragments compile through their builder's grammar so connection-owned behavior such as table prefixes remains correct. Cross-database qualification applies only to plain table-name strings; tableless queries and opaque expressions remain unchanged, and opaque expressions must qualify their own database references.
Query and Eloquent Builder signatures now accept the Query Builder, Eloquent Builder, and Relation inputs handled by their forwarding and queryable paths.
Compatibility and performance
The public Laravel-style API remains intact. This adds no public grammar API, worker-lifetime state, session mutation, generic retry loop, or extra database/network work to ordinary successful queries.
Eloquent subqueries with global scopes now do less work because scopes are applied once. The only added database query is an exact pivot-membership check after a rare unique-constraint collision, where it is required to avoid false success.
Documentation
The database documentation now explains:
EXPLAINstatements are rejected;Verification
Coverage includes focused relation, builder, grammar, and documentation regressions; read/write split behavior; exact pivot collision behavior across supported database engines; and real timeout enforcement on MySQL and MariaDB, including unions,
exists(), and blocked locking reads.The formatter, static analysis, full parallel suite, Testbench suites, and the supported SQLite, MySQL, MariaDB, and PostgreSQL database matrices pass.
Summary by CodeRabbit
New Features
Bug Fixes
EXPLAIN.Documentation