Skip to content

Ignore null bounds in NumberType validation#758

Draft
JeroenDeDauw wants to merge 2 commits intomasterfrom
fix/756-null-bounds-validation
Draft

Ignore null bounds in NumberType validation#758
JeroenDeDauw wants to merge 2 commits intomasterfrom
fix/756-null-bounds-validation

Conversation

@JeroenDeDauw
Copy link
Copy Markdown
Member

@JeroenDeDauw JeroenDeDauw commented Apr 22, 2026

Related to #756

What this changes

Hardens NumberType validation against null bounds at the JSON seam. The TS type declares minimum?: number / maximum?: number — "absent" should mean undefined, not null. Today NumberType.createPropertyDefinitionFromJson copies the JSON values through verbatim, so if a null ever arrives (e.g. from PHP's NumberProperty::nonCoreToJson, or a hand-edited schema page), it lands on the domain object as null and the validator's !== undefined guard lets it through. JS then coerces null to 0 in the numeric comparison, so any non-zero input is rejected with "Maximum value is null." (or "Minimum value is null." for negatives).

Normalize null to undefined at the JSON seam so the domain type stays honest and existing validator guards do the right thing.

Two regression tests cover the maximum and minimum paths independently (value 2000 exercises only maximum > 0; -2000 exercises minimum < 0).

Note on #756

I could not reproduce the user-visible "Maximum value is null." error on master today. The current RestSchemaRepository.getSchema fetches /v1/page/Schema:<name> (raw wikitext source), not the PHP-serialized JSON — and schema wikitext omits unset keys, so minimum/maximum arrive as undefined, not null, and the existing guard works. So this PR is defensive rather than a fix for a currently reachable bug via the SubjectEditor flow. Leaving #756 open — the original screenshot must have come from a different path or state that we haven't identified yet.

Follow-up

The same null-leak pattern exists at the JSON seam for Text.ts (minLength/maxLength) and DateTime.ts (minimum/maximum). Neither triggers a user-visible bug today — TextProperty.php does not emit those keys, and DateTime.ts has a parseStrictDateTime(null) guard that masks the coercion — but both are latent and worth tidying as a separate change.

Result of extensive back and forth with @JeroenDeDauw and this specific refinement of his after empirical verification in the dev environment.
Context: the NeoWiki codebase on master, browser-based reproduction attempt against the running dev wiki, inspection of the PHP serialization and TS deserialization layers.
Written by Claude Code, `Opus 4.7`

JeroenDeDauw and others added 2 commits April 22, 2026 20:32
Fixes #756

The PHP persistence layer serializes a NumberProperty with no bounds
set by emitting `"minimum": null, "maximum": null, "precision": null`.
`NumberType.createPropertyDefinitionFromJson` copied those nulls
straight onto the `NumberProperty`, so the validator's
`!== undefined` guard let them through. JS then coerced `null` to
`0` in the numeric comparison, so any non-zero input was rejected
with "Maximum value is null." (or "Minimum value is null." for
negatives).

Normalize `null` to `undefined` at the JSON seam so the domain type
stays honest and existing validator guards do the right thing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The original regression test used value 2000 with both bounds null,
which only exercises the maximum path (null coerced to 0; 2000 > 0).
Add a mirror test with -2000 to catch regressions that only reintroduce
the null leak on the minimum side.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@JeroenDeDauw
Copy link
Copy Markdown
Member Author

Weird. I no longer get the error on master. CC says it can see the wrong code on master, but perhaps it's incorrect about that code being wrong, and the issue I ran into came from elsewhere?

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