Skip to content

fix(gen-shacl): correct rule-converter path parity, combined bounds, and SPARQL escaping#21

Open
jdsika wants to merge 1 commit into
feat/shaclgen-compositional-rule-fallbackfrom
fix/shaclgen-rule-converter-hardening
Open

fix(gen-shacl): correct rule-converter path parity, combined bounds, and SPARQL escaping#21
jdsika wants to merge 1 commit into
feat/shaclgen-compositional-rule-fallbackfrom
fix/shaclgen-rule-converter-hardening

Conversation

@jdsika

@jdsika jdsika commented Jul 10, 2026

Copy link
Copy Markdown

Summary

Review-hardening pass over the rules → SHACL-SPARQL converters. Three latent
defects, each with regression tests that fail before the fix (re-verified
on this stack: running the new tests against the pre-fix generator source
yields 5 failures):

  1. Induced-slot parity (silent false negative). _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 SPARQL body
    query a property / enum IRI the data never uses, while sh:path used the
    induced IRI. The constraint then silently never fired. Both now resolve
    the induced slot for the class, matching the sh:path logic in the main
    slot loop. Affected code originates in feat(gen-shacl): generate sh:sparql constraints from LinkML rules #11 (framework) — this fix must be
    folded into the framework commit when submitting upstream.

  2. Combined operators (silent under-constraint / false positives). A
    single precondition or member condition dispatched on the first matching
    operator, so a bounded range {minimum_value: X, maximum_value: Y} dropped
    the lower bound. A shared _scalar_filters helper now emits every
    recognised operator (and still returns None — skip, never mis-translate —
    when none is recognised). Affected code originates in the compositional
    fallback (base PR of this one).

  3. SPARQL string escaping (invalid / injectable output). An
    equals_string or permissible-value name containing a ", \, or newline
    produced unparseable SPARQL. New _sparql_string_literal escapes per
    SPARQL 1.1 §19.7.
    Affects every converter that renders string terms (framework,
    presence-implies-value, fallback).

Stack position

Base branch: feat/shaclgen-compositional-rule-fallback — only the hardening
delta shows in the diff.

main
└─ feat/shaclgen-rules-sparql                      #11 (green)
   └─ feat/shaclgen-presence-implies-value-stacked   (PR: presence-implies-value)
      └─ feat/shaclgen-compositional-rule-fallback   (PR: fallback M1–M5)
         └─ fix/shaclgen-rule-converter-hardening   ← this PR

Kept as a separate commit (rather than folded into the feature branches) so
the defects, their failure modes, and the regression tests remain visible to
review. When upstreaming to linkml/linkml, fold each fix into the submission
that introduces the affected code, as noted per defect above.

Provenance

