Repository navigation
fix: classify json.Number values correctly - #81
kavyansh18 wants to merge 3 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Two behaviour changes here need fixing before this can merge: a oneOf schema now accepts an instance the specification requires it to reject, and a null-valued property now produces a hard error where it used to validate. Both are reproduced below, run against main at 794ce5e and this branch at b23ec02.
The premise is right, and the approach is the one the package already uses. jsonNumber has always treated a json.Number as a number, so minimum, maximum, multipleOf, enum and uniqueItems handle it correctly today and type was the outlier. Measured across seven literals decoded both ways against {"type":"integer"}, agreement between a plain json.Unmarshal and a UseNumber decode rises from 2 to 6, and the one remaining disagreement (1.0000000000000000001) is this branch being right where the float64 path rounds.
oneOf accepts a value the spec requires it to reject
Once json.Number passes the type gate, validation continues into the string-keyword gate at validate.go:188, which keys off reflect.Kind and so still sees a string. Draft 2020-12 §7.6.1 uses this exact case as its worked example: "the maxLength keyword ... will only restrict certain strings (that are too long) from being valid. If the instance is a number, boolean, null, array, or object, then it is valid against this assertion." So both branches below match, and oneOf must fail.
func TestOneOf(t *testing.T) {
one := 1
s := &jsonschema.Schema{OneOf: []*jsonschema.Schema{
{Type: "number", MaxLength: &one},
{Type: "integer"},
}}
r, _ := s.Resolve(nil)
dec := json.NewDecoder(strings.NewReader(`42`))
dec.UseNumber()
var v any
_ = dec.Decode(&v)
t.Logf("UseNumber: %v", r.Validate(v))
var f any
_ = json.Unmarshal([]byte(`42`), &f)
t.Logf("float64: %v", r.Validate(f))
}main: UseNumber: oneOf: did not validate against any of [...]
this branch: UseNumber: <nil>
both: float64: oneOf: validated against both <anonymous schema> and <anonymous schema>
The float64 line is the answer the spec asks for, and this branch stops producing it for a UseNumber instance. That string-keyword gate is unchanged by this pull request, so the defect predates it — what changes is that the type keyword no longer shields it.
A null property becomes a hard error
Decoding JSON null into a json.Number destination leaves json.Number(""). big.Rat.SetString("") fails, the new branch returns ("", false), and validate.go:120 turns that into a hard error. No UseNumber is involved.
func TestNullProperty(t *testing.T) {
var m map[string]json.Number
_ = json.Unmarshal([]byte(`{"a":null}`), &m)
s := &jsonschema.Schema{Properties: map[string]*jsonschema.Schema{
"a": {Types: []string{"string", "null"}},
}}
r, _ := s.Resolve(nil)
t.Logf("typed map: %v", r.Validate(m))
var plain map[string]any
_ = json.Unmarshal([]byte(`{"a":null}`), &plain)
t.Logf("any map: %v", r.Validate(plain))
}main: typed map: <nil>
this branch: typed map: type: of type reflect.Value is not a valid JSON value
both: any map: <nil>
{"type":["string","null"]} is an ordinary way to write an optional string, and main gets it right today.
A large-exponent number is reported as not being JSON
big.Rat.SetString refuses exponents above 10⁶, and the comment at util.go:241-243 already notes that such a literal is a valid JSON number. The new branch forwards that failure as ("", false), which jsonType's own doc reserves for a value that is not valid JSON.
json.Number("1e9999999") against four schemas:
| schema | main | this branch | draft 2020-12 |
|---|---|---|---|
{"type":"string"} |
accepted | error | reject |
{"type":"number"} |
rejected | error | accept |
{"type":"integer"} |
rejected | error | accept |
{"type":["number","string"]} |
accepted | error | accept |
This one cuts both ways: the first row improves and the last regresses. §6.2 does acknowledge that an implementation may be unable to represent every JSON number, which is a fair argument for failing here, but it is not an argument for reporting the value as not being JSON.
Cost moves onto the most common keyword
type now builds a big.Rat for every json.Number in the instance. Validating a 2001-byte array of 200 copies of the 9-byte literal 1e1000000 against {"items":{"type":"number"}} takes 4.688s on this branch and returns immediately on main. The same magnitude is already reachable on main through {"items":{"minimum":0}}, which costs 4.716s there, so this is a widening of the trigger rather than a new ceiling, and per-value cost is bounded (1e1000001 is refused in 26µs). Worth knowing because it lands on type, which nearly every schema carries, and because only UseNumber callers can get such a literal into an instance at all.
Ordinary traffic is much cheaper: {"type":"integer"} against json.Number("42") costs 625ns and 12 allocations, against 225ns and 5 allocations for the float64 path.
Nothing exercises this through the public API
UseNumber appears nowhere in the repository, and the conformance harness decodes instances into any with json.Unmarshal, which never produces a json.Number. So no case under testdata/ can reach the new branch however many are added, and the green suite is not evidence about it. A Validate-level test would have caught the first two items above.
The property to hold on to
Whatever decides that a value is a JSON string should be one test used everywhere. Right now jsonType, the string-keyword gate at validate.go:188, equalValue and infer.go:242 each answer it differently. Fixing the oneOf instance without unifying that test would leave the rule broken.
Separately, and pre-existing rather than something for this pull request to carry: For[T] on a struct with a json.Number field emits {"type":"string"}, which disagrees with the classification added here. That round trip already fails on main for an ordinary decode, so it is its own issue — but the two answers ought to be settled together.
Two process notes: this branch conflicts with #65 on both files it touches, and no CI job has run on this head yet.
|
One thing worth borrowing before the next revision. #65 was an independent fix for this same bug, now closed in favour of this one, and it handled the parse-failure case better: where That single difference is what separates the two on the two error regressions above. Measured on the same machine against The rest of this PR's approach is the better one and worth keeping. Exact |
|
Thanks for the review @karolpiotrowicz . I’ve addressed the points you mentioned:
I also ran the full test suite, race tests, |
karolpiotrowicz
left a comment
There was a problem hiding this comment.
All five points from the last round are fixed, and I checked each one by running it rather than reading it. What needs fixing before this can merge is new: the change to equalValue breaks equality for numbers nested inside an array or object, and in three of the shapes I measured it accepts a document the schema requires rejecting. The nested-equality case is the one that has to change. The evaluation order in the string-keyword guard is a one-token fix and worth taking in the same push, and the rest are optional. Everything below was run against main at 794ce5e and this branch at 5cab8f7.
Taking the round-one points first, since they are all closed. oneOf now reports oneOf: validated against both, matching what the float64 path has always said. A null decoded into a json.Number destination validates instead of erroring. 1e9999999 falls back to main's classification rather than being called "not a valid JSON value". isValidJSONNumber is a genuine RFC 8259 number parser — I ran 28 million differential executions against encoding/json and the only disagreements were artifacts of my oracle (surrounding whitespace, a quoted literal, null), not grammar differences. And the tests discriminate now: I replaced the exact classification with the obvious lossy Float64() + math.Modf version and the suite caught it, along with four other mutations over the changed lines. Last round that same mutant survived.
Numbers inside an array or object no longer compare correctly
jsonNumber sees a json.Number through an interface wrapper, because v.Interface().(json.Number) unwraps it. It does not see an int or float64 through one, because CanInt and CanFloat test the static kind, which is Interface. So inside []any or map[string]any, exactly one side of a comparison classifies as a number.
That asymmetry was harmless while the guard was ok1 && ok2: the condition came out false and control fell through to the interface-unwrapping arm, which recursed on the two concrete values and got the right answer. With ok1 || ok2 the function returns before reaching it.
Reproduction — all seven rows, public API only, instance decoded with UseNumber()
func TestNested(t *testing.T) {
un := func(lit string) any {
d := json.NewDecoder(strings.NewReader(lit))
d.UseNumber()
var v any
if err := d.Decode(&v); err != nil {
t.Fatal(err)
}
return v
}
load := func(sch string) *jsonschema.Resolved {
var s jsonschema.Schema
if err := json.Unmarshal([]byte(sch), &s); err != nil {
t.Fatal(err)
}
r, err := s.Resolve(nil)
if err != nil {
t.Fatal(err)
}
return r
}
r := load(`{"if":{"properties":{"meta":{"const":{"level":1}}},"required":["meta"]},
"then":{"required":["auth"]}}`)
t.Logf("if/then, no auth: %v", r.Validate(un(`{"meta":{"level":1}}`)))
t.Logf("not/enum: %v", load(`{"not":{"enum":[[1,2,3]]}}`).Validate(un(`[1,2,3]`)))
t.Logf("enum: %v", load(`{"enum":[[1,2,3]]}`).Validate(un(`[1,2,3]`)))
t.Logf("const: %v", load(`{"const":{"a":1}}`).Validate(un(`{"a":1}`)))
t.Logf("Equal map: %v", jsonschema.Equal(
map[string]any{"k": json.Number("1")}, map[string]any{"k": 1.0}))
t.Logf("Equal slice: %v", jsonschema.Equal(
[]any{json.Number("1")}, []any{1}))
uniq, _ := (&jsonschema.Schema{UniqueItems: true}).Resolve(nil)
t.Logf("uniqueItems: %v", uniq.Validate([]any{json.Number("42"), 42.0}))
} main this branch
if/then, no auth: missing "auth" <nil> <- accepted, `then` never fired
not/enum: not failed <nil> <- denied value accepted
enum: <nil> does not equal any of
const: <nil> does not equal
Equal map: true false
Equal slice: true false
uniqueItems: items are equal <nil> <- duplicate not detected
The first two rows and uniqueItems are the ones that worry me: each accepts something main correctly rejects, silently, so a caller cannot tell. The enum and const rows go the other way and reject conforming instances, which is more visible but still a break. Decoded without UseNumber() every row is unchanged on both, so this is confined to the decode mode the change exists to serve.
{json.Number("42"), 42.0, true} at util_test.go:37 asserts that exact pair is equal. Put it inside a map and this branch says it is not.
Worth saying explicitly: reverting to ok1 && ok2 is not the answer. On main Equal(json.Number("42"), "42") returns true via the string comparison, and the new guard is what makes it false — which util_test.go:38 now pins. The two goals are compatible: I tried an interface look-through at the top of jsonNumber and it restores all seven rows above while keeping every round-one fix and leaving the suite green. That was only to check the thing is fixable, and there may well be a better placement than the one I tried.
The property to hold on to: whether a value is a JSON number should not depend on whether it is standing alone or sitting inside a container.
A schema with no keywords now pays for a big.Rat on every json.Number
At validate.go:188 isString(instance) is the left operand, so Go evaluates it before the keyword-presence test — on every instance, at every schema node, whether or not the schema has a string keyword. main's left operand was instance.Kind() == reflect.String, which costs nothing.
Ten-number array decoded with UseNumber() against {"items":{}}, a schema with no keywords at all, -benchtime 1s -count 5 -cpu 1:
main 2039-2089 ns/op 936 B/op 36 allocs/op
this branch 6093-6173 ns/op 2296 B/op 106 allocs/op
with the operands swapped 2025-2053 ns/op 936 B/op 36 allocs/op
Just under 3x slower with 2.9x the allocations, and swapping the two operands recovers all of it. float64 and string documents are unaffected on all three. It shows up much harder on large literals: 200 copies of 1e1000000 against {"items":{}} take 4.595s here and return immediately on main.
Two things I checked and found unchanged from main, so they are not this branch's to carry: {"type":"string"} still accepts json.Number("1e9999999"), and the numeric bounds are still skipped for it. On that axis the branch is a clear improvement — main classified every json.Number as a string, and this narrows it to the ones big.Rat cannot represent.
A few smaller things are on the lines they concern.
CI has not run on this head yet, so every number above is from my own sandbox.
|
Thanks for the review @karolpiotrowicz . I’ve pushed the updates: Fixed json.Number equality for nested arrays/maps. All tests, race tests, go vet, and git diff --check pass. |
Summary
Fixes incorrect JSON type detection for
json.Numbervalues decoded withjson.Decoder.UseNumber().json.Numberhas an underlyingstringkind, sojsonTypecurrently classifies it as a JSON string instead ofintegerornumber. This causes valid numeric values decoded withUseNumber()to fail schema type validation.This change recognizes
json.Numberexplicitly and reuses the existingjsonNumberhelper to determine whether the value represents an integer or number.Linked issue
Fixes #80
Test plan
go test ./...go test -v -run TestJSONType ./jsonschemagit diff --checkAll checks passed.
Risk / breaking changes
No public API changes.
This is a localized type-detection fix with regression coverage for integer, decimal, exponent, large-integer, negative, and invalid
json.Numbervalues.