From a30b56e2df5459d941c98a0c41bac20fc90a87b0 Mon Sep 17 00:00:00 2001 From: Ousama Ben Younes Date: Thu, 13 Aug 2026 13:30:24 +0100 Subject: [PATCH] fix(sql): distinguish a broken tree-sitter-sql grammar from a missing one (#2602) --- graphify/extract.py | 24 +++++++++++++---- graphify/extractors/sql.py | 14 ++++++++-- tests/test_extract.py | 55 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 7 deletions(-) diff --git a/graphify/extract.py b/graphify/extract.py index a2e5fd3..6e1fca8 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -4877,6 +4877,13 @@ _EXTRA_FOR_EXTENSION = { ".dme": "dm", } +# Substrings an extractor's error carries to classify why a dependency-backed +# file contributed nothing, used by the #1745 warning in extract(). A grammar +# that is present but fails to load (#2602) must not be reported as missing — +# the "install the extra" hint would be a no-op. +_DEP_MISSING_MARKER = "not installed" +_DEP_LOAD_FAILED_MARKER = "failed to load" + # Extensionless executables (CLI entry points like `devctl` or `manage`) carry # their language in the shebang, not the suffix. detect.classify_file already @@ -5464,21 +5471,28 @@ def extract( _missing_dep_error: dict[str, str] = {} for i, _p in enumerate(paths): _err = (per_file[i] or {}).get("error") or "" - if "not installed" in _err: + if _DEP_MISSING_MARKER in _err or _DEP_LOAD_FAILED_MARKER in _err: _ext = _p.suffix.lower() _missing_dep_count[_ext] = _missing_dep_count.get(_ext, 0) + 1 _missing_dep_error.setdefault(_ext, _err) for _ext, _n in sorted(_missing_dep_count.items(), key=lambda kv: (-kv[1], kv[0])): _extra = _EXTRA_FOR_EXTENSION.get(_ext) - if _extra: - _reason = _missing_dep_error[_ext].split(". ")[0] + _err_text = _missing_dep_error[_ext] + if _extra and _DEP_MISSING_MARKER in _err_text: + # Genuinely absent optional extra — point the user at the install. + _reason = _err_text.split(". ")[0] _hint = f' Install it with: pip install "graphifyy[{_extra}]"' + _cause = "a dependency is missing" else: - _reason = _missing_dep_error[_ext] + # Either no known extra, or the grammar is present but failed to + # load (#2602): surface the real error and never suggest reinstall. + _reason = _err_text _hint = "" + _cause = ("a dependency is missing" if _DEP_MISSING_MARKER in _err_text + else "a dependency failed to load") print( f" warning: {_n} {_ext} file(s) contributed nothing to the graph " - f"because a dependency is missing: {_reason}.{_hint} (#1745)", + f"because {_cause}: {_reason}.{_hint} (#1745)", file=sys.stderr, flush=True, ) diff --git a/graphify/extractors/sql.py b/graphify/extractors/sql.py index 4993176..a5dc18c 100644 --- a/graphify/extractors/sql.py +++ b/graphify/extractors/sql.py @@ -32,8 +32,18 @@ def extract_sql(path: Path, content: str | bytes | None = None) -> dict: try: import tree_sitter_sql as tssql from tree_sitter import Language, Parser - except ImportError: - return {"nodes": [], "edges": [], "error": "tree_sitter_sql not installed. Run: pip install tree-sitter-sql"} + except ImportError as e: + import importlib.util + # An installed-but-broken grammar (e.g. a C extension built for a + # different Python ABI, #2602) raises ImportError here too. Reporting + # that as "not installed" sends the user to a no-op `pip install`, so + # distinguish a genuinely-absent module from one that failed to load + # and surface the real exception in the latter case. + if importlib.util.find_spec("tree_sitter_sql") is None: + return {"nodes": [], "edges": [], + "error": "tree_sitter_sql not installed. Run: pip install tree-sitter-sql"} + return {"nodes": [], "edges": [], + "error": f"tree_sitter_sql is installed but failed to load: {e}"} try: language = Language(tssql.language()) diff --git a/tests/test_extract.py b/tests/test_extract.py index c7d1071..cb9d715 100644 --- a/tests/test_extract.py +++ b/tests/test_extract.py @@ -3220,6 +3220,61 @@ def test_extract_no_missing_dep_warning_when_sql_installed(tmp_path, capsys): assert "#1745" not in err +def test_extract_sql_reports_load_failure_not_missing(tmp_path, monkeypatch): + # #2602: an installed-but-broken grammar (e.g. a wheel built for a different + # Python ABI) raises ImportError at import time just like an absent one. The + # extractor must NOT claim "not installed" — that sends the user to a no-op + # `pip install` — but surface the real load exception instead. + import builtins + from graphify.extractors.sql import extract_sql + pytest.importorskip("tree_sitter_sql") # find_spec must see it as installed + + _orig_import = builtins.__import__ + + def _broken_import(name, *args, **kwargs): + if name == "tree_sitter_sql": + raise ImportError("dynamic module does not define module export function") + return _orig_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", _broken_import) + err = extract_sql(tmp_path / "schema.sql", "SELECT 1;").get("error") or "" + assert "failed to load" in err + assert "dynamic module does not define module export function" in err + assert "pip install" not in err + + +def test_extract_warns_sql_grammar_failed_to_load(tmp_path, capsys, monkeypatch): + # #2602: the aggregated #1745 warning must surface a present-but-broken + # grammar with the real cause and WITHOUT the misleading "install the extra" + # hint, so the files are neither silently dropped nor sent to a no-op fix. + import builtins + pytest.importorskip("tree_sitter_sql") + + _orig_import = builtins.__import__ + + def _broken_import(name, *args, **kwargs): + if name == "tree_sitter_sql": + raise ImportError("dynamic module does not define module export function") + return _orig_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", _broken_import) + s1 = tmp_path / "schema.sql"; s1.write_text("CREATE TABLE users (id INT);\n") + s2 = tmp_path / "views.sql"; s2.write_text("CREATE VIEW v AS SELECT * FROM users;\n") + + result = extract([s1, s2], cache_root=tmp_path) + err = capsys.readouterr().err + + assert "2 .sql file(s)" in err + assert "failed to load" in err + assert "#1745" in err + # the no-op fix must NOT be suggested for a present-but-broken grammar + assert "graphifyy[sql]" not in err + assert "pip install" not in err + # #2543: still surfaced as failed so the incremental manifest retries them + failed = {Path(p).name for p in result.get("failed_sources", [])} + assert failed == {"schema.sql", "views.sql"} + + def test_extract_progress_final_line_uses_consistent_denominator(tmp_path, capsys): # #1693: intermediate progress lines count against uncached_work; the final # "100%" line must NOT switch to total_files (which includes cached hits and