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

Add flags.deprecate_package_materialization_builtin_override #9956

Merged
merged 10 commits into from
Apr 23, 2024
6 changes: 6 additions & 0 deletions .changes/unreleased/Features-20240422-173703.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Features
body: Add require_explicit_package_overrides_for_builtin_materializations to dbt_project.yml flags, which can be used to opt-out of overriding built-in materializations from packages
time: 2024-04-22T17:37:03.892268-04:00
custom:
Author: michelleark
Issue: "10007"
2 changes: 1 addition & 1 deletion core/dbt/cli/flags.py
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ def set_common_global_flags(self):
# This is here to prevent mypy from complaining about all of the
# attributes which we added dynamically.
def __getattr__(self, name: str) -> Any:
return super().__get_attribute__(name) # type: ignore
return super().__getattribute__(name) # type: ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to have been broken all along, but was never reached in testing



CommandParams = List[str]
Expand Down
37 changes: 30 additions & 7 deletions core/dbt/contracts/graph/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -571,11 +571,25 @@


class CandidateList(List[M]):
def last_candidate(self) -> Optional[MacroCandidate]:
def last_candidate(
self, valid_localities: Optional[List[Locality]] = None
) -> Optional[MacroCandidate]:
"""
Obtain the last (highest precedence) MacroCandidate from the CandidateList of any locality in valid_localities.
If valid_localities is not specified, return the last MacroCandidate of any locality.
"""
if not self:
return None
self.sort()
return self[-1]

if valid_localities is None:
return self[-1]

for candidate in reversed(self):
if candidate.locality in valid_localities:
return candidate

return None

Check warning on line 592 in core/dbt/contracts/graph/manifest.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/contracts/graph/manifest.py#L592

Added line #L592 was not covered by tests

def last(self) -> Optional[Macro]:
last_candidate = self.last_candidate()
Expand Down Expand Up @@ -946,11 +960,20 @@
and materialization_candidate.locality == Locality.Imported
and core_candidates
):
deprecations.warn(
"package-materialization-override",
package_name=materialization_candidate.macro.package_name,
materialization_name=materialization_name,
)
# preserve legacy behaviour - allow materialization override
if (
get_flags().require_explicit_package_overrides_for_builtin_materializations
is False
):
deprecations.warn(
"package-materialization-override",
package_name=materialization_candidate.macro.package_name,
materialization_name=materialization_name,
)
else:
materialization_candidate = candidates.last_candidate(
valid_localities=[Locality.Core, Locality.Root]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was not able to do this lower-level using the _find_macros_by_name filter parameter because that filter only has a view of the macro candidate being iterated over. For this check, we need to exclude imported materialization macros only if the materialization is builtin in order to continue supporting custom materializations from packages (tested)

)

return materialization_candidate.macro if materialization_candidate else None

Expand Down
2 changes: 2 additions & 0 deletions core/dbt/contracts/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ class ProjectFlags(ExtensibleDbtClassMixin):
partial_parse: Optional[bool] = None
populate_cache: Optional[bool] = None
printer_width: Optional[int] = None
require_explicit_package_overrides_for_builtin_materializations: bool = False
send_anonymous_usage_stats: bool = DEFAULT_SEND_ANONYMOUS_USAGE_STATS
source_freshness_run_project_hooks: bool = False
static_parser: Optional[bool] = None
Expand All @@ -324,6 +325,7 @@ def project_only_flags(self) -> Dict[str, Any]:
return {
"source_freshness_run_project_hooks": self.source_freshness_run_project_hooks,
"allow_spaces_in_model_names": self.allow_spaces_in_model_names,
"require_explicit_package_overrides_for_builtin_materializations": self.require_explicit_package_overrides_for_builtin_materializations,
}


Expand Down
2 changes: 1 addition & 1 deletion core/dbt/events/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ def code(self) -> str:
return "D016"

