Skip to content

fix(shaclgen): emit sh:maxCount 0 for zero maximum_cardinality - #12

Open
jdsika wants to merge 109 commits into
mainfrom
fix/shaclgen-maxcount-zero
Open

fix(shaclgen): emit sh:maxCount 0 for zero maximum_cardinality#12
jdsika wants to merge 109 commits into
mainfrom
fix/shaclgen-maxcount-zero

Conversation

@jdsika

@jdsika jdsika commented May 2, 2026

Copy link
Copy Markdown

Summary

Fix a Python truthiness bug in the SHACL generator that prevents sh:maxCount 0 and sh:minCount 0 from being emitted when maximum_cardinality: 0 or minimum_cardinality: 0 is set in a LinkML schema.

Problem

In shaclgen.py, the cardinality checks use Python truthiness:

if s.minimum_cardinality:    # 0 is falsy in Python!
    prop_pv_literal(SH.minCount, s.minimum_cardinality)
...
if s.maximum_cardinality:    # 0 is falsy in Python!
    prop_pv_literal(SH.maxCount, s.maximum_cardinality)

Since 0 evaluates as False in Python, setting maximum_cardinality: 0 (which should emit sh:maxCount 0 meaning "this property MUST NOT appear") produces no output at all.

Root Cause

The condition if s.maximum_cardinality: fails when the value is 0 because Python treats 0 as falsy. The correct check is if s.maximum_cardinality is not None: which distinguishes "not set" from "explicitly set to zero".

Fix

Changed both checks to use explicit is not None comparisons:

if s.minimum_cardinality is not None:
    prop_pv_literal(SH.minCount, s.minimum_cardinality)
...
if s.maximum_cardinality is not None:
    prop_pv_literal(SH.maxCount, s.maximum_cardinality)

This matches the pattern already used in the OWL generator (owlgen.py lines 627-640) for the same attributes.

Verification

  • W3C SHACL spec explicitly allows sh:maxCount 0 (means "property must not exist on any conforming node")
  • OWL generator already correctly uses is not None and emits owl:maxCardinality 0
  • docgen.py also uses is not None for the same field (line 693)
  • Added regression test that verifies sh:maxCount 0 appears in generated output

Use Case

This is needed for modeling class hierarchies where subclasses restrict inherited properties. For example, slot_usage with maximum_cardinality: 0 is the idiomatic way in LinkML to express "this inherited slot is not applicable on this subclass" --- but without this fix, the SHACL output silently omits the constraint.

How was this tested?

  • Added ChildWithZeroMaxCard class to tests/linkml/test_generators/input/shaclgen/cardinality.yaml
  • Added test_zero_maximum_cardinality_emits_maxcount regression test to test_shaclgen.py
  • Existing tests continue to pass (non-zero cardinalities unaffected by is not None check)

Note on exact_cardinality

The elif s.exact_cardinality: branches (lines 174, 184) have the same truthiness issue for the value 0. However, exact_cardinality: 0 is semantically degenerate (a list with exactly zero items is the same as a forbidden property) and extremely unlikely in practice. This fix focuses on the common and semantically meaningful case. A follow-up can address exact_cardinality if needed.

Areas of uncertainty

  • See "Note on exact_cardinality" above — aligning exact_cardinality: 0 handling is deliberately out of scope here and may deserve an upstream issue of its own.

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 added a commit that referenced this pull request May 2, 2026
Apply same fix as fix/shaclgen-maxcount-zero branch to develop.
Change truthiness checks to explicit `is not None` comparisons
for minimum_cardinality and maximum_cardinality in SHACL generator.

See: #12
jdsika added a commit that referenced this pull request May 2, 2026
Restore shaclgen.py (accidentally emptied) and apply the
is-not-None fix for minimum/maximum_cardinality checks.

See: #12
@jdsika
jdsika force-pushed the fix/shaclgen-maxcount-zero branch 3 times, most recently from abe3f1c to 4f0020c Compare May 3, 2026 08:35
@jdsika
jdsika force-pushed the fix/shaclgen-maxcount-zero branch 6 times, most recently from ae4b34a to 5544abc Compare May 12, 2026 09:39
@jdsika
jdsika force-pushed the fix/shaclgen-maxcount-zero branch from 5544abc to 69f833a Compare June 9, 2026 15:20
When the object code generator produces the OOField object representing
a field in a class (where the field is itself the representation of a
LinkML slot or a LinkML attribute), it fills the `slot_uri` member by
calling the `SchemaView::get_uri` method and passing it the
slot/attribute's name (rather than the slot/attribute's definition).

This forces the SchemaView to look up for the actual definition from the
specified name, which it may fail to do correctly if the name is not the
name of a globally defined slot but of a locally defined slot (which is
expected; you cannot lookup a locally defined attribute by its name
only).

The fix is to provide SchemaView directly with the correct
SlotDefinition object (which the OOCodeGen already has), dispensing it
from having to look it up.

closes linkml#3677
@jdsika
jdsika force-pushed the fix/shaclgen-maxcount-zero branch from ce9d0f8 to 5657d56 Compare July 3, 2026 08:10
@rmessaou
rmessaou force-pushed the fix/shaclgen-maxcount-zero branch from 5657d56 to b9760a8 Compare July 8, 2026 08:19
Python truthiness (`if s.maximum_cardinality:`) treats 0 as falsy, so
`maximum_cardinality: 0`, `minimum_cardinality: 0` and `exact_cardinality: 0`
emitted no constraint at all. `maximum_cardinality: 0` (SHACL `sh:maxCount 0`,
"property must not appear") is the idiomatic way to suppress an inherited slot
on a subclass via slot_usage, and owlgen already emits `owl:maxCardinality 0`
for it -- so the SHACL and OWL output silently diverged.

