fix(database): order foreign key target commands - #9
Closed
binaryfire wants to merge 22 commits into
Closed
Conversation
Route Blueprint execution through the connection-owned schema builder, compile statements once, and fail loudly when a schema statement reports failure. Make foreign-key suppression connection-owned and nest-safe, preserve the incoming MySQL and MariaDB state, bypass application callbacks for physical-session restoration, and invalidate leaked or failed session state before pooled reuse. Reject pooled reconnects that remain unsafe, including shared in-memory SQLite sessions that cannot be replaced without losing their database, and cover execution order, failure handling, nesting, restoration, pool reset, and real database behavior.
Wrap supported multi-statement Blueprint operations in a database transaction while preserving caller-owned transactions, runtime grammar opt-outs, and framework command ordering. Keep online index operations unwrapped because PostgreSQL forbids concurrent index creation inside a transaction, and leave extension-defined compilers on the existing ordered execution path. Cover rollback, nested transaction ownership, every online index form, grammar extensions and overrides, raw compilation, and real PostgreSQL constraint-suppression nesting.
Execute framework-owned multi-statement Blueprints inside guarded SQLite transactions while preserving foreign-key state, caller transactions, pretend mode, command order, and extension compiler behavior. Round-trip index identity and semantics through authoritative SQLite metadata, including expression and partial indexes, collations, descending order, constraint-backed indexes, comma-bearing identifiers, column renames, table options, and supported constraint clauses. Fail before mutation when SQLite metadata cannot reconstruct the original behavior safely. Replace live database-file truncation with guarded catalog cleanup, preserve views during table wipes, reload schema state safely across SQLite versions, and make explicit database-file refresh reject active transactions, in-memory databases, and WAL mode. Add focused unit and real-engine regressions for rollback, rebuild ordering, exact index and constraint behavior, stored definitions, foreign-key safety, writable-schema restoration, WAL and file handling, and every discovered data-integrity failure.
Teach the test database resolver to discard pooled wrappers whose physical session state became unknown, clear both cached connection entries, and complete all resets before rethrowing the first cleanup failure. Add resolver regressions for discard and failure ordering, plus integration coverage proving DatabaseTruncation preserves an initially disabled SQLite foreign-key state.
Correct the stale claim about Hypervel SQLite defaults and document the transaction boundaries that govern SQLite constraint toggles and PostgreSQL constraint deferral.
Record the verified failure modes and final architecture for Blueprint execution, driver-specific transaction boundaries, exact SQLite index reconstruction, connection-owned foreign-key suppression, pooled-session invalidation, and safe SQLite catalog cleanup. Capture the required integration coverage, compatibility guarantees, performance boundaries, public behavior disclosures, and completed review status so the implementation and future maintenance share one concise source of truth.
Read foreign-key constraint state from the write PDO so nested suppression restores the physical session that schema mutations use. MariaDB inherits the same correction through its MySQL builder base.\n\nRoute drop-all table and view statements through the guarded schema executor. Exact false statement results now surface as failures, while native exceptions, SQL ordering, and foreign-key restoration behavior remain unchanged.\n\nAdd regression coverage for write-session reads, failed cleanup statements, and restoration before error propagation.
Route Schema::hasTable(), SQLite pragma and rebuild probes, stored table definitions, and populated-table guards through the write PDO. This keeps mutation decisions consistent with the physical session and schema they govern when read/write connections differ.\n\nKeep SQLite compile-option discovery on the reader because it is process-wide library metadata, and document that deliberate exception.\n\nGuard PostgreSQL drop-all table, view, type, and domain statements against exact false results without changing their SQL or execution order. Add real split-PDO SQLite regressions and strict call-shape coverage across every supported builder.
Verify WITHOUT ROWID preservation through sqlite_master so the assertion works on the same SQLite versions as the schema introspection path. Keep the reachable STRICT version guard and remove the redundant older-version guard.\n\nReplace compile-option assumptions about double-quoted string fallback with a behavioral DDL probe that covers both indexed-column and partial-predicate positions. Unsupported builds skip only on SQLite's missing-column diagnostic, while all other failures remain visible.
Separate the SQLite and PostgreSQL transaction rules so the guidance cannot be read as applying the same way to both drivers.\n\nDocument that PostgreSQL defers only foreign keys created with deferrable(), only inside a transaction, while other constraints remain enforced.
Record that mutation-governing schema and session state belongs to the write connection, including Schema::hasTable() and SQLite rebuild state.\n\nAdd failed drop-all results and reader/writer divergence to the PR behavior and upstream-defect lists. Refresh the remaining-work wording without turning the plan into durable commit or push authority.
Replace the mock-only SQLite restoration test with a real transaction that commits DDL before a controlled foreign-key restoration failure. Assert that the schema change remains committed, foreign keys remain disabled, the restoration error propagates, and the physical session is marked unknown. Narrow the plan's atomicity guarantee to failures that occur before commit.
…afety Harden database schema execution and SQLite rebuilds
Require native class-constant types in newly written Hypervel code wherever PHP permits them.\n\nMake the same modernization explicit for Laravel package ports so upstream untyped constants are upgraded consistently alongside parameters, return values, and properties.
Promote primary, unique, and ordinary index commands ahead of foreign keys when compiling CREATE blueprints. For ALTER blueprints, move generated fluent targets beside their exact owning columns while preserving authored command order. Keep SQLite ALTER ordering intact because its grammar builds ordered rebuild groups. Preserve generated command identity and metadata, use linear command-list rebuilds, and compare removed column identifiers strictly. Add focused coverage for stable CREATE promotion, explicit ALTER ordering, owner placement, named fluent indexes, command identity, non-target commands, SQLite exclusion, and numeric-looking identifiers.
Use strict comparison when selecting schema commands by name. Command names are strings throughout the schema pipeline, so this preserves existing behavior while removing the final loose comparison from schema command lookup.
Exercise self-referencing primary and unique targets through the real Schema builder on every supported database for both CREATE and ALTER callbacks. Add MariaDB coverage for its supported ordinary-index target behavior. Verify the resulting indexes and foreign keys, accept valid references, reject invalid rows, and confirm rejected inserts leave no data behind. Use strict comparison in the adjacent schema macro assertion while updating the file.
Pin the exact SQL emitted when an SQLite ALTER callback adds fluent primary and unique targets while changing an existing column. The regression proves generated target placement remains disabled for SQLite ALTER, avoiding duplicate rebuild groups and preserving unique-index creation after the single rebuild.
Apply online() to the index definition returned by Blueprint::unique() instead of to the column definition. This matches the documented index-modifier API and ensures PostgreSQL emits CREATE INDEX CONCURRENTLY with the expected transaction behavior.
Document the cross-driver failure, the CREATE and ALTER normalization rules, SQLite's ordered rebuild constraint, command-identity guarantees, and schema-only performance cost. Capture the implementation shape, database and unit verification plan, completion criteria, and rejected alternatives so future maintenance preserves the intended ordering boundary without adding public machinery.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR centralizes schema execution, adds nested foreign-key suppression and session-state recovery, preserves detailed SQLite schema metadata during rebuilds, orders foreign-key target indexes, and adds cross-driver unit and integration coverage. ChangesDatabase schema safety
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Blueprint
participant SchemaBuilder
participant SQLiteBuilder
participant SQLiteGrammar
participant Connection
Blueprint->>SchemaBuilder: executeBlueprint(Blueprint)
SchemaBuilder->>SQLiteBuilder: select SQLite execution path
SQLiteBuilder->>Connection: inspect and suppress foreign keys
SQLiteBuilder->>SQLiteGrammar: compile validated rebuild statements
SQLiteGrammar-->>SQLiteBuilder: return ordered SQL
SQLiteBuilder->>Connection: execute statements and restore session state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR strengthens schema-operation ordering and execution safety across supported database drivers.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/database/src/Schema/Blueprint.php | Stably promotes CREATE target indexes and places generated non-SQLite ALTER indexes beside their owning columns while preserving command identity. |
| src/database/src/Schema/Builder.php | Centralizes guarded blueprint execution and nested foreign-key suppression with restoration of the incoming state. |
| src/database/src/Schema/SQLiteBuilder.php | Adds transaction-aware SQLite blueprint execution, guarded catalog cleanup, and safer database-file refresh behavior. |
| src/database/src/Schema/Grammars/SQLiteGrammar.php | Extends SQLite rebuild compilation to preserve richer index metadata, table options, and command-order semantics. |
| src/database/src/Schema/BlueprintState.php | Tracks exact SQLite schema and index metadata through rename, drop, and rebuild operations. |
| src/database/src/Connection.php | Moves foreign-key suppression ownership onto the connection and strengthens physical-session invalidation and synchronization. |
| src/database/src/Pool/PooledConnection.php | Prevents pooled reuse of physical sessions whose state became untrustworthy during cleanup. |
| src/database/src/Schema/PostgresBuilder.php | Makes eligible multi-statement PostgreSQL blueprint execution transactional while retaining online-index exclusions. |
| src/foundation/src/Testing/DatabaseConnectionResolver.php | Discards cached pooled connections whose session state cannot be trusted after test cleanup. |
Reviews (2): Last reviewed commit: "test(database): preserve promoted comman..." | Re-trigger Greptile
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/Database/DatabaseSchemaBlueprintTest.php (1)
254-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert identity for the moved CREATE unique command.
The test moves the command returned by
unique('code'), but it only checks command names. Assert thatgeneratedUniqueCommandis at index 1 and retainstestMetadata. This prevents a future command reconstruction from passing the CREATE-ordering test.Proposed test addition
$this->assertSame( [ 'create', 'unique', 'index', 'primary', 'foreign', 'foreign', 'custom', 'AutoIncrementStartingValues', 'Comment', 'AutoIncrementStartingValues', 'Comment', 'AutoIncrementStartingValues', 'Comment', ], array_column($blueprint->getCommands(), 'name'), ); + $this->assertSame($blueprint->generatedUniqueCommand, $blueprint->getCommands()[1]); + $this->assertSame('preserved', $blueprint->getCommands()[1]->testMetadata);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Database/DatabaseSchemaBlueprintTest.php` around lines 254 - 282, Update testCreatePromotesForeignKeyTargetsWithoutMovingOtherCommands to capture the command returned by unique('code') as generatedUniqueCommand, then assert it is at index 1 in the blueprint commands and retains its testMetadata, in addition to the existing command-name assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/boost/docs/migrations.md`:
- Around line 1660-1664: Remove the blank line separating the NOTE and WARNING
blockquotes in the migration documentation, keeping both blockquote sections
directly adjacent.
In `@src/database/src/Schema/SQLiteBuilder.php`:
- Around line 44-56: Update the in-transaction guard around SQLiteBuilder’s
table rebuild flow to also detect populated child tables whose foreign keys
reference the rebuild target, not just rows in the target itself. Reuse the
existing schema/foreign-key inspection helpers where available, and throw the
same RuntimeException before executeStatements runs when either the target or
any referencing child table is populated while suppression is required.
---
Nitpick comments:
In `@tests/Database/DatabaseSchemaBlueprintTest.php`:
- Around line 254-282: Update
testCreatePromotesForeignKeyTargetsWithoutMovingOtherCommands to capture the
command returned by unique('code') as generatedUniqueCommand, then assert it is
at index 1 in the blueprint commands and retains its testMetadata, in addition
to the existing command-name assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d9e17e76-223e-49d2-bccc-1116e4653fa9
📒 Files selected for processing (37)
AGENTS.mddocs/plans/2026-08-09-0555-database-schema-execution-safety.mddocs/plans/2026-08-09-2010-database-foreign-key-target-command-ordering.mdsrc/boost/docs/migrations.mdsrc/database/src/Connection.phpsrc/database/src/Pool/PooledConnection.phpsrc/database/src/Query/Processors/SQLiteProcessor.phpsrc/database/src/Schema/Blueprint.phpsrc/database/src/Schema/BlueprintState.phpsrc/database/src/Schema/Builder.phpsrc/database/src/Schema/Grammars/Grammar.phpsrc/database/src/Schema/Grammars/SQLiteGrammar.phpsrc/database/src/Schema/MySqlBuilder.phpsrc/database/src/Schema/PostgresBuilder.phpsrc/database/src/Schema/SQLiteBuilder.phpsrc/foundation/src/Testing/DatabaseConnectionResolver.phpsrc/support/src/Facades/Schema.phptests/Database/DatabaseConnectionTest.phptests/Database/DatabaseMariaDbSchemaBuilderTest.phptests/Database/DatabaseMySQLSchemaBuilderTest.phptests/Database/DatabaseMySqlBuilderTest.phptests/Database/DatabasePostgresBuilderTest.phptests/Database/DatabasePostgresSchemaBuilderTest.phptests/Database/DatabaseSQLiteBuilderTest.phptests/Database/DatabaseSQLiteProcessorTest.phptests/Database/DatabaseSQLiteSchemaGrammarTest.phptests/Database/DatabaseSchemaBlueprintTest.phptests/Database/DatabaseSchemaBuilderTest.phptests/Foundation/Testing/DatabaseConnectionResolverTest.phptests/Integration/Database/MariaDb/DatabaseMariaDbSchemaBuilderTest.phptests/Integration/Database/MySql/DatabaseMySqlSchemaBuilderTest.phptests/Integration/Database/PooledConnectionTest.phptests/Integration/Database/Postgres/PostgresSchemaBuilderTest.phptests/Integration/Database/SchemaBuilderTest.phptests/Integration/Database/Sqlite/DatabaseSchemaBlueprintTest.phptests/Integration/Database/Sqlite/DatabaseSqliteSchemaBuilderTest.phptests/Integration/Foundation/Testing/DatabaseTruncationTest.php
Assert that CREATE target promotion keeps the exact unique-command object at its new position. Also pin the command's custom metadata so a future in-place normalization cannot retain identity while discarding extension attributes.
Collaborator
Author
|
@coderabbitai Thanks for the review. I checked each finding against the implementation. Addressed
No change
The focused Blueprint test and repository diff checks pass. |
|
Tip For best results, initiate chat on the files or code changes.
The new assertions in Your rationale for the unchanged items is clear. The current SQLite guard rejects rebuilds only when the rebuild target has rows. This preserves safe empty-target rebuilds and avoids unnecessary child-table queries. 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
Make schema blueprints reliable when a foreign key and its target primary, unique, or ordinary index are declared in the same migration callback.
Background
Blueprint::addFluentIndexes()materializes column-level indexes after the migration callback has finished. The schema builder then compiles and executes commands in list order.This creates two failure modes:
PostgreSQL, MySQL, and MariaDB reject the affected forms according to their foreign-key requirements. SQLite does not have the same command model: CREATE constraints are compiled into the table definition, while ALTER commands feed an ordered table-rebuild state machine.
Implementation
CREATE blueprints now use one stable pass to move
primary,unique, and ordinaryindexcommands ahead of the first foreign key. The implementation moves only target commands. Other commands keep their authored relative order, including custom commands placed after a foreign key.For non-SQLite ALTER blueprints, generated fluent targets are associated with their exact owning
ColumnDefinitionbefore columns becomeaddorchangecommands. The same command objects are moved immediately after their owners, preserving subclass and macro metadata. Explicit ALTER commands remain in callback order.SQLite ALTER is deliberately excluded from target placement so its ordered rebuild grouping remains unchanged.
The change also:
online()is applied to an index definition.No public API, configuration, migration workaround, or compatibility layer is added. The extra work is bounded linear processing during schema compilation and does not affect application query paths.
Testing
Coverage includes:
The complete database-driver suites and the repository
composer fixworkflow pass.Summary by CodeRabbit
New Features
Documentation