Repository navigation
feat(schema-engine): require an external shadow database to be empty or consented before reset - #5852
Open
tensordreams wants to merge 7 commits into
Open
tensordreams wants to merge 7 commits into
tensordreams wants to merge 7 commits into
Conversation
Summary by CodeRabbit
WalkthroughThe change adds Pre-merge checks:
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title names the main change: schema-engine now requires an external shadow database to be empty, or requires consent before it is reset. The changeset adds the emptiness gate, the consent flag thr… |
| Description check | |
The description matches the changeset. It explains the external shadow database gate, the P3026 error, the consent wiring through DatasourceUrls and the Wasm options, the sanitizer, the procedures_cou… |
Full details: Title check
-
Explanation
The title names the main change: schema-engine now requires an external shadow database to be empty, or requires consent before it is reset. The changeset adds the emptiness gate, the consent flag threaded through DatasourceUrls, ConnectorParams, and ExternalShadowDatabase, and the P3026 error. The title is specific and matches the primary behavior change.
Full details: Description check
-
Explanation
The description matches the changeset. It explains the external shadow database gate, the P3026 error, the consent wiring through DatasourceUrls and the Wasm options, the sanitizer, the procedures_count accessor, and the tests. It also gives the rationale and the stacking context.
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Commit unit tests in branch
shadow-db-safety-s2
✨ Simplify code
-
- Create PR with simplified code
- Commit simplified code in branch
shadow-db-safety-s2
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Tip
🎯 Customize your risk assessments
-
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
-
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
CodeRabbit Verify
- Run CodeRabbit Verify to test the changes in this pull request.
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
Wasm Query Compiler File Size
|
A shadow database is reset before a migration history is replayed into it, so one that holds data the user cares about is a configuration the engine has to refuse rather than obey. P3026 says so, names the database in question, and points at the two ways out. Naming the database means printing a connection string, which is also where the credentials live, so it goes through a sanitizer: userinfo, the query parameters that carry secrets, and the JDBC credential properties are dropped, while the server, port, database name and connection settings stay. SQL Server strings are parsed as JDBC — the `url` crate reads their whole property list, password included, as an opaque host — and a string that cannot be parsed is replaced by a placeholder rather than echoed back with its secrets in unknown positions. Signed-off-by: Alexey Orlenko's AI Agent <robot@aqrln.net>
…e wire `resetShadowDatabase` rides the `--datasource` JSON that already carries the shadow database URL, and the equivalent option rides the Wasm engine's constructor. Both default to absent-means-no-consent, so callers that know nothing about either keep working unchanged. From there the bit reaches the two places that will need it: `ConnectorParams`, for the connector that holds a shadow database URL of its own, and both `ExternalShadowDatabase` variants, for the callers that hand a shadow database to the dialect. Nothing reads it yet — this is the wiring, not the check. `diff_target_to_dialect` takes the shadow database as one argument rather than the adapter factory, the preview features and the consent bit separately: the three only ever travel together into `ExternalShadowDatabase::DriverAdapter`. Signed-off-by: Alexey Orlenko's AI Agent <robot@aqrln.net>
…tizing it Which syntax a connection string is written in cannot be inferred from the string itself: `SQLSERVER://host;database=x;password=secret` is a perfectly good URL whose entire property list parses as an opaque host, so a URL sanitizer hands the password straight back, and the scheme check that was meant to catch it only matched lower case. The flavour decides instead, the way it does for comparing two connection strings, and every caller knows it. SQL Server strings are read as JDBC whatever the case of their scheme, and SQLite strings are read as the file paths they are — a bare path is no URL, and used to render as the unparseable-connection-string placeholder even though a path holds nothing to hide. Signed-off-by: Alexey Orlenko's AI Agent <robot@aqrln.net>
…nsented A shadow database provided by the user is reset before the migration history is replayed into it. Until now the engine did that to whatever it was pointed at, so a shadow database that had been given a real one's URL, or one holding data somebody cared about, was emptied without a word. It is now described first: anything in it — a table, `_prisma_migrations` included, a view, an enum, a user-defined type, a stored procedure — means the replay only goes ahead if the user said the contents can be destroyed, and otherwise the command stops with P3026 naming the database. After a successful replay the shadow database is reset again, which is what keeps the promise cheap: a command that replays the history several times finds it empty every time after the first, and so does the next command, so there is no consent to remember anywhere. A failed replay leaves its remains behind on purpose — the next run finding them, and asking, is the signal that it failed. Both ways in go through one place: the dialect-level funnel, and the local Prisma Postgres shadow database, which comes with the main database instead of from the user and used to reach the replay without passing the funnel at all. Signed-off-by: Alexey Orlenko's AI Agent <robot@aqrln.net>
…rless build Every arm of the connection string comparison is behind a flavour feature, so a build with none of them enabled has nothing left to compare and no use for the arguments. That build is what `cargo check -p schema-engine-wasm` does by default. Signed-off-by: Alexey Orlenko's AI Agent <robot@aqrln.net>
…t owning it Consent said the contents of the shadow database can go, and on PostgreSQL, MySQL and SQL Server they went: those connectors reset an external shadow database before replaying a migration history into it. SQLite and the Wasm PostgreSQL connector do not, so the tables the user agreed to part with were still there when the replay tried to create its own, and the run died on `table "Cat" already exists` instead of doing what it was told. The reset now happens where consent is read, so it means the same thing everywhere. Both resets — the consented one and the one that leaves the database empty afterwards — fall back to dropping the objects one by one, the way the flavours' own pre-replay resets already did. A shadow database the engine may create and drop tables in without owning its schema is a configuration the tests have covered since long before this, and dropping and recreating the schema fails there. Signed-off-by: Alexey Orlenko's AI Agent <robot@aqrln.net>
The dirty shadow database is a table the tests put there themselves, standing in for whatever a real database holds: replaying a migration history into it is refused with P3026 on the dev path and on the diff path alike, and the table is still there afterwards, which is the whole point. A shadow database holding nothing but `_prisma_migrations` is refused too — a migration history in a database the engine did not just write is either a real database or the remains of a run that crashed. The refusal names the database and not the way in: the diff-path test gives the shadow database a login of its own and asserts that neither the user nor the password reaches the message. With consent the same setup goes through and the shadow database is left empty. SQLite gets its own test for the shape it did not have before — a file holding a table the migration history also creates is refused, and consented to, reset, and replayed into cleanly. Signed-off-by: Alexey Orlenko's AI Agent <robot@aqrln.net>
tensordreams
force-pushed
the
shadow-db-safety-s2
branch
from
August 3, 2026 14:48
57f582c to
28948ad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@libs/user-facing-errors/src/schema_engine.rs`:
- Around line 310-321: Update the P3026 message on ShadowDbNotEmpty to remove
the unsupported --reset-shadow-database CLI flag and describe only the supported
shadow-database reset configuration or migration behavior. Keep the guidance to
use an empty datasource.shadowDatabaseUrl unchanged.
In `@schema-engine/connectors/sql-schema-connector/src/external_shadow_db.rs`:
- Around line 91-146: The procedure-only emptiness check lacks construction
coverage. In schema-engine/sql-schema-describer/src/lib.rs lines 450-453, add a
public SqlSchema::push_procedure(...) constructor matching the existing schema
object insertion helpers. In
schema-engine/connectors/sql-schema-connector/src/external_shadow_db.rs lines
91-146, add a test that creates a namespace, inserts only a procedure via
push_procedure, and asserts holds_no_objects returns false.
In `@schema-engine/connectors/sql-schema-connector/src/sanitize.rs`:
- Around line 83-111: Update sanitize_url and its caller
sanitize_connection_string to parse PostgreSQL/CockroachDB comma-separated
multi-host connection strings with a parser that supports this format, while
preserving credential and secret-query sanitization and the existing fallback
for invalid input. Add a unit test covering a multi-host URL and verifying that
ShadowDbNotEmpty receives a sanitized database label instead of “unparseable
connection string.”
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c4e8b7bb-daf5-41c3-abad-bece49e02302
📒 Files selected for processing (41)
libs/user-facing-errors/src/schema_engine.rsschema-engine/cli/tests/cli_tests.rsschema-engine/commands/src/commands/dev_diagnostic.rsschema-engine/commands/src/commands/diagnose_migration_history.rsschema-engine/commands/src/commands/diff.rsschema-engine/connectors/mongodb-schema-connector/tests/introspection/test_api/mod.rsschema-engine/connectors/mongodb-schema-connector/tests/migrations/test_api.rsschema-engine/connectors/schema-connector/src/connector_params.rsschema-engine/connectors/schema-connector/src/schema_connector.rsschema-engine/connectors/sql-schema-connector/src/external_shadow_db.rsschema-engine/connectors/sql-schema-connector/src/flavour.rsschema-engine/connectors/sql-schema-connector/src/flavour/mssql.rsschema-engine/connectors/sql-schema-connector/src/flavour/mysql.rsschema-engine/connectors/sql-schema-connector/src/flavour/postgres.rsschema-engine/connectors/sql-schema-connector/src/flavour/postgres/connector/native/mod.rsschema-engine/connectors/sql-schema-connector/src/flavour/postgres/connector/native/shadow_db.rsschema-engine/connectors/sql-schema-connector/src/flavour/postgres/connector/wasm/mod.rsschema-engine/connectors/sql-schema-connector/src/flavour/sqlite.rsschema-engine/connectors/sql-schema-connector/src/flavour/sqlite/connector/native/mod.rsschema-engine/connectors/sql-schema-connector/src/flavour/sqlite/connector/wasm/mod.rsschema-engine/connectors/sql-schema-connector/src/lib.rsschema-engine/connectors/sql-schema-connector/src/same_database.rsschema-engine/connectors/sql-schema-connector/src/sanitize.rsschema-engine/core/src/commands/diff_cli.rsschema-engine/core/src/lib.rsschema-engine/core/src/url.rsschema-engine/schema-engine-wasm/src/wasm/engine.rsschema-engine/sql-introspection-tests/src/test_api.rsschema-engine/sql-introspection-tests/tests/cockroachdb/mod.rsschema-engine/sql-introspection-tests/tests/simple.rsschema-engine/sql-introspection-tests/tests/tables/mysql.rsschema-engine/sql-migration-tests/src/multi_engine_test_api.rsschema-engine/sql-migration-tests/src/test_api.rsschema-engine/sql-migration-tests/src/utils.rsschema-engine/sql-migration-tests/tests/errors/error_tests.rsschema-engine/sql-migration-tests/tests/migrations/diff.rsschema-engine/sql-migration-tests/tests/migrations/jsonrpc.rsschema-engine/sql-migration-tests/tests/migrations/mssql.rsschema-engine/sql-migration-tests/tests/migrations/shadow_database_url_configuration.rsschema-engine/sql-migration-tests/tests/single_migration_tests.rsschema-engine/sql-schema-describer/src/lib.rs
Comment on lines
+91
to
+146
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| fn schema_with_a_namespace() -> SqlSchema { | ||
| let mut schema = SqlSchema::default(); | ||
| schema.push_namespace("public".to_owned()); | ||
| schema | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_schema_without_objects_holds_no_objects() { | ||
| assert!(holds_no_objects(&SqlSchema::default())); | ||
| assert!(holds_no_objects(&schema_with_a_namespace())); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_table_counts() { | ||
| let mut schema = schema_with_a_namespace(); | ||
| schema.push_table("Cat".to_owned(), Default::default(), None); | ||
|
|
||
| assert!(!holds_no_objects(&schema)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn the_migrations_table_alone_counts() { | ||
| let mut schema = schema_with_a_namespace(); | ||
| schema.push_table(crate::MIGRATIONS_TABLE_NAME.to_owned(), Default::default(), None); | ||
|
|
||
| assert!(!holds_no_objects(&schema)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_view_counts() { | ||
| let mut schema = schema_with_a_namespace(); | ||
| schema.push_view("Cats".to_owned(), Default::default(), None, None); | ||
|
|
||
| assert!(!holds_no_objects(&schema)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn an_enum_counts() { | ||
| let mut schema = schema_with_a_namespace(); | ||
| schema.push_enum(Default::default(), "Mood".to_owned(), None); | ||
|
|
||
| assert!(!holds_no_objects(&schema)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_user_defined_type_counts() { | ||
| let mut schema = schema_with_a_namespace(); | ||
| schema.push_udt(Default::default(), "Whiskers".to_owned(), None); | ||
|
|
||
| assert!(!holds_no_objects(&schema)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm no push_procedure-style constructor already exists in SqlSchema.
rg -n 'fn push_' schema-engine/sql-schema-describer/src/lib.rs
rg -n 'push_procedure|Procedure' schema-engine/sql-schema-describer/src/lib.rs -A3 -B3Repository: prisma/prisma-engines
Length of output: 2921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SqlSchema relevant definitions =="
sed -n '150,270p' schema-engine/sql-schema-describer/src/lib.rs
sed
sed -n '380,460p' schema-engine/sql-schema-describer/src/lib.rs
sed -n '610,635p' schema-engine/sql-schema-describer/src/lib.rs
echo "== Procedure construction/export references =="
rg -n 'ProcedureId|Procedure\{|get_procedure|procedures_count|holds_no_objects|push_udt|push_enum' schema-engine -g '*.rs' -A2 -B2
echo "== test module =="
sed -n '80,155p' schema-engine/connectors/sql-schema-connector/src/external_shadow_db.rsRepository: prisma/prisma-engines
Length of output: 6366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Push methods and procedure count definitions =="
sed -n '377,456p' schema-engine/sql-schema-describer/img/lib.rs 2>/dev/null || true
sed -n '377,456p' schema-engine/sql-schema-describer/src/lib.rs
sed -n '613,632p' schema-engine/sql-schema-describer/src/lib.rs
echo "== ProcedureId and Procedure references =="
rg -n 'type ProcedureId|struct ProcedureId|ProcedureId|pub struct Procedure|struct Procedure|get_procedure|procedures_count|holds_no_objects|push_udt|push_enum|push_table' schema-engine/connectors schema-engine/sql-schema-describer -g '*.rs' -A2 -B2
echo "== Test module =="
sed -n '88,152p' schema-engine/connectors/sql-schema-connector/src/external_shadow_db.rsRepository: prisma/prisma-engines
Length of output: 39317
Add construction coverage for procedure-only emptiness checks.
holds_no_objects includes procedures_count(), but the test suite cannot reach a schema containing only a stored procedure because Procedure has no public constructor like push_udt. Add a public push_procedure(...) to SqlSchema and a procedure-counts test in external_shadow_db.rs.
📍 Affects 2 files
schema-engine/connectors/sql-schema-connector/src/external_shadow_db.rs#L91-L146(this comment)schema-engine/sql-schema-describer/src/lib.rs#L450-L453
🤖 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 `@schema-engine/connectors/sql-schema-connector/src/external_shadow_db.rs`
around lines 91 - 146, The procedure-only emptiness check lacks construction
coverage. In schema-engine/sql-schema-describer/src/lib.rs lines 450-453, add a
public SqlSchema::push_procedure(...) constructor matching the existing schema
object insertion helpers. In
schema-engine/connectors/sql-schema-connector/src/external_shadow_db.rs lines
91-146, add a test that creates a namespace, inserts only a procedure via
push_procedure, and asserts holds_no_objects returns false.
aqrln
reviewed
Aug 4, 2026
| url.set_password(None).ok()?; | ||
| } | ||
|
|
||
| let params: Vec<(String, String)> = url |
Member
There was a problem hiding this comment.
or even .collect_vec() if we have itertools in dependencies in this crate
Member
There was a problem hiding this comment.
Also, do we really need any params at all for the error message? Isn't just host+port+dbname enough?
aqrln
reviewed
Aug 4, 2026
| feature = "mssql", | ||
| feature = "sqlite" | ||
| )), | ||
| allow(unused_variables) |
Member
There was a problem hiding this comment.
Suggested change
| allow(unused_variables) | |
| expect(unused_variables) |
This branch has not been deployed
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
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.
Layer 2 of the shadow-database safety work, stacked on #5851. Layer 1 refuses a shadow database URL that denotes the main database; this layer protects everything a URL comparison cannot see — driver-adapter shadow connections, DNS aliases,
prisma+postgres://URLs, and plain foreign data: the engine now refuses to reset a non-empty external shadow database without explicit consent, and leaves it empty afterwards.Changes
sql-schema-connector: newexternal_shadow_dbmodule whose helper owns gate → replay → reset-after-use as one unit.sql_schema_from_migration_history(…, UsingExternalShadowDb::Yes)has exactly one caller — this helper — so every flavour and both transports (connection string, driver adapter) inherit the whole sequence; the postgres local-PPG side door routes through it too. Before replaying, the shadow database is described; if it contains user-visible objects (tables including_prisma_migrations, views, enums, UDTs, procedures) and consent was not given, the engine refuses with the new P3026, naming the sanitized shadow database location. With consent, the helper resets before replaying — uniformly on every flavour (SQLite previously had no reset at all: migrations were applied on top of whatever the file held). After a successful replay it resets again, so the shadow database ends empty. Resets use the flavours' ownreset-then-best_effort_resetidiom, so limited-privilege shadow databases (and SQL Server, whosedboschema can never be dropped) keep working.DatasourceUrlsgains serde-defaultedresetShadowDatabase(arrives via the existing--datasourceJSON; absent meansfalse, so older callers are unaffected); the Wasm engine's constructor options gain the equivalent field; the value threads throughConnectorParamsand bothExternalShadowDatabasevariants.sanitize_connection_string(flavour, s)strips userinfo, secret query parameters (api_key,password,sslpassword) and JDBC credentials. It dispatches on the flavour rather than sniffing the scheme: a SQL Server connection string is a valid URL whose entire property list — password included — parses as an opaque host, so a URL-first sanitizer would leak credentials on case variations likeSQLSERVER://. SQLite renders the file path (no credentials to hide).sql-schema-describer: additiveprocedures_count()accessor — without it a shadow database holding only a stored procedure would have described as empty on MySQL and SQL Server._prisma_migrations-only refusal, consent-proceeds with empty-on-exit assertion, and the SQLite shape change pinned in both halves — across postgres, mysql, mssql and sqlite on the dev path, plus diff-path coverage on postgres. Fullsql-migration-testssuite green on all four flavours locally.Why
Consent is per-invocation by construction rather than by bookkeeping: because every successful replay leaves the shadow database empty, a command that replays several times (
migrate dev's diagnose and evaluate steps) finds it empty on every touch after the first — no cross-call state exists anywhere. The describe round-trip is skipped entirely when consent is given, since its answer could not change the outcome. PostgreSQL standalone sequences and installed extensions are deliberately excluded from the emptiness definition —plpgsqlexists on effectively every database and would make the gate always fire.Release-ordering note: P3026's message names the
--reset-shadow-databaseCLI flag, which the prisma-side follow-up PR introduces; this PR should ride the same release train as that one.Part of the shadow-db-safety project (Linear: TML-3117). Base is the #5851 branch; will be rebased onto
mainwhen that PR merges.Linear tickets
Related to TML-3117.