Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

me: Fix schema handling in migration persistence #3537

Merged
merged 1 commit into from
Dec 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{checksum, BoxFuture, ConnectorError, ConnectorResult};
use crate::{checksum, BoxFuture, ConnectorError, ConnectorResult, Namespaces};

/// A timestamp.
pub type Timestamp = chrono::DateTime<chrono::Utc>;
Expand All @@ -14,7 +14,7 @@ pub trait MigrationPersistence: Send + Sync {
/// If the migration persistence is not present in the target database,
/// check whether the database schema is empty. If it is, initialize the
/// migration persistence. If not, return a DatabaseSchemaNotEmpty error.
fn initialize(&mut self) -> BoxFuture<'_, ConnectorResult<()>>;
fn initialize(&mut self, namespaces: Option<Namespaces>) -> BoxFuture<'_, ConnectorResult<()>>;

/// Implementation in the connector for the core's MarkMigrationApplied
/// command. See the docs there. Note that the started_at and finished_at
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use crate::MongoDbMigrationConnector;
use migration_connector::{BoxFuture, ConnectorResult, MigrationPersistence};
use migration_connector::{BoxFuture, ConnectorResult, MigrationPersistence, Namespaces};

impl MigrationPersistence for MongoDbMigrationConnector {
fn baseline_initialize(&mut self) -> migration_connector::BoxFuture<'_, ConnectorResult<()>> {
unsupported_command_error()
}

fn initialize(&mut self) -> BoxFuture<'_, ConnectorResult<()>> {
fn initialize(&mut self, _namespaces: Option<Namespaces>) -> BoxFuture<'_, ConnectorResult<()>> {
unsupported_command_error()
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::SqlMigrationConnector;
use migration_connector::{
BoxFuture, ConnectorError, ConnectorResult, MigrationPersistence, MigrationRecord, PersistenceNotInitializedError,
BoxFuture, ConnectorError, ConnectorResult, MigrationPersistence, MigrationRecord, Namespaces,
PersistenceNotInitializedError,
};
use quaint::ast::*;
use uuid::Uuid;
Expand All @@ -10,10 +11,9 @@ impl MigrationPersistence for SqlMigrationConnector {
self.flavour.create_migrations_table()
}

fn initialize(&mut self) -> BoxFuture<'_, ConnectorResult<()>> {
fn initialize(&mut self, namespaces: Option<Namespaces>) -> BoxFuture<'_, ConnectorResult<()>> {
Box::pin(async move {
// TODO(MultiSchema): We may need to change this too.
let schema = self.flavour.describe_schema(None).await?;
let schema = self.flavour.describe_schema(namespaces).await?;

if schema
.table_walkers()
Expand Down
6 changes: 3 additions & 3 deletions migration-engine/core/src/commands/apply_migrations.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::{json_rpc::types::*, CoreError, CoreResult};
use migration_connector::{
migrations_directory::{error_on_changed_provider, list_migrations, MigrationDirectory},
ConnectorError, MigrationConnector, MigrationRecord, PersistenceNotInitializedError,
ConnectorError, MigrationConnector, MigrationRecord, Namespaces, PersistenceNotInitializedError,
};
use std::{path::Path, time::Instant};
use tracing::Instrument;
Expand All @@ -10,14 +10,14 @@ use user_facing_errors::migration_engine::FoundFailedMigrations;
pub async fn apply_migrations(
input: ApplyMigrationsInput,
connector: &mut dyn MigrationConnector,
namespaces: Option<Namespaces>,
) -> CoreResult<ApplyMigrationsOutput> {
let start = Instant::now();

error_on_changed_provider(&input.migrations_directory_path, connector.connector_type())?;

connector.acquire_lock().await?;

connector.migration_persistence().initialize().await?;
connector.migration_persistence().initialize(namespaces).await?;

let migrations_from_filesystem = list_migrations(Path::new(&input.migrations_directory_path))?;
let migrations_from_database = connector
Expand Down
6 changes: 5 additions & 1 deletion migration-engine/core/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,12 @@ impl GenericApi for EngineState {
}

async fn apply_migrations(&self, input: ApplyMigrationsInput) -> CoreResult<ApplyMigrationsOutput> {
let namespaces = self.namespaces();
self.with_default_connector(Box::new(move |connector| {
Box::pin(commands::apply_migrations(input, connector).instrument(tracing::info_span!("ApplyMigrations")))
Box::pin(
commands::apply_migrations(input, connector, namespaces)
.instrument(tracing::info_span!("ApplyMigrations")),
)
}))
.await
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,30 @@
use migration_core::{
commands::apply_migrations, json_rpc::types::*, migration_connector::MigrationConnector, CoreError, CoreResult,
commands::apply_migrations,
json_rpc::types::*,
migration_connector::{MigrationConnector, Namespaces},
CoreError, CoreResult,
};
use tempfile::TempDir;

#[must_use = "This struct does nothing on its own. See ApplyMigrations::send()"]
pub struct ApplyMigrations<'a> {
api: &'a mut dyn MigrationConnector,
migrations_directory: &'a TempDir,
namespaces: Option<Namespaces>,
}

impl<'a> ApplyMigrations<'a> {
pub fn new(api: &'a mut dyn MigrationConnector, migrations_directory: &'a TempDir) -> Self {
pub fn new(
api: &'a mut dyn MigrationConnector,
migrations_directory: &'a TempDir,
mut namespaces: Vec<String>,
) -> Self {
let namespaces = Namespaces::from_vec(&mut namespaces);

ApplyMigrations {
api,
migrations_directory,
namespaces,
}
}

Expand All @@ -23,6 +34,7 @@ impl<'a> ApplyMigrations<'a> {
migrations_directory_path: self.migrations_directory.path().to_str().unwrap().to_owned(),
},
self.api,
self.namespaces,
)
.await?;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ impl TestApi {
connector,
connection_info,
tags: self.args.tags(),
namespaces: self.args.namespaces(),
}
}

Expand Down Expand Up @@ -265,12 +266,19 @@ pub struct EngineTestApi {
pub(crate) connector: SqlMigrationConnector,
connection_info: ConnectionInfo,
tags: BitFlags<Tags>,
namespaces: &'static [&'static str],
}

impl EngineTestApi {
/// Plan an `applyMigrations` command
pub fn apply_migrations<'a>(&'a mut self, migrations_directory: &'a TempDir) -> ApplyMigrations<'a> {
ApplyMigrations::new(&mut self.connector, migrations_directory)
let mut namespaces = vec![self.connection_info.schema_name().to_string()];

for namespace in self.namespaces {
namespaces.push(namespace.to_string());
}

ApplyMigrations::new(&mut self.connector, migrations_directory, namespaces)
}

/// Plan a `createMigration` command
Expand Down
11 changes: 9 additions & 2 deletions migration-engine/migration-engine-tests/src/test_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,14 @@ impl TestApi {

/// Plan an `applyMigrations` command
pub fn apply_migrations<'a>(&'a mut self, migrations_directory: &'a TempDir) -> ApplyMigrations<'a> {
ApplyMigrations::new(&mut self.connector, migrations_directory)
let search_path = self.root.admin_conn.connection_info().schema_name();
let mut namespaces = vec![search_path.to_string()];

for namespace in self.root.args.namespaces() {
namespaces.push(namespace.to_string());
}

ApplyMigrations::new(&mut self.connector, migrations_directory, namespaces)
}

pub fn connection_string(&self) -> &str {
Expand Down Expand Up @@ -339,7 +346,7 @@ impl TestApi {
.unwrap()
}

fn generator_block(&self) -> String {
pub fn generator_block(&self) -> String {
let preview_feature_string = if self.root.preview_features().is_empty() {
"".to_string()
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ fn mark_migration_rolled_back_on_an_empty_database_errors(api: TestApi) {

#[test_connector]
fn mark_migration_rolled_back_on_a_database_with_migrations_table_errors(api: TestApi) {
tok(api.migration_persistence().initialize()).unwrap();
tok(api.migration_persistence().initialize(None)).unwrap();

let err = api
.mark_migration_rolled_back("anything")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use pretty_assertions::assert_eq;
fn starting_a_migration_works(api: TestApi) {
let persistence = api.migration_persistence();

tok(persistence.initialize()).unwrap();
tok(persistence.initialize(None)).unwrap();

let script = "CREATE ENUM MyBoolean ( \"TRUE\", \"FALSE\" )";

Expand Down Expand Up @@ -39,7 +39,7 @@ fn starting_a_migration_works(api: TestApi) {
fn finishing_a_migration_works(api: TestApi) {
let persistence = api.migration_persistence();

tok(persistence.initialize()).unwrap();
tok(persistence.initialize(None)).unwrap();

let script = "CREATE ENUM MyBoolean ( \"TRUE\", \"FALSE\" )";

Expand Down Expand Up @@ -76,7 +76,7 @@ fn finishing_a_migration_works(api: TestApi) {
fn updating_then_finishing_a_migration_works(api: TestApi) {
let persistence = api.migration_persistence();

tok(persistence.initialize()).unwrap();
tok(persistence.initialize(None)).unwrap();

let script = "CREATE ENUM MyBoolean ( \"TRUE\", \"FALSE\" )";

Expand Down Expand Up @@ -114,7 +114,7 @@ fn updating_then_finishing_a_migration_works(api: TestApi) {
fn multiple_successive_migrations_work(api: TestApi) {
let persistence = api.migration_persistence();

tok(persistence.initialize()).unwrap();
tok(persistence.initialize(None)).unwrap();

let script_1 = "CREATE ENUM MyBoolean ( \"TRUE\", \"FALSE\" )";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,43 @@ use crate::migrations::multi_schema::*;
use migration_engine_tests::test_api::*;
use sql_schema_describer::DefaultValue;

#[test_connector(tags(Postgres), preview_features("multiSchema"), namespaces("one", "prisma-tests"))]
fn apply_migrations_with_multiple_schemas_where_one_is_search_path_with_a_foreign_key(api: TestApi) {
let datasource = api.datasource_block_with(&[("schemas", r#"["one", "prisma-tests"]"#)]);
let generator = api.generator_block();

let dm = indoc::formatdoc! {r#"
{datasource}

{generator}

model A {{
id Int @id
bs B[]

@@schema("one")
}}

model B {{
id Int @id
aId Int
a A @relation(fields: [aId], references: [id])

@@schema("prisma-tests")
}}
"#};

let dir = api.create_migrations_directory();

api.create_migration("init", &dm, &dir).send_sync();

api.apply_migrations(&dir)
.send_sync()
.assert_applied_migrations(&["init"]);

api.apply_migrations(&dir).send_sync().assert_applied_migrations(&[]);
}

// This is the only "top" level test in this module. It defines a list of tests and executes them.
// If you want to look at the tests, see the `tests` variable below.
#[test_connector(tags(Postgres), preview_features("multiSchema"), namespaces("one", "two"))]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,10 @@ fn multi_schema_reset(mut api: TestApi) {
}}
"#, api.datasource_block_with(&[("schemas", r#"["felines", "rodents"]"#)])
};
api.schema_push(&prisma_schema)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was broken and we didn't realize it before. What happens:

  • We push the schema
  • We then apply migrations, which creates the persistence
  • The persistence cannot find the _prisma_migrations table
  • Before it was ok, but now we look into all the schemas given, so it finds the pushed tables but no migrations table
  • We error out.

What we should do is to use migrate, which creates the migrations table and allows us to continue.

.send()
.assert_green()
.assert_has_executed_steps();

let migrations_dir = api.create_migrations_directory();
api.create_migration("0-init", &prisma_schema, &migrations_dir)
.send_sync();
api.apply_migrations(&migrations_dir).send_sync();
api.raw_cmd("CREATE TABLE randomTable (id INTEGER PRIMARY KEY);");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,6 @@ model m1 {
let expected = expect_test::expect![[r#"
The `mysql` database is a system database, it should not be altered with prisma migrate. Please connect to another database.
0: migration_core::state::SchemaPush
at migration-engine/core/src/state.rs:402"#]];
at migration-engine/core/src/state.rs:406"#]];
expected.assert_eq(&err.to_string());
}