diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c46543b..b2103772 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`ContextGraph.get_node_property`/`get_node_attributes` "not found" contract clarified; `add_node_attribute` mutation-callback exception safety fixed** (#882, closes #877) by @ZohaibHassan16 + - `get_node_property` returned `None` for both "node missing" and "property missing" with no way to distinguish them, and `get_node_attributes` returned `{}` for a missing node while its siblings disagreed on the not-found signal (`get_node_property`/`find_node` → `None`, `get_edge_data` → `{}`). Both now accept a `default=` parameter matching `dict.get()`'s convention, defaulting to their historical return values (`None` and `{}` respectively) for backward compatibility. Callers that need to disambiguate "node missing" from "value legitimately absent" can pass a private sentinel as `default` + - Added Google-style docstrings to `get_node_property`, `get_node_attributes`, `get_edge_data`, and `find_node` documenting each method's not-found contract, addressing #877's "sibling not-found contract undocumented" gap + - **Corrected during review**: the PR as submitted claimed to fix `add_node_attribute` firing its `mutation_callback` "outside `with self._lock`, without holding the lock," but the diff only removed a stray blank line — the callback call remained outside the lock, unchanged. Further investigation found this was not actually a bug: `self._lock` is a `threading.RLock`, and the same release-the-lock-before-invoking-the-callback pattern is used deliberately in `_add_internal_node`/`_add_internal_edge` elsewhere in this class, avoiding holding the lock for the duration of an arbitrary user-supplied callback. The real inconsistency was that, unlike those two siblings, `add_node_attribute`'s callback call wasn't wrapped in `try/except` — a raising callback propagated uncaught here but was caught and logged there. Now wrapped the same way (`except Exception as e: self.logger.warning(...)`) + - 13 tests covering happy path, missing node, missing property, sentinel disambiguation, falsy-zero, callback firing/non-firing, and (added during review) a raising callback no longer propagating out of `add_node_attribute` + - `pytest tests/context/test_context.py -q`: 27 passed + - **Three `tests/normalize/` tests failed for reasons unrelated to the normalize implementations: a missing optional-dependency skip guard, an incomplete chardet allowlist, and a UTC/local timezone mismatch** (#881, closes #860) by @aoright - `test_detect_language`/`test_detect_with_confidence` in `tests/normalize/test_language_detector.py` asserted on real `langdetect` output with no skip guard, even though `langdetect` is an optional dependency absent from `pyproject.toml` that `LanguageDetector` already degrades gracefully without (`LANGDETECT_AVAILABLE = False`, falls back to `default_language`) — any environment without it failed both tests unconditionally, including a fresh CI run without optional extras installed. Both are now gated with `@unittest.skipUnless(LANGDETECT_AVAILABLE, ...)` - `test_detect_encoding` in `tests/normalize/test_encoding_handler.py` asserted `chardet.detect()`'s result against a 3-name allowlist (`iso-8859-1`/`windows-1252`/`latin-1`); on a short Latin-1 sample, chardet is free to return other compatible single-byte codepages (e.g. `windows-1253`), which fails the allowlist and then cascades into `test_convert_to_utf8` decoding the bytes as Greek instead of the original text. The test now uses a longer, unambiguous Latin-1 corpus and asserts that the detected encoding round-trip-decodes the original text instead of matching a fixed name list; `test_convert_to_utf8` now passes `source_encoding="latin-1"` explicitly rather than relying on chardet's heuristic auto-detection diff --git a/semantica/context/context_graph.py b/semantica/context/context_graph.py index 944e1f6a..da252b84 100644 --- a/semantica/context/context_graph.py +++ b/semantica/context/context_graph.py @@ -410,6 +410,9 @@ class ContextEdge: return d +_ATTRS_MISSING = object() + + class ContextGraph: """ Easy-to-Use Context Graph with All Advanced Features. @@ -708,18 +711,71 @@ class ContextGraph: }) return result - def get_node_property(self, node_id: str, property_name: str) -> Any: - with self._lock: - node = self.nodes.get(node_id) - if not node: - return None - return node.properties.get(property_name) + def get_node_property( + self, + node_id: str, + property_name: str, + default: Any = None, + ) -> Any: + """Return the value of *property_name* on *node_id*. - def get_node_attributes(self, node_id: str) -> Dict[str, Any]: + Returns *default* when the node does not exist or when the property is + not set on the node. Both failure modes return the same *default*, so + a sentinel can identify *any not-found result* as distinct from a + property whose value is legitimately ``None``:: + + _MISSING = object() + val = graph.get_node_property(node_id, "score", default=_MISSING) + if val is _MISSING: + ... # node absent or property not set + + To distinguish a missing node from a missing property specifically, + call ``find_node()`` first to check node existence. + + Args: + node_id: ID of the node to look up. + property_name: Name of the property to retrieve. + default: Value returned when the node or property is absent. + Defaults to ``None`` (backward-compatible). + + Returns: + The property value, or *default* if not found. + """ with self._lock: node = self.nodes.get(node_id) - if not node: - return {} + if node is None: + return default + return node.properties.get(property_name, default) + + def get_node_attributes( + self, + node_id: str, + default: Any = _ATTRS_MISSING, + ) -> Any: + """Return a copy of all properties on *node_id*. + + Returns *default* when the node does not exist. The historical + default is ``{}`` (an empty dict), preserved for backward + compatibility. Pass a private sentinel as *default* to detect a + missing node unambiguously:: + + _MISSING = object() + attrs = graph.get_node_attributes(node_id, default=_MISSING) + if attrs is _MISSING: + ... # node does not exist + + Args: + node_id: ID of the node to look up. + default: Value returned when the node is absent. + Defaults to ``{}`` (backward-compatible). + + Returns: + A shallow copy of the node's properties dict, or *default*. + """ + with self._lock: + node = self.nodes.get(node_id) + if node is None: + return {} if default is _ATTRS_MISSING else default return node.properties.copy() def add_node_attribute(self, node_id: str, attributes: Dict[str, Any]) -> None: @@ -730,13 +786,28 @@ class ContextGraph: node.properties.update(attributes) node.metadata.update(attributes) - if getattr(self, "mutation_callback", None) and not getattr( self, "_suspend_mutation_callback", False ): - self.mutation_callback("UPDATE_NODE", node_id, node.to_dict()) + try: + self.mutation_callback("UPDATE_NODE", node_id, node.to_dict()) + except Exception as e: + self.logger.warning(f"Audit trail callback failed for node {node_id}: {e}") def get_edge_data(self, source_id: str, target_id: str) -> Dict[str, Any]: + """Return metadata for the edge between *source_id* and *target_id*. + + Returns an empty dict ``{}`` when no edge exists between the two nodes + or when either node is absent. + + Args: + source_id: ID of the source node. + target_id: ID of the target node. + + Returns: + A dict containing edge metadata (``id``, ``familyId``, ``type``, + ``weight``, plus any custom metadata), or ``{}`` if not found. + """ with self._lock: for edge in self._adjacency.get(source_id, []): if edge.target_id == target_id: @@ -1080,7 +1151,17 @@ class ContextGraph: self.logger.info(f"Loaded context graph from {path}") def find_node(self, node_id: str) -> Optional[Dict[str, Any]]: - """Find a node by ID.""" + """Return a dict representation of the node identified by *node_id*. + + Returns ``None`` when the node does not exist. + + Args: + node_id: ID of the node to look up. + + Returns: + A dict with keys ``id``, ``type``, ``content``, and ``metadata``, + or ``None`` if the node is not found. + """ with self._lock: node = self.nodes.get(node_id) if node: diff --git a/tests/context/test_context.py b/tests/context/test_context.py index 73edaa4e..dd9a706f 100644 --- a/tests/context/test_context.py +++ b/tests/context/test_context.py @@ -252,5 +252,87 @@ class TestContextModule(unittest.TestCase): self.assertIsNotNone(ctx._memory) self.assertEqual(len(ctx._memory.short_term_memory), 1) +class TestContextGraphNodePropertyContract(unittest.TestCase): + + _MISSING = object() + + def _graph_with_node(self): + graph = ContextGraph() + graph.add_node("n1", "person", "Alice", role="engineer", score=0) + return graph + + def test_get_node_property_existing_node_existing_prop(self): + graph = self._graph_with_node() + self.assertEqual(graph.get_node_property("n1", "role"), "engineer") + + def test_get_node_property_existing_node_missing_prop(self): + graph = self._graph_with_node() + self.assertIsNone(graph.get_node_property("n1", "nonexistent")) + + def test_get_node_property_missing_node_returns_default_none(self): + graph = self._graph_with_node() + self.assertIsNone(graph.get_node_property("ghost", "role")) + + def test_get_node_property_returns_default_on_missing_node(self): + graph = self._graph_with_node() + result = graph.get_node_property("ghost", "role", default=self._MISSING) + self.assertIs(result, self._MISSING) + + def test_get_node_property_returns_default_on_missing_prop(self): + graph = self._graph_with_node() + result = graph.get_node_property("n1", "nonexistent", default=self._MISSING) + self.assertIs(result, self._MISSING) + + def test_get_node_property_explicit_default_returned_for_absent_node(self): + graph = self._graph_with_node() + self.assertEqual(graph.get_node_property("ghost", "role", default="fallback"), "fallback") + + def test_get_node_property_prop_value_of_zero_not_swallowed(self): + graph = self._graph_with_node() + self.assertEqual(graph.get_node_property("n1", "score"), 0) + + def test_get_node_attributes_existing_node_returns_copy(self): + graph = self._graph_with_node() + attrs = graph.get_node_attributes("n1") + self.assertIsInstance(attrs, dict) + self.assertEqual(attrs.get("role"), "engineer") + + def test_get_node_attributes_missing_node_returns_empty_dict_by_default(self): + graph = self._graph_with_node() + self.assertEqual(graph.get_node_attributes("ghost"), {}) + + def test_get_node_attributes_missing_node_explicit_default(self): + graph = self._graph_with_node() + result = graph.get_node_attributes("ghost", default={}) + self.assertEqual(result, {}) + + def test_add_node_attribute_mutation_callback_fires_on_update(self): + graph = self._graph_with_node() + fired = [] + graph.mutation_callback = lambda op, nid, data: fired.append((op, nid)) + graph.add_node_attribute("n1", {"extra": "value"}) + self.assertEqual(len(fired), 1) + self.assertEqual(fired[0], ("UPDATE_NODE", "n1")) + + def test_add_node_attribute_missing_node_no_callback(self): + graph = self._graph_with_node() + fired = [] + graph.mutation_callback = lambda op, nid, data: fired.append((op, nid)) + graph.add_node_attribute("ghost", {"extra": "value"}) + self.assertEqual(len(fired), 0) + + def test_add_node_attribute_raising_callback_does_not_propagate(self): + graph = self._graph_with_node() + + def _boom(op, nid, data): + raise RuntimeError("audit sink unavailable") + + graph.mutation_callback = _boom + # Should not raise, matching _add_internal_node/_add_internal_edge, + # which already catch and log mutation_callback exceptions. + graph.add_node_attribute("n1", {"extra": "value"}) + self.assertEqual(graph.get_node_property("n1", "extra"), "value") + + if __name__ == '__main__': unittest.main()