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

deprecate materialization overrides from imported packages #9971

Merged
merged 6 commits into from
Apr 22, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Under the Hood-20240418-172528.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Under the Hood
body: Raise deprecation warning if installed package overrides built-in materialization
time: 2024-04-18T17:25:28.37886-04:00
custom:
Author: michelleark
Issue: "9971"
28 changes: 25 additions & 3 deletions core/dbt/contracts/graph/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
)
from typing_extensions import Protocol

from dbt import deprecations
from dbt import tracking
from dbt.contracts.graph.nodes import (
BaseNode,
Expand Down Expand Up @@ -570,11 +571,15 @@ def __lt__(self, other: object) -> bool:


class CandidateList(List[M]):
def last(self) -> Optional[Macro]:
def last_candidate(self) -> Optional[MacroCandidate]:
if not self:
return None
self.sort()
return self[-1].macro
return self[-1]

def last(self) -> Optional[Macro]:
last_candidate = self.last_candidate()
return last_candidate.macro if last_candidate is not None else None


def _get_locality(macro: Macro, root_project_name: str, internal_packages: Set[str]) -> Locality:
Expand Down Expand Up @@ -930,7 +935,24 @@ def find_materialization_macro_by_name(
for specificity, atype in enumerate(self._get_parent_adapter_types(adapter_type))
)
)
return candidates.last()
core_candidates = [
candidate for candidate in candidates if candidate.locality == Locality.Core
Copy link
Contributor

Choose a reason for hiding this comment

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

]

materialization_candidate = candidates.last_candidate()
# If an imported materialization macro was found that also had a core candidate, fire a deprecation
if (
materialization_candidate is not None
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,
)

return materialization_candidate.macro if materialization_candidate else None

def get_resource_fqns(self) -> Mapping[str, PathSet]:
resource_fqns: Dict[str, Set[Tuple[str, ...]]] = {}
Expand Down
6 changes: 6 additions & 0 deletions core/dbt/deprecations.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ def show(self, *args, **kwargs) -> None:
active_deprecations.add(self.name)


class PackageMaterializationOverrideDeprecation(DBTDeprecation):
_name = "package-materialization-override"
MichelleArk marked this conversation as resolved.
Show resolved Hide resolved
_event = "PackageMaterializationOverrideDeprecation"


def renamed_env_var(old_name: str, new_name: str):
class EnvironmentVariableRenamed(DBTDeprecation):
_name = f"environment-variable-renamed:{old_name}"
Expand Down Expand Up @@ -157,6 +162,7 @@ def warn(name, *args, **kwargs):
CollectFreshnessReturnSignature(),
TestsConfigDeprecation(),
ProjectFlagsMovedDeprecation(),
PackageMaterializationOverrideDeprecation(),
]

deprecations: Dict[str, DBTDeprecation] = {d.name: d for d in deprecations_list}
Expand Down
11 changes: 11 additions & 0 deletions core/dbt/events/core_types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,17 @@ message TotalModelNamesWithSpacesDeprecation {
string level = 3;
}

// D016
message PackageMaterializationOverrideDeprecation {
string package_name = 1;
string materialization_name = 2;
}

message PackageMaterializationOverrideDeprecationMsg {
CoreEventInfo info = 1;
PackageMaterializationOverrideDeprecation data = 2;
}

