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 @@ +