Entity graph endpoint — medium/low review findings (post #37)

[damienriehl]

Entity graph endpoint — medium / low review findings (post #37)

The peer review on #37 surfaced 7 medium and 9 low items in addition to the blocking + high findings already resolved by #104. None are correctness blockers, but several are worth tracking for follow-up. Ordered by severity, then by likely time-to-fix.

Medium

M1 — Use RETURNING from upsert in update_lint_config

Note: This was in the #94 review, not #37 — including here only if relevant. (Skip if not applicable.)

M1 (real) — Concurrent first-time PUT on entity graph config

N/A — entity graph endpoint is read-only; nothing to upsert.

M1' — _child_count re-scans the graph per node

ontokit/services/ontology.py — _child_count(uri) runs graph.subjects(RDFS.subClassOf, uri) and intersects with OWL.Class triples for every node in visited. For a 200-node truncation cap with deep ontologies, that's up to 200 reverse triple lookups. Bounded but redundant — could memoize.

M2 — Worker fails opaquely if migration hasn't run

Note: This was a #94 finding. If the entity graph endpoint has analogous brittleness on missing schema, file separately.

M3 — is_root field redundant with node_type ∈ {"root", "secondary_root"}

ontokit/schemas/graph.py:GraphNode exposes both. After the secondary_root reclassification, is_root no longer maps cleanly. Pick one and drop the other (or rename one to discovered_via: Literal["primary", "see_also"]).

M4 — total_concept_count is misleadingly named

ontokit/schemas/graph.py:EntityGraphResponse.total_concept_count — set to total_discovered, which is "unique IRIs the BFS attempted to materialize, including those rejected by max_nodes." Calling it "concept count" implies it's the count of concepts in the ontology. Rename to discovered_node_count or similar; document that it's the BFS frontier size including truncated nodes.

M5 — Edge id uses string concat with no escaping

ontokit/services/ontology.py:_add_edge — f"{source}->{target}:{edge_type}". IRIs can theoretically contain : (and rarely ->); two distinct edges could collide on the same id, and the dedup check silently drops the second. Use a tuple key (src, dst, edge_type) in edge_ids instead of a stringified id.

M6 — Migration story for the deleted class-hierarchy endpoint

The original PR removed a stub class_hierarchy endpoint that always raised NotImplementedError. Removing it is correct, but a one-line note in the PR description would have clarified for OpenAPI consumers that no working API path was deleted.

M7 — Three different seen variables in same function

build_entity_graph had inner seen: set[str] for the BFS visited tracker, and the seeAlso helpers each had their own seen: set[URIRef]. After H4's extraction this is partially resolved (helpers are top-level now), but the BFS body still uses seen for one purpose; consider renaming for clarity.

Low

L1 — _classify_node's third arg _depth is unused

ontokit/services/ontology.py:_classify_node(uri, is_focus, _depth). Drop the parameter.

L2 — Negative depth convention is undocumented

Ancestors BFS uses negative depths (-(current_depth + 1)) but the schema field depth: int = 0 doesn't document this. Either document in the schema docstring or split into ancestor_depth: int | None / descendant_depth: int | None.

L3 — seeAlso targets discovered with depth=0

Collides with focus depth=0. Frontend has is_focus to disambiguate, but the BFS depth field becomes ambiguous for layout. Worth noting in the schema.

L4 — _get_local_name doesn't handle empty trailing slash

iri.rsplit("/", 1)[-1] returns "" if iri ends with /. Edge case.

L5 — Double-check depth limits stay in sync if classes.py route is reintroduced

Project route uses ge=0, le=10; the (now-deleted) classes.py route used the same. If that route is ever re-added, mirror the limits.

L6 — if TYPE_CHECKING: import + runtime import is duplicative

from ontokit.schemas.graph import EntityGraphResponse is imported both in the TYPE_CHECKING block at module top AND inside the function. With from __future__ import annotations, the TYPE_CHECKING import alone suffices for annotations.

L7 — EXTERNAL_NAMESPACES is a tuple ✅

Verified by grep that it's never mutated. Tuple is correct here. (Already resolved.)

L8 — _service_with_graph(OntologyService(storage=None))

Test relies on storage being None-allowed. Verify OntologyService.__init__ signature matches.

L9 — Test repetition

tests/unit/test_entity_graph.py has many tests that call _base_graph() then add 1–2 triples. A small builder pattern (graph_with_chain(["A", "B", "C"])) would compress and improve readability.

---

If anyone wants to pick any of these up, comment below and we can split into individual issues.