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

[MichelleArk]

resolves https://github.com/dbt-labs/dbt-core/issues/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

• [x] I have read the contributing guide and understand what's expected of me
• [x] I have run this code in development and it appears to resolve the stated issue
• [x] This PR includes tests, or tests are not required/relevant for this PR
• [x] 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
• [x] This PR includes type annotations for new and modified functions

[github-actions[bot]]

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]

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 :warning:
core/dbt/parser/manifest.py	66.66%	1 Missing :warning:

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%)	:arrow_down:
unit	63.29% <25.00%> (+0.01%)	:arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[MichelleArk]

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]

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]

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]

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