Skip to content

Jsonschema accepts np.NaNs as numeric, can I change that behaviour somehow? #357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Casyfill opened this issue Sep 1, 2017 · 7 comments
Closed

Comments

@Casyfill
Copy link

Casyfill commented Sep 1, 2017

No description provided.

@Casyfill Casyfill changed the title Jsonschema accepts np.NaNs as numeric, can I recheck it now? Jsonschema accepts np.NaNs as numeric, can I change that behaviour somehow? Sep 1, 2017
@Julian
Copy link
Member

Julian commented Sep 6, 2017

Pass in a more restrictive type to the types argument for "number" to your validator

The default is numbers.Number, which yes, np.NaN is one of those.

@Julian Julian closed this as completed Sep 6, 2017
@Tahnan
Copy link

Tahnan commented Jan 28, 2019

The thing about this is that it's not exactly easy to exclude np.NaN, because np.NaN seems to be exactly the same thing as float('nan')--that is, its type isn't a numpy subclass of some sort, but rather it's just an ordinary float. (math.nan is the same thing, in sufficiently later versions of Python.)

I realize that you can do something like Draft3Validator.TYPE_CHECKER.is_type(instance, "number") and not math.isnan(instance). But redefining types seems a long way to go just to exclude something that honestly never should have passed in the first place. (For instance, if you look at tdegrunt/jsonschema#53, the decision there was that "nan" should not validate as a number. That suggests to me, anyway, that this implementation also shouldn't accept it as a number.)

Any thoughts on adapting the code to reject nan as a number?

@Julian
Copy link
Member

Julian commented Jan 28, 2019

I think you're making it sound more obvious than it is that NaN isn't a number, both answers seem reasonable to me, so I'm inclined to leave things as-is unless there's really strong reasoning. You'll note obviously that Python itself considers NaN a number, and since NaN isn't a JSON thing, we're again in spec-less territory.

That implementation flip flopped it looks like, and at least in scanning that issue quickly, I don't see the change as being intentional from there alone, so not sure how much weight that issue alone gives.

It's quite easy to change the behavior as you've already found it looks like though.

@Tahnan
Copy link

Tahnan commented Jan 29, 2019

Pondering this...I think it is indeed obvious to me that NaN is, well, a "number" in some sense. But even if it is a number, it's not a valid number, which is why (I would think) that jsonschema would want to reject it when given it in a field that expects a number. (The ultimate conclusion over at the linked repo does seem to have been not to accept NaN: https://github.com/tdegrunt/jsonschema/blob/master/lib/validator.js#L293 is the line that rejects all of NaN, Infinity, and -Infinity, and https://github.com/tdegrunt/jsonschema/blob/master/test/attributes.js#L24 is the test that explicitly shows that validating NaN as {'type': 'number'} should return false. The PR that introduced those lines is tdegrunt/jsonschema#152, which is the one referenced in that discussion.)

Now, all of that said, while I've convinced myself that this is the right approach, I realize that you may not be convinced. :-) And of course, in the meantime, I still need to exclude nan from things that accept numbers. The snippet I pasted above, based on the documentation, would work...in version 3.0, but we're using the version on PyPI, i.e. 2.6.0, which doesn't have the redefine method. It looks like the only thing you can do in 2.6.0 is give it a sequence of things that count as the given type--and now I'm not sure what types I can give the types argument that would allow floats but exclude a particular float like nan. It's possible that disallowing nan can't really be done in 2.6.0, and we'll need to add an extra non-jsonschema-based check to make sure the code isn't being passed a nan. (Alas! But not impractical.)

@Julian
Copy link
Member

Julian commented Jan 29, 2019

Think of it in the current form as "number" is interpreted by jsonschema to mean "float or int", and NaN is a float. Certainly there is another reasonable way to extend number beyond numbers as defined by JSON (the one you're thinking) but yeah IMO both are reasonable extensions and so the current one "wins" for now.

FWIW you can do stupid type trickery to create a type that responds true for everything but NaN, but the ability to redefine these kinds of things is exactly what motivated the type checker interface changes, so possibly you just want to upgrade :)

@slbayer
Copy link

slbayer commented Mar 27, 2024

This issue came up again in #1222, although I didn't really emphasize the right aspect there. I'd like to argue again, as Tahnan argues above, that {"type": "number"} should not validate NaN. While it is absolutely true that a strict JSON interpreter will fail on NaN, the problem is that the jsonschema package isn't working on JSON; it's working on Python elements that were digested from JSON. And it's not Python types that it's supposed to be enforcing, it's JSON types, and NaN is not a legal JSON number, and the JSON schema specification is pretty clear that {"type": "number"} is reserved for JSON numbers. Therefore, it doesn't matter whether Python thinks of NaN as a number; the only numbers that jsonschema should be validating, I argue, are those that correspond to numbers that correspond to JSON numbers. Please reconsider your position.

@Julian
Copy link
Member

Julian commented Mar 27, 2024

Given that:

  • this library implements the specification and essentially is defined in its core behavior (as opposed to what you yourself can extend) only over things the specification actually specifies
  • it's trivial for you a user to customize whatever behavior you want here for NaN

my position hasn't changed here I'm afraid.

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

No branches or pull requests

4 participants