Skip to content

Commit 929c375

Browse files
JeroenDeDauwclaudealistair3149
authored
Validate relation statement targets server-side (#1082)
* Validate relation statement targets server-side 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> * Set valuePartIndex on relation-target violations 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> * Cast relation index to int for the valuePartIndex argument 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> * Correct the relation-target resolution docs and pin part indices 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> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: alistair3149 <alistair31494322@gmail.com>
1 parent 5d802e6 commit 929c375

13 files changed

Lines changed: 401 additions & 10 deletions

File tree

docs/api/validation-codes.md

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -154,11 +154,12 @@ ColorType).
154154

155155
### `single-value-only`
156156

157-
Emitted by `SelectType`.
157+
Emitted by `SelectType` and `RelationType`.
158158
`args`: `[]`.
159159
`valuePartIndex`: never set (Subject-property-level, not part-level).
160160

161-
The Property's `multiple` flag is false and more than one part was supplied.
161+
The Property's `multiple` flag is false and more than one value was supplied — more than one part for
162+
`SelectType`, or more than one relation target for `RelationType`.
162163

163164
### `invalid-datetime`
164165

@@ -198,13 +199,52 @@ Note the create dry-run endpoint (`POST /subject/validate`) instead returns `404
198199
the update dry-run (`POST /subject/{id}/validate`) and both write endpoints surface the violation.
199200
Reconciling that asymmetry is left to the enforcement tier (ADR 21).
200201

202+
### `relation-target-schema-mismatch`
203+
204+
Emitted by `SubjectValidator`.
205+
`args`: `[expectedSchema, actualSchema]` — the target Schema the relation Property declares
206+
(`targetSchema`), and the Schema the resolved target Subject actually uses.
207+
`valuePartIndex`: index of the offending relation target within the `RelationValue`.
208+
`propertyName`: the relation property.
209+
210+
A relation targets a Subject that exists but whose own Schema is not the Property's declared
211+
`targetSchema`. The Schema compared is the target's own stored writer's-schema, read from its revision
212+
slot rather than from a graph node property. Reaching that slot still resolves the target ID through
213+
the subject-to-page index, which lives only in the graph projection, so this check only runs for
214+
targets the graph currently knows about. This is a blocking `error` (ADR 26): it is rejected at write
215+
time when `$wgNeoWikiEnforceValidation` is enabled. The subject editor's picker filters candidates by
216+
target Schema, so this is normally only reachable through the API — for example a bulk import.
217+
218+
### `relation-target-not-found`
219+
220+
Emitted by `SubjectValidator`.
221+
`args`: `[targetId]` — the target Subject ID that did not resolve.
222+
`valuePartIndex`: index of the offending relation target within the `RelationValue`.
223+
`propertyName`: the relation property.
224+
225+
A relation targets a Subject ID that does not resolve to any existing Subject. It is a non-blocking
226+
`warning` (ADR 26), joining [`schema-not-found`](#schema-not-found) and `unregistered-type` in the
227+
hardcoded non-blocking set, so it never rejects a write even under enforcement. This is deliberate:
228+
pointing at a not-yet-created Subject is wiki-native red-link behavior, and an import may legitimately
229+
mint the target later.
230+
231+
Resolution goes through the subject-to-page index, which lives only in the graph projection, so a
232+
target that exists in revision slots but is missing from the graph is reported as not found. An
233+
import populates the slots without updating the projection
234+
([#1022](https://github.com/ProfessionalWiki/NeoWiki/issues/1022)), so imported targets warn until
235+
[the graph is rebuilt](../operations/maintenance.md#rebuilding-the-graph). Being non-blocking, this
236+
misreport never costs a write.
237+
201238
## Known limitations (Foundation round)
202239

203-
The PHP `SubjectValidator` performs two Subject-level checks:
240+
The PHP `SubjectValidator` performs these Subject-level checks:
204241

205242
- **`required`** — PHP iterates the Schema's properties to catch absent-required cases.
206243
- **`type-mismatch`** — PHP compares the Statement's writer's-schema type against the Schema's
207244
current type and surfaces drift per ADR 11 / ADR 12.
245+
- **[`relation-target-schema-mismatch`](#relation-target-schema-mismatch)** and
246+
**[`relation-target-not-found`](#relation-target-not-found)** — documented above; both need a
247+
Subject lookup, which `PropertyType::validate()` has no access to.
208248

209249
### Deliberate behavior, not a gap
210250

extension.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,8 @@
410410
"neowiki-field-schema-not-found",
411411
"neowiki-field-unregistered-type",
412412
"neowiki-field-label-required",
413+
"neowiki-field-relation-target-schema-mismatch",
414+
"neowiki-field-relation-target-not-found",
413415
"neowiki-select-placeholder",
414416
"neowiki-select-no-results",
415417
"neowiki-select-unknown-option",

i18n/en.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,8 @@
8686
"neowiki-field-schema-not-found": "The schema \"$1\" is missing. Restore it before saving.",
8787
"neowiki-field-unregistered-type": "The type \"$1\" is not available on this wiki.",
8888
"neowiki-field-label-required": "Please provide a label for the subject.",
89+
"neowiki-field-relation-target-schema-mismatch": "This relation must point to a subject using the \"$1\" schema, but the selected subject uses \"$2\".",
90+
"neowiki-field-relation-target-not-found": "The subject \"$1\" this relation points to does not exist yet.",
8991
"neowiki-select-placeholder": "Select an option",
9092
"neowiki-select-no-results": "No matching options.",
9193
"neowiki-select-unknown-option": "(unknown option)",

i18n/qqq.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,8 @@
185185
"neowiki-field-min-length": "Validation error shown when a text value is shorter than the property's minimum length. $1 is the minimum number of characters.",
186186
"neowiki-field-max-length": "Validation error shown when a text value is longer than the property's maximum length. $1 is the maximum number of characters.",
187187
"neowiki-field-label-required": "Form-level error shown when the backend reports the Subject is missing its required display label.",
188+
"neowiki-field-relation-target-schema-mismatch": "Validation error shown when a relation points to a subject whose schema is not the relation's declared target schema. $1 is the required target schema; $2 is the schema the target subject actually uses.",
189+
"neowiki-field-relation-target-not-found": "Validation warning shown when a relation points to a subject ID that does not resolve to an existing subject. $1 is the target subject ID. Non-blocking: the target may be created later (for example during import), mirroring wiki red links.",
188190
"neowiki-select-placeholder": "Placeholder text shown in the select dropdown before any option is chosen.",
189191
"neowiki-select-no-results": "Message shown in the multi-select dropdown when the typed text does not match any available option.",
190192
"neowiki-field-invalid-datetime": "Validation error shown when the entered date and time value cannot be parsed. DateTime values must be strict ISO 8601 / xsd:dateTime strings with an explicit timezone offset or 'Z' (for example, '2025-06-15T12:00:00Z' or '2025-06-15T12:00:00+02:00'). Partial values such as year-only or date-only are rejected, and min/max bounds are inclusive.",

src/Application/Validation/SubjectValidator.php

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,19 +4,25 @@
44

55
namespace ProfessionalWiki\NeoWiki\Application\Validation;
66

7+
use ProfessionalWiki\NeoWiki\Application\SubjectLookup;
78
use ProfessionalWiki\NeoWiki\Domain\PropertyType\PropertyTypeLookup;
9+
use ProfessionalWiki\NeoWiki\Domain\Relation\Relation;
10+
use ProfessionalWiki\NeoWiki\Domain\Schema\Property\RelationProperty;
811
use ProfessionalWiki\NeoWiki\Domain\Schema\PropertyDefinition;
912
use ProfessionalWiki\NeoWiki\Domain\Schema\PropertyName;
1013
use ProfessionalWiki\NeoWiki\Domain\Schema\Schema;
14+
use ProfessionalWiki\NeoWiki\Domain\Schema\SchemaName;
1115
use ProfessionalWiki\NeoWiki\Domain\Statement;
1216
use ProfessionalWiki\NeoWiki\Domain\Subject\StatementList;
1317
use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectLabel;
1418
use ProfessionalWiki\NeoWiki\Domain\Validation\Violation;
19+
use ProfessionalWiki\NeoWiki\Domain\Value\RelationValue;
1520

1621
readonly class SubjectValidator {
1722

1823
public function __construct(
1924
private PropertyTypeLookup $propertyTypeLookup,
25+
private SubjectLookup $subjectLookup,
2026
) {
2127
}
2228

@@ -75,9 +81,88 @@ private function validateStatement( Statement $statement, PropertyDefinition $de
7581
$violations[] = $rawViolation->withPropertyName( $propertyName );
7682
}
7783

84+
return array_merge( $violations, $this->validateRelationTargets( $statement, $definition ) );
85+
}
86+
87+
/**
88+
* Server-side relation-target checks. Schema-scoped by necessity: both need the writer's-schema
89+
* RelationProperty (for its declared targetSchema) and a Subject lookup, neither of which
90+
* PropertyType::validate() has access to. This runs after the per-type dispatch, so the
91+
* Statement's type still matches the Schema's relation property here; a type-mismatched
92+
* Statement returned earlier and is not treated as a relation.
93+
*
94+
* A missing target is a non-blocking `relation-target-not-found` warning (red-link philosophy:
95+
* the target may be minted later, e.g. during import); a resolvable target whose own Schema is
96+
* not the declared targetSchema is a blocking `relation-target-schema-mismatch` error.
97+
*
98+
* The Schema compared is the target's own writer's-schema, read from its revision slot rather
99+
* than from a graph node property. Reaching that slot still resolves the target id through the
100+
* subject -> page index, which lives only in the graph projection (see
101+
* {@see \ProfessionalWiki\NeoWiki\NeoWikiExtension::getPageIdentifiersLookup()}), so an
102+
* unrebuilt or stale graph reports an existing target as not found. That is the same
103+
* degradation the read path has, and the reason not-found is non-blocking.
104+
*
105+
* @return Violation[]
106+
*/
107+
private function validateRelationTargets( Statement $statement, PropertyDefinition $definition ): array {
108+
$value = $statement->getValue();
109+
110+
if ( !$definition instanceof RelationProperty || !$value instanceof RelationValue ) {
111+
return [];
112+
}
113+
114+
$violations = [];
115+
116+
foreach ( $value->relations as $index => $relation ) {
117+
$violation = $this->validateRelationTarget(
118+
$relation,
119+
$statement->getPropertyName(),
120+
$definition->getTargetSchema(),
121+
(int)$index
122+
);
123+
124+
if ( $violation !== null ) {
125+
$violations[] = $violation;
126+
}
127+
}
128+
78129
return $violations;
79130
}
80131

132+
/**
133+
* The target's position in the multi-value RelationValue is carried as valuePartIndex so
134+
* ViolationDiff can tell a newly-added bad target apart from a pre-existing same-code
135+
* violation on the same property, exactly as SelectType/UrlType do for their parts.
136+
*/
137+
private function validateRelationTarget(
138+
Relation $relation,
139+
PropertyName $propertyName,
140+
SchemaName $targetSchema,
141+
int $valuePartIndex
142+
): ?Violation {
143+
$target = $this->subjectLookup->getSubject( $relation->targetId );
144+
145+
if ( $target === null ) {
146+
return new Violation(
147+
propertyName: $propertyName,
148+
code: 'relation-target-not-found',
149+
args: [ $relation->targetId->text ],
150+
valuePartIndex: $valuePartIndex,
151+
);
152+
}
153+
154+
if ( $target->getSchemaName()->getText() !== $targetSchema->getText() ) {
155+
return new Violation(
156+
propertyName: $propertyName,
157+
code: 'relation-target-schema-mismatch',
158+
args: [ $targetSchema->getText(), $target->getSchemaName()->getText() ],
159+
valuePartIndex: $valuePartIndex,
160+
);
161+
}
162+
163+
return null;
164+
}
165+
81166
/**
82167
* Catch required-but-missing: the Schema declares the property as required, but no
83168
* Statement for it is present on the Subject. This also covers the "empty Value

src/Domain/PropertyType/Types/RelationType.php

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,13 +41,19 @@ public function validate( NeoValue $value, PropertyDefinition $definition ): arr
4141
return [];
4242
}
4343

44-
$isEmpty = !( $value instanceof RelationValue ) || $value->isEmpty();
44+
$relations = $value instanceof RelationValue ? $value->relations : [];
4545

46-
if ( $definition->isRequired() && $isEmpty ) {
46+
if ( $definition->isRequired() && $relations === [] ) {
4747
return [ new Violation( propertyName: null, code: 'required' ) ];
4848
}
4949

50-
return [];
50+
$violations = [];
51+
52+
if ( !$definition->allowsMultipleValues() && count( $relations ) > 1 ) {
53+
$violations[] = new Violation( propertyName: null, code: 'single-value-only' );
54+
}
55+
56+
return $violations;
5157
}
5258

5359
}

src/Domain/Validation/Violation.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
* is in a degraded state. They are reported but never reject a write. These are the
1414
* `warning` tier that ADR 26 will make configurable per Constraint.
1515
*/
16-
private const NON_BLOCKING_CODES = [ 'schema-not-found', 'unregistered-type' ];
16+
private const NON_BLOCKING_CODES = [ 'schema-not-found', 'unregistered-type', 'relation-target-not-found' ];
1717

1818
public function __construct(
1919
public ?PropertyName $propertyName,

src/NeoWikiExtension.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1052,6 +1052,7 @@ private function isValidationEnforced(): bool {
10521052
public function getSubjectValidator(): SubjectValidator {
10531053
return new SubjectValidator(
10541054
propertyTypeLookup: $this->getPropertyTypeLookup(),
1055+
subjectLookup: $this->getSubjectRepository(),
10551056
);
10561057
}
10571058

tests/phpunit/Application/Actions/CreateSubjectActionTest.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
use ProfessionalWiki\NeoWiki\Tests\Data\TestRelation;
3333
use ProfessionalWiki\NeoWiki\Tests\Data\TestStatement;
3434
use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemorySchemaLookup;
35+
use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemorySubjectLookup;
3536
use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemorySubjectRepository;
3637
use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpySubjectWriteAuthorizer;
3738
use ProfessionalWiki\NeoWiki\Tests\TestDoubles\StubIdGenerator;
@@ -74,7 +75,10 @@ private function newCreateSubjectAction( bool $validationEnforced = false ): Cre
7475
new SelectStatementResolver( new SelectValueResolver() ),
7576
new ProposedSubjectValidator(
7677
schemaLookup: $this->schemaLookup,
77-
subjectValidator: new SubjectValidator( propertyTypeLookup: $registry ),
78+
subjectValidator: new SubjectValidator(
79+
propertyTypeLookup: $registry,
80+
subjectLookup: new InMemorySubjectLookup(),
81+
),
7882
),
7983
$validationEnforced,
8084
);

tests/phpunit/Application/Actions/ReplaceSubjectActionTest.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject;
3636
use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemoryPageIdentifiersLookup;
3737
use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemorySchemaLookup;
38+
use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemorySubjectLookup;
3839
use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemorySubjectRepository;
3940
use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpySubjectWriteAuthorizer;
4041
use ProfessionalWiki\NeoWiki\Tests\TestDoubles\StubIdGenerator;
@@ -74,7 +75,10 @@ private function newAction(
7475
selectStatementResolver: new SelectStatementResolver( new SelectValueResolver() ),
7576
proposedSubjectValidator: new ProposedSubjectValidator(
7677
schemaLookup: $this->schemaLookup,
77-
subjectValidator: new SubjectValidator( propertyTypeLookup: $registry ),
78+
subjectValidator: new SubjectValidator(
79+
propertyTypeLookup: $registry,
80+
subjectLookup: new InMemorySubjectLookup(),
81+
),
7882
),
7983
presenter: $this->presenterSpy,
8084
validationEnforced: $validationEnforced,

0 commit comments

Comments
 (0)