Convert error to conditional warning for unversioned contracted model, fix msg format #8451

emmyoop · 2023-08-18T13:11:57Z

resolves #8282
resolves #8384
docs dbt-labs/docs.getdbt.com/#

Problem

Error message is not clear
dbt throws an error when it's better to throw a warning when a contracted model is unversioned

Solution

Reformat messages
Conditionally throw a warning for unversioned models with the ability to mark that warning to be an error.

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

github-actions · 2023-08-18T13:12:15Z

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.

emmyoop · 2023-08-18T13:13:43Z

Warning output for unversioned models

(env-version-models) emily@Emily-Rockman basic-dbt % dbt run --models state:modified --state ./state
13:07:58  Running with dbt=1.7.0-a1
13:07:58  Registered adapter: postgres=1.7.0-a1
13:07:59  Found 17 models, 1 seed, 11 tests, 1 source, 3 exposures, 1 metric, 468 macros, 2 groups, 1 semantic model
13:07:59  Breaking Change to Unversioned Contract for contracted_model (models/contracted_models/contracted_model.sql)
While comparing to previous project state, dbt detected a breaking change to an unversioned model.
  - Contract enforcement was removed: Previously, this model's configuration included contract: {enforced: true}. It is no longer configured to enforce its contract, and this is a breaking change.
  - Columns were removed:
    - color
    - date_day
  - Enforced column level constraints were removed:
    - id (ConstraintType.not_null)
    - id (ConstraintType.primary_key)
  - Enforced model level constraints were removed:
    - ConstraintType.check -> ['id']
  - Materialization changed with enforced constraints:
    - table -> view

Error output for unversioned models

(env-version-models) emily@Emily-Rockman basic-dbt % dbt --warn-error run --models state:modified --state ./state
13:19:45  Running with dbt=1.7.0-a1
13:19:46  Registered adapter: postgres=1.7.0-a1
13:19:46  Found 17 models, 1 seed, 11 tests, 1 source, 3 exposures, 1 metric, 468 macros, 2 groups, 1 semantic model
13:19:46  Encountered an error:
Compilation Error
  Breaking Change to Unversioned Contract for contracted_model (models/contracted_models/contracted_model.sql)
  While comparing to previous project state, dbt detected a breaking change to an unversioned model.
    - Contract enforcement was removed: Previously, this model's configuration included contract: {enforced: true}. It is no longer configured to enforce its contract, and this is a breaking change.
    - Columns were removed:
      - color
      - date_day
    - Enforced column level constraints were removed:
      - id (ConstraintType.not_null)
      - id (ConstraintType.primary_key)
    - Enforced model level constraints were removed:
      - ConstraintType.check -> ['id']
    - Materialization changed with enforced constraints:
      - table -> view

Error Output for versioned models

(env-version-models) emily@Emily-Rockman basic-dbt % dbt run --models state:modified --state ./state
13:07:16  Running with dbt=1.7.0-a1
13:07:16  Registered adapter: postgres=1.7.0-a1
13:07:17  Found 17 models, 1 seed, 11 tests, 1 source, 3 exposures, 1 metric, 468 macros, 2 groups, 1 semantic model
13:07:17  Encountered an error:
Breaking Change to Contract Error in model contracted_model_versioned (models/contracted_versions_models copy/contracted_model_versioned.sql)
  While comparing to previous project state, dbt detected a breaking change to an enforced contract.
    - Columns were removed:
      - color
      - date_day
    - Enforced column level constraints were removed:
      - id (ConstraintType.not_null)
      - id (ConstraintType.primary_key)
    - Materialization changed with enforced constraints:
      - table -> view
  Consider making an additive (non-breaking) change instead, if possible.
  Otherwise, create a new model version: https://docs.getdbt.com/docs/collaborate/govern/model-versions

codecov · 2023-08-18T13:21:40Z

Codecov Report

