From 9c6abca6cb378917ccce54fb4446e32440bb7ff6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=20Houl=C3=A9?= Date: Fri, 30 Sep 2022 16:25:40 +0200 Subject: [PATCH] psl: fix database name clash validation with multiSchema The validation we have validates that no two models in the whole schema have the same database name. Database name is defined as the contents of the model's `@@map` attribute, or the model name if the model name does not have an `@@map` attribute. It is the name we give to the table/collection corresponding to the model in the database. When the `multiSchema` preview feature is turned on, the tables in the database can be in different schemas, so we have to adapt the validation to allow multiple models to have the same database name, as long as they are in a different schema. This is what this commit does. closes https://github.com/prisma/prisma/issues/15009 --- psl/parser-database/src/attributes.rs | 2 +- psl/parser-database/src/attributes/map.rs | 20 +------- psl/parser-database/src/context.rs | 2 - psl/parser-database/src/lib.rs | 2 +- .../validation_pipeline/validations.rs | 4 +- .../validation_pipeline/validations/models.rs | 48 ++++++++++++++++++- ...icate_models_with_map_on_both_sides.prisma | 23 +++++++++ ...erent_schemas_with_same_mapped_name.prisma | 30 ++++++++++++ ...n_same_schema_with_same_mapped_name.prisma | 31 ++++++++++++ 9 files changed, 136 insertions(+), 26 deletions(-) create mode 100644 psl/psl/tests/validation/attributes/map/duplicate_models_with_map_on_both_sides.prisma create mode 100644 psl/psl/tests/validation/attributes/schema/tables_in_different_schemas_with_same_mapped_name.prisma create mode 100644 psl/psl/tests/validation/attributes/schema/tables_in_same_schema_with_same_mapped_name.prisma diff --git a/psl/parser-database/src/attributes.rs b/psl/parser-database/src/attributes.rs index ac5a0276d450..6e34b6e41ca1 100644 --- a/psl/parser-database/src/attributes.rs +++ b/psl/parser-database/src/attributes.rs @@ -155,7 +155,7 @@ fn resolve_model_attributes<'db>(model_id: ast::ModelId, ast_model: &'db ast::Mo // @@map if ctx.visit_optional_single_attr("map") { - map::model(&mut model_attributes, model_id, ctx); + map::model(&mut model_attributes, ctx); ctx.validate_visited_arguments(); } diff --git a/psl/parser-database/src/attributes/map.rs b/psl/parser-database/src/attributes/map.rs index 5aed9dcff68e..28b9c4444f55 100644 --- a/psl/parser-database/src/attributes/map.rs +++ b/psl/parser-database/src/attributes/map.rs @@ -6,31 +6,13 @@ use crate::{ DatamodelError, StringId, }; -pub(super) fn model(model_attributes: &mut ModelAttributes, model_id: ast::ModelId, ctx: &mut Context<'_>) { +pub(super) fn model(model_attributes: &mut ModelAttributes, ctx: &mut Context<'_>) { let mapped_name = match visit_map_attribute(ctx) { Some(name) => name, None => return, }; model_attributes.mapped_name = Some(mapped_name); - - if let Some(existing_model_id) = ctx.mapped_model_names.insert(mapped_name, model_id) { - let existing_model_name = ctx.ast[existing_model_id].name(); - ctx.push_error(DatamodelError::new_duplicate_model_database_name_error( - &ctx[mapped_name], - existing_model_name, - ctx.ast[model_id].span(), - )); - } - - if let Some(existing_model_id) = ctx.names.tops.get(&mapped_name).and_then(|id| id.as_model_id()) { - let existing_model_name = ctx.ast[existing_model_id].name(); - ctx.push_error(DatamodelError::new_duplicate_model_database_name_error( - &ctx[mapped_name], - existing_model_name, - ctx.current_attribute().span, - )); - } } pub(super) fn scalar_field( diff --git a/psl/parser-database/src/context.rs b/psl/parser-database/src/context.rs index aa37cca7bb6d..54c06ddd9a42 100644 --- a/psl/parser-database/src/context.rs +++ b/psl/parser-database/src/context.rs @@ -30,7 +30,6 @@ pub(crate) struct Context<'db> { attributes: AttributesValidationState, // state machine for attribute validation // @map'ed names indexes. These are not in the db because they are only used for validation. - pub(super) mapped_model_names: HashMap, pub(super) mapped_model_scalar_field_names: HashMap<(ast::ModelId, StringId), ast::FieldId>, pub(super) mapped_composite_type_names: HashMap<(ast::CompositeTypeId, StringId), ast::FieldId>, pub(super) mapped_enum_names: HashMap, @@ -55,7 +54,6 @@ impl<'db> Context<'db> { diagnostics, attributes: AttributesValidationState::default(), - mapped_model_names: Default::default(), mapped_model_scalar_field_names: Default::default(), mapped_enum_names: Default::default(), mapped_enum_value_names: Default::default(), diff --git a/psl/parser-database/src/lib.rs b/psl/parser-database/src/lib.rs index 4fde9cff3c96..c9575b667d47 100644 --- a/psl/parser-database/src/lib.rs +++ b/psl/parser-database/src/lib.rs @@ -140,7 +140,7 @@ impl ParserDatabase { &self.ast } - /// The total number of models. + /// The total number of models. This is O(1). pub fn models_count(&self) -> usize { self.types.model_attributes.len() } diff --git a/psl/psl-core/src/validate/validation_pipeline/validations.rs b/psl/psl-core/src/validate/validation_pipeline/validations.rs index f8fd19e383c3..22474016202b 100644 --- a/psl/psl-core/src/validate/validation_pipeline/validations.rs +++ b/psl/psl-core/src/validate/validation_pipeline/validations.rs @@ -36,10 +36,12 @@ pub(super) fn validate(ctx: &mut Context<'_>) { } } - // Model validations ctx.connector .validate_scalar_field_unknown_default_functions(ctx.db, ctx.diagnostics); + // Model validations + models::database_name_clashes(ctx); + for model in db.walk_models() { models::has_a_strict_unique_criteria(model, ctx); models::has_a_unique_primary_key_name(model, &names, ctx); diff --git a/psl/psl-core/src/validate/validation_pipeline/validations/models.rs b/psl/psl-core/src/validate/validation_pipeline/validations/models.rs index 78b228152899..74dbb5d3aebf 100644 --- a/psl/psl-core/src/validate/validation_pipeline/validations/models.rs +++ b/psl/psl-core/src/validate/validation_pipeline/validations/models.rs @@ -1,13 +1,14 @@ use super::database_name::validate_db_name; use crate::{ + ast, common::preview_features::PreviewFeature, datamodel_connector::{walker_ext_traits::*, ConnectorCapability}, diagnostics::DatamodelError, - parser_database::ast::WithSpan, + parser_database::ast::{WithName, WithSpan}, validate::validation_pipeline::context::Context, }; use parser_database::walkers::{ModelWalker, PrimaryKeyWalker}; -use std::borrow::Cow; +use std::{borrow::Cow, collections::HashMap}; /// A model must have either a primary key, or a unique criteria /// with no optional, commented-out or unsupported fields. @@ -298,3 +299,46 @@ pub(super) fn schema_attribute(model: ModelWalker<'_>, ctx: &mut Context<'_>) { _ => (), } } + +pub(super) fn database_name_clashes(ctx: &mut Context<'_>) { + // (schema_name, model_database_name) -> ModelId + let mut database_names: HashMap<(Option<&str>, &str), ast::ModelId> = HashMap::with_capacity(ctx.db.models_count()); + + for model in ctx.db.walk_models() { + let key = (model.schema().map(|(name, _)| name), model.database_name()); + match database_names.insert(key, model.model_id()) { + // Two branches because we want to put the error on the @@map attribute, and it can be + // on either model. + Some(existing) if model.mapped_name().is_some() => { + let existing_model_name = &ctx.db.ast()[existing].name(); + let attribute = model + .ast_model() + .attributes + .iter() + .find(|attr| attr.name() == "map") + .unwrap(); + + ctx.push_error(DatamodelError::new_duplicate_model_database_name_error( + model.database_name(), + existing_model_name, + attribute.span(), + )); + } + Some(existing) => { + let existing_model = &ctx.db.ast()[existing]; + let attribute = existing_model + .attributes + .iter() + .find(|attr| attr.name() == "map") + .unwrap(); + + ctx.push_error(DatamodelError::new_duplicate_model_database_name_error( + model.database_name(), + model.name(), + attribute.span(), + )); + } + None => (), + } + } +} diff --git a/psl/psl/tests/validation/attributes/map/duplicate_models_with_map_on_both_sides.prisma b/psl/psl/tests/validation/attributes/map/duplicate_models_with_map_on_both_sides.prisma new file mode 100644 index 000000000000..6ee838e2d32b --- /dev/null +++ b/psl/psl/tests/validation/attributes/map/duplicate_models_with_map_on_both_sides.prisma @@ -0,0 +1,23 @@ +datasource mydb { + provider = "sqlite" + url = env("TEST_DB_URL") +} + +model Dog { + id Int @id + + @@map("pets") +} + +model Cat { + id Int @id + + @@map("pets") +} + +// error: The model with database name "pets" could not be defined because another model with this name exists: "Dog" +// --> schema.prisma:15 +//  |  +// 14 |  +// 15 |  @@map("pets") +//  |  diff --git a/psl/psl/tests/validation/attributes/schema/tables_in_different_schemas_with_same_mapped_name.prisma b/psl/psl/tests/validation/attributes/schema/tables_in_different_schemas_with_same_mapped_name.prisma new file mode 100644 index 000000000000..a50b66a4c545 --- /dev/null +++ b/psl/psl/tests/validation/attributes/schema/tables_in_different_schemas_with_same_mapped_name.prisma @@ -0,0 +1,30 @@ +// issue: https://github.com/prisma/prisma/issues/15009 + +generator client { + provider = "prisma-client-js" + previewFeatures = ["multiSchema"] +} + +datasource db { + provider = "postgresql" + url = env("TEST_DATABASE_URL") + schemas = ["base", "transactional"] +} + +model User { + id String @id + email String + posts Post[] + + @@map("some_table") + @@schema("base") +} + +model Post { + title String + authorId String @unique + author User? @relation(fields: [authorId], references: [id]) + + @@map("some_table") + @@schema("transactional") +} diff --git a/psl/psl/tests/validation/attributes/schema/tables_in_same_schema_with_same_mapped_name.prisma b/psl/psl/tests/validation/attributes/schema/tables_in_same_schema_with_same_mapped_name.prisma new file mode 100644 index 000000000000..52ad5c4dc659 --- /dev/null +++ b/psl/psl/tests/validation/attributes/schema/tables_in_same_schema_with_same_mapped_name.prisma @@ -0,0 +1,31 @@ +datasource mydb { + provider = "sqlserver" + url = env("TEST_DB_URL") + schemas = ["base", "transactional"] +} + +generator client { + provider = "prisma-client-js" + previewFeatures = ["multiSchema"] +} + + +model Dog { + id Int @id + + @@map("pets") +} + +model Cat { + id Int @id + + @@map("pets") +} + +// error: The model with database name "pets" could not be defined because another model with this name exists: "Dog" +// --> schema.prisma:15 +//  |  +// 14 |  +// 15 |  @@map("pets") +//  |  +