From d8c33e9ce55253e9feb3cc861f7b109c4dd5a7d5 Mon Sep 17 00:00:00 2001 From: Franck DAKIA Date: Mon, 19 Oct 2026 07:24:13 +0000 Subject: [PATCH] Database: stop leaking query state between statements on a model 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. --- src/Database/Barry/Builder.php | 14 +- src/Database/Barry/Model.php | 45 ++- src/Database/QueryBuilder.php | 107 ++++++ .../Query/BuilderStateIsolationTest.php | 335 ++++++++++++++++++ .../Stubs/BuilderOtherRowModelStub.php | 16 + .../Stubs/BuilderProbeRowModelStub.php | 16 + 6 files changed, 503 insertions(+), 30 deletions(-) create mode 100644 tests/Database/Query/BuilderStateIsolationTest.php create mode 100644 tests/Database/Stubs/BuilderOtherRowModelStub.php create mode 100644 tests/Database/Stubs/BuilderProbeRowModelStub.php diff --git a/src/Database/Barry/Builder.php b/src/Database/Barry/Builder.php index 08fad8f1..b7de7fdb 100644 --- a/src/Database/Barry/Builder.php +++ b/src/Database/Barry/Builder.php @@ -46,12 +46,14 @@ public function eager(string|array $relations): Builder */ public function get(array $columns = []): Model|Collection|null { - $data = parent::get($columns); - - // Read and reset the eager loads now: query() memoizes a shared Builder - // instance, so the list must not leak into the next query on this model. - $eager_loads = $this->eager_loads; - $this->eager_loads = []; + // Read and reset the eager loads even when the query throws, so the + // list never reaches the next query on this builder. + try { + $data = parent::get($columns); + } finally { + $eager_loads = $this->eager_loads; + $this->eager_loads = []; + } if (is_null($data)) { return null; diff --git a/src/Database/Barry/Model.php b/src/Database/Barry/Model.php index 8361d7f5..f6b7259a 100644 --- a/src/Database/Barry/Model.php +++ b/src/Database/Barry/Model.php @@ -236,19 +236,15 @@ public function getConnection(): ?string */ public static function query(): Builder { - if ( - static::$builder instanceof Builder - && static::$builder->getModel() == static::class - ) { - if (DB::getConnectionName() == static::$builder->getAdapterName()) { - return static::$builder; - } - } - - // Reflection action - $reflection = new ReflectionClass(static::class); + // A new builder on every call: one + // builder reused for the whole process replayed what a failed or + // half-built query left in it into the next query on the model. Only + // the class's default properties are cached. static::$builder is + // still assigned below: create() and writeRows() read it. + static $default_properties = []; - $properties = $reflection->getDefaultProperties(); + $properties = $default_properties[static::class] + ??= (new ReflectionClass(static::class))->getDefaultProperties(); if (!isset($properties['table']) || $properties['table'] == null) { $parts = explode('\\', static::class); @@ -356,10 +352,9 @@ public static function first(): array|object|null */ public static function retrieveBy(string $column, mixed $value): Collection { - $model = new static(); - $model->where($column, $value); - - return $model->get(); + // One chain on one builder: query() no longer shares a builder between + // separate calls. + return static::query()->where($column, $value)->get(); } /** @@ -404,14 +399,16 @@ public static function retrieve( $id = (array) $id; $model = new static(); - $model->select($select); - $model->whereIn($model->primary_key, $id); + + // One chain on one builder: query() no longer shares a builder between + // separate calls. + $query = static::query()->select($select)->whereIn($model->primary_key, $id); if (count($id) != 1) { - return $model->get(); + return $query->get(); } - return $model->first(); + return $query->first(); } /** @@ -591,14 +588,14 @@ private function writeRows(Builder $builder): int // Insert information in the database $row_affected = $builder->insert($this->attributes); - // We get a last insertion id value - if (static::$builder->getAdapterName() == 'pgsql') { + // We get a last insertion id value from the builder that inserted + if ($builder->getAdapterName() == 'pgsql') { if ($this->auto_increment) { $sequence = $this->table . "_" . $this->primary_key . '_seq'; - $primary_key_value = static::$builder->getPdo()->lastInsertId($sequence); + $primary_key_value = $builder->getPdo()->lastInsertId($sequence); } } else { - $primary_key_value = static::$builder->getPdo()->lastInsertId(); + $primary_key_value = $builder->getPdo()->lastInsertId(); } if ((int)$primary_key_value == 0) { diff --git a/src/Database/QueryBuilder.php b/src/Database/QueryBuilder.php index 9ecb77a8..aa1384cc 100644 --- a/src/Database/QueryBuilder.php +++ b/src/Database/QueryBuilder.php @@ -1048,6 +1048,22 @@ public function max(string $column): int|float * @return mixed */ private function aggregate($aggregate, $column): mixed + { + try { + return $this->runAggregate($aggregate, $column); + } finally { + $this->resetState(); + } + } + + /** + * The aggregate itself; aggregate() resets the builder around it. + * + * @param $aggregate + * @param string $column + * @return mixed + */ + private function runAggregate($aggregate, $column): mixed { $column = $this->assertSafeIdentifier($column, 'aggregate', true); $sql = 'select ' . $aggregate . '(' . $column . ') from ' . $this->table; @@ -1228,9 +1244,14 @@ public function last(): ?object $where_data_binding = $this->where_data_binding; + // count() resets the whole builder: keep what the final select needs. + $kept = [$this->select, $this->order, $this->limit, $this->as, $this->lock_for_update, $this->shared_lock]; + // We count all. $count = $this->count(); + [$this->select, $this->order, $this->limit, $this->as, $this->lock_for_update, $this->shared_lock] = $kept; + $this->where = $where; $this->where_data_binding = $where_data_binding; @@ -1319,6 +1340,45 @@ public function take(int $limit): QueryBuilder * @throws */ public function get(array $columns = []): array|object|null + { + try { + return $this->runGet($columns); + } finally { + $this->resetState(); + } + } + + /** + * Clear every clause, binding and flag a statement consumes, so nothing a + * statement left behind (failed or not) reaches the next one built on + * this builder. + * + * @return void + */ + private function resetState(): void + { + $this->select = null; + $this->where = null; + $this->where_data_binding = []; + $this->join = null; + $this->limit = null; + $this->group = null; + $this->having = null; + $this->having_data_binding = []; + $this->order = null; + $this->as = null; + $this->first = false; + $this->lock_for_update = false; + $this->shared_lock = false; + } + + /** + * The select itself; get() resets the builder around it. + * + * @param array $columns + * @return array|object|null + */ + private function runGet(array $columns): array|object|null { if (count($columns) > 0) { $this->select($columns); @@ -1421,6 +1481,21 @@ public function jump(int $offset = 0): QueryBuilder * @return int */ public function update(array $data = []): int + { + try { + return $this->runUpdate($data); + } finally { + $this->resetState(); + } + } + + /** + * The update itself; update() resets the builder around it. + * + * @param array $data + * @return int + */ + private function runUpdate(array $data): int { $columns = array_keys($data); @@ -1470,6 +1545,20 @@ public function remove(string $column, mixed $comparator = '=', $value = null): * @return int */ public function delete(): int + { + try { + return $this->runDelete(); + } finally { + $this->resetState(); + } + } + + /** + * The delete itself; delete() resets the builder around it. + * + * @return int + */ + private function runDelete(): int { $sql = 'delete from ' . $this->table; @@ -1522,6 +1611,24 @@ public function decrement(string $column, int $step = 1): int * @return int */ private function incrementAction(string $column, int $step = 1, string $direction = '+') + { + // The bindings were never cleared here before, even on success. + try { + return $this->runIncrementAction($column, $step, $direction); + } finally { + $this->resetState(); + } + } + + /** + * The increment itself; incrementAction() resets the builder around it. + * + * @param string $column + * @param int $step + * @param string $direction + * @return int + */ + private function runIncrementAction(string $column, int $step, string $direction): int { $column = $this->assertSafeIdentifier($column, 'incrementAction'); $sql = 'update ' . $this->table . ' set ' . $column . ' = ' . $column . ' ' . $direction . ' ' . $step; diff --git a/tests/Database/Query/BuilderStateIsolationTest.php b/tests/Database/Query/BuilderStateIsolationTest.php new file mode 100644 index 00000000..4ee8de34 --- /dev/null +++ b/tests/Database/Query/BuilderStateIsolationTest.php @@ -0,0 +1,335 @@ +setValue(null, null); + } + + protected function setUp(): void + { + // The slot is static and survives from one test to the next exactly + // as it survives from one job to the next in a worker: start each + // test from an empty slot so a test only sees its own leak. + (new \ReflectionProperty(Model::class, 'builder'))->setValue(null, null); + + Database::connection('sqlite'); + + Database::statement('DROP TABLE IF EXISTS builder_probe_rows'); + Database::statement( + 'CREATE TABLE builder_probe_rows (id INTEGER PRIMARY KEY, organization_id TEXT NOT NULL,' + . " status TEXT NOT NULL CHECK (status IN ('trial','active','processing','initialized','suspended'))," + . ' external_id TEXT, deleted_at TEXT, hits INTEGER NOT NULL DEFAULT 0)' + ); + Database::statement('DROP TABLE IF EXISTS builder_other_rows'); + Database::statement('CREATE TABLE builder_other_rows (id INTEGER PRIMARY KEY, name TEXT)'); + + Database::table('builder_probe_rows')->insert([ + ['id' => 1, 'organization_id' => 'org-a', 'status' => 'processing'], + ['id' => 2, 'organization_id' => 'org-a', 'status' => 'active'], + ['id' => 3, 'organization_id' => 'org-b', 'status' => 'active'], + ['id' => 4, 'organization_id' => 'org-b', 'status' => 'trial'], + ]); + Database::table('builder_other_rows')->insert([['id' => 1, 'name' => 'other']]); + } + + private static function statusOf(int $id): string + { + return Database::select('SELECT status FROM builder_probe_rows WHERE id = ?', [$id])[0]->status; + } + + public function test_every_query_gets_its_own_builder() + { + $this->assertNotSame(ProbeRow::query(), ProbeRow::query()); + } + + /** + * A job's outcome write fails (here a CHECK violation), its catch block + * then releases the claim on the same model. The failed UPDATE used to + * leave its bindings in the shared builder, so the recovery UPDATE died + * with HY093 (wrong parameter count). + */ + public function test_a_failed_update_does_not_break_the_recovery_query_on_the_same_model() + { + $row = ProbeRow::where('id', 1)->first(); + $row->status = 'bogus'; + + try { + $row->persist(); + $this->fail('the CHECK constraint should reject the update'); + } catch (Throwable) { + // the job's catch block + } + + $released = ProbeRow::where('id', 1) + ->where('status', 'processing') + ->update(['status' => 'initialized']); + + $this->assertSame(1, $released); + $this->assertSame('initialized', self::statusOf(1)); + } + + /** + * The failure was sticky: every later query on the model failed too, + * until a query on ANOTHER model class swapped the single static slot. + */ + public function test_the_model_recovers_after_a_failed_update() + { + $row = ProbeRow::where('id', 1)->first(); + $row->status = 'bogus'; + + try { + $row->persist(); + } catch (Throwable) { + } + + $failures = 0; + + foreach ([1, 2, 3] as $id) { + try { + ProbeRow::where('id', $id)->first(); + } catch (Throwable) { + $failures++; + } + } + + $this->assertSame(0, $failures, 'the next three queries on this model all failed'); + } + + /** + * A chain that throws while it is being BUILT (where() with a null value + * throws) left `organization_id = ?` bound to org A in the builder. The + * next query, for org B, was silently ANDed with it. + */ + public function test_a_chain_that_throws_while_building_does_not_narrow_the_next_query() + { + try { + ProbeRow::where('organization_id', 'org-a')->where('external_id', null)->first(); + $this->fail('where() with a null value should throw'); + } catch (Throwable) { + } + + $rows = ProbeRow::where('organization_id', 'org-b')->get(); + $this->assertCount(2, $rows, "org B rows went missing: the query inherited org A's where"); + + try { + ProbeRow::where('organization_id', 'org-a')->where('external_id', null)->first(); + } catch (Throwable) { + } + + $updated = ProbeRow::where('id', 3)->update(['status' => 'suspended']); + $this->assertSame(1, $updated, "the update on org B's row silently hit 0 rows"); + } + + /** + * Same leak with an OR in the stale clause: the next UPDATE scoped to + * org B became `organization_id = 'org-a' or status = 'trial' and + * organization_id = 'org-b'` and rewrote org A's rows. + */ + public function test_a_stale_or_clause_does_not_widen_an_update_to_another_scope() + { + try { + ProbeRow::where('organization_id', 'org-a') + ->orWhere('status', 'trial') + ->orderBy('created at') // unsafe identifier: throws before execution + ->get(); + $this->fail('orderBy() should reject the identifier'); + } catch (Throwable) { + } + + ProbeRow::where('organization_id', 'org-b')->update(['status' => 'suspended']); + + $this->assertSame('processing', self::statusOf(1), 'org A row 1 was rewritten by an org B update'); + $this->assertSame('active', self::statusOf(2), 'org A row 2 was rewritten by an org B update'); + } + + /** + * A failed first() left `first = true`: the next get() returned ONE model + * instead of a Collection, and a foreach over it ran zero times. + */ + public function test_a_failed_first_does_not_turn_the_next_get_into_a_single_row() + { + try { + ProbeRow::whereNull('no_such_column')->first(); + $this->fail('the unknown column should fail'); + } catch (Throwable) { + } + + $rows = ProbeRow::whereNull('deleted_at')->get(); + + $this->assertInstanceOf(Collection::class, $rows); + $this->assertCount(4, $rows); + } + + /** + * Defence in depth on a builder that IS reused on purpose (a local + * variable): a failed aggregate leaves nothing behind. + */ + public function test_a_failed_count_leaves_nothing_on_a_reused_builder() + { + $query = Database::table('builder_probe_rows'); + + try { + $query->where('organization_id', 'org-a')->count('no_such_column'); + $this->fail('the unknown column should fail'); + } catch (Throwable) { + } + + $this->assertCount(2, $query->where('organization_id', 'org-b')->get()); + } + + /** + * An aggregate is a terminal statement: the order and row lock it does + * not use no longer survive it into the next select on the builder. + */ + public function test_a_count_leaves_no_order_or_lock_behind() + { + $query = Database::table('builder_probe_rows'); + + $this->assertSame(4, (int) $query->orderBy('id', 'desc')->lockForUpdate()->count()); + + // SQLite has no FOR UPDATE: a leaked lock flag makes this a syntax error. + $rows = $query->get(); + + $this->assertSame(1, (int) $rows[0]->id, 'the order of the count leaked into the next select'); + } + + /** + * increment()/decrement() never cleared their bindings, even on success: + * the second increment on the same builder carried the first one's id. + */ + public function test_increment_does_not_carry_its_bindings_into_the_next_statement() + { + $query = Database::table('builder_probe_rows'); + + try { + $query->where('id', 2)->increment('hits'); + $query->where('id', 3)->increment('hits'); + } catch (Throwable) { + } + + $hits = Database::select('SELECT id, hits FROM builder_probe_rows ORDER BY id'); + + $this->assertSame([0, 1, 1, 0], array_map(fn ($row) => (int) $row->hits, $hits)); + } + + /** + * A failed eager-loaded get() left its relation list on the builder: the + * next get() on it tried to load a relation nobody asked for. + */ + public function test_a_failed_eager_load_does_not_reach_the_next_get() + { + $query = ProbeRow::query()->eager('noSuchRelation'); + + try { + $query->whereNull('no_such_column')->get(); + $this->fail('the unknown column should fail'); + } catch (Throwable) { + } + + $this->assertCount(4, $query->get()); + } + + /** + * retrieve()/retrieveBy() used to build their query in separate calls on + * the shared builder; they now chain on one builder. + */ + public function test_retrieve_and_retrieve_by_still_work() + { + $one = ProbeRow::retrieve(3); + $this->assertInstanceOf(ProbeRow::class, $one); + $this->assertSame('org-b', $one->organization_id); + + $narrow = ProbeRow::retrieve(2, ['id', 'status']); + $this->assertSame(['id' => 2, 'status' => 'active'], $narrow->toArray()); + + $many = ProbeRow::retrieve([1, 4]); + $this->assertInstanceOf(Collection::class, $many); + $this->assertCount(2, $many); + + $this->assertNull(ProbeRow::retrieve(99)); + + $this->assertCount(2, ProbeRow::retrieveBy('organization_id', 'org-b')); + } + + /** + * paginate() and last() run a count() next to their select and keep what + * that select needs across it. + */ + public function test_paginate_and_last_still_work() + { + $page = ProbeRow::where('organization_id', 'org-a')->paginate(1, 2); + + $this->assertInstanceOf(Pagination::class, $page); + $this->assertSame(2, $page->total()); + $this->assertCount(1, $page->items()); + $this->assertSame(2, (int) $page->items()->first()->id); + + $this->assertSame(2, (int) ProbeRow::where('organization_id', 'org-a')->last()->id); + $this->assertSame( + 3, + (int) ProbeRow::where('organization_id', 'org-b')->orderBy('id', 'desc')->last()->id, + 'last() lost its order by' + ); + } + + /** + * persist() on a new row reads the insert id from the builder it inserted + * with, whichever model ran a query last. + */ + public function test_persist_reads_the_insert_id_from_its_own_builder() + { + $row = ProbeRow::create(['organization_id' => 'org-c', 'status' => 'trial']); + + OtherRow::query(); // the last query() of ANY model now points elsewhere + + $this->assertSame(1, $row->persist()); + $this->assertSame(5, (int) $row->id); + $this->assertSame('trial', self::statusOf(5)); + } + + /** + * Control: a query on another model class in between replaced the static + * slot with a fresh builder, which is why the leak only bit when the next + * query was on the same model. Passes with and without the fix. + */ + public function test_a_query_on_another_model_clears_the_leak() + { + try { + ProbeRow::where('organization_id', 'org-a')->where('external_id', null)->first(); + } catch (Throwable) { + } + + OtherRow::where('id', 1)->first(); + + $this->assertCount(2, ProbeRow::where('organization_id', 'org-b')->get()); + } +} diff --git a/tests/Database/Stubs/BuilderOtherRowModelStub.php b/tests/Database/Stubs/BuilderOtherRowModelStub.php new file mode 100644 index 00000000..053d38a8 --- /dev/null +++ b/tests/Database/Stubs/BuilderOtherRowModelStub.php @@ -0,0 +1,16 @@ +