fix(sql): distinguish a broken tree-sitter-sql grammar from a missing one (#2602)
This commit is contained in:
committed by
safishamsi
parent
51cf1481b6
commit
a30b56e2df
+19
-5
@@ -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,
|
||||
)
|
||||
|
||||
|
||||
@@ -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())
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user