[Fix] Renaming or removing a contracted model raises a BreakingChange warning/error #10221

MichelleArk · 2024-05-24T00:41:21Z

resolves #10116

Problem

Deleting a contracted model does not warn or error regarding a breaking contract change

Solution

detect deleted nodes in StateModifiedSelector
add same_contract_deleted to ModelNode to handle deletion path

Checklist

I have read the contributing guide and understand what's expected of me
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
This PR includes type annotations for new and modified functions

…ct error

github-actions · 2024-05-24T00:41:35Z

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

codecov-commenter · 2024-05-24T00:48:52Z

Codecov Report

Attention: Patch coverage is 93.75000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 88.67%. Comparing base (366d4ad) to head (e10efff).
Report is 4 commits behind head on main.

Files	Patch %	Lines
core/dbt/contracts/graph/nodes.py	94.11%	1 Missing ⚠️
core/dbt/parser/manifest.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10221      +/-   ##
==========================================
- Coverage   88.68%   88.67%   -0.01%     
==========================================
  Files         180      180              
  Lines       22435    22476      +41     
==========================================
+ Hits        19896    19931      +35     
- Misses       2539     2545       +6

Flag	Coverage Δ
integration	`85.93% <93.75%> (-0.03%)`	⬇️
unit	`63.29% <25.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

MichelleArk · 2024-05-24T17:55:00Z

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 + "_deleted"):


can't say I'm in love with this but it was the best way forward I could think of that:

reuses the state:modified.contract user-facing method (configured here) to detect both changes that affect deleted + non-deleted changes.

keep the diff method localized to the node type instead of a more global search logic in the StateSelection method which felt like overloading what it did + deviating from the current pattern too much.

open to other suggestions!

I think it's good to keep the diff method localized to the node type. It's kind of a bit hacky, but I can't think of a better way.

MichelleArk · 2024-05-24T17:57:56Z

core/dbt/graph/selector_methods.py

@@ -785,6 +787,16 @@ 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 deleted nodes
+        if checker.__name__ in ["check_modified_contract"]:


we could look to implement a new state:modified.deleted selector using this codepath in the future cc @jtcohen6

gshank

Looks good. Is this something that applies to disabled nodes too, or is that handled somewhere else?

gshank · 2024-05-28T13:51:50Z

core/dbt/contracts/graph/nodes.py

+        if self.contract.enforced is False:
+            return True
+
+        # Deleted node is passed its deprecation_date, so deletion does not constitute a contract change


Typo: "past", not "passed"

gshank · 2024-05-28T13:56:24Z

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 + "_deleted"):


I think it's good to keep the diff method localized to the node type. It's kind of a bit hacky, but I can't think of a better way.

MichelleArk · 2024-05-28T16:14:04Z

Is this something that applies to disabled nodes too, or is that handled somewhere else?

hmm, good question. I don't believe we should be raising breaking contract changes for disabled nodes that have been deleted/renamed but cc'ing @jtcohen6 to confirm. Either way it'd be good to add a test around that.

jtcohen6 · 2024-05-28T16:16:00Z

Is this something that applies to disabled nodes too, or is that handled somewhere else?

hmm, good question. I don't believe we should be raising breaking contract changes for disabled nodes that have been deleted/renamed but cc'ing @jtcohen6 to confirm. Either way it'd be good to add a test around that.

@gshank @MichelleArk Good catch, thank you for thinking of this!

If a node was previously disabled, it is not a breaking change to remove/rename it
If a node was previously enabled, with an enforced contract, and it is now disabled, I do believe that is a breaking change. (Same as if the node were deleted - the effect is that it's removed from manifest.nodes)

MichelleArk · 2024-05-28T19:25:43Z

If a node was previously enabled, with an enforced contract, and it is now disabled, I do believe that is a breaking change. (Same as if the node were deleted - the effect is that it's removed from manifest.nodes)

Thank you! That's a case worth handling in this PR.

Updated the "same_contract_deleted" method to "same_contract_removed" and plumbed disabled nodes through the state method selection method and added testing. Should be good for another look @gshank 👍

gshank · 2024-05-28T19:37:09Z

Looks good! It's nice that adding disabled nodes was just a minor code change (and another test).

first pass: removing or renaming contracted model raises/warns contra…

c59aced

…ct error

cla-bot bot added the cla:yes label May 24, 2024

changelog entry

7414955

MichelleArk changed the title ~~first pass: removing or renaming contracted model raises/warns contra…~~ [Fix] Renaming or removing a contracted model raises a BreakingChange warning/error May 24, 2024

add tests

326a887

MichelleArk commented May 24, 2024

View reviewed changes

MichelleArk marked this pull request as ready for review May 24, 2024 18:08

MichelleArk requested a review from a team as a code owner May 24, 2024 18:08

use is_past_deprecation_date in more places + add unit tests

0ebff00

gshank approved these changes May 28, 2024

View reviewed changes

extend state:modified deletion handling to removed (including disabling)

e10efff

MichelleArk merged commit 3e37d77 into main May 28, 2024
63 checks passed

MichelleArk deleted the removing-contracted-model-raises-error branch May 28, 2024 19:48

dbeatty10 mentioned this pull request Oct 1, 2024

[Core] Removing a contracted model now constitutes a breaking change dbt-labs/docs.getdbt.com#5574

Closed

gshank mentioned this pull request Oct 15, 2024

[Regression] 1.9.pre lost the warning at the project level (i.e. start of the run) that notified users that a model depends on a now deprecated model #10833

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] Renaming or removing a contracted model raises a BreakingChange warning/error #10221

[Fix] Renaming or removing a contracted model raises a BreakingChange warning/error #10221

MichelleArk commented May 24, 2024 •

edited

Loading

github-actions bot commented May 24, 2024

codecov-commenter commented May 24, 2024 •

edited by codecov bot

Loading

MichelleArk May 24, 2024 •

edited

Loading

gshank May 28, 2024

MichelleArk May 24, 2024

jtcohen6 May 24, 2024

gshank left a comment

gshank May 28, 2024

gshank May 28, 2024

MichelleArk commented May 28, 2024

jtcohen6 commented May 28, 2024

MichelleArk commented May 28, 2024

gshank commented May 28, 2024

[Fix] Renaming or removing a contracted model raises a BreakingChange warning/error #10221

[Fix] Renaming or removing a contracted model raises a BreakingChange warning/error #10221

Conversation

MichelleArk commented May 24, 2024 • edited Loading

Problem

Solution

Checklist

github-actions bot commented May 24, 2024

codecov-commenter commented May 24, 2024 • edited by codecov bot Loading

Codecov Report

MichelleArk May 24, 2024 • edited Loading

Choose a reason for hiding this comment

gshank May 28, 2024

Choose a reason for hiding this comment

MichelleArk May 24, 2024

Choose a reason for hiding this comment

jtcohen6 May 24, 2024

Choose a reason for hiding this comment

gshank left a comment

Choose a reason for hiding this comment

gshank May 28, 2024

Choose a reason for hiding this comment

gshank May 28, 2024

Choose a reason for hiding this comment

MichelleArk commented May 28, 2024

jtcohen6 commented May 28, 2024

MichelleArk commented May 28, 2024

gshank commented May 28, 2024

MichelleArk commented May 24, 2024 •

edited

Loading

codecov-commenter commented May 24, 2024 •

edited by codecov bot

Loading

MichelleArk May 24, 2024 •

edited

Loading