Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3989 +/- ##
==========================================
+ Coverage 81.62% 84.96% +3.33%
==========================================
Files 162 162
Lines 20518 20634 +116
Branches 4366 4388 +22
==========================================
+ Hits 16748 17531 +783
+ Misses 2876 2169 -707
- Partials 894 934 +40
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7622d70 to
bae4a43
Compare
kevinschaper
left a comment
There was a problem hiding this comment.
Hi @jdsika, there are a couple of inline comments below with things that should be quick and easy to address
| # Semantics: "If the value slot is present, the target slot must be | ||
| # present and hold one of the allowed values." Generalises the | ||
| # boolean guard (equals_string "true") to arbitrary enum values. | ||
| if is_value_present and post_ops in ({"equals_string"}, {"equals_string_in"}): |
There was a problem hiding this comment.
Nice job gating the boolean guard on range == "boolean", but the same kind of bug now shows up here. Presence-implies-value accepts any target range and always compares against a plain string literal, so a typed target never matches. I checked this with pyshacl on this branch:
attributes:
reading: {range: float}
flag: {range: boolean}
count: {range: integer}
rules:
- preconditions: {slot_conditions: {reading: {value_presence: PRESENT}}}
postconditions: {slot_conditions: {flag: {equals_string: "false"}}}
- preconditions: {slot_conditions: {reading: {value_presence: PRESENT}}}
postconditions: {slot_conditions: {count: {equals_string: "3"}}}With the data ex:t1 ex:reading "1.5"^^xsd:float ; ex:flag false ; ex:count 3 . the result is Conforms: False with two violations, because the generated filters are ?target NOT IN ("false") and ?target NOT IN ("3").
To match the PR's "skip, never mis-translate" contract, could this pattern be limited to targets whose range is an enum or a string-based type, and skip everything else? Something like:
target_range = getattr(self._rule_slot(sv, post_slot_name, cls), "range", None)
is_string_like = target_range in sv.all_enums() or (
target_range in sv.all_types() and sv.induced_type(target_range).base == "str"
)
if is_value_present and is_string_like and post_ops in ({"equals_string"}, {"equals_string_in"}):The alternative is to emit typed literals like "3"^^xsd:integer, but that breaks when the data uses a different lexical form (e.g. "03"), so skipping seems safer. A parametrized pyshacl test over boolean "false" and integer "3" would lock this in. It could assert either that the rule is skipped or that conforming data passes.
There was a problem hiding this comment.
Edited for 5c044e7.
Thanks, confirmed and fixed in 5c044e7. Both patterns now share one range check, _value_terms:
- enum or
xsd:stringtype: values are compared as strings (enum values as theirmeaningIRI); xsd:boolean: each value must be a lexical form (true/false/1/0) and is compared by value, so yourboolean "false"case conforms;- anything else, such as your
integer "3": the rule is skipped with a warning.
I check the datatype IRI rather than base == "str". base would also admit date_or_datetime, and typeof: string types with uri: xsd:anyURI or xsd:token, whose values are typed literals or IRIs. Parametrized pyshacl tests cover these ranges.
Related: comparisons now use = instead of NOT IN, because rdflib's IN uses term identity and missed "x"^^xsd:string. My rdflib fix for that, RDFLib/rdflib#3564, is not yet reviewed. It wouldn't have helped your case anyway, since 3 and "3" never compare equal.
| sv, cls, pre_slot_name, pre_cond.equals_string, int(post_cond.maximum_cardinality) | ||
| ) | ||
|
|
||
| # Fallback: a small compositional builder for operator combinations not |
There was a problem hiding this comment.
This looks like a leftover from re-cutting the stack. The comment stops mid-sentence and refers to the compositional builder that only arrives in #3990.
| # Fallback: a small compositional builder for operator combinations not |
There was a problem hiding this comment.
Removed, thanks. I also removed five empty section headers in the tests that belonged to #3990.
| # makes the rule untranslatable — the converters must SKIP such a rule | ||
| # rather than emit a query that silently drops a conjunct (which would | ||
| # widen the trigger or narrow the check: a mis-translation, not a skip). | ||
| _NON_OPERATOR_FIELDS = frozenset( |
There was a problem hiding this comment.
The hand-maintained _NON_OPERATOR_FIELDS list will quietly drift as the metamodel grows. It fails safe, since a new metadata field only causes a rule to be skipped, but it can be derived instead. It's exactly "the slots every element has, minus the slots that make up a slot_expression". owlgen already loads the metamodel this way (package_schemaview, which is lru_cached):
from linkml_runtime.utils.formatutils import underscore
from linkml_runtime.utils.introspection import package_schemaview
_METAMODEL = package_schemaview("linkml_runtime.linkml_model.meta")
# Metadata every element carries, minus anything that is a slot-expression operator.
_NON_OPERATOR_FIELDS = frozenset(underscore(s) for s in _METAMODEL.class_slots("element")) - frozenset(
underscore(s) for s in _METAMODEL.class_slots("slot_expression")
)I checked it against this branch: it produces the same 43 names as the current literal. underscore() is needed because the metamodel spells a few slots with spaces (exact mappings, …) while the Python attributes use underscores. If you'd rather not load the metamodel at import time, the same expression can sit behind a functools.cached classmethod.
There was a problem hiding this comment.
Derived now. I used the generated metamodel dataclasses, fields(Element) - fields(SlotExpression), which gives the same 43 names without loading the metamodel schema at import time. test_rule_non_operator_fields_are_the_metamodel_element_metadata cross-checks this against your package_schemaview derivation, so the two can't drift apart.
2f4ab75 to
1bf1dc9
Compare
1bf1dc9 to
66276ea
Compare
|
Edited for 5c044e7, the current head. Corrections to the first version are at the end. Thanks @kevinschaper. All three comments are addressed in 5c044e7, with replies inline. The other fixes I found while checking against JSON Schema are in the PR description. Questions for maintainers
Positions (please object if you disagree):
Merge order:
Gaps in other generators are tracked in ASCS-eV#42 (jsonschemagen), ASCS-eV#43 (owlgen) and ASCS-eV#44 (sqlvalidationgen). Corrections to the first version
|
66276ea to
1e506c2
Compare
0bc284c to
5c044e7
Compare
5c044e7 to
c7dd738
Compare
The rules-to-SHACL-SPARQL converter recognised two named patterns. This
adds the presence-implies-value pattern: a precondition asserting
`value_presence: PRESENT` on one slot, and a postcondition constraining
another slot with `equals_string` or `equals_string_in`. It reads as
"if the guard slot is present, the target slot must be present and hold
one of the allowed values", and generalises the boolean guard to
arbitrary enum values. The boolean guard becomes the case
`equals_string: "true"` of this pattern on an xsd:boolean-typed flag.
`equals_string` / `equals_string_in` compare strings. The metamodel
defines them for slots of range string; enums and types with datatype
xsd:string are treated alike, and a slot without a range takes the
schema's default_range. On an xsd:boolean-typed slot each value must be
a lexical form of xsd:boolean (XML Schema 1.1 Part 2 section 3.3.2.2:
true, false, 1, 0) and denotes that boolean. A rule applying them to
any other range is skipped with a warning: its RDF values are typed
literals or IRIs that equal no string. Built-in type names resolve
without importing linkml:types, as in the main slot loop; the built-in
name of `curie` was misspelt there and is corrected.
Values are compared with `=`, spelled out as the disjunction SPARQL 1.1
section 17.4.1.9 defines `IN` to be, so the RDF 1.1-identical plain and
xsd:string literal forms both match and booleans compare by value;
rdflib evaluates `IN` and triple-pattern constants by term identity.
COALESCE turns the type error that RDFterm-equal raises for
incomparable literals into false, so a spec-conformant engine reports
such a value instead of dropping the row. Queries are DISTINCT and
project ?path (and ?value where there is one), so each result names the
property and the offending value. A value containing a backslash
followed by "u" or "U" is split with CONCAT, since codepoint escapes are
replaced before a query is parsed.
The exclusive-value pattern takes the precondition `has_member:
{equals_string: V}`, which states "V is one of the values". A bare
`equals_string: V` applies to every value of the slot, as the
specification applies a slot constraint to all members of a collection
and as gen-json-schema reads it, so it no longer matches this pattern:
such a rule is skipped with a warning naming has_member. The pattern
gets the same range check and value comparison, and its
maximum_cardinality > 1 form tests HAVING on the COUNT aggregate:
expressions projected by SELECT are not visible to HAVING (SPARQL 1.1
section 18.2.4.2), so `HAVING (?count > N)` never held and the
constraint never fired.
A rule applies to all members of its class, so a class shape now
carries the rules of its ancestors and mixins, as the JSON Schema
generator applies them. Each rule is translated in the inheriting
class's context, where slot_usage may refine the slots it references.
Classes sharing a class_uri share one shape, which carries each rule
once. With `open_world: true` "the postconditions may be omitted in
instance data", so an absent target is no violation.
Skip warnings name the rule by its position and its title, or its
description without one.
A rule whose conditions carry any operator beyond what a pattern
translates, or name an unknown or identifier slot, is skipped rather
than partially translated. Each problem with a rule is logged once as a
warning naming the declaring class, the rule's position and the class
shapes it affects. The metadata fields the operator accounting ignores
are derived from the metamodel. Slot resolution goes through induced
slots, so slot_usage overrides, slot_uri overrides and alias-form keys
resolve to the IRI sh:path emits.
Behaviour changes for existing schemas: an exclusive-value rule with a
bare equals_string precondition is skipped (write has_member:
{equals_string: V}); the boolean guard rejects a string "true" on an
xsd:boolean flag and skips a flag whose type has another datatype IRI
(main compared with str()); exclusive-value rules with
maximum_cardinality > 1 now fire; inherited rules are now enforced on
subclass shapes; problems with rules are logged at WARNING instead of
DEBUG; rule results carry sh:resultPath and sh:value; sh:message follows
the rule's in_language or default_language.
Co-authored-by: jdsika <carlo.van-driesten@vdl.digital>
Signed-off-by: jdsika <carlo.van-driesten@vdl.digital>
c7dd738 to
f5cfa59
Compare
Summary
Translates the rule pattern "if slot A is present, slot B must hold one of these values" into a SHACL-SPARQL constraint (SHACL §5).
With this rule, a
Documentthat has asignatureviolates the constraint when itsstatusis missing or notPublished. The existing boolean guard is the caseequals_string: "true"on a boolean slot.Semantics
equals_string/equals_string_inare translated on string-valued slots (enums,xsd:stringtypes) and onxsd:booleanslots. Booleans accept the lexical formstrue/false/1/0and compare by value. Rules on other ranges are skipped with a warning, because their RDF values are typed literals or IRIs that equal no string.=, which is how SPARQL definesIN(§17.4.1.9). So"x"also matches"x"^^xsd:string, the same term in RDF 1.1; rdflib'sINuses term identity and misses it. The comparison is wrapped inCOALESCE, so strict engines report incomparable values instead of dropping them (§17.4.1.7).open_world: true: the target may be omitted.deactivatedrule is not emitted ("not executed by the rules engine", metamodel); abidirectionalrule is skipped with a warning; of a rule withelseconditionsthe if/then direction is emitted and a warning says the else branch is not enforced.rulestosh:rule, the inference rules of SHACL Advanced Features, which add triples rather than report violations. A rule is read here as a constraint, as gen-json-schema reads it asif/then.sh:resultPathandsh:value(SHACL §5.3.2).Changes to the existing patterns (#3451)
maximum_cardinality > 1never fired, becauseHAVINGcan't see SELECT aliases (§18.2.4.2). Fixed.has_member: {equals_string: V}, which states "V is one of the values". A bareequals_string: Vapplies to every value of the slot, as05validation.mdmatches a slot condition against all members and as gen-json-schema reads it. Such a rule is now skipped with a warning naminghas_member, where main read it as "one of the values". This answers question 3 in the comments; schemas using the bare form should switch tohas_member.xsd:booleanranges, compares by value, and honoursopen_world.Testing
test_shaclgen.pyandtest_shacl_validation_plugin.pycover each pattern end to end with pyshacl. Cross-generator tests validate the same instances with the generated JSON Schema and, through the generated JSON-LD context, with the generated SHACL; the verdicts must agree. This coversxsd:string-coerced data, subclasses andopen_world.Limitations
equals_stringon IRI-valued ranges (uriorcurie, class references) is skipped (gen-shacl rules: translate equals_string on IRI- and typed-literal-valued ranges (follow-up to linkml/linkml#3989) ASCS-eV/linkml#37).deactivatedrules (gen-json-schema rules: deactivated rules enforced; cardinality, has_member and all_members dropped in rule conditions; class-level multivalued ignored ASCS-eV/linkml#42); gen-shacl follows the metamodel here.sh:instill misses"x"^^xsd:stringunder pyshacl. This comes from rdflib's literal identity (Literal equality fails for datatypesNoneandXSD.stringRDFLib/rdflib#2123) and is unchanged here.Closes ASCS-eV#28 (
NOT INcompared by term), closes ASCS-eV#38 (open_worldon the boolean guard) and closes ASCS-eV#46 (ShaclDataTypenamescurieascuri).Stack: #3989 → #3990 (compositional fallback) → #3991 (docs).