Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 2 additions & 2 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,15 @@ def read(*names, **kwargs):
],
python_requires=">=3.8",
install_requires=[
"click~=8.1.7",
"click>=8.1.7,<9.0",
"pydantic >=2.0,<3.0",
"ruamel.yaml~=0.18.6",
"tabulate~=0.9.0",
"requests>=2.31",
"sqlglot[c]==30.11.0",
"mcp>=1.9.0,<2.0.0",
"pyperclip~=1.8.2",
"python-dotenv~=1.0.0",
"python-dotenv>=1.0.0,<2.0",
],
extras_require={
# eg:
Expand Down
2 changes: 1 addition & 1 deletion src/datapilot/core/platforms/dbt/schemas/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
from vendor.dbt_artifacts_parser.parsers.manifest.manifest_v9 import ManifestV9
from vendor.dbt_artifacts_parser.parsers.manifest.manifest_v10 import ManifestV10
from vendor.dbt_artifacts_parser.parsers.manifest.manifest_v11 import ManifestV11
from vendor.dbt_artifacts_parser.parsers.manifest.manifest_v11 import SupportedLanguage
from vendor.dbt_artifacts_parser.parsers.manifest.manifest_v12 import ManifestV12
from vendor.dbt_artifacts_parser.parsers.manifest.manifest_v12 import SupportedLanguage

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MAJOR: regression for dbt 1.7 (manifest v11) projects with custom materializations

Pointing AltimateSupportedLanguage at the v12 enum breaks the v11 wrapper. ManifestV11Wrapper._get_macro (wrappers/manifest/v11/wrapper.py:184) passes macro.supported_languages (v11 SupportedLanguage members) straight into AltimateManifestMacroNode.supported_languages, whose type is now List[v12.SupportedLanguage]. Pydantic v2 rejects a member of a different Enum class even when the value matches.

Repro: load tests/data/manifest_v11.json, make a macro that has supported_languages project-owned, then call DBTFactory.get_manifest_wrapper(parse_manifest(m)).get_macros().

  • main: OK, [SupportedLanguage.sql]
  • this branch: ValidationError: Input should be 'python', 'sql' or 'javascript' [type=enum, input_value=<SupportedLanguage.sql: 'sql'>, input_type=SupportedLanguage]

DBTInsightGenerator.__init__ (executor.py:63) always calls get_macros(). Project health / governance therefore crashes for any v11 project with a project-owned macro that declares supported_languages, which in practice means any custom materialization. 0.3.7 doesn't have this problem. The failure only needs "sql"; JavaScript plays no part. The v10 wrapper (v10/wrapper.py:184) uses the same pattern and already fails on main, and the same fix covers it.

Suggested fix: in the v10 and v11 wrappers, convert by value as the v12 wrapper already does:

supported_languages=[AltimateSupportedLanguage(lang.value) for lang in macro.supported_languages]
if macro.supported_languages
else None,

Add v10 and v11 counterparts of test_wraps_project_macro_supporting_javascript that use ["sql"]. Optionally, make AltimateSupportedLanguage a standalone enum here (python, sql, javascript) so a future vendor bump can't silently change it again.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks. I reproduced it on the branch with your steps; both v10 and v11 failed with the same ValidationError (input_value=<SupportedLanguage.sql: 'sql'>).

Fixed in e7d2163: the v10 and v11 wrappers now convert by value (AltimateSupportedLanguage(lang.value)), the same way the v12 wrapper does. Added tests/core/platform/dbt/test_manifest_wrapper_macros.py, parametrized over the v10 and v11 fixtures with a project-owned macro using ["sql"]. Both cases fail on 40f62c3 and pass now.

I kept AltimateSupportedLanguage as an alias of the v12 enum for now. Making it a standalone enum is a reasonable hardening step, and I'm happy to do it here if you'd prefer.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verified the fix in e7d2163. Both wrappers now convert by value, and the new parametrized test fails for v10 and v11 when the old wrappers are restored. Resolved. Keeping the alias is fine; the standalone enum can be a follow-up.



class DBTVersion(BaseModel):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from datapilot.core.platforms.dbt.schemas.manifest import AltimateSeedConfig
from datapilot.core.platforms.dbt.schemas.manifest import AltimateSeedNode
from datapilot.core.platforms.dbt.schemas.manifest import AltimateSourceConfig
from datapilot.core.platforms.dbt.schemas.manifest import AltimateSupportedLanguage
from datapilot.core.platforms.dbt.schemas.manifest import AltimateTestConfig
from datapilot.core.platforms.dbt.schemas.manifest import AltimateTestMetadata
from datapilot.core.platforms.dbt.wrappers.manifest.v10.schemas import TEST_TYPE_TO_NODE_MAP
Expand Down Expand Up @@ -181,7 +182,9 @@ def _get_macro(self, macro: MacroNode) -> AltimateManifestMacroNode:
patch_path=macro.patch_path,
arguments=[AltimateMacroArgument(**arg.model_dump()) for arg in macro.arguments] if macro.arguments else None,
created_at=macro.created_at,
supported_languages=macro.supported_languages,
supported_languages=[AltimateSupportedLanguage(lang.value) for lang in macro.supported_languages]
if macro.supported_languages
else None,
)

