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

fix(typed-enum): respect nullable in case of OAS3 document #1376

Merged
merged 2 commits into from
Oct 8, 2020

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented Oct 5, 2020

Fixes #1353.

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

@P0lip P0lip added the t/bug Something isn't working label Oct 5, 2020
@P0lip P0lip requested a review from philsturgeon October 5, 2020 18:10
@P0lip P0lip self-assigned this Oct 5, 2020
@P0lip P0lip changed the title fix: typed-enum should respect nullable in case of OAS3 document fix(typed-enum): respect nullable in case of OAS3 document Oct 5, 2020
@philsturgeon
Copy link
Contributor

I'm not sure I understand this implementation. It seems like OAS2 says no this is not ok (because it doesnt support nullable) and OAS3 says "yeah this is fine" because it does support nullable, and that enum is accepting null as a deviation to "type: string" because nullable is there...

I dont think this is the right solution.

I think the enum should not have null in it, that seems more like user error. The enum constricts the strings that can be used as a value, and nullable makes null be another value that could be used, but that doesn't mean null is now a valid enum value. It can't be, because everything in that enum should be a string...

@P0lip
Copy link
Contributor Author

P0lip commented Oct 5, 2020

I'm not sure I understand this implementation. It seems like OAS2 says no this is not ok (because it doesnt support nullable) and OAS3 says "yeah this is fine" because it does support nullable, and that enum is accepting null as a deviation to "type: string" because nullable is there.

This is what we do in case of schema function.

I think the enum should not have null in it, that seems more like user error

Why so? AFAIK type cannot be an array in case of OAS3, therefore user has to rely on nullable as there is no way to specify "type": ["null", "string"].

I'm not sure how nullable works in case of enum, however, so I might be wrong.

@philsturgeon
Copy link
Contributor

I've read a whole lot of back and forth on this issue but it seems like what you've done is correct, but keep in mind this isn't anything to do with type arrays.

image

From here: https://github.com/OAI/OpenAPI-Specification/blob/master/proposals/003_Clarify-Nullable.md

It seems like nullable: true will only work for a type: string with an enum if null is specified as an option. Who knew?!

If that is what this code is doing, please carry on, but the OAS2 example should maybe use x-nullable as thats a very common vendor extension in OAS2. OAS2 is barely usable without it.

@philsturgeon
Copy link
Contributor

We can go ahead with this. OAS2 users with x-nullable: true might notice trouble here, but it might not be a thing.

@P0lip
Copy link
Contributor Author

P0lip commented Oct 8, 2020

If that is what this code is doing, please carry on, but the OAS2 example should maybe use x-nullable as thats a very common vendor extension in OAS2. OAS2 is barely usable without it.

Ah yeah, I think we actually even have a bug report for it.
#1359
I'll fix it in a separate PR, since I'll need to touch other parts of code.

@P0lip P0lip merged commit 9ba7bbf into develop Oct 8, 2020
@P0lip P0lip deleted the fix/typed-enum-support-nullable branch October 8, 2020 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nullable enum warning for null value in example section
2 participants