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/contracts/graph/manifest.py b/core/dbt/contracts/graph/manifest.py index cdd7f4844d8..cb7d4e3025f 100644 --- a/core/dbt/contracts/graph/manifest.py +++ b/core/dbt/contracts/graph/manifest.py @@ -617,11 +617,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() @@ -930,11 +944,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 0b174517c91..79e7f36334f 100644 --- a/core/dbt/contracts/project.py +++ b/core/dbt/contracts/project.py @@ -295,6 +295,7 @@ class ProjectFlags(ExtensibleDbtClassMixin, Replaceable): 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 static_parser: Optional[bool] = None use_colors: Optional[bool] = None @@ -307,7 +308,9 @@ class ProjectFlags(ExtensibleDbtClassMixin, Replaceable): @property def project_only_flags(self) -> Dict[str, Any]: - return {} + return { + "require_explicit_package_overrides_for_builtin_materializations": self.require_explicit_package_overrides_for_builtin_materializations, + } @dataclass 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 685c1feb5c5..8fcc5109441 100644 --- a/tests/unit/test_manifest.py +++ b/tests/unit/test_manifest.py @@ -1239,7 +1239,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 with mock.patch("dbt.adapters.base.plugin.project_name_from_path") as get_name: get_name.return_value = "foo" @@ -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 + with mock.patch("dbt.adapters.base.plugin.project_name_from_path") as get_name: + get_name.return_value = "foo" + FooPlugin = AdapterPlugin( + adapter=mock.MagicMock(), + credentials=mock.MagicMock(), + include_path="/path/to/root/plugin", + ) + 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"], + ) + 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",