json-schema-org / JSON-Schema-Test-Suite

A language agnostic test suite for the JSON Schema specifications
MIT License
625 stars 209 forks source link

multipleOf contains incorrectly optional (bignum dependent) tests #536

Open jimmylewis opened 2 years ago

jimmylewis commented 2 years ago

I'm working on updating my validator to consume the latest version of the test suite (after about 18 months; I usually aim for once per year but hey...), and hit issues with the multipleOf.json tests now containing bignum scenarios (see #438). I also noticed that there's recent discussion about changing or adding more bignum tests to multipleOf (see #534).

As @Julian called out in #438:

Unless, is arbitrary-precision support optional?

Arbitrary precision is optional yeah. You are allowed to use IEEE floats or doubles (and presumably also this -- 16 byte floats)

Can we agree to place these tests in the optional suite so that implementors who wish to leverage the official suite but don't have bignum support can more easily navigate around this obstacle? In #438, the float version of the test was placed under optional, but the integer version was not.

/cc'ing some folks who've been involved in the other issues: @karenetheridge, @Zac-HD

karenetheridge commented 2 years ago

The tests should already be restricted to optional/ -- the one test in multipleOf.json that tests large numbers is testing that you either 1. can handle bignums and get the correct result, or 2. correctly identify that you overflowed and returned valid: false. I agree that putting a test in multpleOf.json that expects valid: true would be inappropriate, as it is essentially mandating bignum support, which may not be possible for all architectures/languages.

ChALkeR commented 2 years ago

For the tests that I recommended adding in #534 -- I agree that those should be placed in optional/.

jimmylewis commented 2 years ago

In my implementation, the validator returns neither true nor false, but instead an error that the data could not be handled, and thus we cannot determine whether the document is valid. This seems like a fair behavior ("I can't tell you what I don't know"), but the test is making assumptions about the implementation returning false. Unless I've missed something, I don't see an expectation for how implementations should behave in cases of data inputs they can't handle.

Edit: "implementations can't handle X input" sounds bad, but platform interoperability, at least as far as numerical types are concerned, is called out in the JSON standard itself:

A JSON number such as 1E400 or 3.141592653589793238462643383279 may indicate potential interoperability problems, since it suggests that the software that created it expects receiving software to have greater capabilities for numeric magnitude and precision than is widely available.

Recognizing that not all platforms are capable of working with these values should also be respected by the schema test suite, as @gregsdennis pointed out in #378:

This test suite is supposed to test JSON Schema. Anything that isn't supported because of the decisions of the way JSON is implemented is out of the Schema library's control (assuming they're separate libraries).

gregsdennis commented 2 years ago

I think the issue here is a misunderstanding of a nuance in failure modes.

correctly identify that you overflowed and returned valid: false - @karenetheridge

An overflow is an error, so it's incorrect to return valid: false.

If the implementation errors, it's not valid or invalid. The validity can't be determined because the schema/instance can't be processed. The error itself is the correct outcome.

I catch these cases and mark them as "inconclusive" when I'm running the tests. It's not a failure of my code that .Net (the platform I'm running on) can't handle the values, so I don't consider it a test failure.

I agree that these cases should be in the optional tests, though.

Julian commented 2 years ago

@jimmylewis a PR to move these is definitely welcome.

karenetheridge commented 2 years ago

@jimmylewis a PR to move these is definitely welcome.

see #538.