Ignore null bounds in NumberType validation#758
Draft
JeroenDeDauw wants to merge 2 commits intomasterfrom
Draft
Conversation
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>
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? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #756
What this changes
Hardens
NumberTypevalidation againstnullbounds at the JSON seam. The TS type declaresminimum?: number/maximum?: number— "absent" should meanundefined, notnull. TodayNumberType.createPropertyDefinitionFromJsoncopies the JSON values through verbatim, so if anullever arrives (e.g. from PHP'sNumberProperty::nonCoreToJson, or a hand-edited schema page), it lands on the domain object asnulland the validator's!== undefinedguard lets it through. JS then coercesnullto0in the numeric comparison, so any non-zero input is rejected with "Maximum value is null." (or "Minimum value is null." for negatives).Normalize
nulltoundefinedat 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
2000exercises onlymaximum > 0;-2000exercisesminimum < 0).Note on #756
I could not reproduce the user-visible "Maximum value is null." error on master today. The current
RestSchemaRepository.getSchemafetches/v1/page/Schema:<name>(raw wikitext source), not the PHP-serialized JSON — and schema wikitext omits unset keys, sominimum/maximumarrive asundefined, notnull, 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) andDateTime.ts(minimum/maximum). Neither triggers a user-visible bug today —TextProperty.phpdoes not emit those keys, andDateTime.tshas aparseStrictDateTime(null)guard that masks the coercion — but both are latent and worth tidying as a separate change.