Merging #8451 (c58e9a4) into main (37a2907) will increase coverage by 0.00%.
Report is 14 commits behind head on main.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #8451   +/-   ##
=======================================
  Coverage   86.30%   86.30%           
=======================================
  Files         174      174           
  Lines       25561    25573   +12     
=======================================
+ Hits        22061    22072   +11     
- Misses       3500     3501    +1

Flag	Coverage Δ
integration	`83.09% <100.00%> (+<0.01%)`	⬆️
unit	`65.15% <20.00%> (+0.01%)`	⬆️

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

Files Changed	Coverage Δ
core/dbt/events/types_pb2.py	`1.34% <ø> (-0.01%)`	⬇️
core/dbt/contracts/graph/nodes.py	`95.25% <100.00%> (+0.10%)`	⬆️
core/dbt/events/types.py	`98.63% <100.00%> (+<0.01%)`	⬆️
core/dbt/exceptions.py	`68.85% <100.00%> (-0.52%)`	⬇️

... and 6 files with indirect coverage changes

peterallenwebb

Looks good.

emmyoop · 2023-08-22T12:10:55Z

@graciegoheen or @jtcohen6 can I get feedback on the reworked error/warning messages? Examples posted above.

jtcohen6

@emmyoop This is looking really good!

You've made the most important changes. I have two suggestions of how we might make this even more powerful for consumers of structured logs, and a little more consistent in the human-friendly message. Neither suggestion is blocking for merge.

core/dbt/contracts/graph/nodes.py

graciegoheen · 2023-08-22T16:23:09Z

This is looking so much better! A few cosmetic / nit-picks on wording (none are blocking):

Is there a specific reason for capitalizing "Breaking Change", "Unversioned Contract", "Contract Error"?
While comparing to previous project state, dbt detected a breaking change to an unversioned model. Thoughts on changing the wording on this to something like While comparing to previous project state, dbt detected a breaking change to an unversioned, previously contracted model. - this error wouldn't pop up for a plain 'ole unversioned model, so I think it would be nice to callout that it used to be contracted in the initial error message.
Contract enforcement was removed: Previously, this model's configuration included contract: {enforced: true}. It is no longer configured to enforce its contract, and this is a breaking change. I think it would be more human readable to say something like Previously, this model had an enforced contract... rather than including the actual config block contract: {enforced: true}.
+1 to @jtcohen6's suggestions on the constraints suggestions above
Breaking Change to Contract Error in model contracted_model_versioned The Error seems redundant here since this is an error message - is there a reason for it? I would expect to see something like Breaking change to contracted model - contracted_model_versioned instead.
Breaking Change to Unversioned Contract for contracted_model Similarly to above, I might suggest making this clearer - something like, Breaking change to contracted, unversioned model - contracted_model.
When are we outputting a warning or unversioned models vs. an error? Would we ever recommend that they version their model if it's contracted but not versioned? I suppose in this case, the person is actually removing the contract entirely...

@jtcohen6 @emmyoop feel free to disagree with any of the above! Curious to hear your thoughts

emmyoop · 2023-08-22T18:36:02Z

@graciegoheen You actually get this warning on a model that has or had a contract but is unversioned. One of the breaking changes can be removing contract enforcement. In the above examples I did remove the contract but had I not, all of the messages below Contract enforcement was removed: Previously,... would still show.

emmyoop · 2023-08-24T16:20:57Z

New messages:

Warning output for unversioned models

(env-version-models) emily@Emily-Rockman basic-dbt % dbt run --models state:modified --state ./state
16:17:24  Running with dbt=1.7.0-a1
16:17:24  Registered adapter: postgres=1.7.0-a1
16:17:24  Found 18 models, 1 seed, 11 tests, 1 source, 3 exposures, 1 metric, 468 macros, 2 groups, 1 semantic model
16:17:24  Breaking change to contracted, unversioned model contracted_model (models/contracted_models/contracted_model.sql)
While comparing to previous project state, dbt detected a breaking change to an unversioned model.
  - Contract enforcement was removed: Previously, this model had an enforced contract. It is no longer configured to enforce its contract, and this is a breaking change.
  - Columns were removed:
    - color
  - Columns with data_type changes:
    - date_day (date -> text)
  - Enforced column level constraints were removed:
    - 'not_null' constraint on column id
    - 'primary_key' constraint on column id
  - Enforced model level constraints were removed:
    - 'check' constraint on columns ['id']
  - Materialization changed with enforced constraints:
    - table -> view

