Repository navigation
fix: [AI-9453] parse dbt-core 1.11+ manifests and stop downgrading dbt deps #120
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
Changes from all commits
8b20353
40f62c3
e7d2163
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (optional): raising raise original from None |
||
|
|
||
|
|
||
| def parse_manifest( | ||
|
|
||
| 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"] |
| 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(): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (optional): the stub |
||
| """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) | ||
There was a problem hiding this comment.
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
AltimateSupportedLanguageat the v12 enum breaks the v11 wrapper.ManifestV11Wrapper._get_macro(wrappers/manifest/v11/wrapper.py:184) passesmacro.supported_languages(v11SupportedLanguagemembers) straight intoAltimateManifestMacroNode.supported_languages, whose type is nowList[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 hassupported_languagesproject-owned, then callDBTFactory.get_manifest_wrapper(parse_manifest(m)).get_macros().main: OK,[SupportedLanguage.sql]ValidationError: Input should be 'python', 'sql' or 'javascript' [type=enum, input_value=<SupportedLanguage.sql: 'sql'>, input_type=SupportedLanguage]DBTInsightGenerator.__init__(executor.py:63) always callsget_macros(). Project health / governance therefore crashes for any v11 project with a project-owned macro that declaressupported_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 onmain, and the same fix covers it.Suggested fix: in the v10 and v11 wrappers, convert by value as the v12 wrapper already does:
Add v10 and v11 counterparts of
test_wraps_project_macro_supporting_javascriptthat use["sql"]. Optionally, makeAltimateSupportedLanguagea standalone enum here (python,sql,javascript) so a future vendor bump can't silently change it again.There was a problem hiding this comment.
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. Addedtests/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
AltimateSupportedLanguageas 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.There was a problem hiding this comment.
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.