From 3285ab2a2ac27ab2d461f40daea617e485b76ed4 Mon Sep 17 00:00:00 2001 From: Alex Standiford Date: Wed, 29 Apr 2026 19:25:34 -0400 Subject: [PATCH] Hydrate from attributes + ids on create() instead of post-insert read The post-insert read in WithDatastoreHandlerMethods::create() races read-replicas on hosts with write/read-split routers (ProxySQL, MaxScale, RDS Proxy, Aurora, Cloudways Autonomous, etc.). The router routes the autocommit INSERT to master and the immediately-following autocommit SELECT to a replica that hasn't yet received the write, producing RecordNotFoundException after a successful insert. Reproduced live on a customer environment: 14/2200 sequential creates failed (~0.6%), all on the same client connection id, all with LAST_INSERT_ID() returning the right id, all recovering within 10-50ms. Wrapping in an explicit transaction eliminated the failure entirely (because routers pin transactional clients to one backend), but adding transactions to every create has its own trade-offs - and the underlying problem shows up on any backend with eventual consistency (Postgres+pgpool, MongoDB read preferences, Cassandra, search indexes, REST APIs behind CDNs). The cleanest fix is to remove the post-insert read entirely. We already have the data the caller passed in, plus the new identity from the insert response. Construct the model from those in PHP and skip the round-trip. What changed: - Column gains an optional withPhpDefault(callable) setter. The callable is invoked at create time for any column the caller didn't provide, so the in-memory model carries the same value the DB would have defaulted in. Backward compatible: existing columns without a phpDefault behave identically. - DateCreatedFactory and DateModifiedFactory now provide phpDefaults that return the current MySQL-format timestamp. The DB-side DEFAULT CURRENT_TIMESTAMP stays in place as belt-and-suspenders coverage for inserts that bypass the framework. - WithDatastoreHandlerMethods::create() applies phpDefaults to the attributes array, runs the insert, and hydrates the model via modelAdapter->toModel( array_merge(attributes, ids)). No second SELECT. Cache pre-warm via the existing cacheItems() so subsequent reads short-circuit through the cache the same way they do today. The new contract: any column whose value should appear in the post-create() model must either be passed by the caller or come from a column factory that provides a phpDefault. Trigger-set values, generated columns, and DB defaults without a matching phpDefault won't be reflected in the in-memory model - they'll need an explicit refresh by the caller if they're load-bearing. Tests cover: - Column.withPhpDefault / getPhpDefault contract - DateCreatedFactory and DateModifiedFactory produce a working phpDefault - create() hydrates without calling query() (no readback) - create() applies phpDefaults for missing columns - Caller-provided values win over phpDefaults Closes the underlying race that motivated phpnomad/wordpress-integration#30 and phpnomad/mysql-db-integration#12 - both can stay closed. --- lib/Factories/Column.php | 41 ++++- lib/Factories/Columns/DateCreatedFactory.php | 6 +- lib/Factories/Columns/DateModifiedFactory.php | 6 +- lib/Traits/WithDatastoreHandlerMethods.php | 39 ++++- tests/Unit/Factories/ColumnTest.php | 47 ++++++ .../Columns/DateCreatedFactoryTest.php | 31 ++++ .../Columns/DateModifiedFactoryTest.php | 31 ++++ .../WithDatastoreHandlerMethodsTest.php | 141 ++++++++++++++++-- 8 files changed, 321 insertions(+), 21 deletions(-) create mode 100644 tests/Unit/Factories/ColumnTest.php create mode 100644 tests/Unit/Factories/Columns/DateCreatedFactoryTest.php create mode 100644 tests/Unit/Factories/Columns/DateModifiedFactoryTest.php diff --git a/lib/Factories/Column.php b/lib/Factories/Column.php index b6eb0d1..fdc82a4 100644 --- a/lib/Factories/Column.php +++ b/lib/Factories/Column.php @@ -9,6 +9,11 @@ final class Column protected ?array $typeArgs = null; protected array $attributes = []; + /** + * @var callable|null + */ + protected $phpDefault = null; + /** * @param string $name * @param string $type @@ -60,4 +65,38 @@ public function getAttributes(): array { return $this->attributes; } -} \ No newline at end of file + + /** + * Provides a PHP-side default value for this column. + * + * When set, the datastore layer fills in this value at create time for any + * insert that does not already include the column. The value flows into the + * INSERT and into the in-memory model that `create()` returns, which removes + * the need for a post-insert read-back to capture DB-generated defaults. + * + * Pair this with the matching DB-side DEFAULT (e.g. `DEFAULT CURRENT_TIMESTAMP`) + * for belt-and-suspenders coverage when rows are inserted outside the framework. + * + * The callable receives no arguments and should return a value already in the + * shape the column expects (e.g. a MySQL-format datetime string for a TIMESTAMP). + * + * @param callable():mixed $default + * @return self + */ + public function withPhpDefault(callable $default): self + { + $this->phpDefault = $default; + + return $this; + } + + /** + * Returns the PHP-side default callable, or null if none is set. + * + * @return callable|null + */ + public function getPhpDefault(): ?callable + { + return $this->phpDefault; + } +} diff --git a/lib/Factories/Columns/DateCreatedFactory.php b/lib/Factories/Columns/DateCreatedFactory.php index a2bfd21..6a3960f 100644 --- a/lib/Factories/Columns/DateCreatedFactory.php +++ b/lib/Factories/Columns/DateCreatedFactory.php @@ -2,6 +2,7 @@ namespace PHPNomad\Database\Factories\Columns; +use DateTimeImmutable; use PHPNomad\Database\Factories\Column; use PHPNomad\Database\Interfaces\CanConvertToColumn; @@ -9,6 +10,7 @@ class DateCreatedFactory implements CanConvertToColumn { public function toColumn(): Column { - return new Column('dateCreated', 'TIMESTAMP', null, 'NOT NULL DEFAULT CURRENT_TIMESTAMP'); + return (new Column('dateCreated', 'TIMESTAMP', null, 'NOT NULL DEFAULT CURRENT_TIMESTAMP')) + ->withPhpDefault(static fn (): string => (new DateTimeImmutable())->format('Y-m-d H:i:s')); } -} \ No newline at end of file +} diff --git a/lib/Factories/Columns/DateModifiedFactory.php b/lib/Factories/Columns/DateModifiedFactory.php index 951de11..2a8d174 100644 --- a/lib/Factories/Columns/DateModifiedFactory.php +++ b/lib/Factories/Columns/DateModifiedFactory.php @@ -2,6 +2,7 @@ namespace PHPNomad\Database\Factories\Columns; +use DateTimeImmutable; use PHPNomad\Database\Factories\Column; use PHPNomad\Database\Interfaces\CanConvertToColumn; @@ -9,6 +10,7 @@ class DateModifiedFactory implements CanConvertToColumn { public function toColumn(): Column { - return new Column('dateModified','TIMESTAMP', null, 'NOT NULL DEFAULT CURRENT_TIMESTAMP'); + return (new Column('dateModified', 'TIMESTAMP', null, 'NOT NULL DEFAULT CURRENT_TIMESTAMP')) + ->withPhpDefault(static fn (): string => (new DateTimeImmutable())->format('Y-m-d H:i:s')); } -} \ No newline at end of file +} diff --git a/lib/Traits/WithDatastoreHandlerMethods.php b/lib/Traits/WithDatastoreHandlerMethods.php index 866f8c7..8c7908a 100644 --- a/lib/Traits/WithDatastoreHandlerMethods.php +++ b/lib/Traits/WithDatastoreHandlerMethods.php @@ -148,19 +148,50 @@ public function create(array $attributes): DataModel $this->maybeThrowForDuplicateUniqueFields($attributes); + // Apply PHP-side defaults so the values that land in the DB also land + // in the in-memory model we hand back. This eliminates the post-insert + // read-back, which is the operation that races read-replicas behind a + // write/read-split router (ProxySQL, MaxScale, Aurora, etc.). + $attributes = $this->applyPhpDefaults($attributes); + $ids = $this->serviceProvider->queryStrategy->insert($this->table, $attributes); - $result = Arr::first($this->getModels([$ids])); + $result = $this->modelAdapter->toModel(Arr::merge($attributes, $ids)); - if(!$result){ - throw new DatastoreErrorException('Failed to create the record'); - } + // Pre-warm the cache so subsequent reads of this record don't have to + // round-trip the DB at all. Same key the framework's getModels() flow + // would have written, so existing read paths transparently pick it up. + $this->cacheItems([$result]); $this->serviceProvider->eventStrategy->broadcast(new RecordCreated($result)); return $result; } + /** + * Fills in PHP-side defaults for any column the table declares with a + * `phpDefault` callable that wasn't already supplied by the caller. + * + * @param array $attributes + * @return array + */ + protected function applyPhpDefaults(array $attributes): array + { + foreach ($this->table->getColumns() as $column) { + $name = $column->getName(); + if (array_key_exists($name, $attributes)) { + continue; + } + $default = $column->getPhpDefault(); + if ($default === null) { + continue; + } + $attributes[$name] = $default(); + } + + return $attributes; + } + /** * Delete all items that fit the specified condition. * diff --git a/tests/Unit/Factories/ColumnTest.php b/tests/Unit/Factories/ColumnTest.php new file mode 100644 index 0000000..c06300e --- /dev/null +++ b/tests/Unit/Factories/ColumnTest.php @@ -0,0 +1,47 @@ +assertNull($column->getPhpDefault()); + } + + public function testWithPhpDefaultStoresAndReturnsCallable(): void + { + $column = (new Column('createdAt', 'TIMESTAMP')) + ->withPhpDefault(static fn () => 'now'); + + $default = $column->getPhpDefault(); + + $this->assertIsCallable($default); + $this->assertSame('now', $default()); + } + + public function testWithPhpDefaultIsFluent(): void + { + $column = new Column('createdAt', 'TIMESTAMP'); + + $returned = $column->withPhpDefault(static fn () => 'now'); + + $this->assertSame($column, $returned); + } + + public function testWithPhpDefaultDoesNotAffectOtherFields(): void + { + $column = (new Column('createdAt', 'TIMESTAMP', null, 'NOT NULL DEFAULT CURRENT_TIMESTAMP')) + ->withPhpDefault(static fn () => 'now'); + + $this->assertSame('createdAt', $column->getName()); + $this->assertSame('TIMESTAMP', $column->getType()); + $this->assertNull($column->getTypeArgs()); + $this->assertSame(['NOT NULL DEFAULT CURRENT_TIMESTAMP'], $column->getAttributes()); + } +} diff --git a/tests/Unit/Factories/Columns/DateCreatedFactoryTest.php b/tests/Unit/Factories/Columns/DateCreatedFactoryTest.php new file mode 100644 index 0000000..6c2e8e5 --- /dev/null +++ b/tests/Unit/Factories/Columns/DateCreatedFactoryTest.php @@ -0,0 +1,31 @@ +toColumn(); + + $this->assertSame('dateCreated', $column->getName()); + $this->assertSame('TIMESTAMP', $column->getType()); + $this->assertSame(['NOT NULL DEFAULT CURRENT_TIMESTAMP'], $column->getAttributes()); + } + + public function testProvidesPhpDefaultThatReturnsMysqlFormatTimestamp(): void + { + $column = (new DateCreatedFactory())->toColumn(); + $default = $column->getPhpDefault(); + + $this->assertIsCallable($default); + + $value = $default(); + + $this->assertIsString($value); + $this->assertMatchesRegularExpression('/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/', $value); + } +} diff --git a/tests/Unit/Factories/Columns/DateModifiedFactoryTest.php b/tests/Unit/Factories/Columns/DateModifiedFactoryTest.php new file mode 100644 index 0000000..513037a --- /dev/null +++ b/tests/Unit/Factories/Columns/DateModifiedFactoryTest.php @@ -0,0 +1,31 @@ +toColumn(); + + $this->assertSame('dateModified', $column->getName()); + $this->assertSame('TIMESTAMP', $column->getType()); + $this->assertSame(['NOT NULL DEFAULT CURRENT_TIMESTAMP'], $column->getAttributes()); + } + + public function testProvidesPhpDefaultThatReturnsMysqlFormatTimestamp(): void + { + $column = (new DateModifiedFactory())->toColumn(); + $default = $column->getPhpDefault(); + + $this->assertIsCallable($default); + + $value = $default(); + + $this->assertIsString($value); + $this->assertMatchesRegularExpression('/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/', $value); + } +} diff --git a/tests/Unit/Traits/WithDatastoreHandlerMethodsTest.php b/tests/Unit/Traits/WithDatastoreHandlerMethodsTest.php index dbe443b..7993904 100644 --- a/tests/Unit/Traits/WithDatastoreHandlerMethodsTest.php +++ b/tests/Unit/Traits/WithDatastoreHandlerMethodsTest.php @@ -18,6 +18,7 @@ public function broadcast(Event $event): void; namespace PHPNomad\Database\Tests\Unit\Traits { use PHPNomad\Cache\Services\CacheableService; +use PHPNomad\Database\Factories\Column; use PHPNomad\Database\Interfaces\ClauseBuilder; use PHPNomad\Database\Interfaces\QueryBuilder; use PHPNomad\Database\Interfaces\QueryStrategy; @@ -26,6 +27,7 @@ public function broadcast(Event $event): void; use PHPNomad\Database\Services\TableSchemaService; use PHPNomad\Database\Tests\TestCase; use PHPNomad\Database\Traits\WithDatastoreHandlerMethods; +use PHPNomad\Datastore\Events\RecordCreated; use PHPNomad\Datastore\Exceptions\DatastoreErrorException; use PHPNomad\Datastore\Exceptions\RecordNotFoundException; use PHPNomad\Datastore\Interfaces\DataModel; @@ -36,36 +38,44 @@ public function broadcast(Event $event): void; class WithDatastoreHandlerMethodsTest extends TestCase { - public function testCreateRethrowsDatastoreErrorsFromPostInsertReread(): void + public function testCreateHydratesFromAttributesWithoutPostInsertRead(): void { $queryStrategy = $this->createMock(QueryStrategy::class); $queryStrategy->expects($this->once()) ->method('insert') + ->with($this->anything(), ['name' => 'Example']) ->willReturn(['id' => 123]); - $queryStrategy->expects($this->once()) - ->method('query') - ->willThrowException(new DatastoreErrorException('Replica read failed')); + // Critical: no post-insert read. This is the operation that races + // read-replicas behind write/read-split routers. + $queryStrategy->expects($this->never())->method('query'); $loggerStrategy = $this->createMock(LoggerStrategy::class); - $loggerStrategy->expects($this->never()) - ->method('logException'); + + $createdModel = new TestModel(123); $eventStrategy = $this->createMock(EventStrategy::class); - $eventStrategy->expects($this->never()) - ->method('broadcast'); + $eventStrategy->expects($this->once()) + ->method('broadcast') + ->with($this->isInstanceOf(RecordCreated::class)); $cacheableService = $this->createMock(CacheableService::class); + $cacheableService->expects($this->never())->method('exists'); $cacheableService->expects($this->once()) - ->method('exists') - ->willReturn(false); + ->method('set') + ->with(['identities' => ['id' => 123], 'type' => TestModel::class], $createdModel); $table = $this->createMock(Table::class); $table->method('getFieldsForIdentity')->willReturn(['id']); + $table->method('getColumns')->willReturn([]); $tableSchemaService = $this->createMock(TableSchemaService::class); $tableSchemaService->method('getUniqueColumns')->willReturn([]); $modelAdapter = $this->createMock(ModelAdapter::class); + $modelAdapter->expects($this->once()) + ->method('toModel') + ->with(['name' => 'Example', 'id' => 123]) + ->willReturn($createdModel); $serviceProvider = new DatabaseServiceProvider( $loggerStrategy, @@ -84,12 +94,119 @@ public function testCreateRethrowsDatastoreErrorsFromPostInsertReread(): void $modelAdapter ); - $this->expectException(DatastoreErrorException::class); - $this->expectExceptionMessage('Replica read failed'); + $result = $handler->create(['name' => 'Example']); + + $this->assertSame($createdModel, $result); + } + + public function testCreateAppliesPhpDefaultsForMissingColumns(): void + { + $queryStrategy = $this->createMock(QueryStrategy::class); + $queryStrategy->expects($this->once()) + ->method('insert') + ->with($this->anything(), [ + 'name' => 'Example', + 'createdAt' => 'php-default-value', + ]) + ->willReturn(['id' => 123]); + $queryStrategy->expects($this->never())->method('query'); + + $loggerStrategy = $this->createMock(LoggerStrategy::class); + $eventStrategy = $this->createMock(EventStrategy::class); + + $createdModel = new TestModel(123); + + $cacheableService = $this->createMock(CacheableService::class); + $cacheableService->expects($this->once())->method('set'); + + $nameColumn = new Column('name', 'VARCHAR', [255]); + $createdAtColumn = (new Column('createdAt', 'TIMESTAMP')) + ->withPhpDefault(static fn () => 'php-default-value'); + + $table = $this->createMock(Table::class); + $table->method('getFieldsForIdentity')->willReturn(['id']); + $table->method('getColumns')->willReturn([$nameColumn, $createdAtColumn]); + + $tableSchemaService = $this->createMock(TableSchemaService::class); + $tableSchemaService->method('getUniqueColumns')->willReturn([]); + + $modelAdapter = $this->createMock(ModelAdapter::class); + $modelAdapter->expects($this->once()) + ->method('toModel') + ->with([ + 'name' => 'Example', + 'createdAt' => 'php-default-value', + 'id' => 123, + ]) + ->willReturn($createdModel); + + $serviceProvider = new DatabaseServiceProvider( + $loggerStrategy, + $queryStrategy, + new DummyQueryBuilder(), + new DummyClauseBuilder(), + $cacheableService, + $eventStrategy + ); + + $handler = new DummyDatastoreHandler( + $serviceProvider, + $table, + $tableSchemaService, + TestModel::class, + $modelAdapter + ); $handler->create(['name' => 'Example']); } + public function testCreateRespectsCallerProvidedValuesOverPhpDefaults(): void + { + $queryStrategy = $this->createMock(QueryStrategy::class); + $queryStrategy->expects($this->once()) + ->method('insert') + ->with($this->anything(), [ + 'createdAt' => 'caller-provided', // not php-default-value + ]) + ->willReturn(['id' => 7]); + + $loggerStrategy = $this->createMock(LoggerStrategy::class); + $eventStrategy = $this->createMock(EventStrategy::class); + $cacheableService = $this->createMock(CacheableService::class); + + $createdAtColumn = (new Column('createdAt', 'TIMESTAMP')) + ->withPhpDefault(static fn () => 'php-default-value'); + + $table = $this->createMock(Table::class); + $table->method('getFieldsForIdentity')->willReturn(['id']); + $table->method('getColumns')->willReturn([$createdAtColumn]); + + $tableSchemaService = $this->createMock(TableSchemaService::class); + $tableSchemaService->method('getUniqueColumns')->willReturn([]); + + $modelAdapter = $this->createMock(ModelAdapter::class); + $modelAdapter->method('toModel')->willReturn(new TestModel(7)); + + $serviceProvider = new DatabaseServiceProvider( + $loggerStrategy, + $queryStrategy, + new DummyQueryBuilder(), + new DummyClauseBuilder(), + $cacheableService, + $eventStrategy + ); + + $handler = new DummyDatastoreHandler( + $serviceProvider, + $table, + $tableSchemaService, + TestModel::class, + $modelAdapter + ); + + $handler->create(['createdAt' => 'caller-provided']); + } + public function testFindFromCompoundIncludesTableAndIdentityWhenRecordIsMissing(): void { $queryStrategy = $this->createMock(QueryStrategy::class);