diff --git a/.changes/unreleased/Fixes-20220916-104854.yaml b/.changes/unreleased/Fixes-20220916-104854.yaml new file mode 100644 index 00000000000..64e76c43a3f --- /dev/null +++ b/.changes/unreleased/Fixes-20220916-104854.yaml @@ -0,0 +1,7 @@ +kind: Fixes +body: Account for disabled flags on models in schema files more completely +time: 2022-09-16T10:48:54.162273-05:00 +custom: + Author: emmyoop + Issue: "3992" + PR: "5868" diff --git a/core/dbt/contracts/graph/manifest.py b/core/dbt/contracts/graph/manifest.py index edf1a5b40ef..33a09bc3a8c 100644 --- a/core/dbt/contracts/graph/manifest.py +++ b/core/dbt/contracts/graph/manifest.py @@ -1088,8 +1088,13 @@ def add_node_nofile(self, node: ManifestNodes): def add_node(self, source_file: AnySourceFile, node: ManifestNodes, test_from=None): self.add_node_nofile(node) if isinstance(source_file, SchemaSourceFile): - assert test_from - source_file.add_test(node.unique_id, test_from) + if isinstance(node, ParsedGenericTestNode): + assert test_from + source_file.add_test(node.unique_id, test_from) + if isinstance(node, ParsedMetric): + source_file.metrics.append(node.unique_id) + if isinstance(node, ParsedExposure): + source_file.exposures.append(node.unique_id) else: source_file.nodes.append(node.unique_id) diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index 74affdce9e6..43733712018 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -1,3 +1,4 @@ +from copy import deepcopy from dataclasses import dataclass from dataclasses import field from datetime import datetime @@ -368,6 +369,8 @@ def load(self): project, project_parser_files[project.project_name], parser_types ) + self.process_nodes() + self._perf_info.parse_project_elapsed = time.perf_counter() - start_parse_projects # patch_sources converts the UnparsedSourceDefinitions in the @@ -468,6 +471,7 @@ def parse_project( dct = block.file.pp_dict else: dct = block.file.dict_from_yaml + # this is where the schema file gets parsed parser.parse_file(block, dct=dct) # Came out of here with UnpatchedSourceDefinition containing configs at the source level # and not configs at the table level (as expected) @@ -926,6 +930,31 @@ def process_sources(self, current_project: str): continue _process_sources_for_exposure(self.manifest, current_project, exposure) + def process_nodes(self): + # make sure the nodes are in the manifest.nodes or the disabled dict, + # correctly now that the schema files are also parsed + disabled_nodes = [] + for node in self.manifest.nodes.values(): + if not node.config.enabled: + disabled_nodes.append(node.unique_id) + self.manifest.add_disabled_nofile(node) + for unique_id in disabled_nodes: + self.manifest.nodes.pop(unique_id) + + disabled_copy = deepcopy(self.manifest.disabled) + for disabled in disabled_copy.values(): + for node in disabled: + if node.config.enabled: + for dis_index, dis_node in enumerate(disabled): + # Remove node from disabled and unique_id from disabled dict if necessary + del self.manifest.disabled[node.unique_id][dis_index] + if not self.manifest.disabled[node.unique_id]: + self.manifest.disabled.pop(node.unique_id) + + self.manifest.add_node_nofile(node) + + self.manifest.rebuild_ref_lookup() + def invalid_ref_fail_unless_test(node, target_model_name, target_model_package, disabled): diff --git a/core/dbt/parser/partial.py b/core/dbt/parser/partial.py index a14298752f6..37c7ec94cdd 100644 --- a/core/dbt/parser/partial.py +++ b/core/dbt/parser/partial.py @@ -834,17 +834,24 @@ def delete_schema_mssa_links(self, schema_file, dict_key, elem): # remove elem node and remove unique_id from node_patches if elem_unique_id: # might have been already removed - if elem_unique_id in self.saved_manifest.nodes: - node = self.saved_manifest.nodes.pop(elem_unique_id) - self.deleted_manifest.nodes[elem_unique_id] = node + if ( + elem_unique_id in self.saved_manifest.nodes + or elem_unique_id in self.saved_manifest.disabled + ): + if elem_unique_id in self.saved_manifest.nodes: + nodes = [self.saved_manifest.nodes.pop(elem_unique_id)] + else: + # The value of disabled items is a list of nodes + nodes = self.saved_manifest.disabled.pop(elem_unique_id) # need to add the node source_file to pp_files - file_id = node.file_id - # need to copy new file to saved files in order to get content - if file_id in self.new_files: - self.saved_files[file_id] = deepcopy(self.new_files[file_id]) - if self.saved_files[file_id]: - source_file = self.saved_files[file_id] - self.add_to_pp_files(source_file) + for node in nodes: + file_id = node.file_id + # need to copy new file to saved files in order to get content + if file_id in self.new_files: + self.saved_files[file_id] = deepcopy(self.new_files[file_id]) + if self.saved_files[file_id]: + source_file = self.saved_files[file_id] + self.add_to_pp_files(source_file) # remove from patches schema_file.node_patches.remove(elem_unique_id) diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index dcdca271fe5..4a6202ff0b8 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -508,6 +508,8 @@ def parse_file(self, block: FileBlock, dct: Dict = None) -> None: # NonSourceParser.parse(), TestablePatchParser is a variety of # NodePatchParser if "models" in dct: + # the models are already in the manifest as nodes when we reach this code, + # even if they are disabled in the schema file parser = TestablePatchParser(self, yaml_block, "models") for test_block in parser.parse(): self.parse_tests(test_block) @@ -775,9 +777,10 @@ def parse(self) -> List[TestBlock]: refs: ParserRef = ParserRef.from_target(node) else: refs = ParserRef() - # This adds the node_block to self.manifest - # as a ParsedNodePatch or ParsedMacroPatch + + # There's no unique_id on the node yet so cannot add to disabled dict self.parse_patch(node_block, refs) + # This will always be empty if the node a macro or analysis return test_blocks @@ -881,15 +884,34 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: f"Unexpected yaml_key {patch.yaml_key} for patch in " f"file {source_file.path.original_file_path}" ) + # handle disabled nodes if unique_id is None: # Node might be disabled. Following call returns list of matching disabled nodes found_nodes = self.manifest.disabled_lookup.find(patch.name, patch.package_name) if found_nodes: - # There might be multiple disabled nodes for this model + if len(found_nodes) > 1 and patch.config.get("enabled"): + # There are multiple disabled nodes for this model and the schema file wants to enable one. + # We have no way to know which one to enable. + resource_type = found_nodes[0].unique_id.split(".")[0] + msg = ( + f"Found {len(found_nodes)} matching disabled nodes for " + f"{resource_type} '{patch.name}'. Multiple nodes for the same " + "unique id cannot be enabled in the schema file. They must be enabled " + "in `dbt_project.yml` or in the sql files." + ) + raise ParsingException(msg) + + # all nodes in the disabled dict have the same unique_id so just grab the first one + # to append with the uniqe id + source_file.append_patch(patch.yaml_key, found_nodes[0].unique_id) for node in found_nodes: - # We're saving the patch_path because we need to schedule - # re-application of the patch in partial parsing. node.patch_path = source_file.file_id + # re-calculate the node config with the patch config. Always do this + # for the case when no config is set to ensure the default of true gets captured + if patch.config: + self.patch_node_config(node, patch) + + node.patch(patch) else: msg = ( f"Did not find matching node for patch with name '{patch.name}' " @@ -905,11 +927,13 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: if node.patch_path: package_name, existing_file_path = node.patch_path.split("://") raise_duplicate_patch_name(patch, existing_file_path) - source_file.append_patch(patch.yaml_key, unique_id) - # If this patch has config changes, re-calculate the node config - # with the patch config + + source_file.append_patch(patch.yaml_key, node.unique_id) + # re-calculate the node config with the patch config. Always do this + # for the case when no config is set to ensure the default of true gets captured if patch.config: self.patch_node_config(node, patch) + node.patch(patch) diff --git a/test/integration/068_partial_parsing_tests/test-files/models-schema4.yml b/test/integration/068_partial_parsing_tests/test-files/models-schema4.yml new file mode 100644 index 00000000000..8087615fe49 --- /dev/null +++ b/test/integration/068_partial_parsing_tests/test-files/models-schema4.yml @@ -0,0 +1,13 @@ +version: 2 + +models: + - name: model_one + description: "The first model" + - name: model_three + description: "The third model" + config: + enabled: false + columns: + - name: id + tests: + - unique diff --git a/test/integration/068_partial_parsing_tests/test-files/models-schema4b.yml b/test/integration/068_partial_parsing_tests/test-files/models-schema4b.yml new file mode 100644 index 00000000000..e73ffcef1de --- /dev/null +++ b/test/integration/068_partial_parsing_tests/test-files/models-schema4b.yml @@ -0,0 +1,13 @@ +version: 2 + +models: + - name: model_one + description: "The first model" + - name: model_three + description: "The third model" + config: + enabled: true + columns: + - name: id + tests: + - unique diff --git a/test/integration/068_partial_parsing_tests/test_partial_parsing.py b/test/integration/068_partial_parsing_tests/test_partial_parsing.py index 66a2cf90d7c..648abdc4657 100644 --- a/test/integration/068_partial_parsing_tests/test_partial_parsing.py +++ b/test/integration/068_partial_parsing_tests/test_partial_parsing.py @@ -172,6 +172,26 @@ def test_postgres_pp_models(self): results = self.run_dbt(["--partial-parse", "run"]) self.assertEqual(len(results), 3) + # disable model three in the schema file + self.copy_file('test-files/models-schema4.yml', 'models/schema.yml') + results = self.run_dbt(["--partial-parse", "run"]) + self.assertEqual(len(results), 2) + + # update enabled config to be true for model three in the schema file + self.copy_file('test-files/models-schema4b.yml', 'models/schema.yml') + results = self.run_dbt(["--partial-parse", "run"]) + self.assertEqual(len(results), 3) + + # disable model three in the schema file again + self.copy_file('test-files/models-schema4.yml', 'models/schema.yml') + results = self.run_dbt(["--partial-parse", "run"]) + self.assertEqual(len(results), 2) + + # remove disabled config for model three in the schema file to check it gets enabled + self.copy_file('test-files/models-schema3.yml', 'models/schema.yml') + results = self.run_dbt(["--partial-parse", "run"]) + self.assertEqual(len(results), 3) + # Add a macro self.copy_file('test-files/my_macro.sql', 'macros/my_macro.sql') results = self.run_dbt(["--partial-parse", "run"]) diff --git a/tests/functional/configs/fixtures.py b/tests/functional/configs/fixtures.py index eb744f93430..ae3173490b7 100644 --- a/tests/functional/configs/fixtures.py +++ b/tests/functional/configs/fixtures.py @@ -78,6 +78,82 @@ """ +my_model = """ +select 1 as user +""" + +my_model_2 = """ +select * from {{ ref('my_model') }} +""" + +my_model_3 = """ +select * from {{ ref('my_model_2') }} +""" + +my_model_2_disabled = """ +{{ config(enabled=false) }} +select * from {{ ref('my_model') }} +""" + +my_model_3_disabled = """ +{{ config(enabled=false) }} +select * from {{ ref('my_model_2') }} +""" + +my_model_2_enabled = """ +{{ config(enabled=true) }} +select * from {{ ref('my_model') }} +""" + +my_model_3_enabled = """ +{{ config(enabled=true) }} +select * from {{ ref('my_model') }} +""" + +schema_all_disabled_yml = """ +version: 2 +models: + - name: my_model + - name: my_model_2 + config: + enabled: false + - name: my_model_3 + config: + enabled: false +""" + +schema_explicit_enabled_yml = """ +version: 2 +models: + - name: my_model + - name: my_model_2 + config: + enabled: true + - name: my_model_3 + config: + enabled: true +""" + +schema_partial_disabled_yml = """ +version: 2 +models: + - name: my_model + - name: my_model_2 + config: + enabled: false + - name: my_model_3 +""" + +schema_partial_enabled_yml = """ +version: 2 +models: + - name: my_model + - name: my_model_2 + config: + enabled: True + - name: my_model_3 +""" + class BaseConfigProject: @pytest.fixture(scope="class") diff --git a/tests/functional/configs/test_disabled_model.py b/tests/functional/configs/test_disabled_model.py new file mode 100644 index 00000000000..3d8d9f28577 --- /dev/null +++ b/tests/functional/configs/test_disabled_model.py @@ -0,0 +1,385 @@ +import pytest +from dbt.tests.util import run_dbt, get_manifest + +from dbt.exceptions import CompilationException, ParsingException + +from tests.functional.configs.fixtures import ( + schema_all_disabled_yml, + schema_partial_enabled_yml, + schema_partial_disabled_yml, + schema_explicit_enabled_yml, + my_model, + my_model_2, + my_model_2_enabled, + my_model_2_disabled, + my_model_3, + my_model_3_disabled, + my_model_3_enabled, +) + + +# ensure double disabled doesn't throw error when set at schema level +class TestSchemaDisabledConfigs: + @pytest.fixture(scope="class") + def models(self): + return { + "schema.yml": schema_all_disabled_yml, + "my_model.sql": my_model, + "my_model_2.sql": my_model_2, + "my_model_3.sql": my_model_3, + } + + def test_disabled_config(self, project): + run_dbt(["parse"]) + + +# ensure this throws a specific error that the model is disabled +class TestSchemaDisabledConfigsFailure: + @pytest.fixture(scope="class") + def models(self): + return { + "schema.yml": schema_partial_disabled_yml, + "my_model.sql": my_model, + "my_model_2.sql": my_model_2, + "my_model_3.sql": my_model_3, + } + + def test_disabled_config(self, project): + with pytest.raises(CompilationException) as exc: + run_dbt(["parse"]) + exc_str = " ".join(str(exc.value).split()) # flatten all whitespace + expected_msg = "which is disabled" + assert expected_msg in exc_str + + +# ensure double disabled doesn't throw error when set in model configs +class TestModelDisabledConfigs: + @pytest.fixture(scope="class") + def models(self): + return { + "my_model.sql": my_model, + "my_model_2.sql": my_model_2_disabled, + "my_model_3.sql": my_model_3_disabled, + } + + def test_disabled_config(self, project): + run_dbt(["parse"]) + manifest = get_manifest(project.project_root) + assert "model.test.my_model_2" not in manifest.nodes + assert "model.test.my_model_3" not in manifest.nodes + + assert "model.test.my_model_2" in manifest.disabled + assert "model.test.my_model_3" in manifest.disabled + + +# ensure config set in project.yml can be overridden in yaml file +class TestOverrideProjectConfigsInYaml: + @pytest.fixture(scope="class") + def models(self): + return { + "schema.yml": schema_partial_enabled_yml, + "my_model.sql": my_model, + "my_model_2.sql": my_model_2, + "my_model_3.sql": my_model_3, + } + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "models": { + "test": { + "my_model_2": { + "enabled": False, + }, + "my_model_3": { + "enabled": False, + }, + }, + } + } + + def test_override_project_yaml_config(self, project): + run_dbt(["parse"]) + manifest = get_manifest(project.project_root) + assert "model.test.my_model_2" in manifest.nodes + assert "model.test.my_model_3" not in manifest.nodes + + assert "model.test.my_model_2" not in manifest.disabled + assert "model.test.my_model_3" in manifest.disabled + + +# ensure config set in project.yml can be overridden in sql file +class TestOverrideProjectConfigsInSQL: + @pytest.fixture(scope="class") + def models(self): + return { + "my_model.sql": my_model, + "my_model_2.sql": my_model_2_enabled, + "my_model_3.sql": my_model_3, + } + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "models": { + "test": { + "my_model_2": { + "enabled": False, + }, + "my_model_3": { + "enabled": False, + }, + }, + } + } + + def test_override_project_sql_config(self, project): + run_dbt(["parse"]) + manifest = get_manifest(project.project_root) + assert "model.test.my_model_2" in manifest.nodes + assert "model.test.my_model_3" not in manifest.nodes + + assert "model.test.my_model_2" not in manifest.disabled + assert "model.test.my_model_3" in manifest.disabled + + +# ensure false config set in yaml file can be overridden in sql file +class TestOverrideFalseYAMLConfigsInSQL: + @pytest.fixture(scope="class") + def models(self): + return { + "schema.yml": schema_all_disabled_yml, + "my_model.sql": my_model, + "my_model_2.sql": my_model_2_enabled, + "my_model_3.sql": my_model_3, + } + + def test_override_yaml_sql_config(self, project): + run_dbt(["parse"]) + manifest = get_manifest(project.project_root) + assert "model.test.my_model_2" in manifest.nodes + assert "model.test.my_model_3" not in manifest.nodes + + assert "model.test.my_model_2" not in manifest.disabled + assert "model.test.my_model_3" in manifest.disabled + + +# ensure true config set in yaml file can be overridden by false in sql file +class TestOverrideTrueYAMLConfigsInSQL: + @pytest.fixture(scope="class") + def models(self): + return { + "schema.yml": schema_explicit_enabled_yml, + "my_model.sql": my_model, + "my_model_2.sql": my_model_2_enabled, + "my_model_3.sql": my_model_3_disabled, + } + + def test_override_yaml_sql_config(self, project): + run_dbt(["parse"]) + manifest = get_manifest(project.project_root) + assert "model.test.my_model_2" in manifest.nodes + assert "model.test.my_model_3" not in manifest.nodes + + assert "model.test.my_model_2" not in manifest.disabled + assert "model.test.my_model_3" in manifest.disabled + + +# ensure error when enabling in schema file when multiple nodes exist within disabled +class TestMultipleDisabledNodesForUniqueIDFailure: + @pytest.fixture(scope="class") + def models(self): + return { + "schema.yml": schema_partial_enabled_yml, + "my_model.sql": my_model, + "folder_1": { + "my_model_2.sql": my_model_2_disabled, + "my_model_3.sql": my_model_3_disabled, + }, + "folder_2": { + "my_model_2.sql": my_model_2_disabled, + "my_model_3.sql": my_model_3_disabled, + }, + "folder_3": { + "my_model_2.sql": my_model_2_disabled, + "my_model_3.sql": my_model_3_disabled, + }, + } + + def test_disabled_config(self, project): + with pytest.raises(ParsingException) as exc: + run_dbt(["parse"]) + exc_str = " ".join(str(exc.value).split()) # flatten all whitespace + expected_msg = "Found 3 matching disabled nodes for model 'my_model_2'" + assert expected_msg in exc_str + + +# ensure error when enabling in schema file when multiple nodes exist within disabled +class TestMultipleDisabledNodesSuccess: + @pytest.fixture(scope="class") + def models(self): + return { + "my_model.sql": my_model, + "folder_1": { + "my_model_2.sql": my_model_2, + "my_model_3.sql": my_model_3, + }, + "folder_2": { + "my_model_2.sql": my_model_2, + "my_model_3.sql": my_model_3, + }, + } + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "models": { + "test": { + "folder_1": { + "enabled": False, + }, + "folder_2": { + "enabled": True, + }, + }, + } + } + + def test_multiple_disabled_config(self, project): + run_dbt(["parse"]) + manifest = get_manifest(project.project_root) + assert "model.test.my_model_2" in manifest.nodes + assert "model.test.my_model_3" in manifest.nodes + + expected_file_path = "folder_2" + assert expected_file_path in manifest.nodes["model.test.my_model_2"].original_file_path + assert expected_file_path in manifest.nodes["model.test.my_model_3"].original_file_path + + assert "model.test.my_model_2" in manifest.disabled + assert "model.test.my_model_3" in manifest.disabled + + expected_disabled_file_path = "folder_1" + assert ( + expected_disabled_file_path + in manifest.disabled["model.test.my_model_2"][0].original_file_path + ) + assert ( + expected_disabled_file_path + in manifest.disabled["model.test.my_model_3"][0].original_file_path + ) + + +# ensure overrides work when enabling in sql file when multiple nodes exist within disabled +class TestMultipleDisabledNodesOverrideModel: + @pytest.fixture(scope="class") + def models(self): + return { + "my_model.sql": my_model, + "folder_1": { + "my_model_2.sql": my_model_2_enabled, + "my_model_3.sql": my_model_3, + }, + "folder_2": { + "my_model_2.sql": my_model_2, + "my_model_3.sql": my_model_3_enabled, + }, + } + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "models": { + "test": { + "folder_1": { + "enabled": False, + }, + "folder_2": { + "enabled": False, + }, + }, + } + } + + def test_multiple_disabled_config(self, project): + run_dbt(["parse"]) + manifest = get_manifest(project.project_root) + assert "model.test.my_model_2" in manifest.nodes + assert "model.test.my_model_3" in manifest.nodes + + expected_file_path_2 = "folder_1" + assert expected_file_path_2 in manifest.nodes["model.test.my_model_2"].original_file_path + expected_file_path_3 = "folder_2" + assert expected_file_path_3 in manifest.nodes["model.test.my_model_3"].original_file_path + + assert "model.test.my_model_2" in manifest.disabled + assert "model.test.my_model_3" in manifest.disabled + + expected_disabled_file_path_2 = "folder_2" + assert ( + expected_disabled_file_path_2 + in manifest.disabled["model.test.my_model_2"][0].original_file_path + ) + expected_disabled_file_path_3 = "folder_1" + assert ( + expected_disabled_file_path_3 + in manifest.disabled["model.test.my_model_3"][0].original_file_path + ) + + +# ensure everything lands where it should when disabling multiple nodes with the same unique id +class TestManyDisabledNodesSuccess: + @pytest.fixture(scope="class") + def models(self): + return { + "my_model.sql": my_model, + "folder_1": { + "my_model_2.sql": my_model_2, + "my_model_3.sql": my_model_3, + }, + "folder_2": { + "my_model_2.sql": my_model_2, + "my_model_3.sql": my_model_3, + }, + "folder_3": { + "my_model_2.sql": my_model_2, + "my_model_3.sql": my_model_3, + }, + "folder_4": { + "my_model_2.sql": my_model_2, + "my_model_3.sql": my_model_3, + }, + } + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "models": { + "test": { + "folder_1": { + "enabled": False, + }, + "folder_2": { + "enabled": True, + }, + "folder_3": { + "enabled": False, + }, + "folder_4": { + "enabled": False, + }, + }, + } + } + + def test_many_disabled_config(self, project): + run_dbt(["parse"]) + manifest = get_manifest(project.project_root) + assert "model.test.my_model_2" in manifest.nodes + assert "model.test.my_model_3" in manifest.nodes + + expected_file_path = "folder_2" + assert expected_file_path in manifest.nodes["model.test.my_model_2"].original_file_path + assert expected_file_path in manifest.nodes["model.test.my_model_3"].original_file_path + + assert len(manifest.disabled["model.test.my_model_2"]) == 3 + assert len(manifest.disabled["model.test.my_model_3"]) == 3