Skip to content

Commit 897c49f

Browse files
authored
chore: ORM-1115 multi schema todos (#5523)
This PR removes various left over ToDo comments related to the multi schema feature. They all seem irrelevant or even incorrect by now. Some explainers: - Some ToDos mention to consider also a list of schemas given through CLI args. The only command where we accept a list of schemas (via `--schemas`) on the CLI is `db pull` and for it is implemented. - We have a potential issue when a user removes a schema from their PSL file. We then won't consider this schema anymore for diffing in the `migrate dev` call. Hence dropping the schema (and its models) will NOT create a migration that drops that schema and related tables on the database. Which in my opinion is a fine limitation. ([see slack](https://prisma-company.slack.com/archives/C0350LQSUF2/p1752062839564359)) - One ToDo was related to CockroachDB but not MultiSchema at all. - One ToDo was incorrectly doubting a query => replaced it with an explainer
1 parent 6ae89da commit 897c49f

8 files changed

Lines changed: 6 additions & 21 deletions

File tree

schema-engine/commands/src/commands/diagnose_migration_history.rs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -137,8 +137,6 @@ pub async fn diagnose_migration_history(
137137
if input.opt_in_to_shadow_database {
138138
let mut dialect = connector.schema_dialect();
139139
let target = ExternalShadowDatabase::DriverAdapter(adapter_factory);
140-
// TODO(MultiSchema): this should probably fill the following namespaces from the CLI since there is
141-
// no schema to grab the namespaces off, in the case of MultiSchema.
142140
let from = migration_schema_cache
143141
.get_or_insert(&applied_migrations, || async {
144142
connector
@@ -164,8 +162,6 @@ pub async fn diagnose_migration_history(
164162

165163
let error_in_unapplied_migration = if !matches!(drift, Some(DriftDiagnostic::MigrationFailedToApply { .. }))
166164
{
167-
// TODO(MultiSchema): Not entirely sure passing no namespaces here is correct. Probably should
168-
// also grab this as a CLI argument.
169165
dialect
170166
.validate_migrations_with_target(&migrations_from_filesystem, namespaces, target)
171167
.await

schema-engine/commands/src/commands/diff.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,6 @@ pub async fn diff(
2222
// In order to properly handle MultiSchema, we need to make sure the preview feature is
2323
// correctly set, and we need to grab the namespaces from the Schema, if any.
2424
// Note that currently, we union all namespaces and preview features. This may not be correct.
25-
// TODO: This effectively reads and parses (parts of) the schema twice: once here, and once
26-
// below, when defining 'from'/'to'. We should revisit this.
2725
let (namespaces, preview_features) =
2826
namespaces_and_preview_features_from_diff_targets(&[&params.from, &params.to])?;
2927

schema-engine/commands/src/commands/evaluate_data_loss.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,9 @@ pub async fn evaluate_data_loss(
2222

2323
let from = migration_schema_cache
2424
.get_or_insert(&input.migrations_list.migration_directories, || async {
25+
// We only consider the namespaces present in the "to" schema aka the PSL file for the introspection of the "from" schema.
26+
// So when the user removes a previously existing namespace from their PSL file we will not introspect that namespace in the database.
2527
let namespaces = dialect.extract_namespaces(&to);
26-
27-
// TODO(MultiSchema): we may need to do something similar to
28-
// namespaces_and_preview_features_from_diff_targets here as well,
29-
// particularly if it's not correctly setting the preview features flags.
3028
connector
3129
.schema_from_migrations(&migrations_from_directory, namespaces)
3230
.await

schema-engine/commands/src/commands/schema_push.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,9 @@ pub async fn schema_push(input: SchemaPushInput, connector: &mut dyn SchemaConne
2424
let dialect = connector.schema_dialect();
2525

2626
let to = dialect.schema_from_datamodel(sources)?;
27+
// We only consider the namespaces present in the "to" schema aka the PSL file for the introspection of the "from" schema.
28+
// So when the user removes a previously existing namespace from their PSL file we will not modify that namespace in the database.
2729
let namespaces = dialect.extract_namespaces(&to);
28-
29-
// TODO(MultiSchema): we may need to do something similar to
30-
// namespaces_and_preview_features_from_diff_targets here as well,
31-
// particulalry if it's not correctly setting the preview features flags.
3230
let from = connector
3331
.schema_from_database(namespaces)
3432
.instrument(tracing::info_span!("Calculate from database"))

schema-engine/connectors/sql-schema-connector/src/flavour/postgres/renderer.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ impl PostgresRenderer {
5050
}
5151

5252
impl SqlRenderer for PostgresRenderer {
53-
// TODO(MultiSchema): We only do alter_sequence on CockroachDB.
53+
// TODO: We only do alter_sequence on CockroachDB.
5454
fn render_alter_sequence(
5555
&self,
5656
sequence_idx: MigrationPair<u32>,

schema-engine/core/src/commands/diagnose_migration_history_cli.rs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,6 @@ pub async fn diagnose_migration_history_cli(
7676
let (drift, error_in_unapplied_migration) = {
7777
if input.opt_in_to_shadow_database {
7878
let dialect = connector.schema_dialect();
79-
// TODO(MultiSchema): this should probably fill the following namespaces from the CLI since there is
80-
// no schema to grab the namespaces off, in the case of MultiSchema.
8179
let from = migration_schema_cache
8280
.get_or_insert(&applied_migrations, || async {
8381
connector
@@ -103,8 +101,6 @@ pub async fn diagnose_migration_history_cli(
103101

104102
let error_in_unapplied_migration = if !matches!(drift, Some(DriftDiagnostic::MigrationFailedToApply { .. }))
105103
{
106-
// TODO(MultiSchema): Not entirely sure passing no namespaces here is correct. Probably should
107-
// also grab this as a CLI argument.
108104
connector
109105
.validate_migrations(&migrations_from_filesystem, namespaces)
110106
.await

schema-engine/core/src/rpc.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@ async fn run_command(
7878
INTROSPECT_SQL => render(executor.introspect_sql(params.parse()?).await),
7979
MARK_MIGRATION_APPLIED => render(executor.mark_migration_applied(params.parse()?).await),
8080
MARK_MIGRATION_ROLLED_BACK => render(executor.mark_migration_rolled_back(params.parse()?).await),
81-
// TODO(MultiSchema): we probably need to grab the namespaces from the params
8281
RESET => render(executor.reset().await),
8382
SCHEMA_PUSH => render(executor.schema_push(params.parse()?).await),
8483
other => unreachable!("Unknown command {}", other),

schema-engine/sql-schema-describer/src/postgres.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -648,7 +648,7 @@ impl<'a> SqlSchemaDescriber<'a> {
648648
}
649649

650650
async fn get_namespaces(&self, sql_schema: &mut SqlSchema, namespaces: &[&str]) -> DescriberResult<()> {
651-
// TODO: this query isn't really useful. We already have namespaces.
651+
// Although we have a list of namespaces we should introspect, we still need to check whether they actually already exist in the database.
652652
let sql = include_str!("postgres/namespaces_query.sql");
653653

654654
let rows = self.conn.query_raw(sql, &[Value::array(namespaces)]).await?;

0 commit comments

Comments
 (0)