8030/fix contract checksum

.changes/unreleased/Fixes-20230711-171901.yaml

@@ -0,0 +1,6 @@
+kind: Fixes
+body: Detect breaking contract changes to versioned models
+time: 2023-07-11T17:19:01.120379-07:00
+custom:
+  Author: michelleark
+  Issue: "8030"

core/dbt/contracts/graph/nodes.py

@@ -625,6 +625,11 @@ 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.
         # This needs to be executed after contract config is set
+
+        # Avoid rebuilding the checksum if it has already been set.
+        if self.contract.checksum is not None:
+            return
+
         if self.contract.enforced is True:
             contract_state = ""
             # We need to sort the columns so that order doesn't matter

core/dbt/parser/schemas.py

@@ -784,6 +784,9 @@ def parse_patch(self, block: TargetBlock[UnparsedModelUpdate], refs: ParserRef)
 
                 # Includes alias recomputation
                 self.patch_node_config(versioned_model_node, versioned_model_patch)
+
+                # Need to reapply this here, in the case that 'contract: {enforced: true}' was during config-setting
+                versioned_model_node.build_contract_checksum()
                 source_file.append_patch(
                     versioned_model_patch.yaml_key, versioned_model_node.unique_id
                 )

tests/functional/defer_state/fixtures.py

@@ -74,17 +74,25 @@
       - name: name
 """
 
-contract_schema_yml = """
+no_contract_schema_yml = """
 version: 2
 models:
-  - name: view_model
+  - name: table_model
+    config: {}
     columns:
       - name: id
+        data_type: integer
         tests:
           - unique:
               severity: error
           - not_null
       - name: name
+        data_type: text
+"""
+
+contract_schema_yml = """
+version: 2
+models:
   - name: table_model
     config:
       contract:
@@ -103,41 +111,106 @@
 modified_contract_schema_yml = """
 version: 2
 models:
-  - name: view_model
+  - name: table_model
+    config:
+      contract:
+        enforced: True
+    columns:
+      - name: id
+        data_type: integer
+        tests:
+          - unique:
+              severity: error
+          - not_null
+      - name: user_name
+        data_type: text
+"""
+
+disabled_contract_schema_yml = """
+version: 2
+models:
+  - name: table_model
+    config:
+      contract:
+        enforced: False
+    columns:
+      - name: id
+        data_type: integer
+        tests:
+          - unique:
+              severity: error
+          - not_null
+      - name: name
+        data_type: text
+"""
+
+versioned_no_contract_schema_yml = """
+version: 2
+models:
+  - name: table_model
+    config: {}
+    versions:
+      - v: 1
     columns:
       - name: id
+        data_type: integer
         tests:
           - unique:
               severity: error
           - not_null
       - name: name
+        data_type: text
+"""
+
+versioned_contract_schema_yml = """
+version: 2
+models:
   - name: table_model
     config:
       contract:
         enforced: True
+    versions:
+      - v: 1
     columns:
       - name: id
         data_type: integer
         tests:
           - unique:
               severity: error
           - not_null
