From 93d333a23877a2324e7e11bfc41c8dadb370a2f6 Mon Sep 17 00:00:00 2001 From: Matthias Oertel Date: Mon, 2 Mar 2020 09:31:26 +0100 Subject: [PATCH 1/2] add test, start with plumbing --- .../src/calculate_datamodel.rs | 4 +-- .../src/comment_out_unhandled_models.rs | 17 +++++++++++- .../src/misc_helpers.rs | 4 +++ .../tests/common_unit_tests/mod.rs | 27 +++++++++++++++++++ .../remapping_database_names_postgres.rs | 24 +++++++++++++++++ libs/datamodel/core/src/dml/field.rs | 5 ++++ .../datamodel/core/src/json/dmmf/from_dmmf.rs | 1 + 7 files changed, 79 insertions(+), 3 deletions(-) diff --git a/introspection-engine/connectors/sql-introspection-connector/src/calculate_datamodel.rs b/introspection-engine/connectors/sql-introspection-connector/src/calculate_datamodel.rs index 3ac66cb1d0f9..4097855ea89e 100644 --- a/introspection-engine/connectors/sql-introspection-connector/src/calculate_datamodel.rs +++ b/introspection-engine/connectors/sql-introspection-connector/src/calculate_datamodel.rs @@ -1,4 +1,4 @@ -use crate::comment_out_unhandled_models::comment_out_unhandled_models; +use crate::comment_out_unhandled_models::commenting_out_guardrails; use crate::misc_helpers::*; use crate::sanitize_datamodel_names::sanitize_datamodel_names; use crate::SqlIntrospectionResult; @@ -141,8 +141,8 @@ pub fn calculate_model(schema: &SqlSchema) -> SqlIntrospectionResult model.add_field(field); } - comment_out_unhandled_models(&mut data_model); sanitize_datamodel_names(&mut data_model); + commenting_out_guardrails(&mut data_model); debug!("Done calculating data model {:?}", data_model); Ok(data_model) diff --git a/introspection-engine/connectors/sql-introspection-connector/src/comment_out_unhandled_models.rs b/introspection-engine/connectors/sql-introspection-connector/src/comment_out_unhandled_models.rs index 76bad1418bf5..41f6b13d2563 100644 --- a/introspection-engine/connectors/sql-introspection-connector/src/comment_out_unhandled_models.rs +++ b/introspection-engine/connectors/sql-introspection-connector/src/comment_out_unhandled_models.rs @@ -1,8 +1,9 @@ use datamodel::Datamodel; -pub fn comment_out_unhandled_models(datamodel: &mut Datamodel) { +pub fn commenting_out_guardrails(datamodel: &mut Datamodel) { let mut commented_model_names = vec![]; + //models without uniques / ids for model in &mut datamodel.models { if model.id_fields.is_empty() && !model.fields.iter().any(|f| f.is_id || f.is_unique) @@ -17,6 +18,20 @@ pub fn comment_out_unhandled_models(datamodel: &mut Datamodel) { } } + //fields with an empty name + for model in &mut datamodel.models { + for field in &mut model.fields { + if field.name == "".to_string() { + field.documentation = Some( + "This field was commented out because automatic field renaming would have produced an empty name" + .to_string(), + ); + field.name = field.database_names.first().unwrap().to_string(); + field.is_commented_out = true; + } + } + } + for name in &commented_model_names { for model in &mut datamodel.models { model.fields.retain(|f| !f.points_to_model(name)); diff --git a/introspection-engine/connectors/sql-introspection-connector/src/misc_helpers.rs b/introspection-engine/connectors/sql-introspection-connector/src/misc_helpers.rs index d22368914da5..ec21a4351438 100644 --- a/introspection-engine/connectors/sql-introspection-connector/src/misc_helpers.rs +++ b/introspection-engine/connectors/sql-introspection-connector/src/misc_helpers.rs @@ -71,6 +71,7 @@ pub fn calculate_many_to_many_field(foreign_key: &ForeignKey, relation_name: Str is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, } } @@ -128,6 +129,7 @@ pub(crate) fn calculate_scalar_field( is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, } } @@ -218,6 +220,7 @@ pub(crate) fn calculate_relation_field( is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }] } } @@ -280,6 +283,7 @@ pub(crate) fn calculate_backrelation_field( is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, } } diff --git a/introspection-engine/connectors/sql-introspection-connector/tests/common_unit_tests/mod.rs b/introspection-engine/connectors/sql-introspection-connector/tests/common_unit_tests/mod.rs index 49a388646efa..605844d7b410 100644 --- a/introspection-engine/connectors/sql-introspection-connector/tests/common_unit_tests/mod.rs +++ b/introspection-engine/connectors/sql-introspection-connector/tests/common_unit_tests/mod.rs @@ -62,6 +62,7 @@ fn a_data_model_can_be_generated_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, } }) .collect(), @@ -122,6 +123,7 @@ fn arity_is_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "required".to_string(), @@ -135,6 +137,7 @@ fn arity_is_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "list".to_string(), @@ -148,6 +151,7 @@ fn arity_is_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, ], is_generated: false, @@ -229,6 +233,7 @@ fn defaults_are_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "int_default".to_string(), @@ -242,6 +247,7 @@ fn defaults_are_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "bool_default".to_string(), @@ -255,6 +261,7 @@ fn defaults_are_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "float_default".to_string(), @@ -268,6 +275,7 @@ fn defaults_are_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "string_default".to_string(), @@ -281,6 +289,7 @@ fn defaults_are_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, ], is_generated: false, @@ -388,6 +397,7 @@ fn primary_key_is_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }], is_generated: false, indices: vec![], @@ -412,6 +422,7 @@ fn primary_key_is_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }], is_generated: false, indices: vec![], @@ -436,6 +447,7 @@ fn primary_key_is_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }], is_generated: false, indices: vec![], @@ -539,6 +551,7 @@ fn uniqueness_is_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "unique".to_string(), @@ -552,6 +565,7 @@ fn uniqueness_is_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, ], is_generated: false, @@ -626,6 +640,7 @@ fn compound_foreign_keys_are_preserved_when_generating_data_model_from_a_schema( is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "name".to_string(), @@ -639,6 +654,7 @@ fn compound_foreign_keys_are_preserved_when_generating_data_model_from_a_schema( is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, ], is_generated: false, @@ -664,6 +680,7 @@ fn compound_foreign_keys_are_preserved_when_generating_data_model_from_a_schema( is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "city-id".to_string(), @@ -682,6 +699,7 @@ fn compound_foreign_keys_are_preserved_when_generating_data_model_from_a_schema( is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "city-name".to_string(), @@ -700,6 +718,7 @@ fn compound_foreign_keys_are_preserved_when_generating_data_model_from_a_schema( is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, ], is_generated: false, @@ -819,6 +838,7 @@ fn multi_field_uniques_are_preserved_when_generating_data_model_from_a_schema() is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "name".to_string(), @@ -832,6 +852,7 @@ fn multi_field_uniques_are_preserved_when_generating_data_model_from_a_schema() is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "lastname".to_string(), @@ -845,6 +866,7 @@ fn multi_field_uniques_are_preserved_when_generating_data_model_from_a_schema() is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, ], is_generated: false, @@ -935,6 +957,7 @@ fn foreign_keys_are_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "name".to_string(), @@ -948,6 +971,7 @@ fn foreign_keys_are_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "user".to_string(), @@ -966,6 +990,7 @@ fn foreign_keys_are_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, ], is_generated: false, @@ -991,6 +1016,7 @@ fn foreign_keys_are_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, Field { name: "city_id".to_string(), @@ -1009,6 +1035,7 @@ fn foreign_keys_are_preserved_when_generating_data_model_from_a_schema() { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, }, ], is_generated: false, diff --git a/introspection-engine/connectors/sql-introspection-connector/tests/db_specific_introspection/postgres/remapping_database_names_postgres.rs b/introspection-engine/connectors/sql-introspection-connector/tests/db_specific_introspection/postgres/remapping_database_names_postgres.rs index d9e34b3f7977..f66bcfeed646 100644 --- a/introspection-engine/connectors/sql-introspection-connector/tests/db_specific_introspection/postgres/remapping_database_names_postgres.rs +++ b/introspection-engine/connectors/sql-introspection-connector/tests/db_specific_introspection/postgres/remapping_database_names_postgres.rs @@ -274,3 +274,27 @@ async fn remapping_compound_primary_keys_should_work(api: &TestApi) { let result = dbg!(api.introspect().await); custom_assert(&result, dm); } + +#[test_each_connector(tags("postgres"))] +async fn remapping_field_names_to_empty_should_comment_them_out(api: &TestApi) { + api.barrel() + .execute(|migration| { + migration.create_table("User", |t| { + t.add_column("1", types::text()); + t.add_column("last", types::primary()); + }); + }) + .await; + + let dm = r#" + model User { + first_name String + last_name String @map("last@name") + + @@id([first_name, last_name]) + } + "#; + + let result = dbg!(api.introspect().await); + custom_assert(&result, dm); +} diff --git a/libs/datamodel/core/src/dml/field.rs b/libs/datamodel/core/src/dml/field.rs index 7d8158aeab79..8894921fc36c 100644 --- a/libs/datamodel/core/src/dml/field.rs +++ b/libs/datamodel/core/src/dml/field.rs @@ -100,6 +100,9 @@ pub struct Field { /// The data source field specifics, like backing fields and defaults. pub data_source_fields: Vec, + + /// Indicates if this field has to be commented out. + pub is_commented_out: bool, } impl Field { @@ -169,6 +172,7 @@ impl Field { is_generated: false, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, } } /// Creates a new field with the given name and type, marked as generated and optional. @@ -185,6 +189,7 @@ impl Field { is_generated: true, is_updated_at: false, data_source_fields: vec![], + is_commented_out: false, } } } diff --git a/libs/datamodel/core/src/json/dmmf/from_dmmf.rs b/libs/datamodel/core/src/json/dmmf/from_dmmf.rs index 04c52a9f3dad..ab1e0ba24255 100644 --- a/libs/datamodel/core/src/json/dmmf/from_dmmf.rs +++ b/libs/datamodel/core/src/json/dmmf/from_dmmf.rs @@ -80,6 +80,7 @@ fn field_from_dmmf(field: &Field) -> dml::Field { is_updated_at: field.is_updated_at.unwrap_or(false), documentation: field.documentation.clone(), data_source_fields: vec![], + is_commented_out: false, } } From 02c63c4a507486a64bcb62d273cd766cf4207d7f Mon Sep 17 00:00:00 2001 From: Matthias Date: Wed, 4 Mar 2020 14:37:47 +0100 Subject: [PATCH 2/2] test commenting out of empty field names --- .../src/calculate_datamodel.rs | 2 +- ...nhandled_models.rs => commenting_out_guardrails.rs} | 2 +- .../connectors/sql-introspection-connector/src/lib.rs | 2 +- .../postgres/remapping_database_names_postgres.rs | 10 ++-------- libs/datamodel/core/src/ast/field.rs | 2 ++ libs/datamodel/core/src/ast/parser/mod.rs | 2 ++ libs/datamodel/core/src/ast/renderer/mod.rs | 10 ++++++++-- libs/datamodel/core/src/validator/lower.rs | 1 + .../core/src/migration/datamodel_calculator.rs | 2 ++ 9 files changed, 20 insertions(+), 13 deletions(-) rename introspection-engine/connectors/sql-introspection-connector/src/{comment_out_unhandled_models.rs => commenting_out_guardrails.rs} (90%) diff --git a/introspection-engine/connectors/sql-introspection-connector/src/calculate_datamodel.rs b/introspection-engine/connectors/sql-introspection-connector/src/calculate_datamodel.rs index 4097855ea89e..76501c3c9e47 100644 --- a/introspection-engine/connectors/sql-introspection-connector/src/calculate_datamodel.rs +++ b/introspection-engine/connectors/sql-introspection-connector/src/calculate_datamodel.rs @@ -1,4 +1,4 @@ -use crate::comment_out_unhandled_models::commenting_out_guardrails; +use crate::commenting_out_guardrails::commenting_out_guardrails; use crate::misc_helpers::*; use crate::sanitize_datamodel_names::sanitize_datamodel_names; use crate::SqlIntrospectionResult; diff --git a/introspection-engine/connectors/sql-introspection-connector/src/comment_out_unhandled_models.rs b/introspection-engine/connectors/sql-introspection-connector/src/commenting_out_guardrails.rs similarity index 90% rename from introspection-engine/connectors/sql-introspection-connector/src/comment_out_unhandled_models.rs rename to introspection-engine/connectors/sql-introspection-connector/src/commenting_out_guardrails.rs index 41f6b13d2563..9cf9bfed9cfc 100644 --- a/introspection-engine/connectors/sql-introspection-connector/src/comment_out_unhandled_models.rs +++ b/introspection-engine/connectors/sql-introspection-connector/src/commenting_out_guardrails.rs @@ -23,7 +23,7 @@ pub fn commenting_out_guardrails(datamodel: &mut Datamodel) { for field in &mut model.fields { if field.name == "".to_string() { field.documentation = Some( - "This field was commented out because automatic field renaming would have produced an empty name" + "This field was commented out because of an invalid name. Please provide a valid one that matches [a-zA-Z][a-zA-Z0-9_]*" .to_string(), ); field.name = field.database_names.first().unwrap().to_string(); diff --git a/introspection-engine/connectors/sql-introspection-connector/src/lib.rs b/introspection-engine/connectors/sql-introspection-connector/src/lib.rs index b6b337965411..d01dd454f6fb 100644 --- a/introspection-engine/connectors/sql-introspection-connector/src/lib.rs +++ b/introspection-engine/connectors/sql-introspection-connector/src/lib.rs @@ -1,5 +1,5 @@ pub mod calculate_datamodel; // only exported to be able to unit test it -mod comment_out_unhandled_models; +mod commenting_out_guardrails; mod error; mod misc_helpers; mod sanitize_datamodel_names; diff --git a/introspection-engine/connectors/sql-introspection-connector/tests/db_specific_introspection/postgres/remapping_database_names_postgres.rs b/introspection-engine/connectors/sql-introspection-connector/tests/db_specific_introspection/postgres/remapping_database_names_postgres.rs index 54fd6e2ded89..34fa516c1f89 100644 --- a/introspection-engine/connectors/sql-introspection-connector/tests/db_specific_introspection/postgres/remapping_database_names_postgres.rs +++ b/introspection-engine/connectors/sql-introspection-connector/tests/db_specific_introspection/postgres/remapping_database_names_postgres.rs @@ -317,14 +317,8 @@ async fn remapping_field_names_to_empty_should_comment_them_out(api: &TestApi) { }) .await; - let dm = r#" - model User { - /// This field was commented out because automatic field renaming would have produced an empty name - 1 String @map(\"1\") - last Int @default(autoincrement()) @id - } - "#; + let dm = "model User {\n /// This field was commented out because of an invalid name. Please provide a valid one that matches [a-zA-Z][a-zA-Z0-9_]*\n // 1 String @map(\"1\")\n last Int @default(autoincrement()) @id\n}"; let result = dbg!(api.introspect().await); - custom_assert(&result, dm); + assert_eq!(&result, dm); } diff --git a/libs/datamodel/core/src/ast/field.rs b/libs/datamodel/core/src/ast/field.rs index bc97458898ac..43ad1b7ed135 100644 --- a/libs/datamodel/core/src/ast/field.rs +++ b/libs/datamodel/core/src/ast/field.rs @@ -16,6 +16,8 @@ pub struct Field { pub documentation: Option, /// The location of this field in the text representation. pub span: Span, + /// The location of this field in the text representation. + pub is_commented_out: bool, } impl WithIdentifier for Field { diff --git a/libs/datamodel/core/src/ast/parser/mod.rs b/libs/datamodel/core/src/ast/parser/mod.rs index c71520a99880..99f4dd32e29a 100644 --- a/libs/datamodel/core/src/ast/parser/mod.rs +++ b/libs/datamodel/core/src/ast/parser/mod.rs @@ -228,6 +228,7 @@ fn parse_field(token: &pest::iterators::Pair<'_, Rule>) -> Result panic!( "Encountered impossible field declaration during parsing: {:?}", @@ -453,6 +454,7 @@ fn parse_type(token: &pest::iterators::Pair<'_, Rule>) -> Field { directives, documentation: doc_comments_to_string(&comments), span: Span::from_pest(token.as_span()), + is_commented_out: false, }, _ => panic!( "Encountered impossible custom type declaration during parsing: {:?}", diff --git a/libs/datamodel/core/src/ast/renderer/mod.rs b/libs/datamodel/core/src/ast/renderer/mod.rs index 97d23a3e80f9..a6af8cbeceab 100644 --- a/libs/datamodel/core/src/ast/renderer/mod.rs +++ b/libs/datamodel/core/src/ast/renderer/mod.rs @@ -176,7 +176,7 @@ impl<'a> Renderer<'a> { let mut field_formatter = TableFormat::new(); for field in &model.fields { - Self::render_field(&mut field_formatter, &field, comment_out.clone()); + Self::render_field(&mut field_formatter, &field, model.commented_out); } field_formatter.render(self); @@ -231,9 +231,15 @@ impl<'a> Renderer<'a> { self.end_line(); } - fn render_field(target: &mut TableFormat, field: &ast::Field, commented_out: String) { + fn render_field(target: &mut TableFormat, field: &ast::Field, is_commented_out: bool) { Self::render_documentation(&mut target.interleave_writer(), field); + let commented_out = if field.is_commented_out || is_commented_out { + "// ".to_string() + } else { + "".to_string() + }; + target.write(format!("{}{}", &commented_out, &field.name.name).as_ref()); // Type diff --git a/libs/datamodel/core/src/validator/lower.rs b/libs/datamodel/core/src/validator/lower.rs index b12627ae0ce1..a6d63b5d39bf 100644 --- a/libs/datamodel/core/src/validator/lower.rs +++ b/libs/datamodel/core/src/validator/lower.rs @@ -103,6 +103,7 @@ impl LowerDmlToAst { field_type: self.lower_type(&field.field_type), documentation: field.documentation.clone().map(|text| ast::Comment { text }), span: ast::Span::empty(), + is_commented_out: field.is_commented_out, }) } diff --git a/migration-engine/core/src/migration/datamodel_calculator.rs b/migration-engine/core/src/migration/datamodel_calculator.rs index 90343358cd78..68c8c30b1317 100644 --- a/migration-engine/core/src/migration/datamodel_calculator.rs +++ b/migration-engine/core/src/migration/datamodel_calculator.rs @@ -172,6 +172,7 @@ fn apply_create_field(datamodel: &mut ast::SchemaAst, step: &steps::CreateField) span: new_span(), directives: Vec::new(), default_value: None, + is_commented_out: false, }; model.fields.push(field); @@ -482,6 +483,7 @@ fn apply_create_type_alias( arity: step.arity.into(), directives: vec![], field_type: new_ident(step.r#type.clone()), + is_commented_out: false, }; datamodel.tops.push(ast::Top::Type(type_alias));