Skip to content

Commit bb63945

Browse files
innocenziinnocenzi
committed
fix(database): handle uninitialized pkey edge case
1 parent 26f8669 commit bb63945

File tree

2 files changed

+93
-142
lines changed

2 files changed

+93
-142
lines changed

‎packages/database/src/Builder/ModelInspector.php‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -435,12 +435,16 @@ public function getPrimaryKeyValue(): ?PrimaryKey
435435
return null;
436436
}
437437

438-
$primaryKey = $this->getPrimaryKey();
438+
$primaryKeyProperty = $this->getPrimaryKeyProperty();
439+
440+
if ($primaryKeyProperty === null) {
441+
return null;
442+
}
439443

440-
if ($primaryKey === null) {
444+
if (! $primaryKeyProperty->isInitialized($this->instance)) {
441445
return null;
442446
}
443447

444-
return $this->instance->{$primaryKey};
448+
return $primaryKeyProperty->getValue($this->instance);
445449
}
446450
}

‎tests/Integration/Database/Builder/IsDatabaseModelTest.php‎

Lines changed: 86 additions & 139 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use DateTimeImmutable;
1010
use Tempest\Database\BelongsTo;
1111
use Tempest\Database\DatabaseMigration;
12+
use Tempest\Database\Exceptions\DeleteStatementWasInvalid;
1213
use Tempest\Database\Exceptions\RelationWasMissing;
1314
use Tempest\Database\Exceptions\ValueWasMissing;
1415
use Tempest\Database\HasMany;
@@ -499,196 +500,115 @@ public function test_delete(): void
499500
$this->assertNotNull(Foo::get($bar->id));
500501
}
501502

502-
public function test_property_with_carbon_type(): void
503+
public function test_delete_via_model_class_with_where_conditions(): void
503504
{
504505
$this->migrate(
505506
CreateMigrationsTable::class,
506-
CreateCarbonModelTable::class,
507+
FooDatabaseMigration::class,
507508
);
508509

509-
$this->container->get(CasterFactory::class)->addCaster(Carbon::class, CarbonCaster::class);
510-
$this->container->get(SerializerFactory::class)->addSerializer(Carbon::class, CarbonSerializer::class);
511-
512-
new CarbonModel(createdAt: new Carbon('2024-01-01'))->save();
510+
$foo1 = Foo::create(bar: 'delete_me');
511+
$foo2 = Foo::create(bar: 'keep_me');
512+
$foo3 = Foo::create(bar: 'delete_me');
513513

514-
$model = CarbonModel::select()->first();
514+
query(Foo::class)
515+
->delete()
516+
->where('bar', 'delete_me')
517+
->execute();
515518

516-
$this->assertTrue($model->createdAt->equalTo(new Carbon('2024-01-01')));
519+
$this->assertNull(Foo::get($foo1->id));
520+
$this->assertNotNull(Foo::get($foo2->id));
521+
$this->assertNull(Foo::get($foo3->id));
517522
}
518523

519-
public function test_two_way_casters_on_models(): void
524+
public function test_delete_via_model_instance_with_primary_key(): void
520525
{
521526
$this->migrate(
522527
CreateMigrationsTable::class,
523-
CreateCasterModelTable::class,
528+
FooDatabaseMigration::class,
524529
);
525530

526-
new CasterModel(
527-
date: new DateTimeImmutable('2025-01-01 00:00:00'),
528-
array_prop: ['a', 'b', 'c'],
529-
enum_prop: CasterEnum::BAR,
530-
)->save();
531+
$foo1 = Foo::create(bar: 'first');
532+
$foo2 = Foo::create(bar: 'second');
533+
$foo1->delete();
531534

532-
$model = CasterModel::select()->first();
533-
534-
$this->assertSame(new DateTimeImmutable('2025-01-01 00:00:00')->format('c'), $model->date->format('c'));
535-
$this->assertSame(['a', 'b', 'c'], $model->array_prop);
536-
$this->assertSame(CasterEnum::BAR, $model->enum_prop);
535+
$this->assertNull(Foo::get($foo1->id));
536+
$this->assertNotNull(Foo::get($foo2->id));
537+
$this->assertSame('second', Foo::get($foo2->id)->bar);
537538
}
538539

539-
public function test_find(): void
540+
public function test_delete_via_model_instance_without_primary_key(): void
540541
{
541542
$this->migrate(
542543
CreateMigrationsTable::class,
543-
CreateATable::class,
544-
CreateBTable::class,
545-
CreateCTable::class,
546-
);
547-
548-
new C(name: 'one')->save();
549-
new C(name: 'two')->save();
550-
551-
/** @var C[] */
552-
$valid = C::find(name: 'one')->all();
553-
554-
$this->assertCount(1, $valid);
555-
$this->assertSame($valid[0]->name, 'one');
556-
557-
$invalid = C::find(name: 'three')->all();
558-
559-
$this->assertCount(0, $invalid);
560-
}
561-
562-
public function test_table_name_overrides(): void
563-
{
564-
$this->assertEquals('base_models', inspect(BaseModel::class)->getTableDefinition()->name);
565-
$this->assertEquals('custom_attribute_table_name', inspect(AttributeTableNameModel::class)->getTableDefinition()->name);
566-
$this->assertEquals('custom_static_method_table_name', inspect(StaticMethodTableNameModel::class)->getTableDefinition()->name);
567-
}
568-
569-
public function test_validation_on_create(): void
570-
{
571-
$this->expectException(ValidationFailed::class);
572-
573-
ModelWithValidation::create(
574-
index: -1,
575-
);
576-
}
577-
578-
public function test_validation_on_update(): void
579-
{
580-
$model = ModelWithValidation::new(
581-
id: new PrimaryKey(1),
582-
index: 1,
583-
);
584-
585-
$this->expectException(ValidationFailed::class);
586-
587-
$model->update(
588-
index: -1,
544+
CreateModelWithoutPrimaryKeyMigration::class,
589545
);
590-
}
591-
592-
public function test_validation_on_new(): void
593-
{
594-
$model = ModelWithValidation::new(
595-
index: 1,
596-
);
597-
598-
$model->index = -1;
599-
600-
$this->expectException(ValidationFailed::class);
601546

547+
$model = new ModelWithoutPrimaryKey(name: 'Frieren', description: 'Elf mage');
602548
$model->save();
603-
}
604549

605-
public function test_skipped_validation(): void
606-
{
607-
try {
608-
inspect(ModelWithValidation::class)->validate(
609-
index: -1,
610-
skip: -1,
611-
);
612-
} catch (ValidationFailed $validationFailed) {
613-
$this->assertStringContainsString(ModelWithValidation::class, $validationFailed->getMessage());
614-
$this->assertStringContainsString(ModelWithValidation::class, $validationFailed->subject);
615-
$this->assertStringContainsString('index', array_key_first($validationFailed->failingRules));
616-
$this->assertInstanceOf(IsBetween::class, Arr\first($validationFailed->failingRules)[0]);
617-
}
550+
$this->expectException(DeleteStatementWasInvalid::class);
551+
$model->delete();
618552
}
619553

620-
public function test_date_field(): void
554+
public function test_delete_via_model_class_without_primary_key(): void
621555
{
622556
$this->migrate(
623557
CreateMigrationsTable::class,
624-
CreateDateTimeModelTable::class,
558+
CreateModelWithoutPrimaryKeyMigration::class,
625559
);
626560

627-
$id = query(DateTimeModel::class)
628-
->insert([
629-
'phpDateTime' => new NativeDateTime('2024-01-01 00:00:00'),
630-
'tempestDateTime' => DateTime::parse('2024-01-01 00:00:00'),
631-
])
561+
query(ModelWithoutPrimaryKey::class)->create(name: 'Himmel', description: 'Hero');
562+
query(ModelWithoutPrimaryKey::class)->create(name: 'Heiter', description: 'Priest');
563+
query(ModelWithoutPrimaryKey::class)->create(name: 'Eisen', description: 'Warrior');
564+
565+
$this->assertCount(3, query(ModelWithoutPrimaryKey::class)->select()->all());
566+
567+
query(ModelWithoutPrimaryKey::class)
568+
->delete()
569+
->where('name', 'Himmel')
632570
->execute();
633571

634-
/** @var DateTimeModel $model */
635-
$model = query(DateTimeModel::class)->select()->where('id', $id)->first();
572+
$remaining = query(ModelWithoutPrimaryKey::class)->select()->all();
573+
$this->assertCount(2, $remaining);
636574

637-
$this->assertSame('2024-01-01 00:00:00', $model->phpDateTime->format('Y-m-d H:i:s'));
638-
$this->assertSame('2024-01-01 00:00:00', $model->tempestDateTime->format('yyyy-MM-dd HH:mm:ss'));
575+
$names = array_map(fn (ModelWithoutPrimaryKey $model) => $model->name, $remaining);
576+
$this->assertContains('Heiter', $names);
577+
$this->assertContains('Eisen', $names);
578+
$this->assertNotContains('Himmel', $names);
639579
}
640580

