Skip to content

Validate relation statement targets server-side#1082

Merged
alistair3149 merged 4 commits into
masterfrom
feature/relation-target-validation
Jul 20, 2026
Merged

Validate relation statement targets server-side#1082
alistair3149 merged 4 commits into
masterfrom
feature/relation-target-validation

Conversation

@JeroenDeDauw

@JeroenDeDauw JeroenDeDauw commented Jul 17, 2026

Copy link
Copy Markdown
Member

Fixes #1078

RelationType::validate() only emitted required and RelationProperty::getTargetSchema() had no
consumers, so relation statements went unchecked on write. An API write accepted a relation that
points at a Subject that does not exist, at a Subject whose Schema is not the property's
targetSchema, or that holds multiple targets on a multiple: false property. The subject editor's
picker hides this, but API-driven import does not go through the picker.

This adds three validations, using the severity model of ADR 26 as actually implemented (blocking vs.
Violation::NON_BLOCKING_CODES):

  • single-value-only (error) for a multiple: false relation holding more than one target, emitted
    by RelationType::validate() exactly as SelectType does — no lookups needed.
  • relation-target-schema-mismatch (error, new) when the target Subject exists but its own Schema
    is not the property's targetSchema.
  • relation-target-not-found (warning, new, non-blocking) when the target id does not resolve.
    Deliberately non-blocking: pointing at a not-yet-created Subject is wiki-native red-link behaviour,
    and an import may mint the target later.

The last two live in SubjectValidator (the placement PR #1043 uses), since they need a lookup and
the writer's-schema RelationProperty, which PropertyType::validate() cannot reach. Targets are
resolved through the canonical revision-slot-backed SubjectLookup the read path uses
(getSubjectRepository()), never the secondary Neo4j projection; the target's Schema comes from its
own stored Subject. A missing target short-circuits, so it never also reports a schema mismatch.

Enforcement is unchanged: relation-target-not-found joins Violation::NON_BLOCKING_CODES (the
warning tier as implemented), and the newly-introduced-blocking-violation gate is untouched.

RelationInput already renders server violations generically via neowiki-field-<code> messages, so
the two new codes get i18n/en.json, i18n/qqq.json, and extension.json entries and no new UI
code. docs/api/validation-codes.md documents both codes and notes RelationType as a
single-value-only emitter.

ADR 26 (Draft) is intentionally not edited: its per-Constraint severity model is not fully
implemented, and PR #1043 set the precedent of not treating it as a living code catalog. Folding
these two fixed-severity system codes (mismatch = fixed error, not-found = fixed warning) into
ADR 26 is a follow-up for whoever finalizes it.

A textual conflict with open PR #1043 (which also touches SubjectValidator, the same three test
files, validation-codes.md, and the i18n / extension.json message list) is expected.

AI-authored — Claude Code, Opus 4.8 (max); batch execution of the Relations Phase 1 plan directed by @JeroenDeDauw, from a fixed spec with no redirection; not yet human-reviewed; TDD (red then green), full PHPUnit suite (1801 tests) + make cs (phpcs + phpstan) green locally, psalm clean on changed files, production-revert test-verification done; CI green (PHPCS, PHPStan, PHPUnit).

Review notes (batch pr-review pass)

