diff --git a/.changes/unreleased/Fixes-20240523-204251.yaml b/.changes/unreleased/Fixes-20240523-204251.yaml new file mode 100644 index 00000000000..33abfd2ae34 --- /dev/null +++ b/.changes/unreleased/Fixes-20240523-204251.yaml @@ -0,0 +1,6 @@ +kind: Fixes +body: Renaming or removing a contracted model should raise a BreakingChange warning/error +time: 2024-05-23T20:42:51.033946-04:00 +custom: + Author: michelleark + Issue: "10116" diff --git a/core/dbt/contracts/graph/nodes.py b/core/dbt/contracts/graph/nodes.py index 4cc72327332..af5b317f2f8 100644 --- a/core/dbt/contracts/graph/nodes.py +++ b/core/dbt/contracts/graph/nodes.py @@ -19,6 +19,8 @@ from mashumaro.types import SerializableType from dbt import deprecations +from dbt.adapters.base import ConstraintSupport +from dbt.adapters.factory import get_adapter_constraint_support from dbt.artifacts.resources import Analysis as AnalysisResource from dbt.artifacts.resources import ( BaseResource, @@ -469,6 +471,13 @@ def is_external_node(self) -> bool: def is_latest_version(self) -> bool: return self.version is not None and self.version == self.latest_version + @property + def is_past_deprecation_date(self) -> bool: + return ( + self.deprecation_date is not None + and self.deprecation_date < datetime.now().astimezone() + ) + @property def search_name(self): if self.version is None: @@ -570,6 +579,42 @@ def build_contract_checksum(self): data = contract_state.encode("utf-8") self.contract.checksum = hashlib.new("sha256", data).hexdigest() + def same_contract_removed(self) -> bool: + """ + self: the removed (deleted, renamed, or disabled) model node + """ + # If the contract wasn't previously enforced, no contract change has occurred + if self.contract.enforced is False: + return True + + # Removed node is past its deprecation_date, so deletion does not constitute a contract change + if self.is_past_deprecation_date: + return True + + # Disabled, deleted, or renamed node with previously enforced contract. + if not self.config.enabled: + breaking_change = f"Contracted model '{self.unique_id}' was disabled." + else: + breaking_change = f"Contracted model '{self.unique_id}' was deleted or renamed." + + if self.version is None: + warn_or_error( + UnversionedBreakingChange( + breaking_changes=[breaking_change], + model_name=self.name, + model_file_path=self.original_file_path, + ), + node=self, + ) + return False + else: + raise ( + ContractBreakingChangeError( + breaking_changes=[breaking_change], + node=self, + ) + ) + def same_contract(self, old, adapter_type=None) -> bool: # If the contract wasn't previously enforced: if old.contract.enforced is False and self.contract.enforced is False: @@ -601,10 +646,6 @@ def same_contract(self, old, adapter_type=None) -> bool: # Breaking change: the contract was previously enforced, and it no longer is contract_enforced_disabled = True - # TODO: this avoid the circular imports but isn't ideal - from dbt.adapters.base import ConstraintSupport - from dbt.adapters.factory import get_adapter_constraint_support - constraint_support = get_adapter_constraint_support(adapter_type) column_constraints_exist = False diff --git a/core/dbt/graph/selector_methods.py b/core/dbt/graph/selector_methods.py index 380bad2e273..31d6d6c1fc6 100644 --- a/core/dbt/graph/selector_methods.py +++ b/core/dbt/graph/selector_methods.py @@ -719,7 +719,9 @@ def check_modified_contract( ) -> Callable[[Optional[SelectorTarget], SelectorTarget], bool]: # get a function that compares two selector target based on compare method provided def check_modified_contract(old: Optional[SelectorTarget], new: SelectorTarget) -> bool: - if hasattr(new, compare_method): + if new is None and hasattr(old, compare_method + "_removed"): + return getattr(old, compare_method + "_removed")() + elif hasattr(new, compare_method): # when old body does not exist or old and new are not the same return not old or not getattr(new, compare_method)(old, adapter_type) # type: ignore else: @@ -785,6 +787,22 @@ def search(self, included_nodes: Set[UniqueId], selector: str) -> Iterator[Uniqu if checker(previous_node, node, **keyword_args): # type: ignore yield unique_id + # checkers that can handle removed nodes + if checker.__name__ in ["check_modified_contract"]: + # ignore included_nodes, since those cannot contain removed nodes + for previous_unique_id, previous_node in manifest.nodes.items(): + # detect removed (deleted, renamed, or disabled) nodes + removed_node = None + if previous_unique_id in self.manifest.disabled.keys(): + removed_node = self.manifest.disabled[previous_unique_id][0] + elif previous_unique_id not in self.manifest.nodes.keys(): + removed_node = previous_node + + if removed_node: + # do not yield -- removed nodes should never be selected for downstream execution + # as they are not part of the current project's manifest.nodes + checker(removed_node, None, **keyword_args) # type: ignore + class ResultSelectorMethod(SelectorMethod): def search(self, included_nodes: Set[UniqueId], selector: str) -> Iterator[UniqueId]: diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index 62741b2da6d..984185f2cd5 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -570,25 +570,21 @@ def safe_update_project_parser_files_partially(self, project_parser_files: Dict) def check_for_model_deprecations(self): for node in self.manifest.nodes.values(): - if isinstance(node, ModelNode): - if ( - node.deprecation_date - and node.deprecation_date < datetime.datetime.now().astimezone() - ): - warn_or_error( - DeprecatedModel( - model_name=node.name, - model_version=version_to_str(node.version), - deprecation_date=node.deprecation_date.isoformat(), - ) + if isinstance(node, ModelNode) and node.is_past_deprecation_date: + warn_or_error( + DeprecatedModel( + model_name=node.name, + model_version=version_to_str(node.version), + deprecation_date=node.deprecation_date.isoformat(), ) + ) resolved_refs = self.manifest.resolve_refs(node, self.root_project.project_name) resolved_model_refs = [r for r in resolved_refs if isinstance(r, ModelNode)] node.depends_on for resolved_ref in resolved_model_refs: if resolved_ref.deprecation_date: - if resolved_ref.deprecation_date < datetime.datetime.now().astimezone(): + if resolved_ref.is_past_deprecation_date: event_cls = DeprecatedReference else: event_cls = UpcomingReferenceDeprecation diff --git a/tests/functional/defer_state/fixtures.py b/tests/functional/defer_state/fixtures.py index 518cd4cbf19..832bf258f56 100644 --- a/tests/functional/defer_state/fixtures.py +++ b/tests/functional/defer_state/fixtures.py @@ -108,6 +108,25 @@ data_type: text """ +disabled_contract_schema_yml = """ +version: 2 +models: + - name: table_model + config: + contract: + enforced: True + enabled: False + columns: + - name: id + data_type: integer + data_tests: + - unique: + severity: error + - not_null + - name: name + data_type: text +""" + modified_contract_schema_yml = """ version: 2 models: @@ -126,7 +145,7 @@ data_type: text """ -disabled_contract_schema_yml = """ +unenforced_contract_schema_yml = """ version: 2 models: - name: table_model @@ -144,6 +163,25 @@ data_type: text """ +disabled_unenforced_contract_schema_yml = """ +version: 2 +models: + - name: table_model + config: + contract: + enforced: False + enabled: False + columns: + - name: id + data_type: integer + data_tests: + - unique: + severity: error + - not_null + - name: name + data_type: text +""" + versioned_no_contract_schema_yml = """ version: 2 models: @@ -182,6 +220,27 @@ data_type: text """ +disabled_versioned_contract_schema_yml = """ +version: 2 +models: + - name: table_model + config: + contract: + enforced: True + enabled: False + versions: + - v: 1 + columns: + - name: id + data_type: integer + data_tests: + - unique: + severity: error + - not_null + - name: name + data_type: text +""" + versioned_modified_contract_schema_yml = """ version: 2 models: @@ -202,7 +261,28 @@ data_type: text """ -versioned_disabled_contract_schema_yml = """ +disabled_versioned_unenforced_contract_schema_yml = """ +version: 2 +models: + - name: table_model + config: + contract: + enforced: False + enabled: False + versions: + - v: 1 + columns: + - name: id + data_type: integer + data_tests: + - unique: + severity: error + - not_null + - name: name + data_type: text +""" + +versioned_unenforced_contract_schema_yml = """ version: 2 models: - name: table_model diff --git a/tests/functional/defer_state/test_modified_state.py b/tests/functional/defer_state/test_modified_state.py index bfd5648ae61..2ded38e742b 100644 --- a/tests/functional/defer_state/test_modified_state.py +++ b/tests/functional/defer_state/test_modified_state.py @@ -8,6 +8,7 @@ from dbt.exceptions import CompilationError, ContractBreakingChangeError from dbt.tests.util import ( get_manifest, + rm_file, run_dbt, run_dbt_and_capture, update_config_file, @@ -17,6 +18,9 @@ constraint_schema_yml, contract_schema_yml, disabled_contract_schema_yml, + disabled_unenforced_contract_schema_yml, + disabled_versioned_contract_schema_yml, + disabled_versioned_unenforced_contract_schema_yml, ephemeral_model_sql, exposures_yml, infinite_macros_sql, @@ -33,10 +37,11 @@ table_model_now_incremental_sql, table_model_now_view_sql, table_model_sql, + unenforced_contract_schema_yml, versioned_contract_schema_yml, - versioned_disabled_contract_schema_yml, versioned_modified_contract_schema_yml, versioned_no_contract_schema_yml, + versioned_unenforced_contract_schema_yml, view_model_now_table_sql, view_model_sql, ) @@ -507,7 +512,7 @@ class TestChangedContractUnversioned(BaseModifiedState): MODEL_UNIQUE_ID = "model.test.table_model" CONTRACT_SCHEMA_YML = contract_schema_yml MODIFIED_SCHEMA_YML = modified_contract_schema_yml - DISABLED_SCHEMA_YML = disabled_contract_schema_yml + UNENFORCED_SCHEMA_YML = unenforced_contract_schema_yml NO_CONTRACT_SCHEMA_YML = no_contract_schema_yml def test_changed_contract(self, project): @@ -570,8 +575,8 @@ def test_changed_contract(self, project): expected_warning = "While comparing to previous project state, dbt detected a breaking change to an unversioned model" expected_change = "Contract enforcement was removed" - # Now disable the contract. Should throw a warning - force warning into an error. - write_file(self.DISABLED_SCHEMA_YML, "models", "schema.yml") + # Now unenforce the contract. Should throw a warning - force warning into an error. + write_file(self.UNENFORCED_SCHEMA_YML, "models", "schema.yml") with pytest.raises(CompilationError): _, logs = run_dbt_and_capture( [ @@ -591,7 +596,7 @@ class TestChangedContractVersioned(BaseModifiedState): MODEL_UNIQUE_ID = "model.test.table_model.v1" CONTRACT_SCHEMA_YML = versioned_contract_schema_yml MODIFIED_SCHEMA_YML = versioned_modified_contract_schema_yml - DISABLED_SCHEMA_YML = versioned_disabled_contract_schema_yml + UNENFORCED_SCHEMA_YML = versioned_unenforced_contract_schema_yml NO_CONTRACT_SCHEMA_YML = versioned_no_contract_schema_yml def test_changed_contract_versioned(self, project): @@ -643,12 +648,138 @@ def test_changed_contract_versioned(self, project): with pytest.raises(ContractBreakingChangeError): results = run_dbt(["run", "--models", "state:modified.contract", "--state", "./state"]) - # Now disable the contract. Should raise an error. - write_file(self.DISABLED_SCHEMA_YML, "models", "schema.yml") + # Now unenforce the contract. Should raise an error. + write_file(self.UNENFORCED_SCHEMA_YML, "models", "schema.yml") with pytest.raises(ContractBreakingChangeError): results = run_dbt(["run", "--models", "state:modified.contract", "--state", "./state"]) +class TestDeleteUnversionedContractedModel(BaseModifiedState): + MODEL_UNIQUE_ID = "model.test.table_model" + CONTRACT_SCHEMA_YML = contract_schema_yml + + def test_delete_unversioned_contracted_model(self, project): + # ensure table_model is contracted + write_file(self.CONTRACT_SCHEMA_YML, "models", "schema.yml") + self.run_and_save_state() + + # delete versioned contracted model + rm_file(project.project_root, "models", "table_model.sql") + + # since the models are unversioned, they raise a warning but not an error + _, logs = run_dbt_and_capture( + ["run", "--models", "state:modified.contract", "--state", "./state"] + ) + + expected_warning = "While comparing to previous project state, dbt detected a breaking change to an unversioned model" + expected_change = "Contracted model 'model.test.table_model' was deleted or renamed" + assert expected_warning in logs + assert expected_change in logs + + +class TestDeleteVersionedContractedModel(BaseModifiedState): + MODEL_UNIQUE_ID = "model.test.table_model.v1" + CONTRACT_SCHEMA_YML = versioned_contract_schema_yml + + def test_delete_versioned_contracted_model(self, project): + # ensure table_model is versioned + contracted + write_file(self.CONTRACT_SCHEMA_YML, "models", "schema.yml") + self.run_and_save_state() + + # delete versioned contracted model + rm_file(project.project_root, "models", "table_model.sql") + + # since the models are versioned, they raise an error + with pytest.raises(ContractBreakingChangeError) as e: + run_dbt(["run", "--models", "state:modified.contract", "--state", "./state"]) + + assert "Contracted model 'model.test.table_model.v1' was deleted or renamed." in str( + e.value + ) + + +class TestDisableUnversionedContractedModel(BaseModifiedState): + MODEL_UNIQUE_ID = "model.test.table_model" + CONTRACT_SCHEMA_YML = contract_schema_yml + DISABLED_CONTRACT_SCHEMA_YML = disabled_contract_schema_yml + + def test_disable_unversioned_contracted_model(self, project): + # ensure table_model is contracted and enabled + write_file(self.CONTRACT_SCHEMA_YML, "models", "schema.yml") + self.run_and_save_state() + + # disable unversioned + contracted model + write_file(self.DISABLED_CONTRACT_SCHEMA_YML, "models", "schema.yml") + + # since the models are unversioned, they raise a warning but not an error + _, logs = run_dbt_and_capture( + ["run", "--models", "state:modified.contract", "--state", "./state"] + ) + + expected_warning = "While comparing to previous project state, dbt detected a breaking change to an unversioned model" + expected_change = "Contracted model 'model.test.table_model' was disabled" + assert expected_warning in logs + assert expected_change in logs + + +class TestDisableVersionedContractedModel(BaseModifiedState): + MODEL_UNIQUE_ID = "model.test.table_model.v1" + CONTRACT_SCHEMA_YML = versioned_contract_schema_yml + DISABLED_CONTRACT_SCHEMA_YML = disabled_versioned_contract_schema_yml + + def test_disable_versioned_contracted_model(self, project): + # ensure table_model is versioned + contracted + write_file(self.CONTRACT_SCHEMA_YML, "models", "schema.yml") + self.run_and_save_state() + + # disable versioned + contracted model + write_file(self.DISABLED_CONTRACT_SCHEMA_YML, "models", "schema.yml") + + # since the models are versioned, they raise an error + with pytest.raises(ContractBreakingChangeError) as e: + run_dbt(["run", "--models", "state:modified.contract", "--state", "./state"]) + + assert "Contracted model 'model.test.table_model.v1' was disabled." in str(e.value) + + +class TestDisableUnversionedUncontractedModel(BaseModifiedState): + MODEL_UNIQUE_ID = "model.test.table_model" + NO_CONTRACT_SCHEMA_YML = unenforced_contract_schema_yml + DISABLED_NO_CONTRACT_SCHEMA_YML = disabled_unenforced_contract_schema_yml + + def test_delete_versioned_contracted_model(self, project): + # ensure table_model is not contracted + write_file(self.NO_CONTRACT_SCHEMA_YML, "models", "schema.yml") + self.run_and_save_state() + + # disable uncontracted model + write_file(self.DISABLED_NO_CONTRACT_SCHEMA_YML, "models", "schema.yml") + + # since the models are unversioned, no warning or error is raised + _, logs = run_dbt_and_capture( + ["run", "--models", "state:modified.contract", "--state", "./state"] + ) + + assert "breaking change" not in logs.lower() + + +class TestDisableVersionedUncontractedModel(BaseModifiedState): + MODEL_UNIQUE_ID = "model.test.table_model.v1" + NO_CONTRACT_SCHEMA_YML = versioned_unenforced_contract_schema_yml + DISABLED_NO_CONTRACT_SCHEMA_YML = disabled_versioned_unenforced_contract_schema_yml + + def test_delete_versioned_contracted_model(self, project): + # ensure table_model is not contracted + write_file(self.NO_CONTRACT_SCHEMA_YML, "models", "schema.yml") + self.run_and_save_state() + + # disable uncontracted model + write_file(self.DISABLED_NO_CONTRACT_SCHEMA_YML, "models", "schema.yml") + + # since the models are unversioned, no warning or error is raised + run_dbt_and_capture(["run", "--models", "state:modified.contract", "--state", "./state"]) + + class TestChangedConstraintUnversioned(BaseModifiedState): def test_changed_constraint(self, project): self.run_and_save_state() diff --git a/tests/unit/graph/test_nodes.py b/tests/unit/graph/test_nodes.py index 4a62db34b11..59e198c1f86 100644 --- a/tests/unit/graph/test_nodes.py +++ b/tests/unit/graph/test_nodes.py @@ -1,4 +1,5 @@ from copy import deepcopy +from datetime import datetime from typing import List import pytest @@ -8,11 +9,19 @@ DimensionType, EntityType, ) - -from dbt.artifacts.resources import Defaults, Dimension, Entity, Measure, TestMetadata +from freezegun import freeze_time + +from dbt.artifacts.resources import ( + Defaults, + Dimension, + Entity, + FileHash, + Measure, + TestMetadata, +) from dbt.artifacts.resources.v1.semantic_model import NodeRelation from dbt.contracts.graph.model_config import TestConfig -from dbt.contracts.graph.nodes import ColumnInfo, SemanticModel +from dbt.contracts.graph.nodes import ColumnInfo, ModelNode, SemanticModel from dbt.node_types import NodeType from dbt_common.contracts.constraints import ( ColumnLevelConstraint, @@ -22,6 +31,44 @@ from tests.unit.fixtures import generic_test_node, model_node +class TestModelNode: + @pytest.fixture(scope="class") + def default_model_node(self): + return ModelNode( + resource_type=NodeType.Model, + unique_id="model.test_package.test_name", + name="test_name", + package_name="test_package", + schema="test_schema", + alias="test_alias", + fqn=["models", "test_name"], + original_file_path="test_original_file_path", + checksum=FileHash.from_contents("checksum"), + path="test_path", + database=None, + ) + + @pytest.mark.parametrize( + "deprecation_date,current_date,expected_is_past_deprecation_date", + [ + (None, "2024-05-02", False), + ("2024-05-01", "2024-05-02", True), + ("2024-05-01", "2024-05-01", False), + ("2024-05-01", "2024-04-30", False), + ], + ) + def test_is_past_deprecation_date( + self, default_model_node, deprecation_date, current_date, expected_is_past_deprecation_date + ): + with freeze_time(current_date): + if deprecation_date is not None: + default_model_node.deprecation_date = datetime.strptime( + deprecation_date, "%Y-%m-%d" + ).astimezone() + + assert default_model_node.is_past_deprecation_date is expected_is_past_deprecation_date + + class TestSemanticModel: @pytest.fixture(scope="function") def dimensions(self) -> List[Dimension]: