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

Nullable enums with an empty string fail to get generated #118

Closed
czechboy0 opened this issue Jul 14, 2023 · 4 comments · Fixed by #176
Closed

Nullable enums with an empty string fail to get generated #118

czechboy0 opened this issue Jul 14, 2023 · 4 comments · Fixed by #176
Assignees
Labels
area/generator Affects: plugin, CLI, config file. area/openapi Adding/updating a feature defined in OpenAPI. size/S Small task. (A couple of hours of work.)

Comments

@czechboy0
Copy link
Contributor

In #59, we added support for an empty string as a valid case in a string enum, however, we only tested it on enum schemas not marked as nullable.

Turns out, that when marked as nullable, OpenAPIKit parses the values slightly differently, and treats the empty string as a nil string instead. That nil value, when converted to OpenAPIKit's AnyCodable, gets turned into a Void.

Before we introduce a workaround in the generator, @mattpolzin is this intentional behavior, and if so, can you say more about it please?

@czechboy0 czechboy0 added the status/triage Collecting information required to triage the issue. label Jul 14, 2023
@czechboy0
Copy link
Contributor Author

To reproduce, we can just use:

type: string
nullable: true
enum:
  - foo
  - ""

The second enum value will be an empty string when the schema is not nullable, and Void when the schema is nullable.

@mattpolzin
Copy link

Unfortunately this is a known issue with Yams. jpsim/Yams#301

OpenAPIKit has special case decoding of nullable string enums because otherwise a null case gets read in as the string "null" instead of the value null, but it is unintentional that the empty string also gets read in as null.

There is a proposed fix for the issue against Yams and a fork with that fix under the CreateAPI project but no indication of whether the fix will be upstreamed.

@czechboy0
Copy link
Contributor Author

Thanks for the find, @mattpolzin - I wonder if @jpsim can tell us more about the prioritization of landing that fix?

@czechboy0 czechboy0 added status/blocked Waiting for another issue. and removed status/triage Collecting information required to triage the issue. labels Jul 14, 2023
@czechboy0
Copy link
Contributor Author

I suspect we'll need to check for this ourselves in the generator, it's not a big deal, just wanted to try to see if Yams could fix it, but I suspect that'd be a breaking change anyway, so might not be likely.

@czechboy0 czechboy0 added area/generator Affects: plugin, CLI, config file. area/openapi Adding/updating a feature defined in OpenAPI. size/S Small task. (A couple of hours of work.) and removed status/blocked Waiting for another issue. labels Jul 28, 2023
@czechboy0 czechboy0 self-assigned this Aug 8, 2023
czechboy0 added a commit that referenced this issue Aug 8, 2023
Nullable enums with an empty string fail to get generated

### Motivation

Fixes #118.

The issue is that when a `type: string, enum: ...` schema is marked as `nullable: true`, _empty cases are parsed as Void_ instead of _nil_. See the issue for the investigation and identifying the root cause, but the TL;DR is that we need a workaround in the generator in the short term, and it's uncertain if the issue will ever be addressed in our dependency Yams (might be unlikely, as it'd probably be a breaking change for them at this point).

### Modifications

This PR adds code that catches this case and converts the `Void` into an empty string, which is what the OpenAPI document author actually put into their document in the first place.

### Result

Now `nullable: true` string enums with an empty case can be generated correctly. One example of such use is the [GitHub API](https://github.com/github/rest-api-description).

Note that this PR isn't really adding support for nullable schemas in general, that's tracked by #82. We're just working around one specific scenario in which a relatively common input trips up the generator.

### Test Plan

Added a unit test for each of: non-nullable and nullable variants.


Reviewed by: gjcairo, simonjbeaumont

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (docc test) - Build finished. 
     ✔︎ pull request validation (integration test) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#176
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/generator Affects: plugin, CLI, config file. area/openapi Adding/updating a feature defined in OpenAPI. size/S Small task. (A couple of hours of work.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants