Skip to content

fix(database): resolve uuid primary keys from positional insert bindings - #2301

Open
radoslav-grencik wants to merge 3 commits into
tempestphp:3.xfrom
radoslav-grencik:fix/uuid-pk-relation-inserts
Open

radoslav-grencik wants to merge 3 commits into
tempestphp:3.xfrom
radoslav-grencik:fix/uuid-pk-relation-inserts

Conversation

@radoslav-grencik

Copy link
Copy Markdown
Contributor

Fixes #2300

…n keys

Iterable inserts (`insert()` with array data) never generated `#[Uuid]`
primary keys — only the object path (`create()`, `save()`) did — so UUID
models inserted this way ended up with a NULL key (error 1364 on MySQL).

Migrations also had no way to define UUID-typed foreign key columns:
`belongsTo()`/`foreignId()` always compile INTEGER, which cannot
reference a UUID primary key (fails on PostgreSQL).

- generate `#[Uuid]` values in the iterable insert path when the key
  is absent (explicitly provided ids are never overwritten)
- add `uuidColumn()`, `belongsToUuid()` and `foreignUuid()` to
  `CreateTableStatement` (CHAR(36) / UUID / TEXT per dialect)
@github-actions

Copy link
Copy Markdown

Benchmark Results

Comparison of fix/uuid-pk-relation-inserts against 3.x (0f478fb487e12a5891738a15a1663fa44c47295b).

Open to see the benchmark results
Benchmark Set Mem. Peak Time Variability
ViewRenderBench(benchPlainHtml) - 22.036mb 0.00% 245.601μs -13.04% ±2.22% +6.60%
ViewRenderBench(benchExpressions) - 24.593mb 0.00% 274.297μs -14.32% ±0.83% -52.21%
ContainerBench(benchRegisterSingletonInstance) - 6.107mb 0.00% 1.497μs +11.61% ±1.20% +25.72%
ContainerBench(benchRegisterClosureSingleton) - 6.454mb 0.00% 1.490μs -5.78% ±3.99% +25.87%

Generated by phpbench against commit 4d7d1ed

@radoslav-grencik

Copy link
Copy Markdown
Contributor Author

What this PR does

Fixes relation auto-inserts for models with #[Uuid] primary keys, and adds the missing migration API needed to actually define UUID relationships.

1. Bug fix: Query::execute() resolved the wrong primary key after insert

Query::execute() looked up the primary key in $bindings[$primaryKeyColumn], but insert bindings are positional — the lookup never matched and execution always fell back to getLastInsertId(). For #[Uuid] primary keys that is the SQLite rowid / MySQL last_insert_id(), so BelongsToMany pivot rows and HasMany/HasOne child rows persisted a meaningless integer as the parent foreign key:

user_roles: user_id = "1"  -- rowid, not the user's UUID

The primary key is now resolved through its column position in the InsertStatement before falling back to getLastInsertId(). The auto-increment path is unchanged.

Covered by new integration tests (UuidPrimaryKeyTest):

  • uuid_primary_key_belongs_to_many_pivot_uses_generated_uuid
  • uuid_primary_key_has_many_uses_generated_uuid_as_foreign_key

2. Iterable insert() now generates #[Uuid] primary keys

Previously only the object path (create(), save()) generated UUID values; the iterable/array path silently omitted the primary key, which surfaced as Field 'id' doesn't have a default value on MySQL. resolveIterableData() now generates a UUID when the model has a #[Uuid] primary key and the key is absent from the given data. Explicitly provided ids are never overwritten.

This also makes UUID primary keys work for relation child rows inserted from arrays (e.g. chapters: [['title' => ...]]), which go through the iterable path internally.

Covered by uuid_primary_key_generated_for_iterable_insert.

3. Feature (bundled): UUID foreign key columns in migrations

While writing the tests it turned out there is no way to define a UUID-typed foreign key column: belongsTo() / foreignId() always compile INTEGER, and varchar(36) FK columns fail on PostgreSQL (character varying vs uuid type mismatch) — this PR's own CI caught it.

This PR therefore also adds:

  • UuidStatement — a non-PK UUID column (CHAR(36) / UUID / TEXT per dialect)
  • CreateTableStatement::uuidColumn(string $name, bool $nullable = false)
  • CreateTableStatement::belongsToUuid(string $local, string $foreign, ...) — UUID counterpart of belongsTo()
  • CreateTableStatement::foreignUuid(string $local, string $constrainedOn, ...) — UUID counterpart of foreignId()

The new integration test migrations use belongsToUuid() / foreignUuid(), so the UUID relation path is exercised end-to-end with real FK constraints on every dialect. Unit tests cover compilation for all three dialects (UuidStatementTest, CreateTableStatementTest::create_a_uuid_foreign_key_constraint).

Notes / known limitations (follow-ups, not part of this PR)

  • Pivot tables need no primary key at all. Pivot rows are always addressed by their FK pair, never by their own id: inserts go through a table-name string (no model metadata), deletes and joins go through the FK columns. The pivot test table therefore declares no PK (matching the Laravel pivot convention). A composite PK was considered but deliberately not added — Tempest's ORM identity is single-column (PrimaryKey property, get($id), route binding), so a composite key would require a redesign far beyond this PR's scope.
  • Batch insert + relations: execute() returns a single PrimaryKey and after callbacks run once, so relations are attached to the first row only. Pre-existing behavior for all PK types, unchanged by this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UUID primary keys: relation auto-inserts persist the internal last-insert id instead of the generated UUID

1 participant