A fresh-session review pass fixed a blocking enforcement gap (relation-target violations now carry valuePartIndex, 122712b) and verified all three validations end to end (unit + mutation battery + live endpoint). Two architecture-level follow-ups were identified and deliberately left out of this PR pending a decision:

  • The validate path resolves relation targets without page-read authorization, so violation responses can disclose whether an unreadable target exists and what its schema is (the read path gates exactly this per Enforce per-page read permission on Subject, Layout, and RDF read endpoints #1046). Likely fix: thread the read authorizer into target resolution and collapse unreadable targets into relation-target-not-found.
  • Target resolution adds one live lookup per relation target on an anonymous, unrate-limited validate endpoint (no relation cap, no id dedup). Consider a cap, dedup/batching, or a ping limiter before ACL/farm deployments.

Review-pass notes added by Claude Code, Fable 5 (max), summarizing a fresh-session Opus 4.8 review agent's verified findings; not yet human-reviewed.

JeroenDeDauw and others added 3 commits July 18, 2026 00:08
Fixes #1078

`RelationType::validate()` only emitted `required` and `RelationProperty::getTargetSchema()` had no
consumers, so relation statements went unchecked on write. An API write accepted a relation that
points at a Subject that does not exist, at a Subject whose Schema is not the property's
`targetSchema`, or that holds multiple targets on a `multiple: false` property. The subject editor's
picker hides this, but API-driven import does not go through the picker.

This adds three validations, using the severity model of ADR 26 as actually implemented (blocking vs.
`Violation::NON_BLOCKING_CODES`):

- `single-value-only` (`error`) for a `multiple: false` relation holding more than one target, emitted
  by `RelationType::validate()` exactly as `SelectType` does — no lookups needed.
- `relation-target-schema-mismatch` (`error`, new) when the target Subject exists but its own Schema
  is not the property's `targetSchema`.
- `relation-target-not-found` (`warning`, new, non-blocking) when the target id does not resolve.
  Deliberately non-blocking: pointing at a not-yet-created Subject is wiki-native red-link behaviour,
  and an import may mint the target later.

The last two live in `SubjectValidator` (the placement PR #1043 uses), since they need a lookup and
the writer's-schema `RelationProperty`, which `PropertyType::validate()` cannot reach. Targets are
resolved through the canonical revision-slot-backed `SubjectLookup` the read path uses
(`getSubjectRepository()`), never the secondary Neo4j projection; the target's Schema comes from its
own stored Subject. A missing target short-circuits, so it never also reports a schema mismatch.

Enforcement is unchanged: `relation-target-not-found` joins `Violation::NON_BLOCKING_CODES` (the
warning tier as implemented), and the newly-introduced-blocking-violation gate is untouched.

`RelationInput` already renders server violations generically via `neowiki-field-<code>` messages, so
the two new codes get `i18n/en.json`, `i18n/qqq.json`, and `extension.json` entries and no new UI
code. `docs/api/validation-codes.md` documents both codes and notes `RelationType` as a
`single-value-only` emitter.

ADR 26 (Draft) is intentionally not edited: its per-Constraint severity model is not fully
implemented, and PR #1043 set the precedent of not treating it as a living code catalog. Folding
these two fixed-severity system codes (mismatch = fixed `error`, not-found = fixed `warning`) into
ADR 26 is a follow-up for whoever finalizes it.

A textual conflict with open PR #1043 (which also touches `SubjectValidator`, the same three test
files, `validation-codes.md`, and the i18n / extension.json message list) is expected.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The two relation-target violations SubjectValidator emits (relation-target-schema-mismatch and
relation-target-not-found) left valuePartIndex unset. ViolationDiff keys violation identity on
(propertyName, code, valuePartIndex), which the Replace write path uses to block only
newly-introduced violations under $wgNeoWikiEnforceValidation. With no index, two schema-mismatch
violations on the same multi-value relation property share one key, so adding a second
schema-mismatched target when one already exists is not reported as new and slips the enforcement
gate.

Carry the target's index within the RelationValue as valuePartIndex, exactly as SelectType and
UrlType do for their parts, and update docs/api/validation-codes.md (guideline 4) to match.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The foreach key over RelationValue's Relation[] is an array-key to Psalm, which flagged a
MixedArgumentTypeCoercion passing it to the int valuePartIndex parameter. Cast at the call site to
keep the changed file Psalm-clean; PHPStan already inferred int and does not flag the cast as
redundant.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@JeroenDeDauw
JeroenDeDauw marked this pull request as ready for review July 18, 2026 23:28
@alistair3149 alistair3149 self-assigned this Jul 20, 2026
The docblock and validation-codes.md said relation targets are resolved
"never [through] the secondary graph projection". Only half of that holds:
the Schema compared is the target's own writer's-schema read from its
revision slot, but reaching that slot resolves the target id through the
subject -> page index, which lives only in the graph projection. The claim
also contradicted NeoWikiExtension::getPageIdentifiersLookup() and
operations/maintenance.md, which both say the reverse index is Neo4j-only.

Say what actually happens instead, and note the consequence: a target
present in revision slots but missing from the graph reports as not found
until a rebuild, which is the case #1022 leaves behind after an import.
Being non-blocking, that misreport never costs a write.

Also list the two relation checks under "Known limitations", which still
claimed SubjectValidator performs exactly two Subject-level checks.

Add three tests. The existing valuePartIndex test uses two equally invalid
targets, so array position and violation ordinal agree and a counter-based
implementation passes it; the new one puts a valid target first so the two
disagree, and covers the not-found branch that asserted no index at all.
Both fail against a count( $violations ) mutation. The third pins that a
relation property whose JSON omits "multiple" defaults to single-valued,
which now makes a second target a blocking violation.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alistair3149
alistair3149 merged commit 929c375 into master Jul 20, 2026
6 checks passed
@alistair3149
alistair3149 deleted the feature/relation-target-validation branch July 20, 2026 18:11
alistair3149 added a commit that referenced this pull request Jul 20, 2026
Both inputs emit clear-server-violation with valuePartIndex: null, but the
dialogs drop violations by exact index match:

  v.propertyName === payload.propertyName && v.valuePartIndex === payload.valuePartIndex

so 0 === null never matches and a part-indexed violation survives the edit
that fixed it. SelectType already emits invalid-option per part, so the
select case is reachable today; relation targets gain indices in #1082.

Emit one clear per violation the parent is holding, using each violation's
own index. Both fields render a single aggregate error rather than a message
per value part, so clearing only the displayed one would strand the rest
with nothing on screen to explain them, and both reindex their remaining
parts on removal, which would leave a surviving index describing a part it
no longer points at.

Keeping the existing number | null payload rather than widening it to a
wildcard: the union overlaps, so TypeScript would not catch a handler left
unconverted, and the other five inputs would need updating for a change
only these two need.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mint the target later.

Resolution goes through the subject-to-page index, which lives only in the graph projection, so a
target that exists in revision slots but is missing from the graph is reported as not found. An

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alistair3149 does this belong in the docs? Looks like we're now explaining a bunch of implementation details.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can simplify it to A a target that exists in revision slots but is missing from the graph is reported as not found. It is a confusing behavior that should be documented. That will likely be solved with #1040.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm currently in a fierce debate with fable about this text. Its first take is to say the PR is good.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok this took some doing but it now agrees, FWIW

image

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alistair3149 added a commit that referenced this pull request Jul 21, 2026
Both inputs emit clear-server-violation with valuePartIndex: null, but the
dialogs drop violations by exact index match:

  v.propertyName === payload.propertyName && v.valuePartIndex === payload.valuePartIndex

so 0 === null never matches and a part-indexed violation survives the edit
that fixed it. SelectType already emits invalid-option per part, so the
select case is reachable today; relation targets gain indices in #1082.

Emit one clear per violation the parent is holding, using each violation's
own index. Both fields render a single aggregate error rather than a message
per value part, so clearing only the displayed one would strand the rest
with nothing on screen to explain them, and both reindex their remaining
parts on removal, which would leave a surviving index describing a part it
no longer points at.

Keeping the existing number | null payload rather than widening it to a
wildcard: the union overlaps, so TypeScript would not catch a handler left
unconverted, and the other five inputs would need updating for a change
only these two need.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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.

Relation values are not validated server-side: target existence, target schema, and single-value cardinality

2 participants