641-
public function test_model_create_with_has_many_relations(): void
581+
public function test_delete_with_uninitialized_primary_key(): void
642582
{
643583
$this->migrate(
644584
CreateMigrationsTable::class,
645-
CreateTestUserMigration::class,
646-
CreateTestPostMigration::class,
647-
);
648-
649-
$user = TestUser::create(
650-
name: 'Jon',
651-
posts: [
652-
new TestPost('hello', 'world'),
653-
new TestPost('foo', 'bar'),
654-
],
585+
FooDatabaseMigration::class,
655586
);
656587

657-
$this->assertSame('Jon', $user->name);
658-
$this->assertInstanceOf(PrimaryKey::class, $user->id);
659-
660-
$posts = TestPost::select()
661-
->where('test_user_id', $user->id->value)
662-
->all();
588+
$foo = new Foo();
589+
$foo->bar = 'unsaved';
663590

664-
$this->assertCount(2, $posts);
665-
$this->assertSame('hello', $posts[0]->title);
666-
$this->assertSame('world', $posts[0]->body);
667-
$this->assertSame('foo', $posts[1]->title);
668-
$this->assertSame('bar', $posts[1]->body);
591+
$this->expectException(DeleteStatementWasInvalid::class);
592+
$foo->delete();
669593
}
670594

671-
public function test_model_update_with_only_relations(): void
595+
public function test_delete_nonexistent_record(): void
672596
{
673597
$this->migrate(
674598
CreateMigrationsTable::class,
675-
CreateTestUserMigration::class,
676-
CreateTestPostMigration::class,
599+
FooDatabaseMigration::class,
677600
);
678601

679-
$user = TestUser::create(name: 'Frieren');
680-
$user->update(posts: [
681-
new TestPost('hello', 'world'),
682-
]);
602+
$foo = Foo::create(bar: 'test');
603+
$fooId = $foo->id;
683604

684-
$posts = TestPost::select()
685-
->where('test_user_id', $user->id->value)
686-
->all();
605+
// Delete the record
606+
$foo->delete();
687607

688-
$this->assertCount(1, $posts);
689-
$this->assertSame('hello', $posts[0]->title);
690-
$this->assertSame('world', $posts[0]->body);
691-
$this->assertSame('Frieren', $user->name); // Ensure name wasn't changed
608+
// Delete again
609+
$foo->delete();
610+
611+
$this->assertNull(Foo::get($fooId));
692612
}
693613
}
694614

@@ -1100,3 +1020,30 @@ public function down(): ?QueryStatement
11001020
return null;
11011021
}
11021022
}
1023+
1024+
final class ModelWithoutPrimaryKey
1025+
{
1026+
use IsDatabaseModel;
1027+
1028+
public function __construct(
1029+
public string $name,
1030+
public string $description,
1031+
) {}
1032+
}
1033+
1034+
final class CreateModelWithoutPrimaryKeyMigration implements DatabaseMigration
1035+
{
1036+
private(set) string $name = '100-create-model-without-primary-key';
1037+
1038+
public function up(): QueryStatement
1039+
{
1040+
return new CreateTableStatement('model_without_primary_keys')
1041+
->text('name')
1042+
->text('description');
1043+
}
1044+
1045+
public function down(): ?QueryStatement
1046+
{
1047+
return null;
1048+
}
1049+
}

0 commit comments

Comments
 (0)