From 6ab5f26d28120ee7cc10954a8c7430a0953ac212 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=20Houl=C3=A9?= Date: Thu, 19 Mar 2020 10:49:20 +0100 Subject: [PATCH] Avoid unecessary allocation in quaint query building --- Cargo.lock | 3 +- Cargo.toml | 3 + .../src/sql_migration_persistence.rs | 18 +- .../tests/error_tests.rs | 23 ++- .../tests/existing_data_tests.rs | 169 ++++++++++++------ query-engine/prisma/src/tests/execute_raw.rs | 26 ++- 6 files changed, 159 insertions(+), 83 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 75754779b015..2a7626d5a85a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2107,8 +2107,7 @@ dependencies = [ [[package]] name = "quaint" -version = "0.2.0-alpha.9" -source = "git+https://github.com/prisma/quaint#5463232cd3577065013bde095afb5171d96b335a" +version = "0.2.0-alpha.10" dependencies = [ "async-trait", "base64 0.11.0", diff --git a/Cargo.toml b/Cargo.toml index 79ff413288a1..8fbd2dcf1a30 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,3 +23,6 @@ members = [ "libs/prisma-models", "libs/prisma-value", ] + +[patch."https://github.com/prisma/quaint"] +quaint = { path = "../quaint" } \ No newline at end of file diff --git a/migration-engine/connectors/sql-migration-connector/src/sql_migration_persistence.rs b/migration-engine/connectors/sql-migration-connector/src/sql_migration_persistence.rs index 6f20ae91e056..53fd66680cf0 100644 --- a/migration-engine/connectors/sql-migration-connector/src/sql_migration_persistence.rs +++ b/migration-engine/connectors/sql-migration-connector/src/sql_migration_persistence.rs @@ -111,7 +111,10 @@ impl MigrationPersistence for SqlMigrationPersistence<'_> { .value(DATAMODEL_STEPS_COLUMN, model_steps_json) .value(DATABASE_MIGRATION_COLUMN, database_migration_json) .value(ERRORS_COLUMN, errors_json) - .value(STARTED_AT_COLUMN, self.convert_datetime(migration.started_at)) + .value( + STARTED_AT_COLUMN, + self.convert_datetime(migration.started_at), + ) .value(FINISHED_AT_COLUMN, ParameterizedValue::Null); match self.sql_family() { @@ -122,7 +125,7 @@ impl MigrationPersistence for SqlMigrationPersistence<'_> { cloned.revision = usize::try_from(id).unwrap(); } SqlFamily::Postgres => { - let returning_insert = Insert::from(insert).returning(vec!["revision"]); + let returning_insert = Insert::from(insert).returning(&["revision"]); let result_set = self.conn().query(returning_insert.into()).await.unwrap(); result_set.into_iter().next().map(|row| { @@ -236,7 +239,8 @@ fn parse_rows_new(result_set: ResultSet) -> Vec { let datamodel_string: String = row[DATAMODEL_COLUMN].to_string().unwrap(); let datamodel_steps_json: String = row[DATAMODEL_STEPS_COLUMN].to_string().unwrap(); - let database_migration_string: String = row[DATABASE_MIGRATION_COLUMN].to_string().unwrap(); + let database_migration_string: String = + row[DATABASE_MIGRATION_COLUMN].to_string().unwrap(); let errors_json: String = row[ERRORS_COLUMN].to_string().unwrap(); let finished_at = match &row[FINISHED_AT_COLUMN] { @@ -244,11 +248,11 @@ fn parse_rows_new(result_set: ResultSet) -> Vec { x => Some(convert_parameterized_date_value(x)), }; - let datamodel_steps = - serde_json::from_str(&datamodel_steps_json).expect("Error parsing the migration steps"); + let datamodel_steps = serde_json::from_str(&datamodel_steps_json) + .expect("Error parsing the migration steps"); - let database_migration_json = - serde_json::from_str(&database_migration_string).expect("Error parsing the database migration steps"); + let database_migration_json = serde_json::from_str(&database_migration_string) + .expect("Error parsing the database migration steps"); let errors: Vec = serde_json::from_str(&errors_json).unwrap(); Migration { diff --git a/migration-engine/migration-engine-tests/tests/error_tests.rs b/migration-engine/migration-engine-tests/tests/error_tests.rs index c92df2a99f14..0c045c8057b4 100644 --- a/migration-engine/migration-engine-tests/tests/error_tests.rs +++ b/migration-engine/migration-engine-tests/tests/error_tests.rs @@ -117,9 +117,10 @@ async fn unreachable_database_must_return_a_proper_error_on_mysql() { #[tokio::test] async fn unreachable_database_must_return_a_proper_error_on_postgres() { - let mut url: Url = postgres_10_url("unreachable_database_must_return_a_proper_error_on_postgres") - .parse() - .unwrap(); + let mut url: Url = + postgres_10_url("unreachable_database_must_return_a_proper_error_on_postgres") + .parse() + .unwrap(); url.set_port(Some(8787)).unwrap(); @@ -194,7 +195,9 @@ async fn database_access_denied_must_return_a_proper_error_in_rpc() { let url: Url = mysql_url(db_name).parse().unwrap(); let conn = create_mysql_database(&url).await.unwrap(); - conn.execute_raw("DROP USER IF EXISTS jeanyves", &[]).await.unwrap(); + conn.execute_raw("DROP USER IF EXISTS jeanyves", &[]) + .await + .unwrap(); conn.execute_raw("CREATE USER jeanyves IDENTIFIED BY '1234'", &[]) .await .unwrap(); @@ -288,7 +291,11 @@ async fn datamodel_parser_errors_must_return_a_known_error(api: &TestApi) { } "#; - let error = api.infer_apply(bad_dm).send_user_facing().await.unwrap_err(); + let error = api + .infer_apply(bad_dm) + .send_user_facing() + .await + .unwrap_err(); let expected_msg = "\u{1b}[1;91merror\u{1b}[0m: \u{1b}[1mType \"Post\" is neither a built-in type, nor refers to another model, custom type, or enum.\u{1b}[0m\n \u{1b}[1;94m-->\u{1b}[0m \u{1b}[4mschema.prisma:4\u{1b}[0m\n\u{1b}[1;94m | \u{1b}[0m\n\u{1b}[1;94m 3 | \u{1b}[0m id Float @id\n\u{1b}[1;94m 4 | \u{1b}[0m post \u{1b}[1;91mPost[]\u{1b}[0m\n\u{1b}[1;94m | \u{1b}[0m\n"; @@ -302,7 +309,9 @@ async fn datamodel_parser_errors_must_return_a_known_error(api: &TestApi) { } #[test_each_connector] -async fn unique_constraint_errors_in_migrations_must_return_a_known_error(api: &TestApi) -> TestResult { +async fn unique_constraint_errors_in_migrations_must_return_a_known_error( + api: &TestApi, +) -> TestResult { use quaint::ast::*; let dm = r#" @@ -314,7 +323,7 @@ async fn unique_constraint_errors_in_migrations_must_return_a_known_error(api: & api.infer_and_apply(dm).await; - let insert = Insert::multi_into(api.render_table_name("Fruit"), vec!["name"]) + let insert = Insert::multi_into(api.render_table_name("Fruit"), &["name"]) .values(("banana",)) .values(("apple",)) .values(("banana",)); diff --git a/migration-engine/migration-engine-tests/tests/existing_data_tests.rs b/migration-engine/migration-engine-tests/tests/existing_data_tests.rs index a10900cbd23a..005d914503e8 100644 --- a/migration-engine/migration-engine-tests/tests/existing_data_tests.rs +++ b/migration-engine/migration-engine-tests/tests/existing_data_tests.rs @@ -43,7 +43,9 @@ async fn adding_a_required_field_if_there_is_data(api: &TestApi) { } #[test_each_connector] -async fn adding_a_required_field_must_use_the_default_value_for_migrations(api: &TestApi) -> TestResult { +async fn adding_a_required_field_must_use_the_default_value_for_migrations( + api: &TestApi, +) -> TestResult { let dm = r#" model Test { id String @id @default(cuid()) @@ -85,7 +87,10 @@ async fn adding_a_required_field_must_use_the_default_value_for_migrations(api: .column("string") .so_that("id".equals("test")); let result_set = conn.query(query.into()).await.unwrap(); - let row = result_set.into_iter().next().expect("query returned no results"); + let row = result_set + .into_iter() + .next() + .expect("query returned no results"); assert_eq!(row["myint"].as_i64().unwrap(), 1); assert_eq!(row["string"].as_str().unwrap(), "test_string"); @@ -120,7 +125,8 @@ async fn dropping_a_table_with_rows_should_warn(api: &TestApi) { assert_eq!( migration_output.warnings, &[MigrationWarning { - description: "You are about to drop the table `Test`, which is not empty (1 rows).".into() + description: "You are about to drop the table `Test`, which is not empty (1 rows)." + .into() }] ); } @@ -136,7 +142,7 @@ async fn dropping_a_column_with_non_null_values_should_warn(api: &TestApi) { let original_database_schema = api.infer_and_apply(&dm).await.sql_schema; - let insert = Insert::multi_into((api.schema_name(), "Test"), vec!["id", "puppiesCount"]) + let insert = Insert::multi_into((api.schema_name(), "Test"), &["id", "puppiesCount"]) .values(("a", 7)) .values(("b", 8)); @@ -177,9 +183,9 @@ async fn altering_a_column_without_non_null_values_should_not_warn(api: &TestApi let original_database_schema = api.infer_and_apply(&dm).await.sql_schema; - let insert = Insert::multi_into((api.schema_name(), "Test"), vec!["id"]) - .values(vec!["a"]) - .values(vec!["b"]); + let insert = Insert::multi_into((api.schema_name(), "Test"), &["id"]) + .values(("a",)) + .values(("b",)); api.database().query(insert.into()).await.unwrap(); @@ -225,7 +231,9 @@ async fn altering_a_column_with_non_null_values_should_warn(api: &TestApi) -> Te let migration_output = api.infer_apply(&dm2).send().await?; // The schema should not change because the migration should not run if there are warnings // and the force flag isn't passed. - api.assert_schema().await?.assert_equals(&original_database_schema)?; + api.assert_schema() + .await? + .assert_equals(&original_database_schema)?; assert_eq!( migration_output.warnings, @@ -270,11 +278,13 @@ async fn column_defaults_can_safely_be_changed(api: &TestApi) -> TestResult { api.infer_apply(&dm1).force(Some(true)).send().await?; - api.assert_schema().await?.assert_table(model_name, |table| { - table.assert_column("name", |column| { - column.assert_default(Some(first_default.unwrap_or(""))) - }) - })?; + api.assert_schema() + .await? + .assert_table(model_name, |table| { + table.assert_column("name", |column| { + column.assert_default(Some(first_default.unwrap_or(""))) + }) + })?; } // Insert data @@ -295,7 +305,10 @@ async fn column_defaults_can_safely_be_changed(api: &TestApi) -> TestResult { .filter_map(|row| row.get("name").and_then(|val| val.to_string())) .collect(); // TODO: change this when the defaults hack is removed - assert_eq!(&[first_default.unwrap_or(""), "Waterworld"], names.as_slice()); + assert_eq!( + &[first_default.unwrap_or(""), "Waterworld"], + names.as_slice() + ); } // Migrate @@ -330,13 +343,18 @@ async fn column_defaults_can_safely_be_changed(api: &TestApi) -> TestResult { .into_iter() .filter_map(|row| row.get("name").and_then(|val| val.to_string())) .collect(); - assert_eq!(&[first_default.unwrap_or(""), "Waterworld"], names.as_slice()); + assert_eq!( + &[first_default.unwrap_or(""), "Waterworld"], + names.as_slice() + ); - api.assert_schema().await?.assert_table(model_name, |table| { - table.assert_column("name", |column| { - column.assert_default(Some(second_default.unwrap_or(""))) - }) - })?; + api.assert_schema() + .await? + .assert_table(model_name, |table| { + table.assert_column("name", |column| { + column.assert_default(Some(second_default.unwrap_or(""))) + }) + })?; } } @@ -355,7 +373,7 @@ async fn changing_a_column_from_required_to_optional_should_work(api: &TestApi) api.infer_apply(&dm).send().await?; let original_database_schema = api.describe_database().await?; - let insert = Insert::multi_into((api.schema_name(), "Test"), vec!["id", "age"]) + let insert = Insert::multi_into((api.schema_name(), "Test"), &["id", "age"]) .values(("a", 12)) .values(("b", 22)); @@ -383,7 +401,9 @@ async fn changing_a_column_from_required_to_optional_should_work(api: &TestApi) "You are about to alter the column `age` on the `Test` table, which still contains 2 non-null values. The data in that column will be lost.", ); - api.assert_schema().await?.assert_equals(&original_database_schema)?; + api.assert_schema() + .await? + .assert_equals(&original_database_schema)?; } else { // On other databases, the migration should be successful. anyhow::ensure!( @@ -392,7 +412,9 @@ async fn changing_a_column_from_required_to_optional_should_work(api: &TestApi) migration_output.warnings ); - api.assert_schema().await?.assert_ne(&original_database_schema)?; + api.assert_schema() + .await? + .assert_ne(&original_database_schema)?; } // Check that no data was lost. @@ -422,7 +444,7 @@ async fn changing_a_column_from_optional_to_required_must_warn(api: &TestApi) -> api.infer_apply(&dm).send().await?; let original_database_schema = api.describe_database().await?; - let insert = Insert::multi_into((api.schema_name(), "Test"), vec!["id", "age"]) + let insert = Insert::multi_into((api.schema_name(), "Test"), &["id", "age"]) .values(("a", 12)) .values(("b", 22)); @@ -439,7 +461,9 @@ async fn changing_a_column_from_optional_to_required_must_warn(api: &TestApi) -> // The schema should not change because the migration should not run if there are warnings // and the force flag isn't passed. - api.assert_schema().await?.assert_equals(&original_database_schema)?; + api.assert_schema() + .await? + .assert_equals(&original_database_schema)?; assert_eq!( migration_output.warnings, @@ -567,13 +591,18 @@ async fn string_columns_do_not_get_arbitrarily_migrated(api: &TestApi) -> TestRe row.get("kindle_email").unwrap().as_str().unwrap(), "george+kindle@prisma.io" ); - assert_eq!(row.get("email").unwrap().as_str().unwrap(), "george@prisma.io"); + assert_eq!( + row.get("email").unwrap().as_str().unwrap(), + "george@prisma.io" + ); Ok(()) } #[test_each_connector] -async fn altering_the_type_of_a_column_in_an_empty_table_should_not_warn(api: &TestApi) -> TestResult { +async fn altering_the_type_of_a_column_in_an_empty_table_should_not_warn( + api: &TestApi, +) -> TestResult { let dm1 = r#" model User { id String @id @default(cuid()) @@ -599,7 +628,9 @@ async fn altering_the_type_of_a_column_in_an_empty_table_should_not_warn(api: &T api.assert_schema() .await? .assert_table("User", |table| { - table.assert_column("dogs", |col| col.assert_type_is_string()?.assert_is_required()) + table.assert_column("dogs", |col| { + col.assert_type_is_string()?.assert_is_required() + }) }) .map(drop) } @@ -637,7 +668,9 @@ async fn making_a_column_required_in_an_empty_table_should_not_warn(api: &TestAp } #[test_each_connector] -async fn altering_the_type_of_a_column_in_a_non_empty_table_always_warns(api: &TestApi) -> TestResult { +async fn altering_the_type_of_a_column_in_a_non_empty_table_always_warns( + api: &TestApi, +) -> TestResult { let dm1 = r#" model User { id String @id @default(cuid()) @@ -686,7 +719,9 @@ async fn altering_the_type_of_a_column_in_a_non_empty_table_always_warns(api: &T } #[test_each_connector(ignore("mysql"))] -async fn migrating_a_required_column_from_int_to_string_should_warn_and_cast(api: &TestApi) -> TestResult { +async fn migrating_a_required_column_from_int_to_string_should_warn_and_cast( + api: &TestApi, +) -> TestResult { let dm1 = r#" model Test { id String @id @@ -705,7 +740,11 @@ async fn migrating_a_required_column_from_int_to_string_should_warn_and_cast(api let test = api.dump_table("Test").await?; let first_row = test.get(0).unwrap(); assert_eq!( - format!("{:?} {:?}", first_row.get("id"), first_row.get("serialNumber")), + format!( + "{:?} {:?}", + first_row.get("id"), + first_row.get("serialNumber") + ), r#"Some(Text("abcd")) Some(Integer(47))"# ); @@ -744,7 +783,11 @@ async fn migrating_a_required_column_from_int_to_string_should_warn_and_cast(api let test = api.dump_table("Test").await?; let first_row = test.get(0).unwrap(); assert_eq!( - format!("{:?} {:?}", first_row.get("id"), first_row.get("serialNumber")), + format!( + "{:?} {:?}", + first_row.get("id"), + first_row.get("serialNumber") + ), r#"Some(Text("abcd")) Some(Text("47"))"# ); } @@ -778,15 +821,16 @@ async fn enum_variants_can_be_added_without_data_loss(api: &TestApi) -> TestResu .assert_green()?; { - let cat_inserts = quaint::ast::Insert::multi_into(api.render_table_name("Cat"), vec!["id", "mood"]) - .values(( - ParameterizedValue::Text(Cow::Borrowed("felix")), - ParameterizedValue::Enum(Cow::Borrowed("HUNGRY")), - )) - .values(( - ParameterizedValue::Text(Cow::Borrowed("mittens")), - ParameterizedValue::Enum(Cow::Borrowed("HAPPY")), - )); + let cat_inserts = + quaint::ast::Insert::multi_into(api.render_table_name("Cat"), vec!["id", "mood"]) + .values(( + ParameterizedValue::Text(Cow::Borrowed("felix")), + ParameterizedValue::Enum(Cow::Borrowed("HUNGRY")), + )) + .values(( + ParameterizedValue::Text(Cow::Borrowed("mittens")), + ParameterizedValue::Enum(Cow::Borrowed("HAPPY")), + )); api.database().query(cat_inserts.into()).await?; } @@ -818,8 +862,10 @@ async fn enum_variants_can_be_added_without_data_loss(api: &TestApi) -> TestResu // Assertions { let cat_data = api.dump_table("Cat").await?; - let cat_data: Vec> = - cat_data.into_iter().map(|row| row.into_iter().collect()).collect(); + let cat_data: Vec> = cat_data + .into_iter() + .map(|row| row.into_iter().collect()) + .collect(); let expected_cat_data = if api.sql_family().is_mysql() { vec![ @@ -848,8 +894,10 @@ async fn enum_variants_can_be_added_without_data_loss(api: &TestApi) -> TestResu assert_eq!(cat_data, expected_cat_data); let human_data = api.dump_table("Human").await?; - let human_data: Vec> = - human_data.into_iter().map(|row| row.into_iter().collect()).collect(); + let human_data: Vec> = human_data + .into_iter() + .map(|row| row.into_iter().collect()) + .collect(); let expected_human_data: Vec> = Vec::new(); assert_eq!(human_data, expected_human_data); @@ -899,15 +947,16 @@ async fn enum_variants_can_be_dropped_without_data_loss(api: &TestApi) -> TestRe .assert_green()?; { - let cat_inserts = quaint::ast::Insert::multi_into(api.render_table_name("Cat"), vec!["id", "mood"]) - .values(( - ParameterizedValue::Text(Cow::Borrowed("felix")), - ParameterizedValue::Enum(Cow::Borrowed("HUNGRY")), - )) - .values(( - ParameterizedValue::Text(Cow::Borrowed("mittens")), - ParameterizedValue::Enum(Cow::Borrowed("HAPPY")), - )); + let cat_inserts = + quaint::ast::Insert::multi_into(api.render_table_name("Cat"), &["id", "mood"]) + .values(( + ParameterizedValue::Text(Cow::Borrowed("felix")), + ParameterizedValue::Enum(Cow::Borrowed("HUNGRY")), + )) + .values(( + ParameterizedValue::Text(Cow::Borrowed("mittens")), + ParameterizedValue::Enum(Cow::Borrowed("HAPPY")), + )); api.database().query(cat_inserts.into()).await?; } @@ -938,8 +987,10 @@ async fn enum_variants_can_be_dropped_without_data_loss(api: &TestApi) -> TestRe // Assertions { let cat_data = api.dump_table("Cat").await?; - let cat_data: Vec> = - cat_data.into_iter().map(|row| row.into_iter().collect()).collect(); + let cat_data: Vec> = cat_data + .into_iter() + .map(|row| row.into_iter().collect()) + .collect(); let expected_cat_data = if api.sql_family().is_mysql() { vec![ @@ -968,8 +1019,10 @@ async fn enum_variants_can_be_dropped_without_data_loss(api: &TestApi) -> TestRe assert_eq!(cat_data, expected_cat_data); let human_data = api.dump_table("Human").await?; - let human_data: Vec> = - human_data.into_iter().map(|row| row.into_iter().collect()).collect(); + let human_data: Vec> = human_data + .into_iter() + .map(|row| row.into_iter().collect()) + .collect(); let expected_human_data: Vec> = Vec::new(); assert_eq!(human_data, expected_human_data); diff --git a/query-engine/prisma/src/tests/execute_raw.rs b/query-engine/prisma/src/tests/execute_raw.rs index efe8ae81f365..8e91db0a0c8a 100644 --- a/query-engine/prisma/src/tests/execute_raw.rs +++ b/query-engine/prisma/src/tests/execute_raw.rs @@ -129,9 +129,9 @@ async fn querying_model_tables(api: &TestApi) -> anyhow::Result<()> { async fn inserting_into_model_table(api: &TestApi) -> anyhow::Result<()> { let query_engine = api.create_engine(&TODO).await?; - let insert = Insert::multi_into("Todo", vec!["id", "title"]) - .values(vec!["id1", "title1"]) - .values(vec!["id2", "title2"]); + let insert = Insert::multi_into("Todo", &["id", "title"]) + .values(("id1", "title1")) + .values(("id2", "title2")); let (query, params) = api.to_sql_string(insert); @@ -173,8 +173,9 @@ async fn querying_model_tables_with_alias(api: &TestApi) -> anyhow::Result<()> { query_engine.request(mutation).await; - let (query, params) = - api.to_sql_string(Select::from_table("Todo").column(Column::from("title").alias("aliasedTitle"))); + let (query, params) = api.to_sql_string( + Select::from_table("Todo").column(Column::from("title").alias("aliasedTitle")), + ); assert_eq!( json!({ @@ -222,9 +223,12 @@ async fn querying_the_same_column_name_twice_with_aliasing(api: &TestApi) -> any async fn arrays(api: &TestApi) -> anyhow::Result<()> { let query_engine = api.create_engine(&TODO).await?; - let query = "SELECT ARRAY_AGG(columnInfos.attname) AS postgres_array FROM pg_attribute columnInfos"; + let query = + "SELECT ARRAY_AGG(columnInfos.attname) AS postgres_array FROM pg_attribute columnInfos"; let result = query_engine.request(execute_raw(query, vec![])).await; - let array = result["data"]["executeRaw"][0]["postgres_array"].as_array().unwrap(); + let array = result["data"]["executeRaw"][0]["postgres_array"] + .as_array() + .unwrap(); for val in array.into_iter() { assert!(val.is_string()); @@ -236,7 +240,9 @@ async fn arrays(api: &TestApi) -> anyhow::Result<()> { #[test_each_connector] async fn syntactic_errors_bubbling_through_to_the_user(api: &TestApi) -> anyhow::Result<()> { let query_engine = api.create_engine(&TODO).await?; - let result = query_engine.request(execute_raw("SELECT * FROM ", vec![])).await; + let result = query_engine + .request(execute_raw("SELECT * FROM ", vec![])) + .await; let error_code = result["errors"][0]["user_facing_error"]["meta"]["code"].as_str(); match api.connection_info() { @@ -261,7 +267,9 @@ async fn other_errors_bubbling_through_to_the_user(api: &TestApi) -> anyhow::Res let result = query_engine.request(mutation).await; let id = result["data"]["createOneTodo"]["id"].as_str().unwrap(); - let insert = Insert::single_into("Todo").value("id", id).value("title", "irrelevant"); + let insert = Insert::single_into("Todo") + .value("id", id) + .value("title", "irrelevant"); let (query, params) = api.to_sql_string(insert); let result = query_engine.request(execute_raw(&query, params)).await;