Error output for unversioned models

(env-version-models) emily@Emily-Rockman basic-dbt % dbt --warn-error run --models state:modified --state ./state
16:17:58  Running with dbt=1.7.0-a1
16:17:58  Registered adapter: postgres=1.7.0-a1
16:17:58  Found 18 models, 1 seed, 11 tests, 1 source, 3 exposures, 1 metric, 468 macros, 2 groups, 1 semantic model
16:17:58  Encountered an error:
Compilation Error
  Breaking change to contracted, unversioned model contracted_model (models/contracted_models/contracted_model.sql)
  While comparing to previous project state, dbt detected a breaking change to an unversioned model.
    - Contract enforcement was removed: Previously, this model had an enforced contract. It is no longer configured to enforce its contract, and this is a breaking change.
    - Columns were removed:
      - color
    - Columns with data_type changes:
      - date_day (date -> text)
    - Enforced column level constraints were removed:
      - 'not_null' constraint on column id
      - 'primary_key' constraint on column id
    - Enforced model level constraints were removed:
      - 'check' constraint on columns ['id']
    - Materialization changed with enforced constraints:
      - table -> view

Error Output for versioned models

(env-version-models) emily@Emily-Rockman basic-dbt % dbt run --models state:modified --state ./state
16:20:07  Running with dbt=1.7.0-a1
16:20:07  Registered adapter: postgres=1.7.0-a1
16:20:08  Found 18 models, 1 seed, 11 tests, 1 source, 3 exposures, 1 metric, 468 macros, 2 groups, 1 semantic model
16:20:08  Breaking change to contracted, unversioned model contracted_model (models/contracted_models/contracted_model.sql)
While comparing to previous project state, dbt detected a breaking change to an unversioned model.
  - Contract enforcement was removed: Previously, this model had an enforced contract. It is no longer configured to enforce its contract, and this is a breaking change.
  - Columns were removed:
    - color
  - Columns with data_type changes:
    - date_day (date -> text)
  - Enforced column level constraints were removed:
    - 'not_null' constraint on column id
    - 'primary_key' constraint on column id
  - Enforced model level constraints were removed:
    - 'check' constraint on columns ['id']
  - Materialization changed with enforced constraints:
    - table -> view

peterallenwebb · 2023-08-24T21:26:55Z

core/dbt/contracts/graph/nodes.py

-        enforced_model_constraint_removed: List[
-            Tuple[str, List[str]]
-        ] = []  # constraint_type, columns
+        column_type_changes: List[Dict[str, str]] = []


The dict is better than a tuple, but now I'm thinking it would make sense to make this a dataclass called ColumnTypeChange, and analogously for the below, ColumnConstraintRemoval and ModelConstraintRemoval.

I don't think it's a huge deal, though so if you want to go ahead anyway, I'm cool with it. Otherwise this changeset looks good.