message TotalModelNamesWithSpacesDeprecationMsg {
CoreEventInfo info = 1;
TotalModelNamesWithSpacesDeprecation data = 2;
Expand Down
1,146 changes: 575 additions & 571 deletions core/dbt/events/core_types_pb2.py

Large diffs are not rendered by default.

10 changes: 10 additions & 0 deletions core/dbt/events/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,16 @@ def message(self) -> str:
return line_wrap_message(description)


class PackageMaterializationOverrideDeprecation(WarnLevel):
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."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: link to docs on workaround before merging

Copy link
Contributor

Choose a reason for hiding this comment

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

Message looks good!

I figure we can link to the docs for the behavior change flag, which will go here: https://docs.getdbt.com/reference/global-configs/legacy-behaviors#source_freshness_run_project_hooks


return line_wrap_message(warning_tag(description))


# =======================================================
# I - Project parsing
# =======================================================
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/deprecations/test_deprecations.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ def test_exposure_name_fail(self, project):
assert expected_msg in exc_str


class TestPrjectFlagsMovedDeprecation:
class TestProjectFlagsMovedDeprecation:
@pytest.fixture(scope="class")
def profiles_config_update(self):
return {
Expand Down
24 changes: 24 additions & 0 deletions tests/functional/materializations/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,21 @@
{%- endmaterialization -%}
"""

custom_materialization_dep__dbt_project_yml = """
name: custom_materialization_default
macro-paths: ['macros']
"""

custom_materialization_sql = """
{% materialization custom_materialization, default %}
{%- set target_relation = this.incorporate(type='table') %}
{% call statement('main') -%}
select 1 as column1
{%- endcall %}
{{ return({'relations': [target_relation]}) }}
{% endmaterialization %}
"""


@pytest.fixture(scope="class")
def override_view_adapter_pass_dep(project_root):
Expand Down Expand Up @@ -368,3 +383,12 @@ def override_view_return_no_relation(project_root):
},
}
write_project_files(project_root, "override-view-return-no-relation", files)


@pytest.fixture(scope="class")
def custom_materialization_dep(project_root):
files = {
"dbt_project.yml": custom_materialization_dep__dbt_project_yml,
"macros": {"custom_materialization.sql": custom_materialization_sql},
}
write_project_files(project_root, "custom-materialization-dep", files)
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import pytest

from dbt.tests.util import run_dbt

from dbt import deprecations

models__model_sql = """
{{ config(materialized='view') }}
Expand All @@ -10,34 +10,100 @@
"""


models_custom_materialization__model_sql = """
{{ config(materialized='custom_materialization') }}
select 1 as id

"""


@pytest.fixture(scope="class")
def models():
return {"model.sql": models__model_sql}


@pytest.fixture(scope="class")
def set_up_deprecations():
deprecations.reset_deprecations()
assert deprecations.active_deprecations == set()


class TestOverrideAdapterDependency:
# 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"}]}

def test_adapter_dependency(self, project, override_view_adapter_dep):
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):
return {"packages": [{"local": "override-view-default-dep"}]}

def test_default_dependency(self, project, override_view_default_dep):
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()) }}
{% endmaterialization %}
"""


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

@pytest.fixture(scope="class")
def macros(self):
return {"my_view.sql": root_view_override_macro}

def test_default_dependency_with_root_override(
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)

# using an package-overriden built-in materialization in a root matereialization is _not_ deprecated
assert deprecations.active_deprecations == set()


class TestCustomMaterializationDependency:
@pytest.fixture(scope="class")
def models(self):
return {"model.sql": models_custom_materialization__model_sql}

@pytest.fixture(scope="class")
def packages(self):
return {"packages": [{"local": "custom-materialization-dep"}]}

def test_custom_materialization_deopendency(
self, project, custom_materialization_dep, set_up_deprecations
):
run_dbt(["deps"])
# custom materilization is valid
run_dbt(["run"])

# using a custom materialization is from an installed package is _not_ deprecated
assert deprecations.active_deprecations == set()


class TestOverrideAdapterDependencyPassing:
@pytest.fixture(scope="class")
Expand Down
3 changes: 3 additions & 0 deletions tests/unit/test_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,9 @@ def test_event_codes(self):
core_types.TotalModelNamesWithSpacesDeprecation(
count_invalid_names=1, show_debug_hint=True, level=""
),
core_types.PackageMaterializationOverrideDeprecation(
package_name="my_package", materialization_name="view"
),
# E - DB Adapter ======================
adapter_types.AdapterEventDebug(),
adapter_types.AdapterEventInfo(),
Expand Down