Use explicit `is not None` checks for minimum_cardinality, maximum_cardinality
and exact_cardinality, matching the pattern already used in owlgen.py and
docgen.py.

Precedence: an explicit minimum_cardinality wins over the `required` fallback
in the elif cascade, consistent with owlgen.py (which uses the same
`if minimum_cardinality is not None ... elif required` order), so
`required: true` + `minimum_cardinality: 0` yields `sh:minCount 0`. That
combination is a schema-authoring contradiction (the metamodel documents
minimum_cardinality as a multivalued-slot count); the explicit, more specific
constraint is emitted.

Tests cover maximum_cardinality: 0, exact_cardinality: 0, minimum_cardinality: 0,
and the required + minimum_cardinality: 0 precedence case.

Signed-off-by: Carlo van Driesten <[email protected]>
@rmessaou
rmessaou force-pushed the fix/shaclgen-maxcount-zero branch from b9760a8 to c40dfa4 Compare July 8, 2026 08:49
sagehrke and others added 7 commits July 14, 2026 16:17
Added the July 2026 presentation title and link to the project.
The "Audit lockfile for CVEs" step reflects the upstream advisory
database, not the PR diff. When a new advisory lands for an
already-pinned package, every open PR goes red regardless of whether it
touches dependencies.

Guard the audit step with a base-diff check so it only runs on PRs that
change uv.lock or a pyproject.toml. The job still always runs and reports
(no stuck-pending required check), and non-PR events keep auditing so
trunk's signal is intact. The malware sync gate is unchanged.

Closes linkml#3767
Compare the change under test against each event's natural base (PR base,
push's before-sha, merge_group base) and only run the CVE audit when
dependencies actually changed. A pyproject.toml edit always counts; uv.lock
is compared by its resolved (name, version) set via a small tomllib helper,
so non-deterministic lockfile churn with an unchanged resolution is skipped.

This keeps a newly-published upstream advisory from turning unrelated PRs —
and the next innocent merge to main — red. The malware sync gate is unchanged.
The per-change gate deliberately ignores advisories published against
dependencies no PR touched. Cover that case without blocking CI: a weekly
scheduled job audits main's lockfile and keeps a single labelled tracking
issue in sync — opened when vulnerabilities appear, refreshed while they
persist, closed automatically once clean. It never assigns or mentions
anyone, so it stays quiet and stays under the project's control.
Dependabot alerts are enabled on the repo and already cover CVEs on
dependencies no PR touched. A self-hosted rolling-issue audit duplicates
that native detection (and running both double-reports each CVE), so it
isn't worth the standing complexity. Notification noise is better handled
at the Dependabot notification-routing layer than by rebuilding detection.

Reverts the audit-issue job; keeps the per-change scoping and malware gate.
sagehrke and others added 30 commits August 11, 2026 10:54
more white space...
Co-authored-by: Damien Goutte-Gattat <[email protected]>
fix(openapigen): replace raw schema with schemaview provided one
doc: Update the documentation about the Java generator.
Signed-off-by: Silvano Cirujano Cuesta <[email protected]>
Co-authored-by: Corey Cox <[email protected]>
* fix(notebooks): correct the yamlmagic install cell in examples.ipynb

The flags preceded the `install` subcommand, so uv rejected the command
("unexpected argument '--disable-pip-version-check'"), yamlmagic was never
installed, and the following `%reload_ext yamlmagic` raised ModuleNotFoundError.

All notebooks share one venv, so this only surfaced when examples.ipynb
happened to run before a sibling that installs yamlmagic correctly. Ordering
comes from os.listdir, which is filesystem order, so Notebook Tests failed
intermittently on PRs that had nothing to do with notebooks -- and passed the
rest of the time on a dependency it never installed itself.

Verified against a venv with yamlmagic absent: the notebook fails before the
change and passes after, installing yamlmagic itself.

Refs linkml#3879.

* fix(tests): fetch creature schema fixtures from raw.githubusercontent.com

Both remote creature fixtures fetched through github.com/.../raw/..., which
302s to raw.githubusercontent.com. The extra hop through the more aggressively
rate-limited host is where `RemoteDisconnected: Remote end closed connection
without response` kept coming from -- four occurrences today, each taking out
all six parametrisations at once and reddening required test jobs on unrelated
PRs.

Two URLs move: CREATURE_SCHEMA_RAW_URL, used by creature_view_direct_url, and
the mcc prefix in creature_schema_remote.yaml, which is the fetch base for
creature_view_remote's import.

The github.com/.../tree/... URLs elsewhere under mcc/ are deliberately left
alone. Those are schema identifiers and CURIE bases rather than fetch targets,
so rewriting them would change schema identity without fixing anything.

Refs linkml#3421, which stays open: it also covers a biolink.github.io 503 on a
different host that this does not address.
…4.26.0 (linkml#3862)

* fix(ci): retry transient network failures in the link checker
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Corey Cox <[email protected]>
* build(deps): cap open dependabot PRs at 3
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: amc-corey-cox <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Corey Cox <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…pdates

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Corey Cox <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: amc-corey-cox <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Corey Cox <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ml#3894)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.