-      - name: user_name
+      - name: name
         data_type: text
 """
 
-disabled_contract_schema_yml = """
+versioned_modified_contract_schema_yml = """
 version: 2
 models:
-  - name: view_model
+  - name: table_model
+    config:
+      contract:
+        enforced: True
+    versions:
+      - v: 1
     columns:
       - name: id
+        data_type: integer
         tests:
           - unique:
               severity: error
           - not_null
-      - name: name
+      - name: user_name
+        data_type: text
+"""
+
+versioned_disabled_contract_schema_yml = """
+version: 2
+models:
   - name: table_model
+    config:
+      contract:
+        enforced: False
+    versions:
+      - v: 1
     columns:
       - name: id
         data_type: integer

tests/functional/defer_state/test_modified_state.py

@@ -18,10 +18,15 @@
     exposures_yml,
     macros_sql,
     infinite_macros_sql,
+    no_contract_schema_yml,
     contract_schema_yml,
     modified_contract_schema_yml,
     disabled_contract_schema_yml,
     constraint_schema_yml,
+    versioned_no_contract_schema_yml,
+    versioned_contract_schema_yml,
+    versioned_disabled_contract_schema_yml,
+    versioned_modified_contract_schema_yml,
     modified_column_constraint_schema_yml,
     modified_model_constraint_schema_yml,
     table_model_now_view_sql,
@@ -492,11 +497,17 @@ def test_changed_exposure(self, project):
 
 
 class TestChangedContract(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
+    NO_CONTRACT_SCHEMA_YML = no_contract_schema_yml
+
     def test_changed_contract(self, project):
         self.run_and_save_state()
 
         # update contract for table_model
-        write_file(contract_schema_yml, "models", "schema.yml")
+        write_file(self.CONTRACT_SCHEMA_YML, "models", "schema.yml")
 
         # This will find the table_model node modified both through a config change
         # and by a non-breaking change to contract: true
@@ -509,7 +520,7 @@ def test_changed_contract(self, project):
         assert results[0].node.name == "table_model"
 
         manifest = get_manifest(project.project_root)
-        model_unique_id = "model.test.table_model"
+        model_unique_id = self.MODEL_UNIQUE_ID
         model = manifest.nodes[model_unique_id]
         expected_unrendered_config = {"contract": {"enforced": True}, "materialized": "table"}
         assert model.unrendered_config == expected_unrendered_config
@@ -525,7 +536,7 @@ def test_changed_contract(self, project):
         self.copy_state()
 
         # This should raise because a column name has changed
-        write_file(modified_contract_schema_yml, "models", "schema.yml")
+        write_file(self.MODIFIED_SCHEMA_YML, "models", "schema.yml")
         results = run_dbt(["run"], expect_pass=False)
         assert len(results) == 2
         manifest = get_manifest(project.project_root)
@@ -537,16 +548,24 @@ def test_changed_contract(self, project):
             results = run_dbt(["run", "--models", "state:modified.contract", "--state", "./state"])
 
         # Go back to schema file without contract. Should raise an error.
-        write_file(schema_yml, "models", "schema.yml")
+        write_file(self.NO_CONTRACT_SCHEMA_YML, "models", "schema.yml")
         with pytest.raises(ContractBreakingChangeError):
             results = run_dbt(["run", "--models", "state:modified.contract", "--state", "./state"])
 
         # Now disable the contract. Should raise an error.
-        write_file(disabled_contract_schema_yml, "models", "schema.yml")
+        write_file(self.DISABLED_SCHEMA_YML, "models", "schema.yml")
         with pytest.raises(ContractBreakingChangeError):
             results = run_dbt(["run", "--models", "state:modified.contract", "--state", "./state"])
 
 
+class TestChangedContractVersioned(TestChangedContract):
+    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
+    NO_CONTRACT_SCHEMA_YML = versioned_no_contract_schema_yml
+
+
 class TestChangedConstraint(BaseModifiedState):
     def test_changed_constraint(self, project):
         self.run_and_save_state()

tests/unit/test_parser.py

@@ -311,6 +311,18 @@ def assertEqualNodes(node_one, node_two):
             - name: extra
 """
 
+MULTIPLE_TABLE_VERSIONED_MODEL_CONTRACT_ENFORCED = """
+models:
+    - name: my_model
+      config:
+        contract:
+            enforced: true
+      versions:
+        - v: 0
+          defined_in: arbitrary_file_name
+        - v: 2
+"""
+
 MULTIPLE_TABLE_VERSIONED_MODEL_V0 = """
 models:
     - name: my_model
@@ -727,6 +739,18 @@ def test__parsed_versioned_models(self):
         self.parser.parse_file(block, dct)
         self.assert_has_manifest_lengths(self.parser.manifest, nodes=2)
 
+    def test__parsed_versioned_models_contract_enforced(self):
+        block = self.file_block_for(
+            MULTIPLE_TABLE_VERSIONED_MODEL_CONTRACT_ENFORCED, "test_one.yml"
+        )
+        self.parser.manifest.files[block.file.file_id] = block.file
+        dct = yaml_from_file(block.file)
+        self.parser.parse_file(block, dct)
+        self.assert_has_manifest_lengths(self.parser.manifest, nodes=2)
+        for node in self.parser.manifest.nodes.values():
+            assert node.contract.enforced
+            node.build_contract_checksum.assert_called()
+
     def test__parsed_versioned_models_v0(self):
         block = self.file_block_for(MULTIPLE_TABLE_VERSIONED_MODEL_V0, "test_one.yml")
         self.parser.manifest.files[block.file.file_id] = block.file