Repository navigation
Database: stop leaking query state between statements on a model - #436
Merged
Merged
Conversation
Model::query() memoized ONE process-wide builder (Model::$builder) and handed it back for every query on the same model class, and QueryBuilder only reset its state on the success path. Whatever a failed statement, or a chain that threw while being built, left behind (bindings, where clause, the first() flag, order, locks, eager loads) was replayed into the next query on that model in the same process: a recovery UPDATE dying with HY093, a query for one scope ANDed with the previous scope's where, an UPDATE widened to another scope's rows by a stale OR, a get() returning one row instead of a Collection. - Model::query() builds a new builder on every call and caches only each class's default properties. - retrieve() and retrieveBy() chain on one builder. - get(), the aggregates, update(), delete() and incrementAction() reset the whole builder in a finally (resetState()), so a failure leaves nothing behind. increment()/decrement() never cleared their bindings before, even on success. - last() keeps its select, order, limit and locks across the count() it runs, as count() used to leave them. - Barry\Builder::get() resets the eager loads in a finally. - Model::writeRows() reads the insert id from the builder it inserted with, not from whichever model ran query() last. BuilderStateIsolationTest covers each case on in-memory SQLite; ten of its fourteen tests fail without this change.
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.
Model::query() memoized ONE process-wide builder (Model::$builder) and handed it back for every query on the same model class, and QueryBuilder only reset its state on the success path. Whatever a failed statement, or a chain that threw while being built, left behind (bindings, where clause, the first() flag, order, locks, eager loads) was replayed into the next query on that model in the same process: a recovery UPDATE dying with HY093, a query for one scope ANDed with the previous scope's where, an UPDATE widened to another scope's rows by a stale OR, a get() returning one row instead of a Collection.
BuilderStateIsolationTest covers each case on in-memory SQLite; ten of its fourteen tests fail without this change.