…, fix msg format (#8451) * first pass, tests need updates * update proto defn * fixing tests * more test fixes * finish fixing test file * reformat the message * formatting messages * changelog * add event to unit test * feedback on message structure * WIP * fix up event to take in all fields * fix test

* Add compiled node properties to run_results.json * Include compiled-node attributes in run_results.json * Fix typo * Bump schema version of run_results * Fix test assertions * Update expected run_results to reflect new attributes * Code review changes * Fix mypy warnings for ManifestLoader.load() (#8443) * revert python version for docker images (#8445) * revert python version for docker images * add comment to not update python version, update changelog * Bumping version to 1.7.0b1 and generate changelog * [CT-3013] Fix parsing of `window_groupings` (#8454) * Update semantic model parsing tests to check measure non_additive_dimension spec * Make `window_groupings` default to empty list if not specified on `non_additive_dimension` * Add changie doc for `window_groupings` parsing fix * update `Number` class to handle integer values (#8306) * add show test for json data * oh changie my changie * revert unecessary cahnge to fixture * keep decimal class for precision methods, but return __int__ value * jerco updates * update integer type * update other tests * Update .changes/unreleased/Fixes-20230803-093502.yaml --------- Co-authored-by: Emily Rockman <[email protected]> * Improve docker image README (#8212) * Improve docker image README - Fix unnecessary/missing newline escapes - Remove double whitespace between parameters - 2-space indent for extra lines in image build commands * Add changelog entry for #8212 * ADAP-814: Refactor prep for MV updates (#8459) * apply reformatting changes only for #8449 * add logging back to get_create_materialized_view_as_sql * changie * swap trigger (#8463) * update the implementation template (#8466) * update the implementation template * add colon * Split tests into classes (#8474) * add flaky decorator * split up tests into classes * revert update agate for int (#8478) * updated typing and methods to meet mypy standards (#8485) * Convert error to conditional warning for unversioned contracted model, fix msg format (#8451) * first pass, tests need updates * update proto defn * fixing tests * more test fixes * finish fixing test file * reformat the message * formatting messages * changelog * add event to unit test * feedback on message structure * WIP * fix up event to take in all fields * fix test * Fix ambiguous reference error for duplicate model names across packages with tests (#8488) * Safely remove external nodes from manifest (#8495) * [CT-2840] Improved semantic layer protocol satisfaction tests (#8456) * Test `SemanticModel` satisfies protocol when none of it's `Optionals` are specified * Add tests ensuring SourceFileMetadata and FileSlice satisfiy DSI protocols * Add test asserting Defaults obj satisfies protocol * Add test asserting SemanticModel with optionals specified satisfies protocol * Split dimension protocol satisfaction tests into with and without optionals * Simplify DSI Protocol import strategy in protocol satisfaction tests * Add test asserting DimensionValidtyParams satisfies protocol * Add test asserting DimensionTypeParams satisfies protocol * Split entity protocol satisfaction tests into with and without optionals * Split measure protocol satisfication tests and add measure aggregation params satisficaition test * Split metric protocol satisfaction test into optional specified an unspecified Additionally, create where_filter pytest fixture * Improve protocol satisfaction tests for MetricTypeParams and sub protocols Specifically we added/improved protocol satisfaction tests for - MetricTypeParams - MetricInput - MetricInputMeasure - MetricTimeWindow * Convert to using mashumaro jsonschema with acceptable performance (#8437) * Regenerate run_results schema after merging in changes from main. --------- Co-authored-by: Gerda Shank <[email protected]> Co-authored-by: Matthew McKnight <[email protected]> Co-authored-by: Github Build Bot <[email protected]> Co-authored-by: Quigley Malcolm <[email protected]> Co-authored-by: dave-connors-3 <[email protected]> Co-authored-by: Emily Rockman <[email protected]> Co-authored-by: Jaime Martínez Rincón <[email protected]> Co-authored-by: Mike Alfare <[email protected]> Co-authored-by: Michelle Ark <[email protected]>

FishtownBuildBot · 2023-08-31T14:35:43Z

Opened a new issue in dbt-labs/docs.getdbt.com: dbt-labs/docs.getdbt.com#4000

emmyoop added 7 commits August 16, 2023 20:16

first pass, tests need updates

07050fb

update proto defn

8434686

fixing tests

550d338

more test fixes

dd8cb7a

finish fixing test file

3153f2b

reformat the message

2450a62

formatting messages

15ccd50

cla-bot bot added the cla:yes label Aug 18, 2023

emmyoop added 2 commits August 18, 2023 10:40

changelog

489851d

add event to unit test

1048855

emmyoop marked this pull request as ready for review August 18, 2023 16:28

emmyoop requested a review from a team as a code owner August 18, 2023 16:28

emmyoop requested a review from ChenyuLInx August 18, 2023 16:28

emmyoop changed the title ~~Er/ct 2969 breaking contract warning~~ Convert error wot conditional warning for unversioned contracted model, fix msg format Aug 18, 2023

emmyoop changed the title ~~Convert error wot conditional warning for unversioned contracted model, fix msg format~~ Convert error to conditional warning for unversioned contracted model, fix msg format Aug 18, 2023

emmyoop requested review from dbeatty10 and graciegoheen August 18, 2023 16:31

peterallenwebb approved these changes Aug 18, 2023

View reviewed changes

jtcohen6 reviewed Aug 22, 2023

View reviewed changes

core/dbt/contracts/graph/nodes.py Show resolved Hide resolved

core/dbt/contracts/graph/nodes.py Outdated Show resolved Hide resolved

emmyoop added 3 commits August 22, 2023 14:21

feedback on message structure

e9380a5

WIP

5a499a6

fix up event to take in all fields

7adc8c1

fix test

c58e9a4

emmyoop requested review from peterallenwebb and McKnight-42 August 24, 2023 17:24

peterallenwebb approved these changes Aug 24, 2023

View reviewed changes

emmyoop merged commit 1afbb87 into main Aug 25, 2023
50 checks passed

emmyoop deleted the er/ct-2969-breaking-contract-warning branch August 25, 2023 00:29

MichelleArk added the user docs [docs.getdbt.com] Needs better documentation label Aug 31, 2023

tlento mentioned this pull request Nov 6, 2023

Update typing-extensions version to >=4.4 #9012

Merged

5 tasks

tlento mentioned this pull request Feb 26, 2024

Update dbt-semantic-interfaces dependency to compatible range #9671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert error to conditional warning for unversioned contracted model, fix msg format #8451

Convert error to conditional warning for unversioned contracted model, fix msg format #8451

emmyoop commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

emmyoop commented Aug 18, 2023 •

edited

Loading

codecov bot commented Aug 18, 2023 •

edited

Loading

peterallenwebb left a comment

emmyoop commented Aug 22, 2023

jtcohen6 left a comment

graciegoheen commented Aug 22, 2023

emmyoop commented Aug 22, 2023

emmyoop commented Aug 24, 2023

peterallenwebb Aug 24, 2023

peterallenwebb Aug 24, 2023

FishtownBuildBot commented Aug 31, 2023

Convert error to conditional warning for unversioned contracted model, fix msg format #8451

Convert error to conditional warning for unversioned contracted model, fix msg format #8451

Conversation

emmyoop commented Aug 18, 2023

Problem

Solution

Checklist

github-actions bot commented Aug 18, 2023

emmyoop commented Aug 18, 2023 • edited Loading

Warning output for unversioned models

Error output for unversioned models

Error Output for versioned models

codecov bot commented Aug 18, 2023 • edited Loading

Codecov Report

peterallenwebb left a comment

Choose a reason for hiding this comment

emmyoop commented Aug 22, 2023

jtcohen6 left a comment

Choose a reason for hiding this comment

graciegoheen commented Aug 22, 2023

emmyoop commented Aug 22, 2023

emmyoop commented Aug 24, 2023

Warning output for unversioned models

Error output for unversioned models

Error Output for versioned models

peterallenwebb Aug 24, 2023

Choose a reason for hiding this comment

peterallenwebb Aug 24, 2023

Choose a reason for hiding this comment

FishtownBuildBot commented Aug 31, 2023

emmyoop commented Aug 18, 2023 •

edited

Loading

codecov bot commented Aug 18, 2023 •

edited

Loading