migration-engine: fix moving tables across namespaces (multiSchema) (#…

…3430)

migration-engine/connectors/sql-migration-connector/src/sql_renderer/postgres_renderer.rs

@@ -205,6 +205,7 @@ impl SqlRenderer for PostgresFlavour {
         render_step(&mut |step| {
             step.render_statement(&mut |stmt| {
                 stmt.push_str("ALTER TABLE ");
+                // TODO(MultiSchema): This only matters for CockroachDB.
                 stmt.push_display(&Quoted::postgres_ident(tables.previous.name()));
                 stmt.push_str(" ALTER PRIMARY KEY USING COLUMNS (");
                 let column_names = tables
@@ -230,6 +231,8 @@ impl SqlRenderer for PostgresFlavour {
                 stmt.push_str("ALTER INDEX ");
                 stmt.push_str(&index_previous_name);
                 stmt.push_str(" RENAME TO ");
+                // Postgres assumes we use the same schema as the previous name's, so we're not
+                // allowed to qualify this identifier.
                 stmt.push_display(&Quoted::postgres_ident(indexes.next.name()));
             })
         })
@@ -319,6 +322,7 @@ impl SqlRenderer for PostgresFlavour {
             let mut out = Vec::with_capacity(before_statements.len() + after_statements.len() + lines.len());
             out.extend(before_statements.into_iter());
             for line in lines {
+                // TODO(MultiSchema): This will need qualifying when implementing CockroachDB.
                 out.push(format!("ALTER TABLE \"{}\" {}", tables.previous.name(), line))
             }
             out.extend(after_statements.into_iter());
@@ -542,6 +546,7 @@ impl SqlRenderer for PostgresFlavour {
         result
     }
 
+    // TODO(MultiSchema): I _think_ this only exists for CockroachDB.
     fn render_rename_table(&self, name: &str, new_name: &str) -> String {
         format!(
             "ALTER TABLE {} RENAME TO {}",

migration-engine/connectors/sql-migration-connector/src/sql_schema_differ.rs

@@ -17,7 +17,7 @@ use crate::{
 };
 use column::ColumnTypeChange;
 use sql_schema_describer::{walkers::ForeignKeyWalker, ColumnId, IndexId};
-use std::collections::HashSet;
+use std::{borrow::Cow, collections::HashSet};
 use table::TableDiffer;
 
 pub(crate) fn calculate_steps(schemas: Pair<&SqlDatabaseSchema>, flavour: &dyn SqlFlavour) -> Vec<SqlMigrationStep> {
@@ -472,8 +472,10 @@ fn push_foreign_key_pair_changes(
 ) {
     // Is the referenced table being redefined, meaning we need to drop and recreate
     // the foreign key?
-    if db.table_is_redefined(fk.previous.referenced_table().name())
-        && !db.flavour.can_redefine_tables_with_inbound_foreign_keys()
+    if db.table_is_redefined(
+        fk.previous.referenced_table().namespace().map(Cow::Borrowed),
+        fk.previous.referenced_table().name().into(),
+    ) && !db.flavour.can_redefine_tables_with_inbound_foreign_keys()
     {
         steps.push(SqlMigrationStep::DropForeignKey {
             foreign_key_id: fk.previous.id,

migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/differ_database.rs

@@ -11,14 +11,16 @@ use std::{
     ops::Bound,
 };
 
+type Table<'a> = (Option<Cow<'a, str>>, Cow<'a, str>);
+
 pub(crate) struct DifferDatabase<'a> {
     pub(super) flavour: &'a dyn SqlFlavour,
     /// The schemas being diffed
     pub(crate) schemas: Pair<&'a SqlDatabaseSchema>,
     /// Namespace name -> namespace indexes.
     namespaces: HashMap<Cow<'a, str>, Pair<Option<NamespaceId>>>,
     /// Table name -> table indexes.
-    tables: HashMap<Cow<'a, str>, Pair<Option<TableId>>>,
+    tables: HashMap<Table<'a>, Pair<Option<TableId>>>,
     /// (table_idxs, column_name) -> column_idxs. BTreeMap because we want range
     /// queries (-> all the columns in a table).
     columns: BTreeMap<(Pair<TableId>, &'a str), Pair<Option<ColumnId>>>,
@@ -92,7 +94,10 @@ impl<'a> DifferDatabase<'a> {
             } else {
                 Cow::Borrowed(table.name())
             };
-            db.tables.insert(table_name, Pair::new(Some(table.id), None));
+            db.tables.insert(
+                (table.namespace().map(Cow::Borrowed), table_name),
+                Pair::new(Some(table.id), None),
+            );
         }
 
         // Then insert all tables from the next schema. Since we have all the
@@ -108,7 +113,10 @@ impl<'a> DifferDatabase<'a> {
             } else {
                 Cow::Borrowed(table.name())
             };
-            let entry = db.tables.entry(table_name).or_default();
+            let entry = db
+                .tables
+                .entry((table.namespace().map(Cow::Borrowed), table_name))
+                .or_default();
             entry.next = Some(table.id);
 
             // Deal with tables that are both in the previous and the next
@@ -226,9 +234,9 @@ impl<'a> DifferDatabase<'a> {
             .filter(move |differ| !self.tables_to_redefine.contains(&differ.table_ids()))
     }
 
-    pub(crate) fn table_is_redefined(&self, table_name: &str) -> bool {
+    pub(crate) fn table_is_redefined(&self, namespace: Option<Cow<'_, str>>, table_name: Cow<'_, str>) -> bool {
         self.tables
-            .get(table_name)
+            .get(&(namespace, table_name))
             .and_then(|pair| pair.transpose())
             .map(|ids| self.tables_to_redefine.contains(&ids))
             .unwrap_or(false)

migration-engine/migration-engine-tests/tests/migrations/postgres/multi_schema.rs

@@ -417,6 +417,37 @@ fn multi_schema_tests(_api: TestApi) {
             }),
             skip: None,
         },
+        TestData {
+            name: "move table across namespaces",
+            description: "todo",
+            schema: Schema {
+                common: (base_schema.to_owned() + indoc! {r#"
+        "#}),
+                first: indoc! {r#"
+        model First {
+          id Int @id
+          @@schema("one")
+        } "#}.into(),
+                second: Some(indoc!{r#"
+        model First {
+          id Int @id
+          @@schema("two")
+        } "#}.into()),
+            },
+            namespaces: &namespaces,
+            schema_push: SchemaPush::PushAnd(WithSchema::First,
+                           &SchemaPush::PushCustomAnd(CustomPushStep {
+                               warnings: &[] ,
+                               errors: &[],
+                               with_schema: WithSchema::Second,
+                               executed_steps: ExecutedSteps::NonZero,
+                           },
+                              &SchemaPush::Done)),
+            assertion: Box::new(|assert| {
+                assert.assert_has_table_with_ns("two", "First");
+            }),
+            skip: None,
+        },
         TestData {
             name: "recreate not null column with null values",
             description: "Test dropping a nullable column and recreating it as non-nullable, given a row exists with a NULL value",
@@ -1254,7 +1285,7 @@ fn run_schema_step(api: &mut TestApi, test: &TestData, namespaces: Option<Namesp
                 WithSchema::First => test.schema.common.to_owned() + test.schema.first.as_str(),
                 WithSchema::Second => match &test.schema.second {
                     Some(base_second) => test.schema.common.to_owned() + base_second.as_str(),
-                    None => panic!("Trying to run PushTwiceWithSteps but without defining the second migration."),
+                    None => panic!("Trying to run PushCustomAnd but without defining the second migration."),
                 },
             };