From 6603a4415183abe333455f9fb4da4c26e3fdb8cc Mon Sep 17 00:00:00 2001 From: Michelle Ark Date: Thu, 10 Aug 2023 11:29:02 -0400 Subject: [PATCH] Detect changes to model access, deprecation_date, and latest_version in state:modified (#8264) --- .../unreleased/Fixes-20230806-222319.yaml | 6 + core/dbt/contracts/graph/nodes.py | 12 ++ core/dbt/node_types.py | 2 +- .../defer_state/test_modified_state.py | 105 ++++++++++++++++++ tests/unit/test_contracts_graph_parsed.py | 6 + tests/unit/test_graph_selector_methods.py | 3 +- 6 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 .changes/unreleased/Fixes-20230806-222319.yaml diff --git a/.changes/unreleased/Fixes-20230806-222319.yaml b/.changes/unreleased/Fixes-20230806-222319.yaml new file mode 100644 index 00000000000..cd6a0ec9ff1 --- /dev/null +++ b/.changes/unreleased/Fixes-20230806-222319.yaml @@ -0,0 +1,6 @@ +kind: Fixes +body: Detect changes to model access, version, or latest_version in state:modified +time: 2023-08-06T22:23:19.166334-04:00 +custom: + Author: michelleark + Issue: "8189" diff --git a/core/dbt/contracts/graph/nodes.py b/core/dbt/contracts/graph/nodes.py index 59ac576db79..98f961a0363 100644 --- a/core/dbt/contracts/graph/nodes.py +++ b/core/dbt/contracts/graph/nodes.py @@ -626,6 +626,18 @@ def search_name(self): def materialization_enforces_constraints(self) -> bool: return self.config.materialized in ["table", "incremental"] + def same_contents(self, old, adapter_type) -> bool: + return super().same_contents(old, adapter_type) and self.same_ref_representation(old) + + def same_ref_representation(self, old) -> bool: + return ( + # Changing the latest_version may break downstream unpinned refs + self.latest_version == old.latest_version + # Changes to access or deprecation_date may lead to ref-related parsing errors + and self.access == old.access + and self.deprecation_date == old.deprecation_date + ) + def build_contract_checksum(self): # We don't need to construct the checksum if the model does not # have contract enforced, because it won't be used. diff --git a/core/dbt/node_types.py b/core/dbt/node_types.py index 33d8fc4b0f4..b2825b29e09 100644 --- a/core/dbt/node_types.py +++ b/core/dbt/node_types.py @@ -4,8 +4,8 @@ class AccessType(StrEnum): - Protected = "protected" Private = "private" + Protected = "protected" Public = "public" @classmethod diff --git a/tests/functional/defer_state/test_modified_state.py b/tests/functional/defer_state/test_modified_state.py index 1aa28473b78..f44ada2cd67 100644 --- a/tests/functional/defer_state/test_modified_state.py +++ b/tests/functional/defer_state/test_modified_state.py @@ -781,3 +781,108 @@ def test_modified_body_and_contract(self, project): # The model's contract has changed, even if non-breaking, so it should be selected by 'state:modified.contract' results = run_dbt(["list", "-s", "state:modified.contract", "--state", "./state"]) assert results == ["test.my_model"] + + +modified_table_model_access_yml = """ +version: 2 +models: + - name: table_model + access: public +""" + + +class TestModifiedAccess(BaseModifiedState): + def test_changed_access(self, project): + self.run_and_save_state() + + # No access change + assert not run_dbt(["list", "-s", "state:modified", "--state", "./state"]) + + # Modify access (protected -> public) + write_file(modified_table_model_access_yml, "models", "schema.yml") + assert run_dbt(["list", "-s", "state:modified", "--state", "./state"]) + + results = run_dbt(["list", "-s", "state:modified", "--state", "./state"]) + assert results == ["test.table_model"] + + +modified_table_model_access_yml = """ +version: 2 +models: + - name: table_model + deprecation_date: 2020-01-01 +""" + + +class TestModifiedDeprecationDate(BaseModifiedState): + def test_changed_access(self, project): + self.run_and_save_state() + + # No access change + assert not run_dbt(["list", "-s", "state:modified", "--state", "./state"]) + + # Modify deprecation_date (None -> 2020-01-01) + write_file(modified_table_model_access_yml, "models", "schema.yml") + assert run_dbt(["list", "-s", "state:modified", "--state", "./state"]) + + results = run_dbt(["list", "-s", "state:modified", "--state", "./state"]) + assert results == ["test.table_model"] + + +modified_table_model_version_yml = """ +version: 2 +models: + - name: table_model + versions: + - v: 1 + defined_in: table_model +""" + + +class TestModifiedVersion(BaseModifiedState): + def test_changed_access(self, project): + self.run_and_save_state() + + # Change version (null -> v1) + write_file(modified_table_model_version_yml, "models", "schema.yml") + + results = run_dbt(["list", "-s", "state:modified", "--state", "./state"]) + assert results == ["test.table_model.v1"] + + +table_model_latest_version_yml = """ +version: 2 +models: + - name: table_model + latest_version: 1 + versions: + - v: 1 + defined_in: table_model +""" + + +modified_table_model_latest_version_yml = """ +version: 2 +models: + - name: table_model + latest_version: 2 + versions: + - v: 1 + defined_in: table_model + - v: 2 +""" + + +class TestModifiedLatestVersion(BaseModifiedState): + def test_changed_access(self, project): + # Setup initial latest_version: 1 + write_file(table_model_latest_version_yml, "models", "schema.yml") + + self.run_and_save_state() + + # Bump latest version + write_file(table_model_sql, "models", "table_model_v2.sql") + write_file(modified_table_model_latest_version_yml, "models", "schema.yml") + + results = run_dbt(["list", "-s", "state:modified", "--state", "./state"]) + assert results == ["test.table_model.v1", "test.table_model.v2"] diff --git a/tests/unit/test_contracts_graph_parsed.py b/tests/unit/test_contracts_graph_parsed.py index 13f19b23176..b45bc66b08e 100644 --- a/tests/unit/test_contracts_graph_parsed.py +++ b/tests/unit/test_contracts_graph_parsed.py @@ -452,6 +452,8 @@ def test_invalid_bad_materialized(base_parsed_model_dict): lambda u: (u, u.replace(alias="other")), lambda u: (u, u.replace(schema="other")), lambda u: (u, u.replace(database="other")), + # unchanged ref representations - protected is default + lambda u: (u, u.replace(access=AccessType.Protected)), ] @@ -485,6 +487,10 @@ def test_invalid_bad_materialized(base_parsed_model_dict): lambda u: (u, replace_config(u, alias="other")), lambda u: (u, replace_config(u, schema="other")), lambda u: (u, replace_config(u, database="other")), + # changed ref representations + lambda u: (u, replace_config(u, access=AccessType.Public)), + lambda u: (u, replace_config(u, latest_version=2)), + lambda u: (u, replace_config(u, version=2)), ] diff --git a/tests/unit/test_graph_selector_methods.py b/tests/unit/test_graph_selector_methods.py index 564e06d0189..c307aa382d0 100644 --- a/tests/unit/test_graph_selector_methods.py +++ b/tests/unit/test_graph_selector_methods.py @@ -24,6 +24,7 @@ TestConfig, TestMetadata, ColumnInfo, + AccessType, ) from dbt.contracts.graph.manifest import Manifest, ManifestMetadata from dbt.contracts.graph.unparsed import ExposureType, Owner @@ -125,7 +126,7 @@ def make_model( checksum=FileHash.from_contents(""), version=version, latest_version=latest_version, - access=access, + access=access or AccessType.Protected, )