pipermerriam / flex

Swagger schema validator
MIT License
150 stars 76 forks source link

Handle x nullable for enums #191

Closed twiden closed 6 years ago

twiden commented 6 years ago

I've noticed that enums don't work very well with x-nullable=True. I had to include null/None as an enum value for the validation to pass, but that combo of x-nullable and enum value does not work with swagger-ui. So I suggest this change. costumes-for-dogs-banana-dog

twiden commented 6 years ago

Hi Could I get some feedback on this? It would be much appreciated

Thank you

pipermerriam commented 6 years ago

I'm hesitant to add this, at least as a feature that's enabled by default.

My reasoning is that IIUC x-nullable is not part of the core spec, and is more of a convention. Assuming that is correct, it strikes me that this feature should require explicit opt-in using something like an environment variable or configuration flag.

Thoughts?

twiden commented 6 years ago

From a user perspective I think that if I specify both x-nullable=True and enum=[x, y, z], I'm pretty explicit about null being an expected value. This does not add unexpected behaviour to an enum field, but makes the x-nullable extension apply correctly to enums (In my opinion)

However: I see your point about it not being in the core spec and you are the one who will have do deal with questions from users about this behaviour. So if you still think it should be disabled by default I suggest an environment variable X_NULLABLE_ENUMS or similar.

I can rewrite the PR with this change if you think that's the way to go.

pipermerriam commented 6 years ago

Alright, took a night to think it over.

I'd like to meet in the middle. Your user perspective argument holds water for me, but I want a way for people who want strict validation to be able to have it (i.e. not supporting x-nullable).

If you'll update this PR so that x-nullable support is enabled by default, but it can be disable if the environment variable FLEX_DISABLE_X_NULLABLE is set (to anything), then we can get this merged and released.

twiden commented 6 years ago

Good feedback :) Thanks!