mirror of
https://github.com/semantica-agi/semantica.git
synced 2026-08-29 04:26:20 +00:00
fix: address three Qodo review findings in JenaStore
1. Endpoint derivation regression: Detect if self.endpoint already contains a Fuseki service suffix (/query, /update, /sparql) to prevent double-appending (e.g., /ds/query/query). If it does, derive the base and construct both paths properly. 2. Misleading serialize warning: Limit the named-graph data loss warning to single-graph serializer formats (turtle, xml, n3, etc.). Multi-graph formats (trig, nquads, nt) will correctly serialize all graphs without warning. 3. Zero-added error misdiagnosis: Track malformed triples accurately in add_triplets(). If every triplet fails the local validation (ValueError/ AttributeError), raise a formatting-oriented ProcessingError instead of assuming a store connectivity issue. Includes comprehensive regression tests for all three cases.
This commit is contained in:
Sameer6305
@@ -108,9 +108,44 @@ class JenaStore:
|
||||
# requests against the Fuseki update endpoint. The read-only
|
||||
# SPARQLStore was previously used here, which caused every
|
||||
# add_triplets() call to silently fail with TypeError.
|
||||
base = self.endpoint.rstrip("/")
|
||||
query_endpoint = f"{base}/query"
|
||||
update_endpoint = f"{base}/update"
|
||||
# Derive Fuseki sub-paths from the endpoint. The documented
|
||||
# and tested contract is a bare dataset base URL (e.g.
|
||||
# "http://localhost:3030/ds"), from which "/query" and
|
||||
# "/update" are appended. As a defensive measure, detect if
|
||||
# the caller already supplied a full service URL ending in a
|
||||
# recognised Fuseki suffix ("/query", "/sparql", "/update")
|
||||
# and avoid double-appending (e.g. "…/ds/query/query").
|
||||
_QUERY_SUFFIXES = ("/query", "/sparql")
|
||||
_UPDATE_SUFFIXES = ("/update",)
|
||||
_ALL_SUFFIXES = _QUERY_SUFFIXES + _UPDATE_SUFFIXES
|
||||
|
||||
stripped = self.endpoint.rstrip("/")
|
||||
_ends_with_suffix = any(
|
||||
stripped.endswith(sfx) for sfx in _ALL_SUFFIXES
|
||||
)
|
||||
|
||||
if _ends_with_suffix:
|
||||
# Already a full service URL — strip the known suffix to
|
||||
# recover the dataset base, then derive both sub-paths
|
||||
# consistently from that base.
|
||||
_base = stripped
|
||||
for sfx in _ALL_SUFFIXES:
|
||||
if stripped.endswith(sfx):
|
||||
_base = stripped[: -len(sfx)]
|
||||
break
|
||||
self.logger.warning(
|
||||
"endpoint %r already contains a service suffix; "
|
||||
"using %r as dataset base to derive query and update "
|
||||
"sub-paths. Pass a bare dataset base URL (e.g. "
|
||||
"'http://host:3030/ds') to suppress this warning.",
|
||||
self.endpoint,
|
||||
_base,
|
||||
)
|
||||
query_endpoint = f"{_base}/query"
|
||||
update_endpoint = f"{_base}/update"
|
||||
else:
|
||||
query_endpoint = f"{stripped}/query"
|
||||
update_endpoint = f"{stripped}/update"
|
||||
try:
|
||||
store = SPARQLUpdateStore(
|
||||
query_endpoint=query_endpoint,
|
||||
@@ -202,6 +237,7 @@ class JenaStore:
|
||||
if graph_uri is not None:
|
||||
context = self.graph.graph(URIRef(str(graph_uri)))
|
||||
|
||||
malformed_count = 0
|
||||
for triplet in triplets:
|
||||
try:
|
||||
subject = URIRef(triplet.subject)
|
||||
@@ -228,16 +264,23 @@ class JenaStore:
|
||||
# read-only store, authentication error) propagate so they
|
||||
# are not silently swallowed as per-triplet warnings.
|
||||
self.logger.warning(f"Skipping malformed triplet: {e}")
|
||||
malformed_count += 1
|
||||
|
||||
if triplets and added_count == 0:
|
||||
# Every single triplet failed. This almost certainly indicates
|
||||
# a store-level problem (wrong endpoint, auth failure, etc.)
|
||||
# rather than N bad triplets, so we raise instead of returning
|
||||
# a misleading success=True/added=0.
|
||||
raise ProcessingError(
|
||||
f"Failed to add any of the {len(triplets)} triplet(s). "
|
||||
"Check store connectivity and endpoint configuration."
|
||||
)
|
||||
if malformed_count == len(triplets):
|
||||
# Every failure was caught by the per-triplet handler —
|
||||
# the root cause is data formatting, not store connectivity.
|
||||
raise ProcessingError(
|
||||
f"All {len(triplets)} triplet(s) failed validation — "
|
||||
"check triplet subject/predicate/object formatting."
|
||||
)
|
||||
else:
|
||||
# Zero added but failures were not all per-triplet (store
|
||||
# itself raised, or other unexpected path).
|
||||
raise ProcessingError(
|
||||
f"Failed to add any of the {len(triplets)} triplet(s). "
|
||||
"Check store connectivity and endpoint configuration."
|
||||
)
|
||||
|
||||
self.progress_tracker.stop_tracking(
|
||||
tracking_id,
|
||||
@@ -459,12 +502,18 @@ class JenaStore:
|
||||
|
||||
try:
|
||||
# Warn when named-graph triples exist and would be silently dropped
|
||||
# by a single-graph serializer. len(Dataset) counts all graphs;
|
||||
# len(Dataset.default_graph) counts only the default graph.
|
||||
# Any positive difference means named-graph content is present.
|
||||
# by a single-graph serializer. Multi-graph serializers (trig,
|
||||
# nquads, nt/ntriples, trix, json-ld, hext, patch) include all
|
||||
# named graphs and must NOT trigger the warning.
|
||||
# Set verified empirically against rdflib's plugin registry.
|
||||
_SINGLE_GRAPH_FORMATS = frozenset({
|
||||
"turtle", "ttl", "text/turtle", "longturtle",
|
||||
"xml", "application/rdf+xml", "pretty-xml",
|
||||
"n3", "text/n3",
|
||||
})
|
||||
default_count = len(self.graph.default_graph)
|
||||
total_count = len(self.graph)
|
||||
if total_count > default_count:
|
||||
if total_count > default_count and format in _SINGLE_GRAPH_FORMATS:
|
||||
self.logger.warning(
|
||||
"serialize(format=%r) serializes only the default graph. "
|
||||
"%d named-graph triple(s) will be omitted. "
|
||||
|
||||
@@ -511,5 +511,254 @@ class TestJenaStoreDatasetMigration(unittest.TestCase):
|
||||
mock_warn.assert_not_called()
|
||||
|
||||
|
||||
|
||||
|
||||
class TestJenaStoreEndpointDerivation(unittest.TestCase):
|
||||
"""
|
||||
Regression tests for Bug 1 (Qodo): endpoint suffix detection prevents
|
||||
double-appending of /query or /update.
|
||||
"""
|
||||
|
||||
def _captured_kwargs(self, endpoint):
|
||||
"""Return the kwargs passed to SPARQLUpdateStore for a given endpoint."""
|
||||
captured = {}
|
||||
real_init = __import__(
|
||||
"rdflib.plugins.stores.sparqlstore",
|
||||
fromlist=["SPARQLUpdateStore"],
|
||||
).SPARQLUpdateStore.__init__
|
||||
|
||||
def fake_update_store(self_store, **kwargs):
|
||||
captured.update(kwargs)
|
||||
# Avoid real network connection; just store args and bail early
|
||||
raise RuntimeError("stop_after_capture")
|
||||
|
||||
with patch(
|
||||
"semantica.triplet_store.jena_store.SPARQLUpdateStore",
|
||||
) as MockUS:
|
||||
MockUS.side_effect = RuntimeError("stop_after_capture")
|
||||
MockUS.__init__ = fake_update_store
|
||||
try:
|
||||
JenaStore(endpoint=endpoint)
|
||||
except Exception:
|
||||
pass
|
||||
# Extract call kwargs
|
||||
if MockUS.call_args is not None:
|
||||
captured = MockUS.call_args.kwargs
|
||||
return captured
|
||||
|
||||
def test_bare_base_url_appends_query_and_update(self):
|
||||
"""
|
||||
The canonical case: a bare dataset base URL gets /query and /update
|
||||
appended correctly.
|
||||
"""
|
||||
with patch(
|
||||
"semantica.triplet_store.jena_store.SPARQLUpdateStore"
|
||||
) as MockUS, patch("semantica.triplet_store.jena_store.Dataset"):
|
||||
MockUS.return_value = MagicMock()
|
||||
MockUS.return_value.graph_aware = True
|
||||
JenaStore(endpoint="http://localhost:3030/ds")
|
||||
kwargs = MockUS.call_args.kwargs
|
||||
self.assertEqual(kwargs["query_endpoint"], "http://localhost:3030/ds/query")
|
||||
self.assertEqual(kwargs["update_endpoint"], "http://localhost:3030/ds/update")
|
||||
|
||||
def test_bare_base_url_with_trailing_slash_normalised(self):
|
||||
"""Trailing slash on the base URL must not produce a double slash."""
|
||||
with patch(
|
||||
"semantica.triplet_store.jena_store.SPARQLUpdateStore"
|
||||
) as MockUS, patch("semantica.triplet_store.jena_store.Dataset"):
|
||||
MockUS.return_value = MagicMock()
|
||||
MockUS.return_value.graph_aware = True
|
||||
JenaStore(endpoint="http://localhost:3030/ds/")
|
||||
kwargs = MockUS.call_args.kwargs
|
||||
self.assertEqual(kwargs["query_endpoint"], "http://localhost:3030/ds/query")
|
||||
self.assertEqual(kwargs["update_endpoint"], "http://localhost:3030/ds/update")
|
||||
|
||||
def test_endpoint_already_ending_in_query_not_double_appended(self):
|
||||
"""
|
||||
If the caller passes 'http://localhost:3030/ds/query' (already a full
|
||||
service URL), the derived query_endpoint must still be
|
||||
'http://localhost:3030/ds/query', not '.../ds/query/query'.
|
||||
"""
|
||||
with patch(
|
||||
"semantica.triplet_store.jena_store.SPARQLUpdateStore"
|
||||
) as MockUS, patch("semantica.triplet_store.jena_store.Dataset"):
|
||||
MockUS.return_value = MagicMock()
|
||||
MockUS.return_value.graph_aware = True
|
||||
JenaStore(endpoint="http://localhost:3030/ds/query")
|
||||
kwargs = MockUS.call_args.kwargs
|
||||
self.assertEqual(kwargs["query_endpoint"], "http://localhost:3030/ds/query")
|
||||
self.assertEqual(kwargs["update_endpoint"], "http://localhost:3030/ds/update")
|
||||
# Crucially: no double-suffix
|
||||
self.assertNotIn("query/query", kwargs["query_endpoint"])
|
||||
self.assertNotIn("query/update", kwargs["update_endpoint"])
|
||||
|
||||
def test_endpoint_already_ending_in_sparql_not_double_appended(self):
|
||||
"""
|
||||
Some Fuseki deployments use /sparql as the query service name.
|
||||
Passing that as the endpoint must not produce '.../ds/sparql/query'.
|
||||
"""
|
||||
with patch(
|
||||
"semantica.triplet_store.jena_store.SPARQLUpdateStore"
|
||||
) as MockUS, patch("semantica.triplet_store.jena_store.Dataset"):
|
||||
MockUS.return_value = MagicMock()
|
||||
MockUS.return_value.graph_aware = True
|
||||
JenaStore(endpoint="http://localhost:3030/ds/sparql")
|
||||
kwargs = MockUS.call_args.kwargs
|
||||
self.assertEqual(kwargs["query_endpoint"], "http://localhost:3030/ds/query")
|
||||
self.assertEqual(kwargs["update_endpoint"], "http://localhost:3030/ds/update")
|
||||
self.assertNotIn("sparql/query", kwargs["query_endpoint"])
|
||||
self.assertNotIn("sparql/update", kwargs["update_endpoint"])
|
||||
|
||||
def test_endpoint_already_ending_in_update_not_double_appended(self):
|
||||
"""
|
||||
Endpoint pre-set to '.../ds/update' must not produce '.../ds/update/update'.
|
||||
"""
|
||||
with patch(
|
||||
"semantica.triplet_store.jena_store.SPARQLUpdateStore"
|
||||
) as MockUS, patch("semantica.triplet_store.jena_store.Dataset"):
|
||||
MockUS.return_value = MagicMock()
|
||||
MockUS.return_value.graph_aware = True
|
||||
JenaStore(endpoint="http://localhost:3030/ds/update")
|
||||
kwargs = MockUS.call_args.kwargs
|
||||
self.assertEqual(kwargs["query_endpoint"], "http://localhost:3030/ds/query")
|
||||
self.assertEqual(kwargs["update_endpoint"], "http://localhost:3030/ds/update")
|
||||
self.assertNotIn("update/update", kwargs["update_endpoint"])
|
||||
|
||||
|
||||
class TestJenaStoreSerializeWarningFormats(unittest.TestCase):
|
||||
"""
|
||||
Regression tests for Bug 2 (Qodo): serialize warning must fire only for
|
||||
single-graph formats (turtle, xml, n3, …) and must NOT fire for
|
||||
multi-graph formats (trig, nquads, nt, json-ld, …).
|
||||
"""
|
||||
|
||||
def _store_with_named_graph_content(self):
|
||||
from rdflib import URIRef
|
||||
store = JenaStore()
|
||||
store.add_triplets([
|
||||
Triplet("http://s1", "http://p", "default_val")
|
||||
])
|
||||
store.add_triplets([
|
||||
Triplet("http://s2", "http://p", "named_val")
|
||||
], graph="http://example.org/ng")
|
||||
return store
|
||||
|
||||
# --- formats that MUST trigger the warning ---
|
||||
|
||||
def test_turtle_triggers_warning(self):
|
||||
store = self._store_with_named_graph_content()
|
||||
with patch.object(store.logger, "warning") as mock_warn:
|
||||
store.serialize(format="turtle")
|
||||
mock_warn.assert_called_once()
|
||||
|
||||
def test_xml_triggers_warning(self):
|
||||
store = self._store_with_named_graph_content()
|
||||
with patch.object(store.logger, "warning") as mock_warn:
|
||||
store.serialize(format="xml")
|
||||
mock_warn.assert_called_once()
|
||||
|
||||
def test_n3_triggers_warning(self):
|
||||
store = self._store_with_named_graph_content()
|
||||
with patch.object(store.logger, "warning") as mock_warn:
|
||||
store.serialize(format="n3")
|
||||
mock_warn.assert_called_once()
|
||||
|
||||
# --- formats that must NOT trigger the warning ---
|
||||
|
||||
def test_trig_no_warning(self):
|
||||
"""trig is a multi-graph format — includes all named graphs, no warning."""
|
||||
store = self._store_with_named_graph_content()
|
||||
with patch.object(store.logger, "warning") as mock_warn:
|
||||
output = store.serialize(format="trig")
|
||||
mock_warn.assert_not_called()
|
||||
# Sanity: both triples are actually in the output
|
||||
self.assertIn("named_val", output)
|
||||
|
||||
def test_nquads_no_warning(self):
|
||||
"""nquads is a multi-graph format — no warning."""
|
||||
store = self._store_with_named_graph_content()
|
||||
with patch.object(store.logger, "warning") as mock_warn:
|
||||
output = store.serialize(format="nquads")
|
||||
mock_warn.assert_not_called()
|
||||
self.assertIn("named_val", output)
|
||||
|
||||
def test_nt_no_warning(self):
|
||||
"""nt (N-Triples) serializes all contexts — no warning."""
|
||||
store = self._store_with_named_graph_content()
|
||||
with patch.object(store.logger, "warning") as mock_warn:
|
||||
store.serialize(format="nt")
|
||||
mock_warn.assert_not_called()
|
||||
|
||||
|
||||
class TestJenaStoreZeroAddedErrorMessage(unittest.TestCase):
|
||||
"""
|
||||
Regression tests for Bug 3 (Qodo): when added_count==0, the ProcessingError
|
||||
message must distinguish data-formatting failures from connectivity failures.
|
||||
"""
|
||||
|
||||
def test_all_malformed_triplets_gives_formatting_error(self):
|
||||
"""
|
||||
When every triplet fails the per-triplet ValueError/AttributeError handler,
|
||||
the raised ProcessingError must mention validation / formatting, NOT
|
||||
connectivity or endpoint configuration.
|
||||
"""
|
||||
from semantica.utils.exceptions import ProcessingError
|
||||
|
||||
store = JenaStore()
|
||||
|
||||
# object=None triggers AttributeError on None.startswith("http") in
|
||||
# add_triplets' obj-resolution line — this IS caught by the per-triplet
|
||||
# (ValueError, AttributeError) handler and increments malformed_count.
|
||||
# (Note: subject=None would raise TypeError on URIRef(None), which is
|
||||
# NOT caught by the per-triplet handler and would escape to the outer
|
||||
# except, producing a different message path — don't use that.)
|
||||
bad = Triplet(subject="http://s", predicate="http://p", object=None)
|
||||
|
||||
with self.assertRaises(ProcessingError) as ctx:
|
||||
store.add_triplets([bad, bad, bad])
|
||||
|
||||
msg = str(ctx.exception).lower()
|
||||
# Must mention validation/formatting
|
||||
self.assertTrue(
|
||||
"validation" in msg or "formatting" in msg or "format" in msg,
|
||||
f"Expected validation/formatting message, got: {ctx.exception}",
|
||||
)
|
||||
# Must NOT suggest connectivity
|
||||
self.assertNotIn("connectivity", msg)
|
||||
self.assertNotIn("endpoint configuration", msg)
|
||||
|
||||
def test_connectivity_error_gives_connectivity_message(self):
|
||||
"""
|
||||
When a store-level exception (not per-triplet ValueError) causes zero adds,
|
||||
the message must mention connectivity/endpoint — not validation.
|
||||
This simulates a store that raises a non-ValueError on .add().
|
||||
"""
|
||||
from semantica.utils.exceptions import ProcessingError
|
||||
from rdflib import Dataset
|
||||
|
||||
store = JenaStore()
|
||||
|
||||
# Replace the Dataset with a mock whose .add() raises a non-per-triplet error
|
||||
mock_ds = MagicMock(spec=Dataset)
|
||||
mock_ds.default_graph = MagicMock()
|
||||
mock_ds.default_graph.__len__ = MagicMock(return_value=0)
|
||||
mock_ds.__len__ = MagicMock(return_value=0)
|
||||
|
||||
# Raise RuntimeError (not ValueError/AttributeError) — store-level failure
|
||||
mock_ds.add.side_effect = RuntimeError("connection refused")
|
||||
store.graph = mock_ds
|
||||
|
||||
triplet = Triplet("http://s", "http://p", "http://o")
|
||||
|
||||
with self.assertRaises(ProcessingError) as ctx:
|
||||
store.add_triplets([triplet])
|
||||
|
||||
# The RuntimeError propagates past the per-triplet handler and is caught
|
||||
# by the outer except — the message comes from the outer re-raise
|
||||
msg = str(ctx.exception).lower()
|
||||
# Should mention "failed to add triplets" (the outer handler wraps it)
|
||||
self.assertIn("failed", msg)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
Reference in New Issue
Block a user