From 4b2ef260cc5ac03f73fd71834d0fa91d4a7ff09a Mon Sep 17 00:00:00 2001 From: jdsika Date: Thu, 9 Jul 2026 08:13:51 +0200 Subject: [PATCH] fix(gen-shacl): correct rule-converter path parity, combined bounds, and SPARQL escaping Review hardening for the SHACL-SPARQL rule converters. Three defects, each with a regression test that fails before this change: - Induced-slot parity: _slot_uri and _resolve_enum_value_ref resolved the *base* slot, so a slot_usage override of slot_uri (or a narrowed enum range) made the generated SPARQL query a property/enum the data never uses while sh:path used the induced IRI. The constraint then silently never fired (false negative). Both now resolve the induced slot for the class, matching the sh:path logic in the main slot loop. - Combined operators: a single precondition / member condition dispatched on the first matching operator, so {minimum_value: X, maximum_value: Y} dropped the lower bound and under-constrained the trigger (false positives). A shared _scalar_filters helper now emits every recognised operator, and still returns None -- skip, never mis-translate -- when none is recognised. - SPARQL string escaping: an equals_string / permissible-value name containing a double quote, backslash or newline produced invalid, unparseable SPARQL. New _sparql_string_literal escapes per SPARQL 1.1 section 19.7. Tests: 6 new regression tests (structural, prepareQuery syntax, and pyshacl end-to-end) covering all three defects; full shaclgen suite green (111). Signed-off-by: Carlo van Driesten Signed-off-by: jdsika (cherry picked from commit e263def7fdc3a4d38feaf019c692f7b39201aa09) --- .../linkml/src/linkml/generators/shaclgen.py | 155 +++++--- tests/linkml/test_generators/test_shaclgen.py | 331 ++++++++++++++++++ 2 files changed, 438 insertions(+), 48 deletions(-) diff --git a/packages/linkml/src/linkml/generators/shaclgen.py b/packages/linkml/src/linkml/generators/shaclgen.py index e856774995..aa5452bb38 100644 --- a/packages/linkml/src/linkml/generators/shaclgen.py +++ b/packages/linkml/src/linkml/generators/shaclgen.py @@ -600,46 +600,75 @@ def _compose_rule_sparql(self, sv, cls: ClassDefinition, rule) -> str | None: body = "\n".join(f" {line}" for line in (pre_lines + violation)) return f"SELECT $this WHERE {{\n{body}\n}}" + def _scalar_filters(self, var: str, cond, resolve: Callable[[str], str]) -> list[str] | None: + """Return the SPARQL ``FILTER`` lines for the scalar operators on *cond*. + + Unlike a first-match dispatch, **every** recognised operator contributes + a line, so a condition combining operators — e.g. a bounded range + ``{minimum_value: X, maximum_value: Y}`` — emits *both* bounds instead of + silently keeping only the first and under-constraining the query. + ``value_presence: PRESENT`` contributes no filter (the caller's triple + binding already enforces presence). + + *resolve* maps an ``equals_string`` value to a SPARQL term (an enum + ``meaning`` IRI or an escaped string literal). + + Returns ``None`` when *cond* sets none of the recognised scalar + operators, so the caller skips a rule it cannot faithfully translate + rather than emitting an under-constrained (or vacuous) query. + """ + filters: list[str] = [] + recognized = getattr(cond, "value_presence", None) == PresenceEnum(PresenceEnum.PRESENT) + equals = getattr(cond, "equals_string", None) + if equals is not None: + filters.append(f"FILTER ( {var} = {resolve(equals)} )") + recognized = True + minimum = getattr(cond, "minimum_value", None) + if minimum is not None: + filters.append(f"FILTER ( {var} >= {self._sparql_number(minimum)} )") + recognized = True + maximum = getattr(cond, "maximum_value", None) + if maximum is not None: + filters.append(f"FILTER ( {var} <= {self._sparql_number(maximum)} )") + recognized = True + return filters if recognized else None + def _precondition_patterns(self, sv, cls: ClassDefinition, pre_slots) -> list[str] | None: """Translate a conjunction of precondition slot conditions into SPARQL graph patterns (plus ``FILTER`` lines) that bind focus nodes satisfying every condition. - Returns ``None`` if any condition uses an operator not handled here. + Returns ``None`` if any condition sets no recognised operator. - Supported operators: ``value_presence: PRESENT``, ``equals_string``, - and the numeric thresholds ``minimum_value`` / ``maximum_value`` - (inclusive bounds, per the LinkML metamodel). + Supported operators, which **combine** on a single condition (so a + bounded range ``{minimum_value: X, maximum_value: Y}`` emits both + bounds): ``value_presence: PRESENT``, ``equals_string``, and the numeric + thresholds ``minimum_value`` / ``maximum_value`` (inclusive, per the + LinkML metamodel). A ``range_expression`` with inner ``slot_conditions`` + reaches one hop into an inlined child object. """ lines: list[str] = [] for i, (slot_name, cond) in enumerate(pre_slots.items()): path = self._slot_uri(sv, slot_name, cls) var = f"?pre{i}" - if getattr(cond, "value_presence", None) == PresenceEnum(PresenceEnum.PRESENT): - lines.append(f"$this <{path}> {var} .") - elif getattr(cond, "equals_string", None) is not None: - ref = self._resolve_enum_value_ref(sv, slot_name, cond.equals_string) - lines.append(f"$this <{path}> {var} .") - lines.append(f"FILTER ( {var} = {ref} )") - elif getattr(cond, "maximum_value", None) is not None: - lines.append(f"$this <{path}> {var} .") - lines.append(f"FILTER ( {var} <= {self._sparql_number(cond.maximum_value)} )") - elif getattr(cond, "minimum_value", None) is not None: - lines.append(f"$this <{path}> {var} .") - lines.append(f"FILTER ( {var} >= {self._sparql_number(cond.minimum_value)} )") - elif getattr(cond, "range_expression", None) is not None and getattr( - cond.range_expression, "slot_conditions", None - ): + range_expr = getattr(cond, "range_expression", None) + if range_expr is not None and getattr(range_expr, "slot_conditions", None): # One-hop into an inlined child object: bind the child node and # apply the inner slot conditions to it. node = f"{var}_node" lines.append(f"$this <{path}> {node} .") - inner = self._member_conditions(sv, cls, slot_name, node, cond.range_expression.slot_conditions) + inner = self._member_conditions(sv, cls, slot_name, node, range_expr.slot_conditions) if inner is None: return None lines.extend(inner) - else: + continue + filters = self._scalar_filters( + var, cond, lambda v, sn=slot_name: self._resolve_enum_value_ref(sv, sn, v, cls) + ) + if filters is None: return None + lines.append(f"$this <{path}> {var} .") + lines.extend(filters) return lines def _member_conditions( @@ -650,27 +679,23 @@ class of *container_slot_name* — by a set of inner slot conditions. Shared by the nested ``range_expression`` precondition (single inlined child) and the ``has_member`` postcondition (a list member). Enum - values are resolved against the container slot's range class. Returns - ``None`` for unsupported inner operators. + values are resolved against the container slot's range class, and (like + preconditions) combining operators on one condition emits all of them. + Returns ``None`` for unsupported inner operators. """ lines: list[str] = [] for j, (inner_name, icond) in enumerate(slot_conditions.items()): ipath = self._slot_uri(sv, inner_name, cls) ivar = f"{node_var}_{j}" - if getattr(icond, "value_presence", None) == PresenceEnum(PresenceEnum.PRESENT): - lines.append(f"{node_var} <{ipath}> {ivar} .") - elif getattr(icond, "equals_string", None) is not None: - iref = self._resolve_member_enum_ref(sv, container_slot_name, inner_name, icond.equals_string) - lines.append(f"{node_var} <{ipath}> {ivar} .") - lines.append(f"FILTER ( {ivar} = {iref} )") - elif getattr(icond, "maximum_value", None) is not None: - lines.append(f"{node_var} <{ipath}> {ivar} .") - lines.append(f"FILTER ( {ivar} <= {self._sparql_number(icond.maximum_value)} )") - elif getattr(icond, "minimum_value", None) is not None: - lines.append(f"{node_var} <{ipath}> {ivar} .") - lines.append(f"FILTER ( {ivar} >= {self._sparql_number(icond.minimum_value)} )") - else: + filters = self._scalar_filters( + ivar, + icond, + lambda v, inm=inner_name: self._resolve_member_enum_ref(sv, container_slot_name, inm, v), + ) + if filters is None: return None + lines.append(f"{node_var} <{ipath}> {ivar} .") + lines.extend(filters) return lines def _resolve_member_enum_ref(self, sv, container_slot_name: str, inner_slot_name: str, value_name: str) -> str: @@ -706,6 +731,27 @@ def _sparql_number(value) -> str: """ return str(value) + @staticmethod + def _sparql_string_literal(value: str) -> str: + """Render *value* as a double-quoted SPARQL string literal, escaping the + characters the grammar forbids raw. + + ``equals_string`` / permissible-value names are schema-controlled but + may legitimately contain a double quote, backslash, or newline; without + escaping these would break the ``sh:select`` query (or allow SPARQL + injection). See `SPARQL 1.1 §19.7 escape sequences + `_. + """ + escaped = ( + str(value) + .replace("\\", "\\\\") + .replace('"', '\\"') + .replace("\n", "\\n") + .replace("\r", "\\r") + .replace("\t", "\\t") + ) + return f'"{escaped}"' + def _postcondition_violation(self, sv, cls: ClassDefinition, slot_name: str, cond) -> list[str] | None: """Translate a single postcondition slot condition into SPARQL that matches a *violation* of it. @@ -793,7 +839,7 @@ def _build_presence_implies_value_sparql( """ value_uri = self._slot_uri(sv, value_slot_name, cls) target_uri = self._slot_uri(sv, target_slot_name, cls) - refs = ", ".join(self._resolve_enum_value_ref(sv, target_slot_name, v) for v in allowed_values) + refs = ", ".join(self._resolve_enum_value_ref(sv, target_slot_name, v, cls) for v in allowed_values) return ( f"SELECT $this WHERE {{\n" @@ -830,7 +876,7 @@ def _build_exclusive_value_sparql( ``$this`` is pre-bound to each focus node. """ slot_uri = self._slot_uri(sv, slot_name, cls) - value_ref = self._resolve_enum_value_ref(sv, slot_name, value_name) + value_ref = self._resolve_enum_value_ref(sv, slot_name, value_name, cls) if max_card == 1: return ( @@ -853,15 +899,23 @@ def _build_exclusive_value_sparql( f"}}" ) - def _resolve_enum_value_ref(self, sv, slot_name: str, value_name: str) -> str: + def _resolve_enum_value_ref(self, sv, slot_name: str, value_name: str, cls: ClassDefinition | None = None) -> str: """Resolve an enum value name to a SPARQL term (IRI or literal). Looks up the slot's range as an enum, finds the permissible value matching *value_name*, and returns its ``meaning`` as a full IRI - wrapped in angle brackets. Falls back to a quoted literal if the - slot is not an enum or the value lacks a ``meaning``. + wrapped in angle brackets. Falls back to an escaped quoted literal if + the slot is not an enum or the value lacks a ``meaning``. + + When *cls* is given and the slot is declared on it, the slot is resolved + in the class's induced context, so a range narrowed via ``slot_usage`` + (a class-specific enum) selects the correct permissible values instead + of the base slot's enum. """ - slot = sv.get_slot(slot_name) + if cls is not None and slot_name in sv.class_slots(cls.name): + slot = sv.induced_slot(slot_name, cls.name) + else: + slot = sv.get_slot(slot_name) if slot: range_name = slot.range if range_name and range_name in sv.all_enums(): @@ -870,17 +924,22 @@ def _resolve_enum_value_ref(self, sv, slot_name: str, value_name: str) -> str: if pv and pv.meaning: iri = sv.expand_curie(pv.meaning) return f"<{iri}>" - return f'"{value_name}"' + return self._sparql_string_literal(value_name) def _slot_uri(self, sv, slot_name: str, cls: ClassDefinition) -> str: """Resolve a slot name to a full IRI string for use in SPARQL queries. - Mirrors the resolution logic used for ``sh:path`` in the main slot loop: - prefer ``sv.get_uri()`` for slots registered in the schema map, fall - back to ``default_prefix:underscored_name``. + Mirrors the resolution logic used for ``sh:path`` in the main slot loop, + including the **induced** (class-specific) slot: a ``slot_usage`` + override of ``slot_uri`` must yield the same IRI as ``sh:path``. + Otherwise the SPARQL body would query a property the data never uses and + the constraint would silently never fire (a false negative). """ - slot = sv.get_slot(slot_name) - if slot and slot_name in sv.element_by_schema_map(): + if slot_name in sv.class_slots(cls.name): + slot = sv.induced_slot(slot_name, cls.name) + else: + slot = sv.get_slot(slot_name) + if slot and slot.name in sv.element_by_schema_map(): return sv.get_uri(slot, expand=True) pfx = sv.schema.default_prefix return sv.expand_curie(f"{pfx}:{underscore(slot_name)}") diff --git a/tests/linkml/test_generators/test_shaclgen.py b/tests/linkml/test_generators/test_shaclgen.py index 5e7249ab43..b0551ecc5e 100644 --- a/tests/linkml/test_generators/test_shaclgen.py +++ b/tests/linkml/test_generators/test_shaclgen.py @@ -3494,3 +3494,334 @@ def test_has_member_pyshacl_end_to_end(): advanced=True, ) assert not conforms, f"Fog without a front-fog-light group should fail:\n{txt}" + + +# =========================================================================== +# Rule-converter robustness regressions (review hardening) +# +# These guard three defects found while reviewing the rule converters: +# 1. A single precondition combining minimum_value + maximum_value dropped +# all but the first bound (silent under-constraint / false positives). +# 2. A slot_usage `slot_uri` (or enum `range`) override made the SPARQL body +# query the *base* IRI while `sh:path` used the *induced* IRI, so the +# constraint silently never fired (false negative). +# 3. An `equals_string` value containing a quote/backslash produced invalid, +# unparsable SPARQL (broken artifact / injection). +# =========================================================================== + +_COMBINED_BOUNDS_SCHEMA_YAML = """ +id: https://example.org/combined-bounds +name: combined_bounds_rules +prefixes: + linkml: https://w3id.org/linkml/ + ex: https://example.org/combined-bounds/ +imports: + - linkml:types +default_prefix: ex +default_range: string + +slots: + reading_value: + range: integer + slot_uri: ex:reading_value + reading_note: + range: string + slot_uri: ex:reading_note + +classes: + Reading: + class_uri: ex:Reading + slots: + - reading_value + - reading_note + rules: + - description: A mid-range reading requires an explanatory note. + preconditions: + slot_conditions: + reading_value: + minimum_value: 10 + maximum_value: 20 + postconditions: + slot_conditions: + reading_note: + required: true +""" + +EX_CB = rdflib.Namespace("https://example.org/combined-bounds/") + + +def test_rule_precondition_combines_min_and_max_bounds(): + """A precondition with both minimum_value and maximum_value must emit both + bounds; the pre-fix first-match dispatch kept only the maximum.""" + g = _parse_shacl(_COMBINED_BOUNDS_SCHEMA_YAML) + + nodes = list(g.objects(EX_CB.Reading, SH.sparql)) + assert len(nodes) == 1, f"Expected 1 sh:sparql constraint, got {len(nodes)}" + query = str(list(g.objects(nodes[0], SH.select))[0]) + assert ">= 10" in query, f"lower bound must be emitted, got:\n{query}" + assert "<= 20" in query, f"upper bound must be emitted, got:\n{query}" + + +def test_rule_combined_bounds_pyshacl_end_to_end(): + """End-to-end: only values inside [10, 20] trigger the required note. + + The below-threshold case is the key assertion — without the lower bound it + would be flagged as a violation.""" + import pyshacl + + shacl_ttl = ShaclGenerator(_COMBINED_BOUNDS_SCHEMA_YAML, mergeimports=False, emit_rules=True).serialize() + + conforming = """ + @prefix ex: . + @prefix xsd: . + + ex:mid a ex:Reading ; ex:reading_value 15 ; ex:reading_note "in range" . + ex:low a ex:Reading ; ex:reading_value 5 . + ex:high a ex:Reading ; ex:reading_value 25 . + """ + conforms, _, txt = pyshacl.validate( + data_graph=conforming, + shacl_graph=shacl_ttl, + data_graph_format="turtle", + shacl_graph_format="turtle", + advanced=True, + ) + assert conforms, f"Out-of-range readings must not require a note:\n{txt}" + + violating = """ + @prefix ex: . + @prefix xsd: . + + ex:bad a ex:Reading ; ex:reading_value 15 . + """ + conforms, _, txt = pyshacl.validate( + data_graph=violating, + shacl_graph=shacl_ttl, + data_graph_format="turtle", + shacl_graph_format="turtle", + advanced=True, + ) + assert not conforms, f"A mid-range reading without a note must fail:\n{txt}" + + +_SLOT_URI_OVERRIDE_SCHEMA_YAML = """ +id: https://example.org/slot-uri-override +name: slot_uri_override_rules +prefixes: + linkml: https://w3id.org/linkml/ + ex: https://example.org/slot-uri-override/ +imports: + - linkml:types +default_prefix: ex +default_range: string + +slots: + trigger: + range: string + slot_uri: ex:GLOBAL_trigger + dependent: + range: string + slot_uri: ex:GLOBAL_dependent + +classes: + Scene: + class_uri: ex:Scene + slots: + - trigger + - dependent + slot_usage: + trigger: + slot_uri: ex:LOCAL_trigger + dependent: + slot_uri: ex:LOCAL_dependent + rules: + - description: If the trigger is present the dependent slot is required. + preconditions: + slot_conditions: + trigger: + value_presence: PRESENT + postconditions: + slot_conditions: + dependent: + required: true +""" + +EX_OVR = rdflib.Namespace("https://example.org/slot-uri-override/") + + +def test_rule_slot_uri_override_matches_sh_path(): + """The SPARQL body must use the same induced (class-local) IRIs as sh:path. + + A slot_usage slot_uri override changes sh:path; if the SPARQL keeps the base + IRI the query targets a property the data never uses and never fires.""" + g = _parse_shacl(_SLOT_URI_OVERRIDE_SCHEMA_YAML) + + paths = {str(o) for o in g.objects(None, SH.path)} + assert str(EX_OVR.LOCAL_trigger) in paths + assert str(EX_OVR.LOCAL_dependent) in paths + + nodes = list(g.objects(EX_OVR.Scene, SH.sparql)) + assert len(nodes) == 1 + query = str(list(g.objects(nodes[0], SH.select))[0]) + assert str(EX_OVR.LOCAL_trigger) in query, f"SPARQL must use the induced IRI, got:\n{query}" + assert str(EX_OVR.LOCAL_dependent) in query, f"SPARQL must use the induced IRI, got:\n{query}" + assert "GLOBAL_" not in query, f"SPARQL must not fall back to the base slot_uri, got:\n{query}" + + +def test_rule_slot_uri_override_pyshacl_end_to_end(): + """End-to-end: the constraint actually fires on data that uses the induced + (LOCAL) IRIs. Before the fix the SPARQL queried the base IRIs, so a missing + dependent slot slipped through as conforming.""" + import pyshacl + + shacl_ttl = ShaclGenerator(_SLOT_URI_OVERRIDE_SCHEMA_YAML, mergeimports=False, emit_rules=True).serialize() + + conforming = """ + @prefix ex: . + + ex:ok a ex:Scene ; ex:LOCAL_trigger "t" ; ex:LOCAL_dependent "d" . + ex:noTrigger a ex:Scene ; ex:LOCAL_dependent "d" . + """ + conforms, _, txt = pyshacl.validate( + data_graph=conforming, + shacl_graph=shacl_ttl, + data_graph_format="turtle", + shacl_graph_format="turtle", + advanced=True, + ) + assert conforms, f"Trigger-with-dependent (and no-trigger) must pass:\n{txt}" + + violating = """ + @prefix ex: . + + ex:bad a ex:Scene ; ex:LOCAL_trigger "t" . + """ + conforms, _, txt = pyshacl.validate( + data_graph=violating, + shacl_graph=shacl_ttl, + data_graph_format="turtle", + shacl_graph_format="turtle", + advanced=True, + ) + assert not conforms, f"Trigger present without the required dependent must fail:\n{txt}" + + +_ENUM_NARROWING_SCHEMA_YAML = """ +id: https://example.org/enum-narrowing +name: enum_narrowing_rules +prefixes: + linkml: https://w3id.org/linkml/ + ex: https://example.org/enum-narrowing/ +imports: + - linkml:types +default_prefix: ex +default_range: string + +enums: + BaseMode: + permissible_values: + Active: + meaning: ex:GLOBAL_Active + SceneMode: + permissible_values: + Active: + meaning: ex:LOCAL_Active + +slots: + activator: + range: string + slot_uri: ex:activator + mode: + range: BaseMode + slot_uri: ex:mode + +classes: + Scene: + class_uri: ex:Scene + slots: + - activator + - mode + slot_usage: + mode: + range: SceneMode + rules: + - description: If an activator is present the mode must be Active. + preconditions: + slot_conditions: + activator: + value_presence: PRESENT + postconditions: + slot_conditions: + mode: + equals_string: Active +""" + +EX_EN = rdflib.Namespace("https://example.org/enum-narrowing/") + + +def test_rule_enum_range_narrowed_by_slot_usage(): + """A slot_usage range override to a class-specific enum must resolve the + value's meaning against the induced (narrowed) enum, not the base range.""" + g = _parse_shacl(_ENUM_NARROWING_SCHEMA_YAML) + + nodes = list(g.objects(EX_EN.Scene, SH.sparql)) + assert len(nodes) == 1 + query = str(list(g.objects(nodes[0], SH.select))[0]) + assert str(EX_EN.LOCAL_Active) in query, f"must resolve the narrowed enum meaning, got:\n{query}" + assert "GLOBAL_Active" not in query, f"must not resolve the base enum meaning, got:\n{query}" + + +_ESCAPING_SCHEMA_YAML = """ +id: https://example.org/escaping +name: escaping_rules +prefixes: + linkml: https://w3id.org/linkml/ + ex: https://example.org/escaping/ +imports: + - linkml:types +default_prefix: ex +default_range: string + +slots: + trigger: + range: string + slot_uri: ex:trigger + label: + range: string + slot_uri: ex:label + +classes: + Item: + class_uri: ex:Item + slots: + - trigger + - label + rules: + - description: If a trigger is present the label must equal the quoted marker. + preconditions: + slot_conditions: + trigger: + value_presence: PRESENT + postconditions: + slot_conditions: + label: + equals_string: 'a"b\\\\c' +""" + +EX_ESC = rdflib.Namespace("https://example.org/escaping/") + + +def test_rule_equals_string_special_chars_escaped(): + """An equals_string value with a quote and backslash must be escaped so the + generated SPARQL stays syntactically valid (no injection / broken query).""" + from rdflib.plugins.sparql import prepareQuery + + g = _parse_shacl(_ESCAPING_SCHEMA_YAML) + nodes = list(g.objects(EX_ESC.Item, SH.sparql)) + assert len(nodes) == 1 + query = str(list(g.objects(nodes[0], SH.select))[0]) + + # Would raise ParseException on the unescaped `... = "a"b\c"` form. + prepareQuery(query) + assert '\\"' in query, f"double quote must be escaped, got:\n{query}" + assert "\\\\" in query, f"backslash must be escaped, got:\n{query}"