def message(self) -> str:
description = f"Installed package '{self.package_name}' is overriding the built-in materialization '{self.materialization_name}'. Overrides of built-in materializations from installed packages will be deprecated in future versions of dbt. Please refer to https://docs.getdbt.com/reference/global-configs/legacy-behaviors#deprecate_package_materialization_builtin_override for detailed documentation and suggested workarounds."
description = f"Installed package '{self.package_name}' is overriding the built-in materialization '{self.materialization_name}'. Overrides of built-in materializations from installed packages will be deprecated in future versions of dbt. Please refer to https://docs.getdbt.com/reference/global-configs/legacy-behaviors#require_explicit_package_overrides_for_builtin_materializations for detailed documentation and suggested workarounds."

return line_wrap_message(warning_tag(description))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,56 @@ def test_adapter_dependency(self, project, override_view_adapter_dep, set_up_dep
assert deprecations.active_deprecations == {"package-materialization-override"}


class TestOverrideAdapterDependencyDeprecated:
# make sure that if there's a dependency with an adapter-specific
# materialization, we honor that materialization
@pytest.fixture(scope="class")
def packages(self):
return {"packages": [{"local": "override-view-adapter-dep"}]}

@pytest.fixture(scope="class")
def project_config_update(self):
return {
"flags": {
"require_explicit_package_overrides_for_builtin_materializations": True,
},
}

def test_adapter_dependency_deprecate_overrides(
self, project, override_view_adapter_dep, set_up_deprecations
):
run_dbt(["deps"])
# this should pass because the override is buggy and unused
run_dbt(["run"])

# no deprecation warning -- flag used correctly
assert deprecations.active_deprecations == set()


class TestOverrideAdapterDependencyLegacy:
# make sure that if there's a dependency with an adapter-specific
# materialization, we honor that materialization
@pytest.fixture(scope="class")
def packages(self):
return {"packages": [{"local": "override-view-adapter-dep"}]}

@pytest.fixture(scope="class")
def project_config_update(self):
return {
"flags": {
"require_explicit_package_overrides_for_builtin_materializations": False,
},
}

def test_adapter_dependency(self, project, override_view_adapter_dep, set_up_deprecations):
run_dbt(["deps"])
# this should error because the override is buggy
run_dbt(["run"], expect_pass=False)

# overriding a built-in materialization scoped to adapter from package is deprecated
assert deprecations.active_deprecations == {"package-materialization-override"}


class TestOverrideDefaultDependency:
@pytest.fixture(scope="class")
def packages(self):
Expand All @@ -58,6 +108,52 @@ def test_default_dependency(self, project, override_view_default_dep, set_up_dep
assert deprecations.active_deprecations == {"package-materialization-override"}


class TestOverrideDefaultDependencyDeprecated:
@pytest.fixture(scope="class")
def packages(self):
return {"packages": [{"local": "override-view-default-dep"}]}

@pytest.fixture(scope="class")
def project_config_update(self):
return {
"flags": {
"require_explicit_package_overrides_for_builtin_materializations": True,
},
}

def test_default_dependency_deprecated(
self, project, override_view_default_dep, set_up_deprecations
):
run_dbt(["deps"])
# this should pass because the override is buggy and unused
run_dbt(["run"])

# overriding a built-in materialization from package is deprecated
assert deprecations.active_deprecations == set()


class TestOverrideDefaultDependencyLegacy:
@pytest.fixture(scope="class")
def packages(self):
return {"packages": [{"local": "override-view-default-dep"}]}

@pytest.fixture(scope="class")
def project_config_update(self):
return {
"flags": {
"require_explicit_package_overrides_for_builtin_materializations": False,
},
}

def test_default_dependency(self, project, override_view_default_dep, set_up_deprecations):
run_dbt(["deps"])
# this should error because the override is buggy
run_dbt(["run"], expect_pass=False)

# overriding a built-in materialization from package is deprecated
assert deprecations.active_deprecations == {"package-materialization-override"}


root_view_override_macro = """
{% materialization view, default %}
{{ return(view_default_override.materialization_view_default()) }}
Expand Down
Loading