Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CT-2378] [Bug] state:modified not catching breaking changes if model body also changes #7282

Closed
jtcohen6 opened this issue Apr 5, 2023 · 1 comment · Fixed by #7283
Closed
Assignees
Labels

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Apr 5, 2023

If I change both the contract and the body of a model with an enforced contract, state:modified selects the model without raising the exception added in #6869 / #7216.

Repro case

models:
  - name: my_model
    config:
      contract:
        enforced: true
    columns:
      - name: id
        data_type: int
select 1 as id
$ mkdir state
$ dbt parse
$ mv target/manifest.json state/

If I change just the yaml to data_type: string:

$ dbt ls -s state:modified --state state/
19:17:06  Found 1 model, 0 tests, 0 snapshots, 1 analysis, 522 macros, 0 operations, 1 seed file, 0 sources, 0 exposures, 0 metrics, 0 groups
19:17:06  Running with dbt=1.5.0-b5
...
dbt.exceptions.ModelContractError: Contract Error in model my_model (models/my_model.sql)
  There is a breaking change in the model contract because column definitions have changed; you may need to create a new version. See: https://docs.getdbt.com/docs/collaborate/publish/model-versions

But if I also change the model's SQL to select 1::text as id, such that the contract works again — but has still undergone a breaking change! — it's no longer being caught during the state:modified comparison:

$ dbt ls -s state:modified --state state/
19:17:26  Running with dbt=1.5.0-b5
19:17:26  Found 1 model, 0 tests, 0 snapshots, 1 analysis, 522 macros, 0 operations, 1 seed file, 0 sources, 0 exposures, 0 metrics, 0 groups
test.my_model

If I had to take a wild guess, this is because the model is first selected on the basis of the change to its body, before we get to the same_contract comparison?


If I had my druthers / as a UX improvement, the ModelContractError message would also make clear what in the contract changed (similar to #7209). In practice, the change would also be reflected in the git diff, which someone could inspect in order to understand the cause for the failing CI check.

@github-actions github-actions bot changed the title [Bug] state:modified.contracts not catching breaking changes if model body also changes [CT-2378] [Bug] state:modified.contracts not catching breaking changes if model body also changes Apr 5, 2023
@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Apr 5, 2023

If I had to take a wild guess, this is because the model is first selected on the basis of the change to its body, before we get to the same_contract comparison?

Looks like yes:

def same_contents(self, old) -> bool:
if old is None:
return False
return (
self.same_body(old)
and self.same_config(old)
and self.same_persisted_description(old)
and self.same_fqn(old)
and self.same_database_representation(old)
and self.same_contract(old)
and True
)

If I switch the order so that self.same_contract(old) comes first, before self.same_body(old), then I see the exception I was expecting:

dbt.exceptions.ModelContractError: Contract Error in model my_model (models/my_model.sql)
  There is a breaking change in the model contract because column definitions have changed; you may need to create a new version. See: https://docs.getdbt.com/docs/collaborate/publish/model-versions

Reason: As soon as same_body() returns False, Python doesn't evaluate the rest of the methods in the and'ed conditional. (Right?)

@gshank gshank self-assigned this Apr 5, 2023
@jtcohen6 jtcohen6 changed the title [CT-2378] [Bug] state:modified.contracts not catching breaking changes if model body also changes [CT-2378] [Bug] state:modified not catching breaking changes if model body also changes Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants