mirror of
https://github.com/safishamsi/graphify.git
synced 2026-08-27 16:56:36 +00:00
fix(build): inherit the on-disk directed flag in build_merge (#2342)
build_merge took graph direction from its caller only, defaulting to directed=False, and rebuilt with that default at build.py:1327 — discarding the direction of the graph it had just loaded from graph_path. Since the only call site (cli.py:3547) passes no directed=, every incremental `graphify update` through the merge path silently returned an undirected graph for a directed one. _load_existing_graph now also returns the on-disk `directed` flag, parsed from the JSON it already reads, and build_merge's parameter becomes bool | None: None inherits that flag when a graph exists and falls back to False when there is none, while an explicit True/False still wins.
This commit is contained in:
+15
-6
@@ -1188,9 +1188,9 @@ def deduplicate_by_label(nodes: list[dict], edges: list[dict]) -> tuple[list[dic
|
||||
return deduped_nodes, deduped_edges
|
||||
|
||||
|
||||
def _load_existing_graph(graph_path: Path) -> "tuple[list, list, list] | None":
|
||||
"""Load (nodes, edges, hyperedges) from an existing graph.json for an
|
||||
incremental merge, accepting both the ``links`` and ``edges`` spellings.
|
||||
def _load_existing_graph(graph_path: Path) -> "tuple[list, list, list, bool] | None":
|
||||
"""Load (nodes, edges, hyperedges, directed) from an existing graph.json for
|
||||
an incremental merge, accepting both the ``links`` and ``edges`` spellings.
|
||||
|
||||
Reads the JSON directly instead of going through node_link_graph().
|
||||
The latter rebuilds an undirected nx.Graph and then enumerating
|
||||
@@ -1220,6 +1220,7 @@ def _load_existing_graph(graph_path: Path) -> "tuple[list, list, list] | None":
|
||||
list(data.get("nodes", [])),
|
||||
list(data.get(links_key, [])),
|
||||
list(data.get("hyperedges", [])),
|
||||
bool(data.get("directed", False)),
|
||||
)
|
||||
|
||||
|
||||
@@ -1257,7 +1258,7 @@ def merge_raw_extraction(
|
||||
loaded = _load_existing_graph(graph_path)
|
||||
if loaded is None:
|
||||
return new
|
||||
existing_nodes, existing_edges, existing_hyperedges = loaded
|
||||
existing_nodes, existing_edges, existing_hyperedges, _ = loaded
|
||||
|
||||
_eff_root = (
|
||||
str(Path(root).resolve()) if root is not None
|
||||
@@ -1324,7 +1325,7 @@ def build_merge(
|
||||
graph_path: str | Path | None = None,
|
||||
prune_sources: list[str] | None = None,
|
||||
*,
|
||||
directed: bool = False,
|
||||
directed: bool | None = None,
|
||||
dedup: bool = True,
|
||||
dedup_llm_backend: str | None = None,
|
||||
root: str | Path | None = None,
|
||||
@@ -1337,17 +1338,25 @@ def build_merge(
|
||||
preserved unchanged; deleted files are removed via prune_sources.
|
||||
Safe to call repeatedly.
|
||||
root: if given, absolute source_file paths in new_chunks are made relative (#932).
|
||||
directed: if None (default), honor the on-disk graph's own ``directed`` flag
|
||||
when one exists, so an incremental merge can't silently flip a directed
|
||||
graph undirected (#2342). Falls back to False when there is no existing
|
||||
graph to inherit from. An explicit True/False always overrides the on-disk
|
||||
flag.
|
||||
"""
|
||||
graph_path = Path(graph_path if graph_path is not None else _default_graph_json())
|
||||
_loaded = _load_existing_graph(graph_path)
|
||||
if _loaded is not None:
|
||||
existing_nodes, existing_edges, existing_hyperedges = _loaded
|
||||
existing_nodes, existing_edges, existing_hyperedges, existing_directed = _loaded
|
||||
had_graph = True
|
||||
else:
|
||||
existing_nodes = []
|
||||
existing_edges = []
|
||||
existing_hyperedges = []
|
||||
existing_directed = False
|
||||
had_graph = False
|
||||
if directed is None:
|
||||
directed = existing_directed if had_graph else False
|
||||
|
||||
# Effective root for relativizing absolute source_file / prune paths back to the
|
||||
# stored relative source_file keys. When the caller passes root we use it;
|
||||
|
||||
@@ -565,6 +565,97 @@ def test_build_merge_preserves_call_edge_direction(tmp_path):
|
||||
)
|
||||
|
||||
|
||||
def test_build_merge_directed_edge_direction_survives_round_trip(tmp_path):
|
||||
"""Regression for #2342: once build_merge correctly inherits the on-disk
|
||||
`directed` flag, the resulting graph must actually be a DiGraph whose
|
||||
edges are readable in the right direction (a -> b, not b -> a)."""
|
||||
from graphify.extract import extract_js
|
||||
from graphify.export import to_json
|
||||
|
||||
src = "function b() {}\nfunction a() { b(); }\n"
|
||||
src_file = tmp_path / "x.js"
|
||||
src_file.write_text(src)
|
||||
|
||||
extraction = extract_js(src_file)
|
||||
call_edges = [e for e in extraction["edges"] if e["relation"] == "calls"]
|
||||
assert len(call_edges) == 1
|
||||
truth_src = call_edges[0]["source"]
|
||||
truth_tgt = call_edges[0]["target"]
|
||||
|
||||
G1 = build([extraction], directed=True, dedup=False)
|
||||
graph_path = tmp_path / "graph.json"
|
||||
assert to_json(G1, {}, str(graph_path), force=True)
|
||||
|
||||
G2 = build_merge([], graph_path, dedup=False)
|
||||
assert G2.is_directed() is True
|
||||
assert G2.has_edge(truth_src, truth_tgt)
|
||||
assert not G2.has_edge(truth_tgt, truth_src)
|
||||
|
||||
|
||||
def test_build_merge_inherits_directed_flag_from_disk(tmp_path):
|
||||
"""Regression for #2342.
|
||||
|
||||
build_merge with no explicit `directed=` must honor the on-disk graph's
|
||||
own `directed` flag instead of silently defaulting to False, or an
|
||||
incremental --update on a directed graph downgrades it to undirected.
|
||||
"""
|
||||
ext = {
|
||||
"nodes": [{"id": "a", "label": "a", "file_type": "concept",
|
||||
"source_file": "x.md", "source_location": "L1"}],
|
||||
"edges": [],
|
||||
}
|
||||
from graphify.export import to_json
|
||||
|
||||
graph_path = tmp_path / "graph.json"
|
||||
|
||||
# Directed graph on disk -> no directed= kwarg -> stays directed.
|
||||
G1 = build([ext], directed=True, dedup=False)
|
||||
assert to_json(G1, {}, str(graph_path), force=True)
|
||||
G2 = build_merge([], graph_path, dedup=False)
|
||||
assert G2.is_directed() is True
|
||||
saved = json.loads(graph_path.read_text())
|
||||
assert saved.get("directed") is True
|
||||
|
||||
# Undirected graph on disk -> no directed= kwarg -> stays undirected (no regression).
|
||||
G3 = build([ext], directed=False, dedup=False)
|
||||
assert to_json(G3, {}, str(graph_path), force=True)
|
||||
G4 = build_merge([], graph_path, dedup=False)
|
||||
assert G4.is_directed() is False
|
||||
|
||||
|
||||
def test_build_merge_fresh_graph_defaults_undirected(tmp_path):
|
||||
"""No existing graph.json + no directed= kwarg -> falls back to the
|
||||
current default (False), same as before #2342."""
|
||||
graph_path = tmp_path / "does_not_exist.json"
|
||||
G = build_merge([], graph_path, dedup=False)
|
||||
assert G.is_directed() is False
|
||||
|
||||
|
||||
def test_build_merge_explicit_directed_overrides_disk_flag(tmp_path):
|
||||
"""An explicit directed=True/False from the caller must still win over
|
||||
whatever is stored on disk (#2342)."""
|
||||
ext = {
|
||||
"nodes": [{"id": "a", "label": "a", "file_type": "concept",
|
||||
"source_file": "x.md", "source_location": "L1"}],
|
||||
"edges": [],
|
||||
}
|
||||
from graphify.export import to_json
|
||||
|
||||
graph_path = tmp_path / "graph.json"
|
||||
|
||||
# Directed on disk, explicit directed=False -> caller wins.
|
||||
G1 = build([ext], directed=True, dedup=False)
|
||||
assert to_json(G1, {}, str(graph_path), force=True)
|
||||
G2 = build_merge([], graph_path, directed=False, dedup=False)
|
||||
assert G2.is_directed() is False
|
||||
|
||||
# Undirected on disk, explicit directed=True -> caller wins.
|
||||
G3 = build([ext], directed=False, dedup=False)
|
||||
assert to_json(G3, {}, str(graph_path), force=True)
|
||||
G4 = build_merge([], graph_path, directed=True, dedup=False)
|
||||
assert G4.is_directed() is True
|
||||
|
||||
|
||||
def test_build_from_json_preserves_first_direction_on_bidirectional_pair(tmp_path):
|
||||
"""Regression for #1061.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user