diff --git a/.changes/unreleased/Features-20240422-173703.yaml b/.changes/unreleased/Features-20240422-173703.yaml new file mode 100644 index 00000000000..3c957af40c1 --- /dev/null +++ b/.changes/unreleased/Features-20240422-173703.yaml @@ -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" diff --git a/core/dbt/cli/flags.py b/core/dbt/cli/flags.py index d87c1aa96eb..9d49d339543 100644 --- a/core/dbt/cli/flags.py +++ b/core/dbt/cli/flags.py @@ -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 CommandParams = List[str] diff --git a/core/dbt/contracts/graph/manifest.py b/core/dbt/contracts/graph/manifest.py index 0c84c7a0033..b7b8142d72e 100644 --- a/core/dbt/contracts/graph/manifest.py +++ b/core/dbt/contracts/graph/manifest.py @@ -571,11 +571,25 @@ def __lt__(self, other: object) -> bool: 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 def last(self) -> Optional[Macro]: last_candidate = self.last_candidate() @@ -946,11 +960,20 @@ def find_materialization_macro_by_name( 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] + ) return materialization_candidate.macro if materialization_candidate else None diff --git a/core/dbt/contracts/project.py b/core/dbt/contracts/project.py index 46c9742952f..a74b883005d 100644 --- a/core/dbt/contracts/project.py +++ b/core/dbt/contracts/project.py @@ -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 @@ -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, } diff --git a/core/dbt/events/types.py b/core/dbt/events/types.py index 4990b7bfaa6..b618ec6ba22 100644 --- a/core/dbt/events/types.py +++ b/core/dbt/events/types.py @@ -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)) diff --git a/tests/functional/materializations/test_custom_materialization.py b/tests/functional/materializations/test_custom_materialization.py index dd165514ec1..2c3ec4e74c2 100644 --- a/tests/functional/materializations/test_custom_materialization.py +++ b/tests/functional/materializations/test_custom_materialization.py @@ -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): @@ -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()) }} diff --git a/tests/unit/test_manifest.py b/tests/unit/test_manifest.py index ea443d1147f..e335a09295a 100644 --- a/tests/unit/test_manifest.py +++ b/tests/unit/test_manifest.py @@ -1240,7 +1240,7 @@ def test_find_generate_macros_by_name(macros, expectations): FindMaterializationSpec = namedtuple("FindMaterializationSpec", "macros,adapter_type,expected") -def _materialization_parameter_sets(): +def _materialization_parameter_sets_legacy(): # inject the plugins used for materialization parameter tests FooPlugin = AdapterPlugin( adapter=mock.MagicMock(), @@ -1386,12 +1386,187 @@ def id_mat(arg): return "_".join(arg) +@pytest.mark.parametrize( + "macros,adapter_type,expected", + _materialization_parameter_sets_legacy(), + ids=id_mat, +) +def test_find_materialization_by_name_legacy(macros, adapter_type, expected): + set_from_args( + Namespace( + SEND_ANONYMOUS_USAGE_STATS=False, + REQUIRE_EXPLICIT_PACKAGE_OVERRIDES_FOR_BUILTIN_MATERIALIZATIONS=False, + ), + None, + ) + + manifest = make_manifest(macros=macros) + result = manifest.find_materialization_macro_by_name( + project_name="root", + materialization_name="my_materialization", + adapter_type=adapter_type, + ) + if expected is None: + assert result is expected + else: + expected_package, expected_adapter_type = expected + assert result.adapter_type == expected_adapter_type + assert result.package_name == expected_package + + +def _materialization_parameter_sets(): + # inject the plugins used for materialization parameter tests + FooPlugin = AdapterPlugin( + adapter=mock.MagicMock(), + credentials=mock.MagicMock(), + include_path="/path/to/root/plugin", + project_name="foo", + ) + FooPlugin.adapter.type.return_value = "foo" + inject_plugin(FooPlugin) + + BarPlugin = AdapterPlugin( + adapter=mock.MagicMock(), + credentials=mock.MagicMock(), + include_path="/path/to/root/plugin", + dependencies=["foo"], + project_name="bar", + ) + BarPlugin.adapter.type.return_value = "bar" + inject_plugin(BarPlugin) + + sets = [ + FindMaterializationSpec(macros=[], adapter_type="foo", expected=None), + ] + + # default only, each project + sets.extend( + FindMaterializationSpec( + macros=[MockMaterialization(project, adapter_type=None)], + adapter_type="foo", + expected=(project, "default"), + ) + for project in ["root", "dep", "dbt"] + ) + + # other type only, each project + sets.extend( + FindMaterializationSpec( + macros=[MockMaterialization(project, adapter_type="bar")], + adapter_type="foo", + expected=None, + ) + for project in ["root", "dep", "dbt"] + ) + + # matching type only, each project + sets.extend( + FindMaterializationSpec( + macros=[MockMaterialization(project, adapter_type="foo")], + adapter_type="foo", + expected=(project, "foo"), + ) + for project in ["root", "dep", "dbt"] + ) + + sets.extend( + [ + # matching type and default everywhere + FindMaterializationSpec( + macros=[ + MockMaterialization(project, adapter_type=atype) + for (project, atype) in product(["root", "dep", "dbt"], ["foo", None]) + ], + adapter_type="foo", + expected=("root", "foo"), + ), + # default in core, override is in dep, and root has unrelated override + # should find the dbt default because default materializations cannot be overwritten by packages. + FindMaterializationSpec( + macros=[ + MockMaterialization("root", adapter_type="bar"), + MockMaterialization("dep", adapter_type="foo"), + MockMaterialization("dbt", adapter_type=None), + ], + adapter_type="foo", + expected=("dbt", "default"), + ), + # default in core, unrelated override is in dep, and root has an override + # should find the root override. + FindMaterializationSpec( + macros=[ + MockMaterialization("root", adapter_type="foo"), + MockMaterialization("dep", adapter_type="bar"), + MockMaterialization("dbt", adapter_type=None), + ], + adapter_type="foo", + expected=("root", "foo"), + ), + # default in core, override is in dep, and root has an override too. + # should find the root override. + FindMaterializationSpec( + macros=[ + MockMaterialization("root", adapter_type="foo"), + MockMaterialization("dep", adapter_type="foo"), + MockMaterialization("dbt", adapter_type=None), + ], + adapter_type="foo", + expected=("root", "foo"), + ), + # core has default + adapter, dep has adapter, root has default + # should find the default adapter implementation, because it's the most specific + # and default materializations cannot be overwritten by packages + FindMaterializationSpec( + macros=[ + MockMaterialization("root", adapter_type=None), + MockMaterialization("dep", adapter_type="foo"), + MockMaterialization("dbt", adapter_type=None), + MockMaterialization("dbt", adapter_type="foo"), + ], + adapter_type="foo", + expected=("dbt", "foo"), + ), + ] + ) + + # inherit from parent adapter + sets.extend( + FindMaterializationSpec( + macros=[MockMaterialization(project, adapter_type="foo")], + adapter_type="bar", + expected=(project, "foo"), + ) + for project in ["root", "dep", "dbt"] + ) + sets.extend( + FindMaterializationSpec( + macros=[ + MockMaterialization(project, adapter_type="foo"), + MockMaterialization(project, adapter_type="bar"), + ], + adapter_type="bar", + expected=(project, "bar"), + ) + for project in ["root", "dep", "dbt"] + ) + + return sets + + @pytest.mark.parametrize( "macros,adapter_type,expected", _materialization_parameter_sets(), ids=id_mat, ) def test_find_materialization_by_name(macros, adapter_type, expected): + set_from_args( + Namespace( + SEND_ANONYMOUS_USAGE_STATS=False, + REQUIRE_EXPLICIT_PACKAGE_OVERRIDES_FOR_BUILTIN_MATERIALIZATIONS=True, + ), + None, + ) + manifest = make_manifest(macros=macros) result = manifest.find_materialization_macro_by_name( project_name="root",