Harden database schema execution and SQLite rebuilds - #8
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.
📝 WalkthroughWalkthroughThe PR centralizes schema blueprint execution and adds driver-specific safety rules. It preserves SQLite schema metadata during rebuilds, tracks nested foreign-key suppression, invalidates unsafe pooled sessions, and adds broad unit and integration coverage. ChangesBlueprint execution and suppression
PostgreSQL and MySQL boundaries
SQLite schema state and rebuilds
Session state and pool recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Blueprint
participant SQLiteBuilder
participant SQLiteProcessor
participant SQLiteDatabase
Blueprint->>SQLiteBuilder: executeBlueprint(Blueprint)
SQLiteBuilder->>SQLiteDatabase: read table and index schema state
SQLiteDatabase-->>SQLiteProcessor: columns and index metadata
SQLiteProcessor-->>SQLiteBuilder: decoded schema-state metadata
SQLiteBuilder->>SQLiteDatabase: execute guarded rebuild statements
SQLiteDatabase-->>SQLiteBuilder: success or failure
🚥 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 centralizes guarded Blueprint execution and strengthens SQLite schema reconstruction, cleanup, and pooled-session recovery.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains in the available follow-up review context. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/database/src/Schema/SQLiteBuilder.php | Adds guarded transactional Blueprint execution, exact foreign-key restoration, catalog-based cleanup, and validated database-file refresh behavior. |
| src/database/src/Schema/Grammars/SQLiteGrammar.php | Reconstructs SQLite tables and indexes from richer authoritative metadata while rejecting unsupported lossy rebuilds. |
| src/database/src/Schema/BlueprintState.php | Tracks exact column, index, constraint, and stored table-definition state across ordered schema mutations. |
| src/database/src/Query/Processors/SQLiteProcessor.php | Decodes lossless SQLite index metadata, including comma-bearing identifiers, collations, ordering, and reconstructibility. |
| src/database/src/Schema/Builder.php | Introduces the shared guarded Blueprint execution boundary and connection-owned nested foreign-key suppression lifecycle. |
| src/database/src/Connection.php | Tracks suppression depth and physical-session trust so failed restoration or leaked scopes can invalidate pooled sessions. |
| src/database/src/Pool/PooledConnection.php | Prevents unknown physical sessions from being returned to the pool or reported as successfully reconnected. |
| src/database/src/Schema/PostgresBuilder.php | Wraps eligible multi-statement Blueprint operations in transactions while preserving online-index restrictions and caller-owned transactions. |
| src/foundation/src/Testing/DatabaseConnectionResolver.php | Discards unsafe cached pooled wrappers and completes cache cleanup before surfacing reset failures. |
Reviews (2): Last reviewed commit: "docs(plans): record schema write-connect..." | Re-trigger Greptile
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 1663-1664: Update the warning in the migration documentation to
state that PostgreSQL defers only DEFERRABLE constraints within a transaction;
non-deferrable foreign keys remain enforced. Clarify that
withoutForeignKeyConstraints() is not a general PostgreSQL foreign-key disabling
operation while preserving the existing SQLite and transaction guidance.
In `@src/database/src/Schema/MySqlBuilder.php`:
- Around line 50-58: Update MySqlBuilder::foreignKeyConstraintsAreEnabled() to
call Connection::scalar() with $useReadPdo set to false, ensuring it reads
@@foreign_key_checks from the write session used by setForeignKeyConstraints().
Adjust the corresponding scalar expectations in DatabaseMySqlBuilderTest to
assert the false argument.
In `@tests/Database/DatabaseMySqlBuilderTest.php`:
- Around line 62-66: Update MySqlBuilder::dropAllTables() to propagate a false
result from the DROP TABLE statement by routing it through executeStatements()
or throwing on failure, while still restoring FOREIGN_KEY_CHECKS. Add a
regression test in DatabaseMySqlBuilderTest that makes statement() return false
and verifies the operation fails.
In `@tests/Integration/Database/Sqlite/DatabaseSchemaBlueprintTest.php`:
- Around line 559-605: Add the #[RequiresDatabase('sqlite', '>=3.37.0')]
attribute to both testRebuildPreservesWithoutRowid() and
testRebuildPreservesStrictTables() so these SQLite 3.37+-specific tests are
skipped on older database versions.
- Around line 992-1022: Update
testSQLiteDoubleQuotedStringFallbackChangesUniqueIndexSemantics to detect the
SQLite SQLITE_DQS compile option via pragma_compile_options before creating the
indices, and skip or assert the expected unsupported behavior when DQS is
disabled. Preserve the existing fallback assertions only for builds that permit
double-quoted string literals, without changing unrelated tests.
In `@tests/Integration/Database/Sqlite/DatabaseSqliteSchemaBuilderTest.php`:
- Around line 302-330: Remove process-global chdir usage from both tests: in
tests/Integration/Database/Sqlite/DatabaseSqliteSchemaBuilderTest.php:302-330,
use the absolute database path for the PDO DSN while continuing to pass the
relative name to SQLiteConnection; in
tests/Integration/Database/Sqlite/DatabaseSqliteSchemaBuilderTest.php:360-393,
verify the temporary directory does not contain a :memory: file without changing
directories.
🪄 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: 01b2559d-d204-44ec-ab6c-efa3af07ae5c
📒 Files selected for processing (30)
docs/plans/2026-08-09-0555-database-schema-execution-safety.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/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/DatabaseMySqlBuilderTest.phptests/Database/DatabasePostgresBuilderTest.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/Sqlite/DatabaseSchemaBlueprintTest.phptests/Integration/Database/Sqlite/DatabaseSqliteSchemaBuilderTest.phptests/Integration/Foundation/Testing/DatabaseTruncationTest.php
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/plans/2026-08-09-0555-database-schema-execution-safety.md (1)
37-43: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winLimit the SQLite atomicity claim to failures before commit.
Foreign-key restoration runs in
finallyafter the DDL transaction completes. A restoration failure can therefore throw after the schema change commits. The connection can be marked unknown, but the committed DDL cannot be rolled back.Line 43 currently overstates the guarantee. Limit it to statement and transaction failures before commit. Document that post-commit restoration failures can leave committed schema changes and require connection invalidation. Add a test for this case to prevent unsafe retries.
This follows the plan’s
finallyrestoration flow and unknown-session handling.Suggested wording
- All SQLite multi-statement Blueprint failures roll back completely. + SQLite statement and transaction failures roll back completely. A foreign-key restoration failure may occur after commit; it throws and invalidates the physical session but cannot undo committed DDL.🤖 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 `@docs/plans/2026-08-09-0555-database-schema-execution-safety.md` around lines 37 - 43, Revise the SQLite atomicity statement to guarantee rollback only for statement or transaction failures occurring before commit. Document that foreign-key restoration in the post-commit finally path may fail after schema changes are committed, requiring connection invalidation and preventing safe retries; add a test covering this restoration-failure case.
🤖 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.
Outside diff comments:
In `@docs/plans/2026-08-09-0555-database-schema-execution-safety.md`:
- Around line 37-43: Revise the SQLite atomicity statement to guarantee rollback
only for statement or transaction failures occurring before commit. Document
that foreign-key restoration in the post-commit finally path may fail after
schema changes are committed, requiring connection invalidation and preventing
safe retries; add a test covering this restoration-failure case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b16ebb5-776b-4818-8039-6f70d8fd45cd
📒 Files selected for processing (15)
docs/plans/2026-08-09-0555-database-schema-execution-safety.mdsrc/boost/docs/migrations.mdsrc/database/src/Schema/Builder.phpsrc/database/src/Schema/Grammars/SQLiteGrammar.phpsrc/database/src/Schema/MySqlBuilder.phpsrc/database/src/Schema/PostgresBuilder.phpsrc/database/src/Schema/SQLiteBuilder.phptests/Database/DatabaseMariaDbSchemaBuilderTest.phptests/Database/DatabaseMySQLSchemaBuilderTest.phptests/Database/DatabaseMySqlBuilderTest.phptests/Database/DatabasePostgresBuilderTest.phptests/Database/DatabasePostgresSchemaBuilderTest.phptests/Database/DatabaseSQLiteBuilderTest.phptests/Database/DatabaseSQLiteSchemaGrammarTest.phptests/Integration/Database/Sqlite/DatabaseSchemaBlueprintTest.php
🚧 Files skipped from review as they are similar to previous changes (5)
- src/boost/docs/migrations.md
- tests/Database/DatabaseSQLiteSchemaGrammarTest.php
- src/database/src/Schema/Grammars/SQLiteGrammar.php
- src/database/src/Schema/SQLiteBuilder.php
- tests/Database/DatabaseSQLiteBuilderTest.php
Summary
This PR fixes several correctness problems in schema execution and SQLite table rebuilds:
falsecould be treated as successful.The public Schema and Blueprint APIs remain Laravel-compatible. The changes are concentrated in schema execution, migrations, database cleanup, and pool release.
Schema execution
Blueprint execution now goes through the schema builder owned by the Blueprint's connection. Statements are compiled once, executed in order, and a
falsestatement result throws with the failed SQL instead of reporting success.Builder::executeBlueprint()is the shared execution boundary. Custom builders remain authoritative, and extension-defined compiler methods retain their existing ordered, unwrapped behavior.PostgreSQL
Supported multi-statement Blueprint operations run in a transaction when the caller is not already in one. Existing transactions keep ownership of commit and rollback.
Online index operations remain unwrapped because PostgreSQL does not allow
CREATE INDEX CONCURRENTLYinside a transaction. Mixed online and ordinary commands are neither split nor reordered.SQLite
Framework-owned multi-statement Blueprints run in one guarded transaction. Rebuilds preserve the incoming foreign-key state and restore it even after failure.
SQLite ignores foreign-key toggles inside an active transaction. An empty-table rebuild can use a savepoint safely, while a populated-table rebuild with foreign keys enabled now fails before mutation rather than risking cascading data loss.
Pretend mode continues to compile and log SQL without changing physical session state.
Exact SQLite schema reconstruction
SQLite table alteration rebuilds now use authoritative schema metadata instead of inferring index behavior from names or comma-separated strings.
The rebuilt schema preserves:
COLLATE BINARYoverrides on non-binary columns;WITHOUT ROWIDandSTRICTtable options.Simple indexes are regenerated through the normal grammar so their SQL stays canonical. Rich indexes retain their stored definition. Rebuilds fail before mutation when SQLite does not expose enough information to preserve behavior exactly, including unsafe same-Blueprint rename and legacy drop cases.
The stored table definition is also checked before rebuilding. Behavior-changing clauses that the grammar cannot reproduce are rejected rather than silently removed.
Two public introspection corrections are intentional:
Foreign-key suppression and pooled sessions
Foreign-key suppression depth now belongs to the database connection, so nested scopes work across separate schema-builder instances.
SQLite, MySQL, and MariaDB read and restore the actual incoming constraint state. PostgreSQL keeps its transaction-only deferred-constraint behavior. Internal restoration uses the physical PDO directly so query callbacks cannot veto cleanup or add internal maintenance to query logs.
If restoration fails or a suppression scope leaks, the physical session is marked unknown. Normal pools replace that session before reuse. Shared in-memory SQLite fails clearly because replacing its only PDO would discard the database.
Test connection cleanup now discards unsafe pooled wrappers and clears all cached references before reporting cleanup failures.
SQLite cleanup and file refresh
dropAllTables()anddropAllViews()now use SQLite's catalog cleanup path for both in-memory and file-backed databases. This keeps the database inode and journal mode intact, updates every live connection, and avoids truncating a database with an active WAL.Table cleanup preserves views, matching the behavior of the other supported drivers. A preserved view becomes usable again after its table is recreated.
Writable-schema mode is restored exactly. Modern SQLite reloads its schema cache with
writable_schema=RESET; older versions use guarded cleanup and invalidation behavior.refreshDatabaseFile()remains an explicit filesystem operation. The no-argument form now rejects active transactions, in-memory databases, and WAL mode, and resolves the canonical main database path for URI and relative-path connections.Compatibility
Testing
The full formatter, static analysis, framework, Testbench, and dogfood checks pass.
Coverage includes unit and real-database tests for SQLite, PostgreSQL, MySQL, and MariaDB, including rollback behavior, command ordering, index and constraint reconstruction, nested foreign-key suppression, pooled-session invalidation, writable-schema restoration, WAL handling, and database truncation.
Summary by CodeRabbit