def _get_exposure(self, exposure: ExposureNode) -> AltimateManifestExposureNode:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from datapilot.core.platforms.dbt.schemas.manifest import AltimateSeedConfig
from datapilot.core.platforms.dbt.schemas.manifest import AltimateSeedNode
from datapilot.core.platforms.dbt.schemas.manifest import AltimateSourceConfig
from datapilot.core.platforms.dbt.schemas.manifest import AltimateSupportedLanguage
from datapilot.core.platforms.dbt.schemas.manifest import AltimateTestConfig
from datapilot.core.platforms.dbt.schemas.manifest import AltimateTestMetadata
from datapilot.core.platforms.dbt.wrappers.manifest.v11.schemas import TEST_TYPE_TO_NODE_MAP
Expand Down Expand Up @@ -181,7 +182,9 @@ def _get_macro(self, macro: MacroNode) -> AltimateManifestMacroNode:
patch_path=macro.patch_path,
arguments=[AltimateMacroArgument(**arg.model_dump()) for arg in macro.arguments] if macro.arguments else None,
created_at=macro.created_at,
supported_languages=macro.supported_languages,
supported_languages=[AltimateSupportedLanguage(lang.value) for lang in macro.supported_languages]
if macro.supported_languages
else None,
)

def _get_exposure(self, exposure: ExposureNode) -> AltimateManifestExposureNode:
Expand Down
22 changes: 13 additions & 9 deletions src/vendor/dbt_artifacts_parser/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,25 +85,29 @@ def parse_catalog_v1(catalog: dict) -> CatalogV1:
#
# manifest
#
def _strip_unused_fields(manifest: dict) -> dict:
"""Remove fields that have strict discriminated unions but are unused downstream.
def _null_unused_fields(manifest: dict) -> dict:
"""Null out fields that have strict discriminated unions but are unused downstream.

These fields (e.g. `disabled`) use complex Pydantic unions that break when
dbt Cloud changes its schema, but our wrappers never read them.
dbt Cloud changes its schema, but our wrappers never read them. They are
set to None rather than dropped because v11+ declare them required (but nullable).
"""
return {k: v for k, v in manifest.items() if k not in _UNUSED_STRICT_FIELDS}
return {k: (None if k in _UNUSED_STRICT_FIELDS else v) for k, v in manifest.items()}


def _try_parse_manifest(manifest: dict, model_class):
"""Attempt to parse manifest, falling back to stripping unused fields on failure."""
"""Attempt to parse manifest, falling back to nulling unused fields on failure.

If the fallback also fails, the original error is raised so it is not masked.
"""
try:
return model_class(**manifest)
except Exception:
stripped = _strip_unused_fields(manifest)
except Exception as original:
logger.debug("Manifest parse failed; retrying with %s nulled", sorted(_UNUSED_STRICT_FIELDS), exc_info=True)
try:
return model_class(**stripped)
return model_class(**_null_unused_fields(manifest))
except Exception:
raise
raise original

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit (optional): raising original inside the nested except attaches the retry error as __context__. The traceback prints the retry failure first, then "During handling of the above exception, another exception occurred", and only then the original. It's harmless, and the retry error can be useful context. If you'd prefer a cleaner traceback:

            raise original from None



def parse_manifest(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1640,6 +1640,8 @@ class Argument(BaseParserModel):
class SupportedLanguage(Enum):
python = "python"
sql = "sql"
# dbt-core 1.11+ ships `materialization_function_default` supporting UDFs in JavaScript
javascript = "javascript"


class Macros(BaseParserModel):
Expand Down
33 changes: 33 additions & 0 deletions tests/core/platform/dbt/test_manifest_wrapper_macros.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
"""Tests for converting project-owned macro `supported_languages` in the manifest wrappers.

