From 7faebbcfc3f7da0aa1b3f63856684edc30372393 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 31 Aug 2023 16:09:09 -0400 Subject: [PATCH] Fix ambiguous reference error for duplicate model names across packages with tests (#8488) (#8497) --- .../unreleased/Fixes-20230824-161024.yaml | 7 ++ core/dbt/parser/schema_generic_tests.py | 2 +- core/dbt/parser/schemas.py | 4 +- .../duplicates/test_duplicate_model.py | 86 +++++++++++++++++++ tests/unit/test_parser.py | 4 +- 5 files changed, 98 insertions(+), 5 deletions(-) create mode 100644 .changes/unreleased/Fixes-20230824-161024.yaml diff --git a/.changes/unreleased/Fixes-20230824-161024.yaml b/.changes/unreleased/Fixes-20230824-161024.yaml new file mode 100644 index 00000000000..d39ee14ce99 --- /dev/null +++ b/.changes/unreleased/Fixes-20230824-161024.yaml @@ -0,0 +1,7 @@ +kind: Fixes +body: fix ambiguous reference error for tests and versions when model name is duplicated across + packages +time: 2023-08-24T16:10:24.437362-04:00 +custom: + Author: michelleark + Issue: "8327 8493" diff --git a/core/dbt/parser/schema_generic_tests.py b/core/dbt/parser/schema_generic_tests.py index 29b71c21e92..c4994e98a74 100644 --- a/core/dbt/parser/schema_generic_tests.py +++ b/core/dbt/parser/schema_generic_tests.py @@ -233,7 +233,7 @@ def _lookup_attached_node( attached_node = None # type: Optional[Union[ManifestNode, GraphMemberNode]] if not isinstance(target, UnpatchedSourceDefinition): attached_node_unique_id = self.manifest.ref_lookup.get_unique_id( - target.name, None, version + target.name, target.package_name, version ) if attached_node_unique_id: attached_node = self.manifest.nodes[attached_node_unique_id] diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index fbc95a73df7..24106a85224 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -693,7 +693,7 @@ def parse_patch(self, block: TargetBlock[UnparsedModelUpdate], refs: ParserRef) ) # ref lookup without version - version is not set yet versioned_model_unique_id = self.manifest.ref_lookup.get_unique_id( - versioned_model_name, None, None + versioned_model_name, target.package_name, None ) versioned_model_node = None @@ -702,7 +702,7 @@ def parse_patch(self, block: TargetBlock[UnparsedModelUpdate], refs: ParserRef) # If this is the latest version, it's allowed to define itself in a model file name that doesn't have a suffix if versioned_model_unique_id is None and unparsed_version.v == latest_version: versioned_model_unique_id = self.manifest.ref_lookup.get_unique_id( - block.name, None, None + block.name, target.package_name, None ) if versioned_model_unique_id is None: diff --git a/tests/functional/duplicates/test_duplicate_model.py b/tests/functional/duplicates/test_duplicate_model.py index 17be1ff20b9..854bd42486d 100644 --- a/tests/functional/duplicates/test_duplicate_model.py +++ b/tests/functional/duplicates/test_duplicate_model.py @@ -43,6 +43,26 @@ """ +local_dep_schema_yml = """ +models: + - name: table_model + config: + alias: table_model_local_dep + columns: + - name: id + tests: + - unique +""" + +local_dep_versions_schema_yml = """ +models: + - name: table_model + config: + alias: table_model_local_dep + versions: + - v: 1 +""" + class TestDuplicateModelEnabled: @pytest.fixture(scope="class") @@ -142,6 +162,72 @@ def test_duplicate_model_disabled_across_packages(self, project): assert model_id in manifest.disabled +class TestDuplicateModelNameWithTestAcrossPackages: + @pytest.fixture(scope="class", autouse=True) + def setUp(self, project_root): + local_dependency_files = { + "dbt_project.yml": dbt_project_yml, + "models": {"table_model.sql": enabled_model_sql, "schema.yml": local_dep_schema_yml}, + } + write_project_files(project_root, "local_dependency", local_dependency_files) + + @pytest.fixture(scope="class") + def models(self): + return {"table_model.sql": enabled_model_sql} + + @pytest.fixture(scope="class") + def packages(self): + return {"packages": [{"local": "local_dependency"}]} + + def test_duplicate_model_name_with_test_across_packages(self, project): + run_dbt(["deps"]) + manifest = run_dbt(["parse"]) + assert len(manifest.nodes) == 3 + + # model nodes with duplicate names exist + local_dep_model_node_id = "model.local_dep.table_model" + root_model_node_id = "model.test.table_model" + assert local_dep_model_node_id in manifest.nodes + assert root_model_node_id in manifest.nodes + + # test node exists and is attached to correct node + test_node_id = "test.local_dep.unique_table_model_id.1da9e464d9" + assert test_node_id in manifest.nodes + assert manifest.nodes[test_node_id].attached_node == local_dep_model_node_id + + +class TestDuplicateModelNameWithVersionAcrossPackages: + @pytest.fixture(scope="class", autouse=True) + def setUp(self, project_root): + local_dependency_files = { + "dbt_project.yml": dbt_project_yml, + "models": { + "table_model.sql": enabled_model_sql, + "schema.yml": local_dep_versions_schema_yml, + }, + } + write_project_files(project_root, "local_dependency", local_dependency_files) + + @pytest.fixture(scope="class") + def models(self): + return {"table_model.sql": enabled_model_sql} + + @pytest.fixture(scope="class") + def packages(self): + return {"packages": [{"local": "local_dependency"}]} + + def test_duplicate_model_name_with_test_across_packages(self, project): + run_dbt(["deps"]) + manifest = run_dbt(["parse"]) + assert len(manifest.nodes) == 2 + + # model nodes with duplicate names exist + local_dep_model_node_id = "model.local_dep.table_model.v1" + root_model_node_id = "model.test.table_model" + assert local_dep_model_node_id in manifest.nodes + assert root_model_node_id in manifest.nodes + + class TestModelTestOverlap: @pytest.fixture(scope="class") def models(self): diff --git a/tests/unit/test_parser.py b/tests/unit/test_parser.py index e04f93c367b..64bed3825ce 100644 --- a/tests/unit/test_parser.py +++ b/tests/unit/test_parser.py @@ -612,7 +612,7 @@ class SchemaParserVersionedModels(SchemaParserTest): def setUp(self): super().setUp() my_model_v1_node = MockNode( - package="root", + package="snowplow", name="arbitrary_file_name", config=mock.MagicMock(enabled=True), refs=[], @@ -621,7 +621,7 @@ def setUp(self): file_id="snowplow://models/arbitrary_file_name.sql", ) my_model_v2_node = MockNode( - package="root", + package="snowplow", name="my_model_v2", config=mock.MagicMock(enabled=True), refs=[],