Cherry-pick of e263def7 from feat/shacl-rule-converters (#18), where the
full CI matrix is green.

How was this tested?

  • tests/linkml/test_generators/test_shaclgen.py on this branch:
    106 passed (base suites + 6 regression tests: structural, prepareQuery
    syntax, and pyshacl end-to-end — including the below-threshold case proving
    the lower bound is enforced, and an end-to-end proof that a
    slot_usage-overridden constraint actually fires).
  • Negative control: with this branch's tests but the pre-fix generator
    source, the regression tests fail (5 failures) — the tests bite.
  • ruff check and ruff format --check clean on both changed files.

Areas of uncertainty

Checklist

  • My code follows the contributor guidelines
  • I have added tests that prove my fix/feature works
  • Existing tests pass locally with my changes

AI Assistance

If you used AI tools while preparing this PR, you are still the author and responsible for understanding, verifying, and defending your submission. Please engage with reviewers personally rather than through your agent during feedback and revisions. See our AI Covenant for details.

@jdsika

jdsika commented Jul 10, 2026

Copy link
Copy Markdown
Author

Adversarial audit of the hardening fixes (probed at this tip)

Fix claims that held under attack: top-level induced-slot parity (sh:path and SPARQL body byte-agree for slot_usage-overridden slot_uri, incl. attributes — class_induced_slots never mangles names, so the element_by_schema_map membership test matches the main slot loop); combined {minimum_value, maximum_value} emits both bounds; quote/backslash/newline escaping produces parseable queries (\b/\f are legal raw in SPARQL STRING_LITERAL2, so leaving them unescaped is fine).

C1 — real bug: the parity fix stops one hop short, and the new class_slots check regresses the name-collision case.
Nested conditions (range_expression, has_member members) still resolve the inner slot via _slot_uri(sv, inner_name, cls) with the outer class:

  • Without collision: a child class narrowing an inner slot's slot_uri via its own slot_usage gets sh:path ≠ SPARQL body (the claimed defect class, one hop deeper).
  • With collision (regression): when the inner slot name also exists on the outer class with a different slot_usage URI, the new if slot_name in sv.class_slots(cls.name): induced_slot(...) makes the outer induced slot win. The pre-fix code (plain get_slot) emitted the correct child URI in this case; the post-fix code emits the parent's. For has_member the wrong predicate makes FILTER NOT EXISTS vacuously true → false positives on conforming data (demonstrated with pyshacl).
    Same family: _resolve_member_enum_ref uses non-induced sv.get_slot(container_slot_name), so container-slot range narrowing resolves the wrong enum. See also the matching finding on the base PR (fallback), where the fix belongs.

C2 — edge case: _scalar_filters drops value_presence: ABSENT when combined with another operator.
Pure ABSENT preconditions are correctly rejected (verified — no inverted-semantics constraint is emitted). But {value_presence: ABSENT, minimum_value: 5} translates as if the slot must be present with the bound — the opposite of the declared trigger. Only reachable via a self-contradictory condition, but it breaches this PR's own "skip, never mis-translate" contract. Fix: return None when value_presence is set to anything other than PRESENT (same hole in _member_conditions inner conditions).

C3 — real bug (pre-existing; this PR's new docstring mis-states it): _sparql_number passes non-numeric values raw.
The docstring added here claims LinkML parses minimum_value/maximum_value as int/float — the metamodel range is Anything. A YAML date bound 2020-01-01 emits FILTER ( ?pre0 >= 2020-01-01 ), which parses as the arithmetic expression 2020−01−01 = 2018 (silent false negative); "abc"/.nan produce unparseable sh:select that makes pyshacl raise on the entire shapes graph. Gate on real numerics and skip otherwise.

C4 — cosmetic: alias-form rule keys silently diverge. A rule key written my_slot for a slot named my slot misses class_slots/get_slot and falls back to default_prefix:my_slot while sh:path uses the overridden URI — a vacuous constraint with no warning. linkml-runtime's induced_slot normalizes aliases via slot_name_mappings(); here it diverges silently instead of skipping/warning.

Also verified: malformed meaning CURIEs fail loudly at generation (no injection path); remaining interpolation sites (rule description via rdflib.Literal, int-cast cardinalities, get_uri/expand_curie IRIs) are safe.

Suggest a follow-up commit on this stack for C1–C3 before upstreaming (C1's fix best lands where the nested emitters live, i.e. the fallback commit, when folding for upstream submission).

@jdsika

jdsika commented Jul 11, 2026

Copy link
Copy Markdown
Author

The substantive audit findings above are resolved in #22 (fix/shaclgen-rule-converter-audit-findings, stacked on this series) — one commit, 19 regression tests, 18 of which fail on the pre-fix source. See the finding→fix table in the #22 description.

…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 <carlo.van-driesten@vdl.digital>
Signed-off-by: jdsika <carlo.van-driesten@vdl.digital>
(cherry picked from commit e263def)
@jdsika jdsika force-pushed the fix/shaclgen-rule-converter-hardening branch from 15c4d0d to 4b2ef26 Compare July 11, 2026 10:38
@jdsika jdsika force-pushed the feat/shaclgen-compositional-rule-fallback branch from 8442eb8 to 1ea1ea2 Compare July 11, 2026 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant