Skip to content
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

enum shouldn't override other values, right? #87

Closed
toddobryan opened this issue Jun 7, 2017 · 7 comments
Closed

enum shouldn't override other values, right? #87

toddobryan opened this issue Jun 7, 2017 · 7 comments

Comments

@toddobryan
Copy link

toddobryan commented Jun 7, 2017

According to the last example on this page

https://spacetelescope.github.io/understanding-json-schema/reference/generic.html#enumerated-values

it's possible to have a type in addition to an enum. And, in fact, you should be able to add the enum constraint to basically any schema. (At least, I think that's what the spec says.)

http://json-schema.org/latest/json-schema-validation.html#rfc.section.6.23

and

http://json-schema.org/latest/json-schema-validation.html#rfc.section.4.4

However, when I run this code:

    JSONObject rawSchema = new JSONObject(new JSONTokener(
            new StringReader("{'type': 'string', 'enum': ['abc', '123']}".replace('\'', '"'))));
    Schema s = SchemaLoader.load(rawSchema);
    System.out.println(s.toString());

I get this:

    {"type":"enum","enum":["123","abc"]}

From the Spec, it doesn't look like "enum" is even allowed as a value for type.

http://json-schema.org/latest/json-schema-validation.html#rfc.section.6.25

I'd like to fix this, but want to make sure people agree it's wrong before I do.

@erosb
Copy link
Contributor

erosb commented Jun 7, 2017

Hello @toddobryan , thank you for reporting it. You are right, "type":"enum" shouldn't be there, so it should be removed from EnumSchema#describePropertiesTo(). It is a pretty trivial fix, please go ahead with creating a PR, much appreciated.

@erosb erosb closed this as completed Jun 7, 2017
@erosb erosb reopened this Jun 7, 2017
@toddobryan
Copy link
Author

But shouldn't "type": "string" be there, as well?

@erosb
Copy link
Contributor

erosb commented Jun 7, 2017

I can't see any reason for that. It is unnecessary for your specific case, and shouldn't work that way in generic cases (since enum values can be any values, not only strings).

@toddobryan
Copy link
Author

See the first link above. According to the spec, you can attach enum to any schema. (They list it with title, description, and default in the guide, and according to the last example on that page, {"type": "string", "enum": [...]} is valid and should check that the value is both of type string and is a member of the enum.

This could be useful in weird cases. For example, imagine this schema:

{"enum": [true, false, 0, 1, "true", "false"]}

representing things that can be recognized as booleans. The schema:

{ "type": "string", "enum": [true, false, 0, 1, "true", "false"]}

would limit someone to the string values. Similarly, you could limit someone to the boolean or integer values.

In any case, according to the spec, it's perfectly valid to combine enum with any other keywords. Keywords that don't apply to types are valid, but just vacuously pass according to this:

http://json-schema.org/latest/json-schema-validation.html#rfc.section.4.1

Are you interested in handling all valid schemas according to the spec, or just ones that make sense?

@erosb
Copy link
Contributor

erosb commented Jun 7, 2017

@toddobryan I understand your point.

It is a quite generic design flaw of the library that the inheritance hierarchy of Schema subclasses express type-specific grouping of possible schemas, which means that weird mixtures can't always be expressed.

I'm not sure how serious problem it is. The official test suite doesn't contain such "mixed" examples (neither for draft 4 nor for draft 6). So the "type":"enum" should be removed definitely, but supporting further keywords in enum schemas won't happen this time I'm afraid.

@erosb
Copy link
Contributor

erosb commented Jun 8, 2017

One more sidenote: you are referring to sections of the latest version of the specification (which is unofficially called draft-6), while this library is the implementation of draft-4 (draft-4 core and draft-4 validation ). This is somewhat misleading, you can read a bit more on this wiki page.

Draft-6 was released in April. A new version of this library supporting draft-6 is on the way, i'm working on it, but there is no ETA.

erosb added a commit that referenced this issue Jul 20, 2017
@erosb
Copy link
Contributor

erosb commented Jul 21, 2017

Hello @toddobryan , I released the new version yesterday.

Links:

@erosb erosb closed this as completed Jul 21, 2017
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

2 participants