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

Should enum really include null? #258

Closed
Relequestual opened this issue Feb 22, 2017 · 11 comments
Closed

Should enum really include null? #258

Relequestual opened this issue Feb 22, 2017 · 11 comments
Milestone

Comments

@Relequestual
Copy link
Member

The specification says that enum may include null.
If type does not also include null, although the validation check for the enum key word will pass, the validation check for the type key word will mean the instance fails overall validation. The inverse is also true.

Usecase:
I have a value which may be a number of strings, or null.
Valid values are "a", "b", or NULL.
To define and pass validation:
I must set the type to ["string", "null"]
AND
I must set enum to ["a", "b", null]

If we want to keep this behaviour, as we are commenting that enum may be null, we should point out that when doing so, if you expect validation to pass for an instance where you define the type, you must also include null in the type.

@epoberezkin
Copy link
Member

That is correct. I don't see it as a problem, because when you use "enum" there is no much point in using "type" as well (unless all items are the same type and you want to make sure it is the case)...

@handrews
Copy link
Contributor

unless all items are the same type and you want to make sure it is the case

This actually came up yesterday at my day job :-) The idea was to ensure that a strongly typed language can safely make assumptions about type without examining every enum entry. While technically not necessary, it is helpful to include type in this case.

we should point out that when doing so, if you expect validation to pass for an instance where you define the type, you must also include null in the type.

I think this falls under the general "nonsense schema" principle- there are many, many ways to write schemas that can't validate. Set "maximum" less than "minimum" for example. We can comment that "enum" doesn't override "type" or something like that, but it's not limited to null anyway. If you have a "type" of string and include a number in your enum that won't work either.

@Relequestual
Copy link
Member Author

I was considering the other direction to you @handrews.
Say my property defines the type as string and null. I add some strings to an enum, but not null, because I've already said null is OK in my type definition. This is the trap I fell into anyway.

I agree we shouldn't comment on nonsensical schemas (as a rule), however I feel this is something that's omitted that isn't obvious. I think we should add in the enum section something like "if you have included null in your type, you need to also add it to enum or the enum validation check will fail.

I mean, if you guys dissagree that this isn't obvious, then I'm fine. Maybe just me having not done this for a while.

Close at your discretion.

@handrews
Copy link
Contributor

Oh, I see. I still want to think if there's a more general way to word this. There are a ton of special cases and I don't want to have to include each one- the "keyword independence" section under general principles is what governs here.

If we had type-independent keywords ("type", "enum", and "const") grouped into a section then we could put this in the group intro paragraph :-/

@Relequestual
Copy link
Member Author

...because when you use "enum" there is no much point in using "type" as well
@epoberezkin

That depends how you're building your schema documents. I'm building mine from our model definitions. It's fine, I've fixed my generation code to include null in the enum. I just didn't expect I would need to. My fault for not properly understanding the spec.

Maybe such a change doesn't belong in the spec, I dunno. If you guys are unsure...

@handrews
Copy link
Contributor

Maybe such a change doesn't belong in the spec, I dunno. If you guys are unsure...

I'd like to avoid opening up more wording debates for Draft 06. We can revisit this after it's out. At minimum it should go on the more general guidance that we keep saying we're going to add tot he web site. Basically, it's an example of a behavior that's already documented in the spec- it makes a good example, but I'm not 100% sure it deserves its' own note in the specification.

On the other hand, if you come up with some wording that isn't too much of a digression into details I'm fine with it.

@Relequestual Relequestual added this to the draft-future milestone Feb 27, 2017
@Relequestual
Copy link
Member Author

Totally happy to review this post draft-6.

@handrews
Copy link
Contributor

I think this is best handled by writing some guides on the web site. There are a bunch of pitfalls of this sort and I think trying to call them all out in the specification will just bloat it.

Please re-open if you disagree- I'm closing because earlier int he conversation you seemed fine with that.

@Urigo
Copy link

Urigo commented Nov 29, 2023

any chances to consider this again?

@Relequestual
Copy link
Member Author

any chances to consider this again?

I would say no, because, per above...

The idea was to ensure that a strongly typed language can safely make assumptions about type without examining every enum entry. While technically not necessary, it is helpful to include type in this case.

@Urigo
Copy link

Urigo commented Nov 29, 2023

Ohh I missed that part, makes a lot of sense, thanks @Relequestual !

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