Skip to content

Commit

Permalink
Convert error to conditional warning for unversioned contracted model…
Browse files Browse the repository at this point in the history 
…, 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
  • Loading branch information
@emmyoop
emmyoop authored Aug 25, 2023
1 parent d18a74d commit 1afbb87
Show file tree
Hide file tree
Showing 8 changed files with 1,149 additions and 948 deletions.
6 changes: 6 additions & 0 deletions .changes/unreleased/Fixes-20230818-103802.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Fixes
body: Turn breaking changes to contracted models into warnings for unversioned models
time: 2023-08-18T10:38:02.251286-05:00
custom:
Author: emmyoop
Issue: 8384 8282
117 changes: 92 additions & 25 deletions core/dbt/contracts/graph/nodes.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
SeedExceedsLimitSamePath,
SeedExceedsLimitAndPathChanged,
SeedExceedsLimitChecksumChanged,
UnversionedBreakingChange,
)
from dbt.events.contextvars import set_log_contextvars
from dbt.flags import get_flags
Expand Down Expand Up @@ -682,11 +683,11 @@ def same_contract(self, old, adapter_type=None) -> bool:
# These are the categories of breaking changes:
contract_enforced_disabled: bool = False
columns_removed: List[str] = []
column_type_changes: List[Tuple[str, str, str]] = []
enforced_column_constraint_removed: List[Tuple[str, str]] = [] # column, constraint_type
enforced_model_constraint_removed: List[
Tuple[str, List[str]]
] = [] # constraint_type, columns
column_type_changes: List[Dict[str, str]] = []
enforced_column_constraint_removed: List[
Dict[str, str]
] = [] # column_name, constraint_type
enforced_model_constraint_removed: List[Dict[str, Any]] = [] # constraint_type, columns
materialization_changed: List[str] = []

if old.contract.enforced is True and self.contract.enforced is False:
Expand All @@ -708,11 +709,11 @@ def same_contract(self, old, adapter_type=None) -> bool:
# Has this column's data type changed?
elif old_value.data_type != self.columns[old_key].data_type:
column_type_changes.append(
(
str(old_value.name),
str(old_value.data_type),
str(self.columns[old_key].data_type),
)
{
"column_name": str(old_value.name),
"previous_column_type": str(old_value.data_type),
"current_column_type": str(self.columns[old_key].data_type),
}
)

# track if there are any column level constraints for the materialization check late
Expand All @@ -733,7 +734,11 @@ def same_contract(self, old, adapter_type=None) -> bool:
and constraint_support[old_constraint.type] == ConstraintSupport.ENFORCED
):
enforced_column_constraint_removed.append(
(old_key, str(old_constraint.type))
{
"column_name": old_key,
"constraint_name": old_constraint.name,
"constraint_type": ConstraintType(old_constraint.type),
}
)

# Now compare the model level constraints
Expand All @@ -744,7 +749,11 @@ def same_contract(self, old, adapter_type=None) -> bool:
and constraint_support[old_constraint.type] == ConstraintSupport.ENFORCED
):
enforced_model_constraint_removed.append(
(str(old_constraint.type), old_constraint.columns)
{
"constraint_name": old_constraint.name,
"constraint_type": ConstraintType(old_constraint.type),
"columns": old_constraint.columns,
}
)

# Check for relevant materialization changes.
Expand All @@ -758,7 +767,8 @@ def same_contract(self, old, adapter_type=None) -> bool:
# If a column has been added, it will be missing in the old.columns, and present in self.columns
# That's a change (caught by the different checksums), but not a breaking change

