From 73a74657ffb579c91246e83449c2d33b3e610352 Mon Sep 17 00:00:00 2001 From: Michelle Ark Date: Mon, 31 Jul 2023 19:43:11 -0400 Subject: [PATCH 1/7] ModelNode.same_ref_representation --- core/dbt/contracts/graph/nodes.py | 13 +++++++++++++ core/dbt/node_types.py | 9 ++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/core/dbt/contracts/graph/nodes.py b/core/dbt/contracts/graph/nodes.py index 59ac576db79..415a8266c3a 100644 --- a/core/dbt/contracts/graph/nodes.py +++ b/core/dbt/contracts/graph/nodes.py @@ -626,6 +626,19 @@ 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 a version may break downstream refs + self.version == old.version + # Changing the latest_version may break downstream unpinned refs + and self.latest_version == old.latest_version + # Tightening access may break downstream refs + and self.access >= old.access + ) + 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..02d0c7ead22 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 @@ -16,6 +16,13 @@ def is_valid(cls, item): return False return True + def __lt__(self, other): + if not isinstance(other, AccessType): + raise NotImplementedError + + members = list(AccessType) + return members.index(self) < members.index(other) + class NodeType(StrEnum): Model = "model" From df3fab7ab36c9edcfaa1f4010d9eb38ee2c95730 Mon Sep 17 00:00:00 2001 From: Michelle Ark Date: Sun, 6 Aug 2023 15:10:10 -0400 Subject: [PATCH 2/7] fix unit tests --- tests/unit/test_graph_selector_methods.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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, ) From ce433fa5e1a5132a138e7d66c184d62884abc875 Mon Sep 17 00:00:00 2001 From: Michelle Ark Date: Sun, 6 Aug 2023 15:16:02 -0400 Subject: [PATCH 3/7] add unit test --- tests/unit/test_access.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 tests/unit/test_access.py diff --git a/tests/unit/test_access.py b/tests/unit/test_access.py new file mode 100644 index 00000000000..69c67c23bef --- /dev/null +++ b/tests/unit/test_access.py @@ -0,0 +1,22 @@ +import pytest + +from dbt.contracts.graph.nodes import AccessType + + +class TestAccess: + @pytest.mark.parametrize( + "access_type1,access_type2,expected_lt", + [ + (AccessType.Public, AccessType.Public, False), + (AccessType.Protected, AccessType.Public, True), + (AccessType.Private, AccessType.Public, True), + (AccessType.Public, AccessType.Protected, False), + (AccessType.Protected, AccessType.Protected, False), + (AccessType.Private, AccessType.Protected, True), + (AccessType.Public, AccessType.Private, False), + (AccessType.Protected, AccessType.Private, False), + (AccessType.Private, AccessType.Private, False), + ], + ) + def test_access_comparison(self, access_type1, access_type2, expected_lt): + assert (access_type1 < access_type2) == expected_lt From d4e53831459f9691f2ceb62a9b4be34ea627d776 Mon Sep 17 00:00:00 2001 From: Michelle Ark Date: Sun, 6 Aug 2023 15:26:51 -0400 Subject: [PATCH 4/7] more unit tests --- tests/unit/test_contracts_graph_parsed.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/unit/test_contracts_graph_parsed.py b/tests/unit/test_contracts_graph_parsed.py index 7a05c1363f7..0b9ae8290e1 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 + lambda u: (u, u.replace(access=AccessType.Private)), ] @@ -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)), ] From 1336abf22e97872568157ca563db4bf127a53da8 Mon Sep 17 00:00:00 2001 From: Michelle Ark Date: Sun, 6 Aug 2023 22:22:25 -0400 Subject: [PATCH 5/7] integration tests --- core/dbt/contracts/graph/nodes.py | 3 +- .../defer_state/test_modified_state.py | 96 +++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/core/dbt/contracts/graph/nodes.py b/core/dbt/contracts/graph/nodes.py index 415a8266c3a..5d2921286c6 100644 --- a/core/dbt/contracts/graph/nodes.py +++ b/core/dbt/contracts/graph/nodes.py @@ -635,7 +635,8 @@ def same_ref_representation(self, old) -> bool: self.version == old.version # Changing the latest_version may break downstream unpinned refs and self.latest_version == old.latest_version - # Tightening access may break downstream refs + # Tightening (reducing) access may break downstream refs; + # increasing or no change implies same ref representation and self.access >= old.access ) diff --git a/tests/functional/defer_state/test_modified_state.py b/tests/functional/defer_state/test_modified_state.py index 1aa28473b78..bc26d7f28ef 100644 --- a/tests/functional/defer_state/test_modified_state.py +++ b/tests/functional/defer_state/test_modified_state.py @@ -781,3 +781,99 @@ 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_my_model_widen_access_yml = """ +version: 2 +models: + - name: table_model + access: public +""" + +modified_my_model_tighten_access_yml = """ +version: 2 +groups: + - name: my_group + owner: {name: my_group_owner} +models: + - name: table_model + access: private + group: my_group +""" + + +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"]) + + # Tighten access (protected -> public) + write_file(modified_my_model_widen_access_yml, "models", "schema.yml") + assert not run_dbt(["list", "-s", "state:modified", "--state", "./state"]) + + # Tighten access (protected -> private) + write_file(modified_my_model_tighten_access_yml, "models", "schema.yml") + + results = run_dbt(["list", "-s", "state:modified", "--state", "./state"]) + assert results == ["test.table_model"] + + +modified_my_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_my_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"] From 9217526516b32e5a8f725be73cc98dc52bb76fd7 Mon Sep 17 00:00:00 2001 From: Michelle Ark Date: Sun, 6 Aug 2023 22:23:27 -0400 Subject: [PATCH 6/7] changelog entry --- .changes/unreleased/Fixes-20230806-222319.yaml | 6 ++++++ 1 file changed, 6 insertions(+) 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" From 8e171b8df57ad4ed39c48c47e569c2c24b7eee41 Mon Sep 17 00:00:00 2001 From: Michelle Ark Date: Mon, 7 Aug 2023 13:37:52 -0400 Subject: [PATCH 7/7] simplify access check, add deprecation_date modification detection --- core/dbt/contracts/graph/nodes.py | 10 ++--- core/dbt/node_types.py | 7 ---- .../defer_state/test_modified_state.py | 41 +++++++++++-------- tests/unit/test_access.py | 22 ---------- tests/unit/test_contracts_graph_parsed.py | 4 +- 5 files changed, 31 insertions(+), 53 deletions(-) delete mode 100644 tests/unit/test_access.py diff --git a/core/dbt/contracts/graph/nodes.py b/core/dbt/contracts/graph/nodes.py index 5d2921286c6..98f961a0363 100644 --- a/core/dbt/contracts/graph/nodes.py +++ b/core/dbt/contracts/graph/nodes.py @@ -631,13 +631,11 @@ def same_contents(self, old, adapter_type) -> bool: def same_ref_representation(self, old) -> bool: return ( - # Changing a version may break downstream refs - self.version == old.version # Changing the latest_version may break downstream unpinned refs - and self.latest_version == old.latest_version - # Tightening (reducing) access may break downstream refs; - # increasing or no change implies same ref representation - and self.access >= old.access + 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): diff --git a/core/dbt/node_types.py b/core/dbt/node_types.py index 02d0c7ead22..b2825b29e09 100644 --- a/core/dbt/node_types.py +++ b/core/dbt/node_types.py @@ -16,13 +16,6 @@ def is_valid(cls, item): return False return True - def __lt__(self, other): - if not isinstance(other, AccessType): - raise NotImplementedError - - members = list(AccessType) - return members.index(self) < members.index(other) - class NodeType(StrEnum): Model = "model" diff --git a/tests/functional/defer_state/test_modified_state.py b/tests/functional/defer_state/test_modified_state.py index bc26d7f28ef..f44ada2cd67 100644 --- a/tests/functional/defer_state/test_modified_state.py +++ b/tests/functional/defer_state/test_modified_state.py @@ -783,44 +783,53 @@ def test_modified_body_and_contract(self, project): assert results == ["test.my_model"] -modified_my_model_widen_access_yml = """ +modified_table_model_access_yml = """ version: 2 models: - name: table_model access: public """ -modified_my_model_tighten_access_yml = """ + +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 -groups: - - name: my_group - owner: {name: my_group_owner} models: - name: table_model - access: private - group: my_group + deprecation_date: 2020-01-01 """ -class TestModifiedAccess(BaseModifiedState): +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"]) - # Tighten access (protected -> public) - write_file(modified_my_model_widen_access_yml, "models", "schema.yml") - assert not run_dbt(["list", "-s", "state:modified", "--state", "./state"]) - - # Tighten access (protected -> private) - write_file(modified_my_model_tighten_access_yml, "models", "schema.yml") + # 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_my_model_version_yml = """ +modified_table_model_version_yml = """ version: 2 models: - name: table_model @@ -835,7 +844,7 @@ def test_changed_access(self, project): self.run_and_save_state() # Change version (null -> v1) - write_file(modified_my_model_version_yml, "models", "schema.yml") + 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"] diff --git a/tests/unit/test_access.py b/tests/unit/test_access.py deleted file mode 100644 index 69c67c23bef..00000000000 --- a/tests/unit/test_access.py +++ /dev/null @@ -1,22 +0,0 @@ -import pytest - -from dbt.contracts.graph.nodes import AccessType - - -class TestAccess: - @pytest.mark.parametrize( - "access_type1,access_type2,expected_lt", - [ - (AccessType.Public, AccessType.Public, False), - (AccessType.Protected, AccessType.Public, True), - (AccessType.Private, AccessType.Public, True), - (AccessType.Public, AccessType.Protected, False), - (AccessType.Protected, AccessType.Protected, False), - (AccessType.Private, AccessType.Protected, True), - (AccessType.Public, AccessType.Private, False), - (AccessType.Protected, AccessType.Private, False), - (AccessType.Private, AccessType.Private, False), - ], - ) - def test_access_comparison(self, access_type1, access_type2, expected_lt): - assert (access_type1 < access_type2) == expected_lt diff --git a/tests/unit/test_contracts_graph_parsed.py b/tests/unit/test_contracts_graph_parsed.py index 0b9ae8290e1..dd89729bdd7 100644 --- a/tests/unit/test_contracts_graph_parsed.py +++ b/tests/unit/test_contracts_graph_parsed.py @@ -452,8 +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 - lambda u: (u, u.replace(access=AccessType.Private)), + # unchanged ref representations - protected is default + lambda u: (u, u.replace(access=AccessType.Protected)), ]