`AltimateManifestMacroNode.supported_languages` is typed with the v12 `SupportedLanguage`
enum. The v10/v11 wrappers passed their own vendored enum members through unchanged, and
Pydantic v2 rejects members of a different Enum class even when the value matches. So
`get_macros()`, which `DBTInsightGenerator` always calls, failed for any v10/v11 project
with a custom materialization.
"""
import json
from pathlib import Path

import pytest

from datapilot.core.platforms.dbt.factory import DBTFactory
from vendor.dbt_artifacts_parser.parser import parse_manifest

DATA = Path(__file__).parent.parent.parent.parent / "data"


@pytest.mark.parametrize("fixture", ["manifest_v10.json", "manifest_v11.json"])
def test_wraps_project_macro_with_supported_languages(fixture):
"""GIVEN a v10/v11 manifest whose project owns a macro declaring `supported_languages`
WHEN the parsed manifest is wrapped for insights
THEN the languages convert by value without a ValidationError."""
with (DATA / fixture).open() as f:
manifest = json.load(f)
macro_id = next(k for k, m in manifest["macros"].items() if m.get("supported_languages"))
manifest["macros"][macro_id]["package_name"] = manifest["metadata"]["project_name"]
manifest["macros"][macro_id]["supported_languages"] = ["sql"]

macros = DBTFactory.get_manifest_wrapper(parse_manifest(manifest)).get_macros()

assert [lang.value for lang in macros[macro_id].supported_languages] == ["sql"]
95 changes: 95 additions & 0 deletions tests/test_vendor/test_manifest_v12.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
"""Tests for manifest v12 parsing against manifests produced by dbt-core 1.11+.

dbt-core 1.11 added the built-in ``materialization_function_default`` macro, whose
``supported_languages`` include ``"javascript"``. The v12 ``SupportedLanguage`` enum
only allowed ``python``/``sql``, so EVERY manifest from dbt-core 1.11+ failed to parse
and project governance broke for all users on current dbt.

The failure was masked by the ``disabled`` fallback: it dropped ``disabled`` from the
manifest, but v12 declares that field required (nullable), so the retry always raised
``disabled: Field required`` instead of the real error.
"""
import copy
import json
from pathlib import Path

import pytest

from datapilot.core.platforms.dbt.factory import DBTFactory
from vendor.dbt_artifacts_parser.parser import _try_parse_manifest
from vendor.dbt_artifacts_parser.parser import parse_manifest
from vendor.dbt_artifacts_parser.parsers.manifest.manifest_v12 import ManifestV12

MANIFEST_V12 = Path(__file__).parent.parent / "data" / "manifest_v12.json"
FUNCTION_MACRO_ID = "macro.dbt.materialization_function_default"


@pytest.fixture
def manifest() -> dict:
with MANIFEST_V12.open() as f:
return json.load(f)


def _with_function_macro(manifest: dict) -> dict:
"""Add the macro dbt-core 1.11+ ships, cloned from an existing macro in the fixture."""
manifest = copy.deepcopy(manifest)
macro = copy.deepcopy(next(iter(manifest["macros"].values())))
macro.update(
unique_id=FUNCTION_MACRO_ID,
name="materialization_function_default",
supported_languages=["sql", "python", "javascript"],
)
manifest["macros"][FUNCTION_MACRO_ID] = macro
return manifest


def test_parses_javascript_supported_language(manifest):
"""GIVEN a dbt-core 1.11+ manifest containing a macro that supports JavaScript
WHEN it is parsed
THEN parsing succeeds and the language is preserved."""
parsed = parse_manifest(_with_function_macro(manifest))

assert isinstance(parsed, ManifestV12)
languages = [lang.value for lang in parsed.macros[FUNCTION_MACRO_ID].supported_languages]
assert languages == ["sql", "python", "javascript"]


def test_wraps_project_macro_supporting_javascript(manifest):
"""GIVEN a project-owned macro (e.g. a custom UDF materialization) supporting JavaScript
WHEN the parsed manifest is wrapped for insights
THEN the macro's languages convert without a ValueError."""
manifest = _with_function_macro(manifest)
project = manifest["metadata"]["project_name"]
manifest["macros"][FUNCTION_MACRO_ID]["package_name"] = project

macros = DBTFactory.get_manifest_wrapper(parse_manifest(manifest)).get_macros()

languages = [lang.value for lang in macros[FUNCTION_MACRO_ID].supported_languages]
assert languages == ["sql", "python", "javascript"]


def test_unparseable_disabled_falls_back_to_none(manifest):
"""GIVEN a manifest whose ``disabled`` section does not match the strict schema
WHEN it is parsed
THEN the fallback nulls ``disabled`` (required in v12) and parsing succeeds."""
manifest["disabled"] = {"model.proj.broken": [{"resource_type": "not-a-real-type"}]}

parsed = parse_manifest(manifest)

assert isinstance(parsed, ManifestV12)
assert parsed.disabled is None


def test_failed_fallback_raises_the_original_error():

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit (optional): the stub Model is the right way to prove the "original error wins" contract. Replacing test_unrelated_errors_still_raise did drop the end-to-end check that a real non-disabled validation error (the cobol case) still comes out of parse_manifest, though. Keeping both would cost only a few lines.

"""GIVEN a manifest whose first parse and `disabled`-nulled retry fail with different errors
WHEN it is parsed
THEN the original error is raised, not the retry's."""

class Model:
def __init__(self, **manifest):
if manifest["disabled"] is None:
raise ValueError("retry error")
raise ValueError("original error")

with pytest.raises(ValueError, match="^original error$"):
_try_parse_manifest({"disabled": {}}, Model)
Loading