# Did we find any changes that we consider breaking? If so, that's an error
# Did we find any changes that we consider breaking? If there's an enforced contract, that's
# a warning unless the model is versioned, then it's an error.
if (
contract_enforced_disabled
or columns_removed
Expand All @@ -767,21 +777,78 @@ def same_contract(self, old, adapter_type=None) -> bool:
or enforced_column_constraint_removed
or materialization_changed
):
raise (
ContractBreakingChangeError(
contract_enforced_disabled=contract_enforced_disabled,
columns_removed=columns_removed,
column_type_changes=column_type_changes,
enforced_column_constraint_removed=enforced_column_constraint_removed,
enforced_model_constraint_removed=enforced_model_constraint_removed,
materialization_changed=materialization_changed,

breaking_changes = []
if contract_enforced_disabled:
breaking_changes.append(
"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."
)
if columns_removed:
columns_removed_str = "\n - ".join(columns_removed)
breaking_changes.append(f"Columns were removed: \n - {columns_removed_str}")
if column_type_changes:
column_type_changes_str = "\n - ".join(
[
f"{c['column_name']} ({c['previous_column_type']} -> {c['current_column_type']})"
for c in column_type_changes
]
)
breaking_changes.append(
f"Columns with data_type changes: \n - {column_type_changes_str}"
)
if enforced_column_constraint_removed:
column_constraint_changes_str = "\n - ".join(
[
f"'{c['constraint_name'] if c['constraint_name'] is not None else c['constraint_type']}' constraint on column {c['column_name']}"
for c in enforced_column_constraint_removed
]
)
breaking_changes.append(
f"Enforced column level constraints were removed: \n - {column_constraint_changes_str}"
)
if enforced_model_constraint_removed:
model_constraint_changes_str = "\n - ".join(
[
f"'{c['constraint_name'] if c['constraint_name'] is not None else c['constraint_type']}' constraint on columns {c['columns']}"
for c in enforced_model_constraint_removed
]
)
breaking_changes.append(
f"Enforced model level constraints were removed: \n - {model_constraint_changes_str}"
)
if materialization_changed:
materialization_changes_str = (
f"{materialization_changed[0]} -> {materialization_changed[1]}"
)

breaking_changes.append(
f"Materialization changed with enforced constraints: \n - {materialization_changes_str}"
)

if self.version is None:
warn_or_error(
UnversionedBreakingChange(
contract_enforced_disabled=contract_enforced_disabled,
columns_removed=columns_removed,
column_type_changes=column_type_changes,
enforced_column_constraint_removed=enforced_column_constraint_removed,
enforced_model_constraint_removed=enforced_model_constraint_removed,
breaking_changes=breaking_changes,
model_name=self.name,
model_file_path=self.original_file_path,
),
node=self,
)
)
else:
raise (
ContractBreakingChangeError(
breaking_changes=breaking_changes,
node=self,
)
)

# Otherwise, though we didn't find any *breaking* changes, the contract has still changed -- same_contract: False
else:
return False
# Otherwise, the contract has changed -- same_contract: False
return False


# TODO: rm?
Expand Down
39 changes: 39 additions & 0 deletions core/dbt/events/types.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,27 @@ message ReferenceKeyMsg {
string identifier = 3;
}

//ColumnType
message ColumnType {
string column_name = 1;
string previous_column_type = 2;
string current_column_type = 3;
}

// ColumnConstraint
message ColumnConstraint {
string column_name = 1;
string constraint_name = 2;
string constraint_type = 3;
}

// ModelConstraint
message ModelConstraint {
string constraint_name = 1;
string constraint_type = 2;
repeated string columns = 3;
}

// GenericMessage, used for deserializing only
message GenericMessage {
EventInfo info = 1;
Expand Down Expand Up @@ -1248,6 +1269,24 @@ message SemanticValidationFailureMsg {
SemanticValidationFailure data = 2;
}

// I071
message UnversionedBreakingChange {
repeated string breaking_changes = 1;
string model_name = 2;
string model_file_path = 3;
bool contract_enforced_disabled = 4;
repeated string columns_removed = 5;
repeated ColumnType column_type_changes = 6;
repeated ColumnConstraint enforced_column_constraint_removed = 7;
repeated ModelConstraint enforced_model_constraint_removed = 8;
repeated string materialization_changed = 9;
}

message UnversionedBreakingChangeMsg {
EventInfo info = 1;
UnversionedBreakingChange data = 2;
}


// M - Deps generation

Expand Down
14 changes: 14 additions & 0 deletions core/dbt/events/types.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1233,6 +1233,20 @@ def message(self) -> str:
return self.msg


class UnversionedBreakingChange(WarnLevel):
def code(self):
return "I071"

def message(self) -> str:
reasons = "\n - ".join(self.breaking_changes)

return (
f"Breaking change to contracted, unversioned model {self.model_name} ({self.model_file_path})"
"\nWhile comparing to previous project state, dbt detected a breaking change to an unversioned model."
f"\n - {reasons}\n"
)


# =======================================================
# M - Deps generation
# =======================================================
Expand Down
1,708 changes: 855 additions & 853 deletions core/dbt/events/types_pb2.py
View file

Large diffs are not rendered by default.

57 changes: 6 additions & 51 deletions core/dbt/exceptions.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import re
import io
import agate
from typing import Any, Dict, List, Mapping, Optional, Tuple, Union
from typing import Any, Dict, List, Mapping, Optional, Union

from dbt.dataclass_schema import ValidationError
from dbt.events.helpers import env_secrets, scrub_secrets
Expand Down Expand Up @@ -213,67 +213,22 @@ class ContractBreakingChangeError(DbtRuntimeError):

def __init__(
self,
contract_enforced_disabled: bool,
columns_removed: List[str],
column_type_changes: List[Tuple[str, str, str]],
enforced_column_constraint_removed: List[Tuple[str, str]],
enforced_model_constraint_removed: List[Tuple[str, List[str]]],
materialization_changed: List[str],
breaking_changes: List[str],
node=None,
):
self.contract_enforced_disabled = contract_enforced_disabled
self.columns_removed = columns_removed
self.column_type_changes = column_type_changes
self.enforced_column_constraint_removed = enforced_column_constraint_removed
self.enforced_model_constraint_removed = enforced_model_constraint_removed
self.materialization_changed = materialization_changed
self.breaking_changes = breaking_changes
super().__init__(self.message(), node)

@property
def type(self):
return "Breaking Change to Contract"
return "Breaking change to contract"

def message(self):
breaking_changes = []
if self.contract_enforced_disabled:
breaking_changes.append("The contract's enforcement has been disabled.")
if self.columns_removed:
columns_removed_str = "\n - ".join(self.columns_removed)
breaking_changes.append(f"Columns were removed: \n - {columns_removed_str}")
if self.column_type_changes:
column_type_changes_str = "\n - ".join(
[f"{c[0]} ({c[1]} -> {c[2]})" for c in self.column_type_changes]
)
breaking_changes.append(
f"Columns with data_type changes: \n - {column_type_changes_str}"
)
if self.enforced_column_constraint_removed:
column_constraint_changes_str = "\n - ".join(
[f"{c[0]} ({c[1]})" for c in self.enforced_column_constraint_removed]
)
breaking_changes.append(
f"Enforced column level constraints were removed: \n - {column_constraint_changes_str}"
)
if self.enforced_model_constraint_removed:
model_constraint_changes_str = "\n - ".join(
[f"{c[0]} -> {c[1]}" for c in self.enforced_model_constraint_removed]
)
breaking_changes.append(
f"Enforced model level constraints were removed: \n - {model_constraint_changes_str}"
)
if self.materialization_changed:
materialization_changes_str = "\n - ".join(
f"{self.materialization_changed[0]} -> {self.materialization_changed[1]}"
)
breaking_changes.append(
f"Materialization changed with enforced constraints: \n - {materialization_changes_str}"
)

reasons = "\n\n".join(breaking_changes)
reasons = "\n - ".join(self.breaking_changes)

return (
"While comparing to previous project state, dbt detected a breaking change to an enforced contract."
f"\n\n{reasons}\n\n"
f"\n - {reasons}\n"
"Consider making an additive (non-breaking) change instead, if possible.\n"
"Otherwise, create a new model version: https://docs.getdbt.com/docs/collaborate/govern/model-versions"
)
Expand Down
Loading

0 comments on commit 1afbb87

Please sign in to comment.