From 1a68f8ae9af7da83e1ce5fbf89bbf279230354a2 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Wed, 14 Sep 2022 11:15:14 -0500 Subject: [PATCH 01/22] WIP --- core/dbt/context/context_config.py | 2 + core/dbt/parser/base.py | 3 + core/dbt/parser/manifest.py | 4 + core/dbt/parser/schemas.py | 12 +- .../functional/configs/test_model_disabled.py | 127 ++++++++++++++++++ 5 files changed, 147 insertions(+), 1 deletion(-) create mode 100644 tests/functional/configs/test_model_disabled.py diff --git a/core/dbt/context/context_config.py b/core/dbt/context/context_config.py index 2b0aafe7189..f8845cabb06 100644 --- a/core/dbt/context/context_config.py +++ b/core/dbt/context/context_config.py @@ -143,6 +143,7 @@ def calculate_node_config( # When schema files patch config, it has lower precedence than # config in the models (config_call_dict), so we add the patch_config_dict # before the config_call_dict + # breakpoint() if patch_config_dict: result = self._update_from_config(result, patch_config_dict) @@ -327,6 +328,7 @@ def build_config_dict( # TODO CT-211 src = UnrenderedConfigGenerator(self._active_project) # type: ignore[assignment] + # breakpoint() return src.calculate_node_config_dict( config_call_dict=self._config_call_dict, fqn=self._fqn, diff --git a/core/dbt/parser/base.py b/core/dbt/parser/base.py index 2786a7c5744..1aa908f6bcb 100644 --- a/core/dbt/parser/base.py +++ b/core/dbt/parser/base.py @@ -315,6 +315,9 @@ def update_parsed_node_config( parsed_node.config_call_dict = config._config_call_dict + # breakpoint() + # self.manifest.add_disabled_nofile(node) + # do this once before we parse the node database/schema/alias, so # parsed_node.config is what it would be if they did nothing self.update_parsed_node_config_dict(parsed_node, config_dict) diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index 74affdce9e6..b735e3c587a 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -370,6 +370,7 @@ def load(self): self._perf_info.parse_project_elapsed = time.perf_counter() - start_parse_projects + # breakpoint() # patch_sources converts the UnparsedSourceDefinitions in the # Manifest.sources to ParsedSourceDefinition via 'patch_source' # in SourcePatcher @@ -463,11 +464,13 @@ def parse_project( for file_id in parser_files[parser_name]: block = FileBlock(self.manifest.files[file_id]) if isinstance(parser, SchemaParser): + # breakpoint() assert isinstance(block.file, SchemaSourceFile) if self.partially_parsing: 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) @@ -829,6 +832,7 @@ def process_refs(self, current_project: str): for node in self.manifest.nodes.values(): if node.created_at < self.started_at: continue + # breakpoint() _process_refs_for_node(self.manifest, current_project, node) for exposure in self.manifest.exposures.values(): if exposure.created_at < self.started_at: diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index dcdca271fe5..a3e834986e2 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -507,6 +507,7 @@ def parse_file(self, block: FileBlock, dct: Dict = None) -> None: # NonSourceParser.parse(), TestablePatchParser is a variety of # NodePatchParser + # breakpoint() if "models" in dct: parser = TestablePatchParser(self, yaml_block, "models") for test_block in parser.parse(): @@ -777,7 +778,11 @@ def parse(self) -> List[TestBlock]: refs = ParserRef() # This adds the node_block to self.manifest # as a ParsedNodePatch or ParsedMacroPatch - self.parse_patch(node_block, refs) + breakpoint() + if node.config.get("enabled"): + self.parse_patch(node_block, refs) + else: + self.manifest.add_disabled_nofile(node) # This will always be empty if the node a macro or analysis return test_blocks @@ -845,6 +850,7 @@ def patch_node_config(self, node, patch): ) # We need to re-apply the config_call_dict after the patch config config._config_call_dict = node.config_call_dict + # breakpoint() self.schema_parser.update_parsed_node_config(node, config, patch_config_dict=patch.config) @@ -912,6 +918,10 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: self.patch_node_config(node, patch) node.patch(patch) + if not node.config.enabled: + # add to disabled dict + self.manifest.add_disabled_nofile(node) + class TestablePatchParser(NodePatchParser[UnparsedNodeUpdate]): def get_block(self, node: UnparsedNodeUpdate) -> TestBlock: diff --git a/tests/functional/configs/test_model_disabled.py b/tests/functional/configs/test_model_disabled.py new file mode 100644 index 00000000000..41480fc5fc4 --- /dev/null +++ b/tests/functional/configs/test_model_disabled.py @@ -0,0 +1,127 @@ +import pytest +from dbt.tests.util import run_dbt + +from dbt.exceptions import CompilationException + +my_model = """ +select 1 as user +""" + +my_model_2_enabled = """ +select * from {{ ref('my_model') }} +""" + +my_model_3_enabled = """ +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') }} +""" + +schema_yml = """ +version: 2 +models: + - name: my_model + - name: my_model_2 + config: + enabled: false + - name: my_model_3 + config: + enabled: false +""" + + +# 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_yml, + "my_model.sql": my_model, + "my_model_2.sql": my_model_2_enabled, + "my_model_3.sql": my_model_3_enabled, + } + + # def test_disabled_config(self, project): + # run_dbt(["parse"]) + + +schema_partial_disabled_yml = """ +version: 2 +models: + - name: my_model + - name: my_model_2 + config: + enabled: false + - name: my_model_3 +""" + + +# 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_enabled, + "my_model_3.sql": my_model_3_enabled, + } + + 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 + breakpoint() + 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"]) + + +# ensure double disabled doesn't throw error when set in project.yml +class TestProjectFileDisabledConfigs: + @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_enabled, + } + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "models": { + "test": { + "my_model_2": { + "enabled": False, + }, + "my_model_3": { + "enabled": False, + }, + }, + } + } + + # def test_disabled_config(self, project): + # run_dbt(["parse"]) From 4702502caa4cb54baf6aedd036bc83d1fe5226a6 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 16 Sep 2022 08:59:43 -0500 Subject: [PATCH 02/22] WIP --- core/dbt/contracts/graph/manifest.py | 1 + core/dbt/parser/base.py | 3 + core/dbt/parser/manifest.py | 11 ++- core/dbt/parser/schemas.py | 33 ++++++-- .../functional/configs/test_model_disabled.py | 81 +++++++++---------- 5 files changed, 77 insertions(+), 52 deletions(-) diff --git a/core/dbt/contracts/graph/manifest.py b/core/dbt/contracts/graph/manifest.py index edf1a5b40ef..398f29578eb 100644 --- a/core/dbt/contracts/graph/manifest.py +++ b/core/dbt/contracts/graph/manifest.py @@ -224,6 +224,7 @@ def __init__(self, manifest: "Manifest"): self.populate(manifest) def populate(self, manifest): + # breakpoint() for node in list(chain.from_iterable(manifest.disabled.values())): self.add_node(node) diff --git a/core/dbt/parser/base.py b/core/dbt/parser/base.py index 1aa908f6bcb..9eed5b813d8 100644 --- a/core/dbt/parser/base.py +++ b/core/dbt/parser/base.py @@ -371,6 +371,8 @@ def render_update(self, node: IntermediateNode, config: ContextConfig) -> None: raise ParsingException(msg, node=node) from exc def add_result_node(self, block: FileBlock, node: ManifestNodes): + # since we haven't parsed the yaml files yet here, we don't know + # if the config is disabled there or not! if node.config.enabled: self.manifest.add_node(block.file, node) else: @@ -390,6 +392,7 @@ def parse_node(self, block: ConfiguredBlockType) -> FinalNode: ) self.render_update(node, config) result = self.transform(node) + # breakpoint() self.add_result_node(block, result) return result diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index b735e3c587a..41ccc961e09 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -367,7 +367,7 @@ def load(self): self.parse_project( project, project_parser_files[project.project_name], parser_types ) - + # breakpoint() self._perf_info.parse_project_elapsed = time.perf_counter() - start_parse_projects # breakpoint() @@ -460,9 +460,11 @@ def parse_project( parser_start_timer = time.perf_counter() # Parse the project files for this parser + # breakpoint() parser: Parser = parser_cls(project, self.manifest, self.root_project) for file_id in parser_files[parser_name]: block = FileBlock(self.manifest.files[file_id]) + # breakpoint() if isinstance(parser, SchemaParser): # breakpoint() assert isinstance(block.file, SchemaSourceFile) @@ -475,6 +477,7 @@ def parse_project( # Came out of here with UnpatchedSourceDefinition containing configs at the source level # and not configs at the table level (as expected) else: + # breakpoint() parser.parse_file(block) project_parsed_path_count += 1 @@ -832,8 +835,9 @@ def process_refs(self, current_project: str): for node in self.manifest.nodes.values(): if node.created_at < self.started_at: continue - # breakpoint() - _process_refs_for_node(self.manifest, current_project, node) + # if the node is disabled, no need to resolve the refs + if node.config.enabled: + _process_refs_for_node(self.manifest, current_project, node) for exposure in self.manifest.exposures.values(): if exposure.created_at < self.started_at: continue @@ -1251,6 +1255,7 @@ def _process_refs_for_node(manifest: Manifest, current_project: str, node: Manif node.package_name, ) + # breakpoint() if target_model is None or isinstance(target_model, Disabled): # This may raise. Even if it doesn't, we don't want to add # this node to the graph b/c there is no destination node diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index a3e834986e2..83f14d333e0 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -1,9 +1,11 @@ +from asyncio import format_helpers import itertools import os import pathlib from abc import ABCMeta, abstractmethod from hashlib import md5 +from tkinter import DISABLED from typing import Iterable, Dict, Any, Union, List, Optional, Generic, TypeVar, Type from dbt.dataclass_schema import ValidationError, dbtClassMixin @@ -509,7 +511,9 @@ def parse_file(self, block: FileBlock, dct: Dict = None) -> None: # NodePatchParser # breakpoint() if "models" in dct: + # the models are already in the manifest as nodes when we reach this code parser = TestablePatchParser(self, yaml_block, "models") + # breakpoint() for test_block in parser.parse(): self.parse_tests(test_block) @@ -767,6 +771,7 @@ def parse(self) -> List[TestBlock]: for node in self.get_unparsed_target(): # node_block is a TargetBlock (Macro or Analysis) # or a TestBlock (all of the others) + # breakpoint() node_block = self.get_block(node) if isinstance(node_block, TestBlock): # TestablePatchParser = models, seeds, snapshots @@ -776,13 +781,19 @@ def parse(self) -> List[TestBlock]: refs: ParserRef = ParserRef.from_target(node) else: refs = ParserRef() + + # breakpoint() + # if node.config.get('enabled') == False: + # # add to DISABLED + # breakpoint() + # unique_id = self.manifest.ref_lookup.get_unique_id(node.name, None) + # self.manifest.add_disabled_nofile(node) + # else: # This adds the node_block to self.manifest # as a ParsedNodePatch or ParsedMacroPatch - breakpoint() - if node.config.get("enabled"): - self.parse_patch(node_block, refs) - else: - self.manifest.add_disabled_nofile(node) + # The enabled key could not exist (defaults to true later if + # not explicitly set) or could be set to True explicitly + self.parse_patch(node_block, refs) # This will always be empty if the node a macro or analysis return test_blocks @@ -873,12 +884,15 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: assert isinstance(self.yaml.file, SchemaSourceFile) source_file: SchemaSourceFile = self.yaml.file if patch.yaml_key in ["models", "seeds", "snapshots"]: + # breakpoint() unique_id = self.manifest.ref_lookup.get_unique_id(patch.name, None) if unique_id: resource_type = NodeType(unique_id.split(".")[0]) if resource_type.pluralize() != patch.yaml_key: warn_invalid_patch(patch, resource_type) return + if self.manifest.nodes[unique_id].config.get('enabled') == False: + self.manifest.add_disabled_nofile(self.manifest.nodes[unique_id]) elif patch.yaml_key == "analyses": unique_id = self.manifest.analysis_lookup.get_unique_id(patch.name, None) @@ -887,6 +901,7 @@ 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}" ) + # breakpoint() 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) @@ -907,10 +922,12 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: # patches can't be overwritten node = self.manifest.nodes.get(unique_id) + # breakpoint() if node: 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 @@ -918,9 +935,9 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: self.patch_node_config(node, patch) node.patch(patch) - if not node.config.enabled: - # add to disabled dict - self.manifest.add_disabled_nofile(node) + if block.target.config.enabled: + # add to disabled dict + self.manifest.add_disabled_nofile(node) class TestablePatchParser(NodePatchParser[UnparsedNodeUpdate]): diff --git a/tests/functional/configs/test_model_disabled.py b/tests/functional/configs/test_model_disabled.py index 41480fc5fc4..2bba526beb4 100644 --- a/tests/functional/configs/test_model_disabled.py +++ b/tests/functional/configs/test_model_disabled.py @@ -79,49 +79,48 @@ 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 - breakpoint() 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"]) - - -# ensure double disabled doesn't throw error when set in project.yml -class TestProjectFileDisabledConfigs: - @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_enabled, - } - - @pytest.fixture(scope="class") - def project_config_update(self): - return { - "models": { - "test": { - "my_model_2": { - "enabled": False, - }, - "my_model_3": { - "enabled": False, - }, - }, - } - } - - # def test_disabled_config(self, project): - # run_dbt(["parse"]) +# 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"]) + + +# # ensure double disabled doesn't throw error when set in project.yml +# class TestProjectFileDisabledConfigs: +# @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_enabled, +# } + +# @pytest.fixture(scope="class") +# def project_config_update(self): +# return { +# "models": { +# "test": { +# "my_model_2": { +# "enabled": False, +# }, +# "my_model_3": { +# "enabled": False, +# }, +# }, +# } +# } + +# def test_disabled_config(self, project): +# run_dbt(["parse"]) From dfd7af8a7b7d4ff5a4126c1345ffd3d37cde2a4e Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 16 Sep 2022 10:31:28 -0500 Subject: [PATCH 03/22] clean up debugging --- core/dbt/context/context_config.py | 2 - core/dbt/contracts/graph/manifest.py | 1 - core/dbt/parser/base.py | 7 +- core/dbt/parser/manifest.py | 7 -- core/dbt/parser/schemas.py | 30 ++----- .../functional/configs/test_model_disabled.py | 88 +++++++++---------- 6 files changed, 51 insertions(+), 84 deletions(-) diff --git a/core/dbt/context/context_config.py b/core/dbt/context/context_config.py index f8845cabb06..2b0aafe7189 100644 --- a/core/dbt/context/context_config.py +++ b/core/dbt/context/context_config.py @@ -143,7 +143,6 @@ def calculate_node_config( # When schema files patch config, it has lower precedence than # config in the models (config_call_dict), so we add the patch_config_dict # before the config_call_dict - # breakpoint() if patch_config_dict: result = self._update_from_config(result, patch_config_dict) @@ -328,7 +327,6 @@ def build_config_dict( # TODO CT-211 src = UnrenderedConfigGenerator(self._active_project) # type: ignore[assignment] - # breakpoint() return src.calculate_node_config_dict( config_call_dict=self._config_call_dict, fqn=self._fqn, diff --git a/core/dbt/contracts/graph/manifest.py b/core/dbt/contracts/graph/manifest.py index 398f29578eb..edf1a5b40ef 100644 --- a/core/dbt/contracts/graph/manifest.py +++ b/core/dbt/contracts/graph/manifest.py @@ -224,7 +224,6 @@ def __init__(self, manifest: "Manifest"): self.populate(manifest) def populate(self, manifest): - # breakpoint() for node in list(chain.from_iterable(manifest.disabled.values())): self.add_node(node) diff --git a/core/dbt/parser/base.py b/core/dbt/parser/base.py index 9eed5b813d8..8aa656dec67 100644 --- a/core/dbt/parser/base.py +++ b/core/dbt/parser/base.py @@ -273,7 +273,6 @@ def update_parsed_node_config( generate and set the true values to use, overriding the temporary parse values set in _build_intermediate_parsed_node. """ - # build_config_dict takes the config_call_dict in the ContextConfig object # and calls calculate_node_config to combine dbt_project configs and # config calls from SQL files, plus patch configs (from schema files) @@ -315,9 +314,6 @@ def update_parsed_node_config( parsed_node.config_call_dict = config._config_call_dict - # breakpoint() - # self.manifest.add_disabled_nofile(node) - # do this once before we parse the node database/schema/alias, so # parsed_node.config is what it would be if they did nothing self.update_parsed_node_config_dict(parsed_node, config_dict) @@ -371,7 +367,7 @@ def render_update(self, node: IntermediateNode, config: ContextConfig) -> None: raise ParsingException(msg, node=node) from exc def add_result_node(self, block: FileBlock, node: ManifestNodes): - # since we haven't parsed the yaml files yet here, we don't know + # since we haven't parsed the yaml files yet here, we don't know # if the config is disabled there or not! if node.config.enabled: self.manifest.add_node(block.file, node) @@ -392,7 +388,6 @@ def parse_node(self, block: ConfiguredBlockType) -> FinalNode: ) self.render_update(node, config) result = self.transform(node) - # breakpoint() self.add_result_node(block, result) return result diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index 41ccc961e09..b8c01647629 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -367,10 +367,8 @@ def load(self): self.parse_project( project, project_parser_files[project.project_name], parser_types ) - # breakpoint() self._perf_info.parse_project_elapsed = time.perf_counter() - start_parse_projects - # breakpoint() # patch_sources converts the UnparsedSourceDefinitions in the # Manifest.sources to ParsedSourceDefinition via 'patch_source' # in SourcePatcher @@ -460,13 +458,10 @@ def parse_project( parser_start_timer = time.perf_counter() # Parse the project files for this parser - # breakpoint() parser: Parser = parser_cls(project, self.manifest, self.root_project) for file_id in parser_files[parser_name]: block = FileBlock(self.manifest.files[file_id]) - # breakpoint() if isinstance(parser, SchemaParser): - # breakpoint() assert isinstance(block.file, SchemaSourceFile) if self.partially_parsing: dct = block.file.pp_dict @@ -477,7 +472,6 @@ def parse_project( # Came out of here with UnpatchedSourceDefinition containing configs at the source level # and not configs at the table level (as expected) else: - # breakpoint() parser.parse_file(block) project_parsed_path_count += 1 @@ -1255,7 +1249,6 @@ def _process_refs_for_node(manifest: Manifest, current_project: str, node: Manif node.package_name, ) - # breakpoint() if target_model is None or isinstance(target_model, Disabled): # This may raise. Even if it doesn't, we don't want to add # this node to the graph b/c there is no destination node diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index 83f14d333e0..cae63e99d6c 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -1,11 +1,9 @@ -from asyncio import format_helpers import itertools import os import pathlib from abc import ABCMeta, abstractmethod from hashlib import md5 -from tkinter import DISABLED from typing import Iterable, Dict, Any, Union, List, Optional, Generic, TypeVar, Type from dbt.dataclass_schema import ValidationError, dbtClassMixin @@ -509,11 +507,9 @@ def parse_file(self, block: FileBlock, dct: Dict = None) -> None: # NonSourceParser.parse(), TestablePatchParser is a variety of # NodePatchParser - # breakpoint() if "models" in dct: # the models are already in the manifest as nodes when we reach this code parser = TestablePatchParser(self, yaml_block, "models") - # breakpoint() for test_block in parser.parse(): self.parse_tests(test_block) @@ -771,7 +767,6 @@ def parse(self) -> List[TestBlock]: for node in self.get_unparsed_target(): # node_block is a TargetBlock (Macro or Analysis) # or a TestBlock (all of the others) - # breakpoint() node_block = self.get_block(node) if isinstance(node_block, TestBlock): # TestablePatchParser = models, seeds, snapshots @@ -782,18 +777,11 @@ def parse(self) -> List[TestBlock]: else: refs = ParserRef() - # breakpoint() - # if node.config.get('enabled') == False: - # # add to DISABLED - # breakpoint() - # unique_id = self.manifest.ref_lookup.get_unique_id(node.name, None) - # self.manifest.add_disabled_nofile(node) - # else: # This adds the node_block to self.manifest # as a ParsedNodePatch or ParsedMacroPatch - # The enabled key could not exist (defaults to true later if - # not explicitly set) or could be set to True explicitly + # There's no unique_id on the node yet so cannot add to disabled dict yet self.parse_patch(node_block, refs) + # This will always be empty if the node a macro or analysis return test_blocks @@ -861,7 +849,6 @@ def patch_node_config(self, node, patch): ) # We need to re-apply the config_call_dict after the patch config config._config_call_dict = node.config_call_dict - # breakpoint() self.schema_parser.update_parsed_node_config(node, config, patch_config_dict=patch.config) @@ -884,15 +871,12 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: assert isinstance(self.yaml.file, SchemaSourceFile) source_file: SchemaSourceFile = self.yaml.file if patch.yaml_key in ["models", "seeds", "snapshots"]: - # breakpoint() unique_id = self.manifest.ref_lookup.get_unique_id(patch.name, None) if unique_id: resource_type = NodeType(unique_id.split(".")[0]) if resource_type.pluralize() != patch.yaml_key: warn_invalid_patch(patch, resource_type) return - if self.manifest.nodes[unique_id].config.get('enabled') == False: - self.manifest.add_disabled_nofile(self.manifest.nodes[unique_id]) elif patch.yaml_key == "analyses": unique_id = self.manifest.analysis_lookup.get_unique_id(patch.name, None) @@ -901,7 +885,6 @@ 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}" ) - # breakpoint() 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) @@ -922,7 +905,6 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: # patches can't be overwritten node = self.manifest.nodes.get(unique_id) - # breakpoint() if node: if node.patch_path: package_name, existing_file_path = node.patch_path.split("://") @@ -933,11 +915,11 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: # with the patch config if patch.config: self.patch_node_config(node, patch) - node.patch(patch) - if block.target.config.enabled: - # add to disabled dict - self.manifest.add_disabled_nofile(node) + if not node.config.enabled: + self.manifest.add_disabled_nofile(node) + + node.patch(patch) class TestablePatchParser(NodePatchParser[UnparsedNodeUpdate]): diff --git a/tests/functional/configs/test_model_disabled.py b/tests/functional/configs/test_model_disabled.py index 2bba526beb4..2e087d22507 100644 --- a/tests/functional/configs/test_model_disabled.py +++ b/tests/functional/configs/test_model_disabled.py @@ -25,7 +25,7 @@ select * from {{ ref('my_model_2') }} """ -schema_yml = """ +schema_all_disabled_yml = """ version: 2 models: - name: my_model @@ -43,14 +43,14 @@ class TestSchemaDisabledConfigs: @pytest.fixture(scope="class") def models(self): return { - "schema.yml": schema_yml, + "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_enabled, } - # def test_disabled_config(self, project): - # run_dbt(["parse"]) + def test_disabled_config(self, project): + run_dbt(["parse"]) schema_partial_disabled_yml = """ @@ -84,43 +84,43 @@ def test_disabled_config(self, project): # 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"]) - - -# # ensure double disabled doesn't throw error when set in project.yml -# class TestProjectFileDisabledConfigs: -# @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_enabled, -# } - -# @pytest.fixture(scope="class") -# def project_config_update(self): -# return { -# "models": { -# "test": { -# "my_model_2": { -# "enabled": False, -# }, -# "my_model_3": { -# "enabled": False, -# }, -# }, -# } -# } - -# def test_disabled_config(self, project): -# run_dbt(["parse"]) +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"]) + + +# ensure double disabled doesn't throw error when set in project.yml +class TestProjectFileDisabledConfigs: + @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_enabled, + } + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "models": { + "test": { + "my_model_2": { + "enabled": False, + }, + "my_model_3": { + "enabled": False, + }, + }, + } + } + + def test_disabled_config(self, project): + run_dbt(["parse"]) From 21c9d0343a50b57b35f51b102c61f75d05b832a9 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 16 Sep 2022 10:47:01 -0500 Subject: [PATCH 04/22] reword some comments --- core/dbt/parser/base.py | 6 ++++-- core/dbt/parser/manifest.py | 1 + core/dbt/parser/schemas.py | 5 +++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/core/dbt/parser/base.py b/core/dbt/parser/base.py index 8aa656dec67..a3f3fac19dc 100644 --- a/core/dbt/parser/base.py +++ b/core/dbt/parser/base.py @@ -273,6 +273,7 @@ def update_parsed_node_config( generate and set the true values to use, overriding the temporary parse values set in _build_intermediate_parsed_node. """ + # build_config_dict takes the config_call_dict in the ContextConfig object # and calls calculate_node_config to combine dbt_project configs and # config calls from SQL files, plus patch configs (from schema files) @@ -367,8 +368,9 @@ def render_update(self, node: IntermediateNode, config: ContextConfig) -> None: raise ParsingException(msg, node=node) from exc def add_result_node(self, block: FileBlock, node: ManifestNodes): - # since we haven't parsed the yaml files yet here, we don't know - # if the config is disabled there or not! + # TODO: this logic should be reworked to incorporate if models are diabled + # in the schema file. Since we haven't parsed the yaml files yet here, we + # don't know if the config is disabled there or not if node.config.enabled: self.manifest.add_node(block.file, node) else: diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index b8c01647629..61b8d6ebc45 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -367,6 +367,7 @@ def load(self): self.parse_project( project, project_parser_files[project.project_name], parser_types ) + self._perf_info.parse_project_elapsed = time.perf_counter() - start_parse_projects # patch_sources converts the UnparsedSourceDefinitions in the diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index cae63e99d6c..54a7c95439d 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -508,7 +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 + # 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) @@ -779,7 +780,7 @@ def parse(self) -> List[TestBlock]: # 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 yet + # 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 From 5a98b26a3b071edbfadde8398eba12c14cc0e8fa Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 16 Sep 2022 10:49:04 -0500 Subject: [PATCH 05/22] changelog --- .changes/unreleased/Fixes-20220916-104854.yaml | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changes/unreleased/Fixes-20220916-104854.yaml 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" From 61268a1f35720542a644a43f15f5a41e1df15899 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 16 Sep 2022 13:51:39 -0500 Subject: [PATCH 06/22] add more tests --- core/dbt/parser/base.py | 3 - .../functional/configs/test_model_disabled.py | 97 ++++++++++++++++--- 2 files changed, 86 insertions(+), 14 deletions(-) diff --git a/core/dbt/parser/base.py b/core/dbt/parser/base.py index a3f3fac19dc..2786a7c5744 100644 --- a/core/dbt/parser/base.py +++ b/core/dbt/parser/base.py @@ -368,9 +368,6 @@ def render_update(self, node: IntermediateNode, config: ContextConfig) -> None: raise ParsingException(msg, node=node) from exc def add_result_node(self, block: FileBlock, node: ManifestNodes): - # TODO: this logic should be reworked to incorporate if models are diabled - # in the schema file. Since we haven't parsed the yaml files yet here, we - # don't know if the config is disabled there or not if node.config.enabled: self.manifest.add_node(block.file, node) else: diff --git a/tests/functional/configs/test_model_disabled.py b/tests/functional/configs/test_model_disabled.py index 2e087d22507..f6c0441bee5 100644 --- a/tests/functional/configs/test_model_disabled.py +++ b/tests/functional/configs/test_model_disabled.py @@ -1,5 +1,5 @@ import pytest -from dbt.tests.util import run_dbt +from dbt.tests.util import run_dbt, get_manifest from dbt.exceptions import CompilationException @@ -7,11 +7,11 @@ select 1 as user """ -my_model_2_enabled = """ +my_model_2 = """ select * from {{ ref('my_model') }} """ -my_model_3_enabled = """ +my_model_3 = """ select * from {{ ref('my_model_2') }} """ @@ -25,6 +25,11 @@ select * from {{ ref('my_model_2') }} """ +my_model_2_enabled = """ +{{ config(enabled=true) }} +select * from {{ ref('my_model') }} +""" + schema_all_disabled_yml = """ version: 2 models: @@ -45,8 +50,8 @@ 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_enabled, + "my_model_2.sql": my_model_2, + "my_model_3.sql": my_model_3, } def test_disabled_config(self, project): @@ -71,8 +76,8 @@ def models(self): return { "schema.yml": schema_partial_disabled_yml, "my_model.sql": my_model, - "my_model_2.sql": my_model_2_enabled, - "my_model_3.sql": my_model_3_enabled, + "my_model_2.sql": my_model_2, + "my_model_3.sql": my_model_3, } def test_disabled_config(self, project): @@ -97,14 +102,59 @@ def test_disabled_config(self, project): run_dbt(["parse"]) -# ensure double disabled doesn't throw error when set in project.yml -class TestProjectFileDisabledConfigs: +schema_partial_enabled_yml = """ +version: 2 +models: + - name: my_model + - name: my_model_2 + config: + enabled: True + - name: my_model_3 +""" + +# 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_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_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_enabled, + "my_model_3.sql": my_model_3, } @pytest.fixture(scope="class") @@ -122,5 +172,30 @@ def project_config_update(self): } } - def test_disabled_config(self, project): + def test_override_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_3" in manifest.disabled + + +# ensure config set in yaml file can be overridden in sql file +class TestOverrideYAMLConfigsInSQL: + @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_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_3" in manifest.disabled From a750db7f80b66a1208068ee283de121830a876f7 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 16 Sep 2022 14:21:27 -0500 Subject: [PATCH 07/22] move around the manifest.node --- core/dbt/parser/manifest.py | 4 +- core/dbt/parser/schemas.py | 44 ++++++++++++++++--- .../functional/configs/test_model_disabled.py | 1 + 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index 61b8d6ebc45..aea7eec0e45 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -830,9 +830,7 @@ def process_refs(self, current_project: str): for node in self.manifest.nodes.values(): if node.created_at < self.started_at: continue - # if the node is disabled, no need to resolve the refs - if node.config.enabled: - _process_refs_for_node(self.manifest, current_project, node) + _process_refs_for_node(self.manifest, current_project, node) for exposure in self.manifest.exposures.values(): if exposure.created_at < self.started_at: continue diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index 54a7c95439d..19c6f0b3ce1 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -892,9 +892,20 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: if found_nodes: # There might be multiple disabled nodes for this model 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 + # If this yaml file is enabled but the project config is not, we need to move + # the node from disabled to manifest.nodes + if patch.config.get("enabled"): + test_from = {"key": block.target.yaml_key, "name": block.target.name} + self.manifest.add_node(source_file, node, test_from) + self.manifest.rebuild_ref_lookup() + unique_id = self.manifest.ref_lookup.get_unique_id(patch.name, None) + if node.unique_id in self.manifest.disabled: + self.remove_disabled(source_file, node.unique_id) + else: + # 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 + else: msg = ( f"Did not find matching node for patch with name '{patch.name}' " @@ -917,10 +928,33 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: if patch.config: self.patch_node_config(node, patch) - if not node.config.enabled: + if node.config.enabled: + # This node may have been enabled via the schema file but was part of teh disabled + # dict because it was disabled at the project level. Go ahead and remove it from + # the disabled dict now. + if node.unique_id in self.manifest.disabled: + self.remove_disabled(source_file, node.unique_id) + + node.patch(patch) + else: + if node.unique_id in self.manifest.nodes: + # the node is already in the manifest.nodes because we process the models + # before the sdchema files, remove it and rebuild refs not based on the + # enabled config set in the yaml file + self.manifest.nodes.pop(node.unique_id) + self.manifest.rebuild_ref_lookup() self.manifest.add_disabled_nofile(node) - node.patch(patch) + def remove_disabled(self, source_file: SchemaSourceFile, unique_id: str) -> None: + # This node is disabled. Find the node and remove it from disabled dictionary. + for dis_index, dis_node in enumerate(self.manifest.disabled[unique_id]): + if dis_node.file_id == source_file.file_id: + break + if dis_node: + # Remove node from disabled and unique_id from disabled dict if necessary + del self.manifest.disabled[unique_id][dis_index] + if not self.manifest.disabled[unique_id]: + self.manifest.disabled.pop(unique_id) class TestablePatchParser(NodePatchParser[UnparsedNodeUpdate]): diff --git a/tests/functional/configs/test_model_disabled.py b/tests/functional/configs/test_model_disabled.py index f6c0441bee5..43cea9d8744 100644 --- a/tests/functional/configs/test_model_disabled.py +++ b/tests/functional/configs/test_model_disabled.py @@ -112,6 +112,7 @@ def test_disabled_config(self, project): - name: my_model_3 """ + # ensure config set in project.yml can be overridden in yaml file class TestOverrideProjectConfigsInYaml: @pytest.fixture(scope="class") From 3a4bf39d9ffaaa8020782a370ba67eaa77a3493b Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 16 Sep 2022 14:26:20 -0500 Subject: [PATCH 08/22] fix typos --- core/dbt/parser/schemas.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index 19c6f0b3ce1..a0909f2d8b7 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -929,7 +929,7 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: self.patch_node_config(node, patch) if node.config.enabled: - # This node may have been enabled via the schema file but was part of teh disabled + # This node may have been enabled via the schema file but was part of the disabled # dict because it was disabled at the project level. Go ahead and remove it from # the disabled dict now. if node.unique_id in self.manifest.disabled: @@ -939,14 +939,13 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: else: if node.unique_id in self.manifest.nodes: # the node is already in the manifest.nodes because we process the models - # before the sdchema files, remove it and rebuild refs not based on the + # before the schema files, remove it and rebuild refs not based on the # enabled config set in the yaml file self.manifest.nodes.pop(node.unique_id) self.manifest.rebuild_ref_lookup() self.manifest.add_disabled_nofile(node) def remove_disabled(self, source_file: SchemaSourceFile, unique_id: str) -> None: - # This node is disabled. Find the node and remove it from disabled dictionary. for dis_index, dis_node in enumerate(self.manifest.disabled[unique_id]): if dis_node.file_id == source_file.file_id: break From b6ed9b837db30d83710f38d3f980d4004f0f92ec Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Thu, 22 Sep 2022 19:38:05 -0500 Subject: [PATCH 09/22] WIP --- core/dbt/contracts/graph/manifest.py | 9 +++- core/dbt/parser/schemas.py | 35 +++++++------ .../functional/configs/test_model_disabled.py | 52 +++++++++++++++++-- 3 files changed, 74 insertions(+), 22 deletions(-) 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/schemas.py b/core/dbt/parser/schemas.py index a0909f2d8b7..bc803b5857b 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -889,22 +889,27 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: 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: + # TODO: If there are multiple nodes with the same unique id, can we do anything or do we error? + if len(found_nodes) > 1: # There might be multiple disabled nodes for this model - for node in found_nodes: - # If this yaml file is enabled but the project config is not, we need to move - # the node from disabled to manifest.nodes - if patch.config.get("enabled"): - test_from = {"key": block.target.yaml_key, "name": block.target.name} - self.manifest.add_node(source_file, node, test_from) - self.manifest.rebuild_ref_lookup() - unique_id = self.manifest.ref_lookup.get_unique_id(patch.name, None) - if node.unique_id in self.manifest.disabled: - self.remove_disabled(source_file, node.unique_id) - else: - # 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 + pass + # TODO: throw error + elif found_nodes: + node = found_nodes[0] + # If this yaml file is enabled but the project config is not, we need to move + # the node from disabled to manifest.nodes + # Is there a way to know it was disabled in the sql file and not the project? Could we loop through and compare the original file path? + breakpoint() + if patch.config.get("enabled"): + self.manifest.add_node(source_file, node) + self.manifest.rebuild_ref_lookup() + unique_id = self.manifest.ref_lookup.get_unique_id(patch.name, None) + if node.unique_id in self.manifest.disabled: + self.remove_disabled(source_file, node.unique_id) + else: + # 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 else: msg = ( diff --git a/tests/functional/configs/test_model_disabled.py b/tests/functional/configs/test_model_disabled.py index 43cea9d8744..cb74c2960f3 100644 --- a/tests/functional/configs/test_model_disabled.py +++ b/tests/functional/configs/test_model_disabled.py @@ -42,6 +42,18 @@ enabled: false """ +schema_all_enabled_yml = """ +version: 2 +models: + - name: my_model + - name: my_model_2 + config: + enabled: true + - name: my_model_3 + config: + enabled: true +""" + # ensure double disabled doesn't throw error when set at schema level class TestSchemaDisabledConfigs: @@ -100,6 +112,12 @@ def models(self): 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 schema_partial_enabled_yml = """ @@ -139,12 +157,13 @@ def project_config_update(self): } } - def test_override_config(self, project): + 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 @@ -173,17 +192,18 @@ def project_config_update(self): } } - def test_override_config(self, project): + 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 config set in yaml file can be overridden in sql file -class TestOverrideYAMLConfigsInSQL: +# ensure false config set in yaml file can be overridden in sql file +class TestOverrideFalseYAMLConfigsInSQL: @pytest.fixture(scope="class") def models(self): return { @@ -193,10 +213,32 @@ def models(self): "my_model_3.sql": my_model_3, } - def test_override_config(self, project): + 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 in sql file +class TestOverrideTrueYAMLConfigsInSQL: + @pytest.fixture(scope="class") + def models(self): + return { + "schema.yml": schema_all_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" in manifest.nodes + + assert "model.test.my_model_2" not in manifest.disabled + assert "model.test.my_model_3" not in manifest.disabled From 1bc9c0f01cc05ec5bd4db8b29ec9b828be26a269 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 23 Sep 2022 09:58:44 -0500 Subject: [PATCH 10/22] still WIP --- core/dbt/parser/schemas.py | 19 ++++++++++--------- .../functional/configs/test_model_disabled.py | 6 +++--- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index bc803b5857b..fd74da761c9 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -889,17 +889,11 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: 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) - # TODO: If there are multiple nodes with the same unique id, can we do anything or do we error? - if len(found_nodes) > 1: - # There might be multiple disabled nodes for this model - pass - # TODO: throw error - elif found_nodes: + if len(found_nodes) == 1: node = found_nodes[0] # If this yaml file is enabled but the project config is not, we need to move # the node from disabled to manifest.nodes - # Is there a way to know it was disabled in the sql file and not the project? Could we loop through and compare the original file path? - breakpoint() + # Is there a way to know it was disabled in the sql file and not the project? Could we loop through and compare the original file path? if patch.config.get("enabled"): self.manifest.add_node(source_file, node) self.manifest.rebuild_ref_lookup() @@ -910,7 +904,14 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: # 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 - + elif len(found_nodes) > 1: + # There are multiple disabled nodes for this model. We have no way to know + msg = ( + f"Found {len(found_nodes)} matching disabled nodes for '{patch.name}'. " + "Multiple nodes for thge same unique id cannot be disabled in the schema " + "file. They must be disabled in `dbt_project.yml` or in the sql files." + ) + raise ParsingException(msg) else: msg = ( f"Did not find matching node for patch with name '{patch.name}' " diff --git a/tests/functional/configs/test_model_disabled.py b/tests/functional/configs/test_model_disabled.py index cb74c2960f3..a81bdb20d90 100644 --- a/tests/functional/configs/test_model_disabled.py +++ b/tests/functional/configs/test_model_disabled.py @@ -223,7 +223,7 @@ def test_override_yaml_sql_config(self, project): assert "model.test.my_model_3" in manifest.disabled -# ensure true config set in yaml file can be overridden in sql file +# ensure true config set in yaml file can be overridden by false in sql file class TestOverrideTrueYAMLConfigsInSQL: @pytest.fixture(scope="class") def models(self): @@ -238,7 +238,7 @@ 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" 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" not in manifest.disabled + assert "model.test.my_model_3" in manifest.disabled From e464f5d4e022b09c55245d1f6b6944deda6ccee3 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Mon, 26 Sep 2022 09:05:46 -0500 Subject: [PATCH 11/22] all tests passing --- core/dbt/parser/manifest.py | 10 ++ core/dbt/parser/schemas.py | 42 +++---- tests/functional/configs/fixtures.py | 71 +++++++++++ ...del_disabled.py => test_disabled_model.py} | 119 +++++++----------- 4 files changed, 144 insertions(+), 98 deletions(-) rename tests/functional/configs/{test_model_disabled.py => test_disabled_model.py} (78%) diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index aea7eec0e45..22607b4fddc 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -389,6 +389,7 @@ def load(self): # These check the created_at time on the nodes to # determine whether they need processing. start_process = time.perf_counter() + self.process_nodes(self.root_project.project_name) self.process_sources(self.root_project.project_name) self.process_refs(self.root_project.project_name) self.process_docs(self.root_project) @@ -927,6 +928,15 @@ def process_sources(self, current_project: str): continue _process_sources_for_exposure(self.manifest, current_project, exposure) + def process_nodes(self, current_project: str): + # TODO: process that everything is in the right place + pass + # for node in self.manifest.nodes.values(): + # breakpoint() + + # for node in self.manifest.disabled.values(): + # breakpoint() + def invalid_ref_fail_unless_test(node, target_model_name, target_model_package, disabled): diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index fd74da761c9..76c1c035cb5 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -889,29 +889,21 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: 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 len(found_nodes) == 1: - node = found_nodes[0] - # If this yaml file is enabled but the project config is not, we need to move - # the node from disabled to manifest.nodes - # Is there a way to know it was disabled in the sql file and not the project? Could we loop through and compare the original file path? - if patch.config.get("enabled"): - self.manifest.add_node(source_file, node) - self.manifest.rebuild_ref_lookup() - unique_id = self.manifest.ref_lookup.get_unique_id(patch.name, None) - if node.unique_id in self.manifest.disabled: - self.remove_disabled(source_file, node.unique_id) - else: + if found_nodes: + if len(found_nodes) == 1: + node = found_nodes[0] # 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 - elif len(found_nodes) > 1: - # There are multiple disabled nodes for this model. We have no way to know - msg = ( - f"Found {len(found_nodes)} matching disabled nodes for '{patch.name}'. " - "Multiple nodes for thge same unique id cannot be disabled in the schema " - "file. They must be disabled in `dbt_project.yml` or in the sql files." - ) - raise ParsingException(msg) + elif 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. + msg = ( + f"Found {len(found_nodes)} matching disabled nodes for '{patch.name}'. " + "Multiple nodes for the same unique id cannot be disabled in the schema " + "file. They must be disabled in `dbt_project.yml` or in the sql files." + ) + raise ParsingException(msg) else: msg = ( f"Did not find matching node for patch with name '{patch.name}' " @@ -922,12 +914,13 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: return # patches can't be overwritten - node = self.manifest.nodes.get(unique_id) - if node: + else: + node = self.manifest.nodes.get(unique_id) if node.patch_path: package_name, existing_file_path = node.patch_path.split("://") raise_duplicate_patch_name(patch, existing_file_path) + if node: source_file.append_patch(patch.yaml_key, unique_id) # If this patch has config changes, re-calculate the node config # with the patch config @@ -940,8 +933,9 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: # the disabled dict now. if node.unique_id in self.manifest.disabled: self.remove_disabled(source_file, node.unique_id) + if node.unique_id not in self.manifest.nodes: + self.manifest.add_node(source_file, node) - node.patch(patch) else: if node.unique_id in self.manifest.nodes: # the node is already in the manifest.nodes because we process the models @@ -951,6 +945,8 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: self.manifest.rebuild_ref_lookup() self.manifest.add_disabled_nofile(node) + node.patch(patch) + def remove_disabled(self, source_file: SchemaSourceFile, unique_id: str) -> None: for dis_index, dis_node in enumerate(self.manifest.disabled[unique_id]): if dis_node.file_id == source_file.file_id: diff --git a/tests/functional/configs/fixtures.py b/tests/functional/configs/fixtures.py index eb744f93430..16e2c7ca3af 100644 --- a/tests/functional/configs/fixtures.py +++ b/tests/functional/configs/fixtures.py @@ -78,6 +78,77 @@ """ +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') }} +""" + +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_model_disabled.py b/tests/functional/configs/test_disabled_model.py similarity index 78% rename from tests/functional/configs/test_model_disabled.py rename to tests/functional/configs/test_disabled_model.py index a81bdb20d90..5caa0b40086 100644 --- a/tests/functional/configs/test_model_disabled.py +++ b/tests/functional/configs/test_disabled_model.py @@ -1,58 +1,20 @@ import pytest from dbt.tests.util import run_dbt, get_manifest -from dbt.exceptions import CompilationException - -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') }} -""" - -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_all_enabled_yml = """ -version: 2 -models: - - name: my_model - - name: my_model_2 - config: - enabled: true - - name: my_model_3 - config: - enabled: true -""" +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, +) # ensure double disabled doesn't throw error when set at schema level @@ -70,17 +32,6 @@ def test_disabled_config(self, project): run_dbt(["parse"]) -schema_partial_disabled_yml = """ -version: 2 -models: - - name: my_model - - name: my_model_2 - config: - enabled: false - - name: my_model_3 -""" - - # ensure this throws a specific error that the model is disabled class TestSchemaDisabledConfigsFailure: @pytest.fixture(scope="class") @@ -120,17 +71,6 @@ def test_disabled_config(self, project): assert "model.test.my_model_3" in manifest.disabled -schema_partial_enabled_yml = """ -version: 2 -models: - - name: my_model - - name: my_model_2 - config: - enabled: True - - name: my_model_3 -""" - - # ensure config set in project.yml can be overridden in yaml file class TestOverrideProjectConfigsInYaml: @pytest.fixture(scope="class") @@ -228,7 +168,7 @@ class TestOverrideTrueYAMLConfigsInSQL: @pytest.fixture(scope="class") def models(self): return { - "schema.yml": schema_all_enabled_yml, + "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, @@ -242,3 +182,32 @@ def test_override_yaml_sql_config(self, project): 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 'my_model_2'" + assert expected_msg in exc_str From 42bfdf73a96567d85d16505b9ab837e47c55a1ec Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Mon, 26 Sep 2022 13:38:14 -0500 Subject: [PATCH 12/22] move logic for moving around nodes --- core/dbt/parser/manifest.py | 36 +++++++++++++++++++++++++++--------- core/dbt/parser/schemas.py | 30 ++++++------------------------ 2 files changed, 33 insertions(+), 33 deletions(-) diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index 22607b4fddc..39a2b0625f2 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -368,6 +368,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 @@ -389,7 +391,6 @@ def load(self): # These check the created_at time on the nodes to # determine whether they need processing. start_process = time.perf_counter() - self.process_nodes(self.root_project.project_name) self.process_sources(self.root_project.project_name) self.process_refs(self.root_project.project_name) self.process_docs(self.root_project) @@ -928,14 +929,31 @@ def process_sources(self, current_project: str): continue _process_sources_for_exposure(self.manifest, current_project, exposure) - def process_nodes(self, current_project: str): - # TODO: process that everything is in the right place - pass - # for node in self.manifest.nodes.values(): - # breakpoint() - - # for node in self.manifest.disabled.values(): - # breakpoint() + def process_nodes(self): + # make sure the nodes are in the manifest.nodes or the disabled dict + disable_nodes = [] + for node in self.manifest.nodes: + if not node.config.enabled: + disable_nodes.append(node) + for node in disable_nodes: + self.manifest.nodes.pop(node.unique_id) + self.manifest.add_disabled_nofile(node) + + enable_nodes = {} + for disabled in self.manifest.disabled: + 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 + enable_nodes[dis_index] = dis_node + for index, node in enable_nodes.items(): + del self.manifest.disabled[node.unique_id][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/schemas.py b/core/dbt/parser/schemas.py index 76c1c035cb5..59ad5a05e3b 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -890,12 +890,7 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> 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: - if len(found_nodes) == 1: - node = found_nodes[0] - # 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 - elif patch.config.get("enabled"): + 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. msg = ( @@ -904,6 +899,11 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: "file. They must be disabled in `dbt_project.yml` or in the sql files." ) raise ParsingException(msg) + + node = found_nodes[0] + # 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 else: msg = ( f"Did not find matching node for patch with name '{patch.name}' " @@ -927,24 +927,6 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: if patch.config: self.patch_node_config(node, patch) - if node.config.enabled: - # This node may have been enabled via the schema file but was part of the disabled - # dict because it was disabled at the project level. Go ahead and remove it from - # the disabled dict now. - if node.unique_id in self.manifest.disabled: - self.remove_disabled(source_file, node.unique_id) - if node.unique_id not in self.manifest.nodes: - self.manifest.add_node(source_file, node) - - else: - if node.unique_id in self.manifest.nodes: - # the node is already in the manifest.nodes because we process the models - # before the schema files, remove it and rebuild refs not based on the - # enabled config set in the yaml file - self.manifest.nodes.pop(node.unique_id) - self.manifest.rebuild_ref_lookup() - self.manifest.add_disabled_nofile(node) - node.patch(patch) def remove_disabled(self, source_file: SchemaSourceFile, unique_id: str) -> None: From c5ee79e30535eb5af03089dcf163fa8a905912c9 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Mon, 26 Sep 2022 13:54:52 -0500 Subject: [PATCH 13/22] add tests --- core/dbt/parser/manifest.py | 7 +- tests/functional/configs/fixtures.py | 5 + .../functional/configs/test_disabled_model.py | 113 ++++++++++++++++++ 3 files changed, 122 insertions(+), 3 deletions(-) diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index 39a2b0625f2..e4a25c21535 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -930,9 +930,10 @@ def process_sources(self, current_project: str): _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 + # make sure the nodes are in the manifest.nodes or the disabled dict, + # correctly now that the schema files are also aprsed disable_nodes = [] - for node in self.manifest.nodes: + for node in self.manifest.nodes.values(): if not node.config.enabled: disable_nodes.append(node) for node in disable_nodes: @@ -940,7 +941,7 @@ def process_nodes(self): self.manifest.add_disabled_nofile(node) enable_nodes = {} - for disabled in self.manifest.disabled: + for disabled in self.manifest.disabled.values(): for node in disabled: if node.config.enabled: for dis_index, dis_node in enumerate(disabled): diff --git a/tests/functional/configs/fixtures.py b/tests/functional/configs/fixtures.py index 16e2c7ca3af..ae3173490b7 100644 --- a/tests/functional/configs/fixtures.py +++ b/tests/functional/configs/fixtures.py @@ -105,6 +105,11 @@ select * from {{ ref('my_model') }} """ +my_model_3_enabled = """ +{{ config(enabled=true) }} +select * from {{ ref('my_model') }} +""" + schema_all_disabled_yml = """ version: 2 models: diff --git a/tests/functional/configs/test_disabled_model.py b/tests/functional/configs/test_disabled_model.py index 5caa0b40086..5b26dbd98dc 100644 --- a/tests/functional/configs/test_disabled_model.py +++ b/tests/functional/configs/test_disabled_model.py @@ -14,6 +14,7 @@ my_model_2_disabled, my_model_3, my_model_3_disabled, + my_model_3_enabled, ) @@ -211,3 +212,115 @@ def test_disabled_config(self, project): exc_str = " ".join(str(exc.value).split()) # flatten all whitespace expected_msg = "Found 3 matching disabled nodes for '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 = "models/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 = "models/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 error when enabling in schema 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 = "models/folder_1" + assert expected_file_path_2 in manifest.nodes["model.test.my_model_2"].original_file_path + expected_file_path_3 = "models/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 = "models/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 = "models/folder_1" + assert ( + expected_disabled_file_path_3 + in manifest.disabled["model.test.my_model_3"][0].original_file_path + ) From 1cbb960d37bb6e86aded26fb84ab7351218e13f4 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Mon, 26 Sep 2022 14:22:32 -0500 Subject: [PATCH 14/22] more cleanup --- core/dbt/parser/schemas.py | 15 +---- .../functional/configs/test_disabled_model.py | 61 ++++++++++++++++++- 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index 59ad5a05e3b..5e37a9269b9 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -900,9 +900,9 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: ) raise ParsingException(msg) - node = found_nodes[0] # We're saving the patch_path because we need to schedule # re-application of the patch in partial parsing. + node = found_nodes[0] node.patch_path = source_file.file_id else: msg = ( @@ -912,9 +912,8 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: ) warn_or_error(msg, log_fmt=warning_tag("{}")) return - - # patches can't be overwritten else: + # patches can't be overwritten node = self.manifest.nodes.get(unique_id) if node.patch_path: package_name, existing_file_path = node.patch_path.split("://") @@ -929,16 +928,6 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: node.patch(patch) - def remove_disabled(self, source_file: SchemaSourceFile, unique_id: str) -> None: - for dis_index, dis_node in enumerate(self.manifest.disabled[unique_id]): - if dis_node.file_id == source_file.file_id: - break - if dis_node: - # Remove node from disabled and unique_id from disabled dict if necessary - del self.manifest.disabled[unique_id][dis_index] - if not self.manifest.disabled[unique_id]: - self.manifest.disabled.pop(unique_id) - class TestablePatchParser(NodePatchParser[UnparsedNodeUpdate]): def get_block(self, node: UnparsedNodeUpdate) -> TestBlock: diff --git a/tests/functional/configs/test_disabled_model.py b/tests/functional/configs/test_disabled_model.py index 5b26dbd98dc..aad938dd576 100644 --- a/tests/functional/configs/test_disabled_model.py +++ b/tests/functional/configs/test_disabled_model.py @@ -269,7 +269,7 @@ def test_multiple_disabled_config(self, project): ) -# ensure error when enabling in schema file when multiple nodes exist within disabled +# ensure overrides work when enabling in sql file when multiple nodes exist within disabled class TestMultipleDisabledNodesOverrideModel: @pytest.fixture(scope="class") def models(self): @@ -324,3 +324,62 @@ def test_multiple_disabled_config(self, project): 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 = "models/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 From f4d765b807704de64d1048c383e2f7a8c64c8969 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Mon, 26 Sep 2022 16:04:20 -0500 Subject: [PATCH 15/22] fix failing pp test --- core/dbt/parser/schemas.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index 5e37a9269b9..750335fa57e 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -920,7 +920,7 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: raise_duplicate_patch_name(patch, existing_file_path) if node: - source_file.append_patch(patch.yaml_key, unique_id) + source_file.append_patch(patch.yaml_key, node.unique_id) # If this patch has config changes, re-calculate the node config # with the patch config if patch.config: From 931c603267b9b2547a0e04a45bd3cb82f07a1739 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Mon, 26 Sep 2022 16:22:46 -0500 Subject: [PATCH 16/22] remove comments --- core/dbt/parser/schemas.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index 750335fa57e..9fff39dc225 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -778,8 +778,6 @@ def parse(self) -> List[TestBlock]: 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) From 2374de1663a91941bdff9f83776db5cfaa53682b Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Wed, 28 Sep 2022 12:17:27 -0500 Subject: [PATCH 17/22] add mroe tests, patch all disabled nodes --- core/dbt/parser/schemas.py | 30 +++++++++++-------- .../test-files/models-schema4.yml | 13 ++++++++ .../test-files/models-schema4b.yml | 13 ++++++++ .../test_partial_parsing.py | 20 +++++++++++++ 4 files changed, 64 insertions(+), 12 deletions(-) create mode 100644 test/integration/068_partial_parsing_tests/test-files/models-schema4.yml create mode 100644 test/integration/068_partial_parsing_tests/test-files/models-schema4b.yml diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index 9fff39dc225..67338c8c021 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -884,6 +884,7 @@ 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) @@ -898,10 +899,16 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: ) raise ParsingException(msg) - # We're saving the patch_path because we need to schedule - # re-application of the patch in partial parsing. - node = found_nodes[0] - node.patch_path = source_file.file_id + # 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: + 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 + self.patch_node_config(node, patch) + + node.patch(patch) else: msg = ( f"Did not find matching node for patch with name '{patch.name}' " @@ -910,19 +917,18 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: ) warn_or_error(msg, log_fmt=warning_tag("{}")) return - else: - # patches can't be overwritten - node = self.manifest.nodes.get(unique_id) + + # patches can't be overwritten + node = self.manifest.nodes.get(unique_id) + if node: if node.patch_path: package_name, existing_file_path = node.patch_path.split("://") raise_duplicate_patch_name(patch, existing_file_path) - if node: source_file.append_patch(patch.yaml_key, node.unique_id) - # If this patch has config changes, re-calculate the node config - # with the patch config - if patch.config: - self.patch_node_config(node, patch) + # 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 + 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"]) From 834e825ba474186e0759f11085a68c92af18f661 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Wed, 28 Sep 2022 13:47:10 -0500 Subject: [PATCH 18/22] fix test for windows --- tests/functional/configs/test_disabled_model.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/functional/configs/test_disabled_model.py b/tests/functional/configs/test_disabled_model.py index aad938dd576..5e7b79a7bf4 100644 --- a/tests/functional/configs/test_disabled_model.py +++ b/tests/functional/configs/test_disabled_model.py @@ -251,14 +251,14 @@ def test_multiple_disabled_config(self, project): assert "model.test.my_model_2" in manifest.nodes assert "model.test.my_model_3" in manifest.nodes - expected_file_path = "models/folder_2" + 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 = "models/folder_1" + expected_disabled_file_path = "folder_1" assert ( expected_disabled_file_path in manifest.disabled["model.test.my_model_2"][0].original_file_path @@ -306,20 +306,20 @@ def test_multiple_disabled_config(self, project): assert "model.test.my_model_2" in manifest.nodes assert "model.test.my_model_3" in manifest.nodes - expected_file_path_2 = "models/folder_1" + 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 = "models/folder_2" + 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 = "models/folder_2" + 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 = "models/folder_1" + 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 @@ -377,7 +377,7 @@ def test_many_disabled_config(self, project): assert "model.test.my_model_2" in manifest.nodes assert "model.test.my_model_3" in manifest.nodes - expected_file_path = "models/folder_2" + 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 From e9f88aa3bc5a4d266b7d12bea34aa50084c694c9 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Wed, 28 Sep 2022 20:05:12 -0500 Subject: [PATCH 19/22] fix node processing to not overwrite enabled nodes --- core/dbt/parser/manifest.py | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index e4a25c21535..9abcbdec6dc 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 @@ -931,28 +932,24 @@ def process_sources(self, current_project: str): 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 aprsed - disable_nodes = [] - for node in self.manifest.nodes.values(): + # correctly now that the schema files are also parsed + disable_node_copy = deepcopy(self.manifest.nodes) + for node in disable_node_copy.values(): if not node.config.enabled: - disable_nodes.append(node) - for node in disable_nodes: - self.manifest.nodes.pop(node.unique_id) - self.manifest.add_disabled_nofile(node) + self.manifest.nodes.pop(node.unique_id) + self.manifest.add_disabled_nofile(node) - enable_nodes = {} - for disabled in self.manifest.disabled.values(): + 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 - enable_nodes[dis_index] = dis_node - for index, node in enable_nodes.items(): - del self.manifest.disabled[node.unique_id][index] - if not self.manifest.disabled[node.unique_id]: - self.manifest.disabled.pop(node.unique_id) + 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.add_node_nofile(node) self.manifest.rebuild_ref_lookup() From 5789d643c091ae2dcc8e3d03782ae086f602b3ae Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Thu, 29 Sep 2022 10:23:37 -0500 Subject: [PATCH 20/22] add checking disabled in pp, fix error msg --- core/dbt/parser/partial.py | 27 +++++++++++++++++---------- core/dbt/parser/schemas.py | 10 ++++++---- 2 files changed, 23 insertions(+), 14 deletions(-) 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 67338c8c021..e9357ae484f 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -894,8 +894,8 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: # We have no way to know which one to enable. msg = ( f"Found {len(found_nodes)} matching disabled nodes for '{patch.name}'. " - "Multiple nodes for the same unique id cannot be disabled in the schema " - "file. They must be disabled in `dbt_project.yml` or in the sql files." + "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) @@ -906,7 +906,8 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: 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 - self.patch_node_config(node, patch) + if patch.config: + self.patch_node_config(node, patch) node.patch(patch) else: @@ -928,7 +929,8 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: 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 - self.patch_node_config(node, patch) + if patch.config: + self.patch_node_config(node, patch) node.patch(patch) From b2a6156753035aa70fcc86fbaa56125d35122b2a Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Thu, 29 Sep 2022 11:41:15 -0500 Subject: [PATCH 21/22] stop deepcopying all nodes when processing --- core/dbt/parser/manifest.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index 9abcbdec6dc..43733712018 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -933,11 +933,13 @@ def process_sources(self, current_project: str): 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 - disable_node_copy = deepcopy(self.manifest.nodes) - for node in disable_node_copy.values(): + disabled_nodes = [] + for node in self.manifest.nodes.values(): if not node.config.enabled: - self.manifest.nodes.pop(node.unique_id) + 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(): From 78c58af37e60f97a4e1886ec0e2aeb45c7d0e20a Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Thu, 29 Sep 2022 14:29:52 -0500 Subject: [PATCH 22/22] update error message --- core/dbt/parser/schemas.py | 8 +++++--- tests/functional/configs/test_disabled_model.py | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index e9357ae484f..4a6202ff0b8 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -892,10 +892,12 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None: 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 '{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." + 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) diff --git a/tests/functional/configs/test_disabled_model.py b/tests/functional/configs/test_disabled_model.py index 5e7b79a7bf4..3d8d9f28577 100644 --- a/tests/functional/configs/test_disabled_model.py +++ b/tests/functional/configs/test_disabled_model.py @@ -210,7 +210,7 @@ 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 'my_model_2'" + expected_msg = "Found 3 matching disabled nodes for model 'my_model_2'" assert expected_msg in exc_str