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 parsing of non-string enum values #363

Merged
merged 1 commit into from
Dec 24, 2016
Merged

Fix parsing of non-string enum values #363

merged 1 commit into from
Dec 24, 2016

Conversation

kevinoid
Copy link
Contributor

JSON Schema v4 allows enum to contain values of any type, explicitly including null. Swagger v2.0 adopts this definition without change (as far as I am aware) but the parser previously parsed any non-string values as null. This PR fixes this to return the string representation of non-string primitive values and to warn about unsupported non-primitive values and add test cases to cover these.

For reference, I discovered this issue when attempting to run swagger-codegen against a Swagger declaration with an integer enumeration, which produced the following error:

Exception in thread "main" java.lang.RuntimeException: Could not process model 'MyEnum'.Please make sure that your schema is correct!
        at io.swagger.codegen.DefaultGenerator.generateModels(DefaultGenerator.java:290)
        at io.swagger.codegen.DefaultGenerator.generate(DefaultGenerator.java:647)
        at io.swagger.codegen.cmd.Generate.run(Generate.java:223)
        at io.swagger.codegen.SwaggerCodegen.main(SwaggerCodegen.java:41)
Caused by: java.lang.NullPointerException
        at io.swagger.codegen.DefaultCodegen.postProcessModelsEnum(DefaultCodegen.java:229)
        at io.swagger.codegen.languages.JavaClientCodegen.postProcessModelsEnum(JavaClientCodegen.java:316)
        at io.swagger.codegen.languages.AbstractJavaCodegen.postProcessModels(AbstractJavaCodegen.java:719)
        at io.swagger.codegen.DefaultGenerator.processModels(DefaultGenerator.java:919)
        at io.swagger.codegen.DefaultGenerator.generateModels(DefaultGenerator.java:285)
        ... 3 more

@fehguy
Copy link
Contributor

fehguy commented Dec 22, 2016

I believe this is not legal in the spec. @webron can you confirm?

@kevinoid
Copy link
Contributor Author

@fehguy In the JSON Schema spec or the Open API Spec? Could you provide a link or an explanation for why you think so?

@fehguy
Copy link
Contributor

fehguy commented Dec 22, 2016

OpenAPI spec is what the parser works towards, that is the bible for us. As I said, @webron can help point to the language or clarify any ambiguity

@webron
Copy link
Contributor

webron commented Dec 22, 2016

So there are a few parts in the spec that are unfortunately not documented properly, however, not everything that @kevinoid provided is actually supported.

Since null is not a valid value by the spec, it is not valid in enums.
Since mixed types are not supported by the spec, it can be inferred that mixed types for enums are also not supported. This is not clear in the spec, and will be clarified in the next version.

That said, the spec does support homogeneous enums, regardless of the type. So enums of strings, integers, numbers (and, well, even booleans) are supported.

@kevinoid
Copy link
Contributor Author

Thanks @webron, I'll update the test to remove MixedEnum. Could you point me to where in the spec it says that null is not a valid value or that mixed types are not supported? I must have overlooked that part somehow.

JSON Schema v4 allows enum to contain values of any type, but the parser
previously parsed any non-string values as null.  Fix this to return the
string representation of non-string primitive values and to warn about
unsupported non-primitive values.

Add tests to cover these cases.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
@webron
Copy link
Contributor

webron commented Dec 22, 2016

The lack of support for null is a bit hidden, but is suggested in https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#parameterType. While the Schema Object does not document the same restriction, that was the intent.

As mentioned in the original comment, the spec does not document that mixed types are not supported, but they are not.

@kevinoid
Copy link
Contributor Author

Ah, I see. That explains how I missed it. Thanks @webron!

Also, PR updated. Any remaining issues for me to address?

@fehguy fehguy merged commit df14c69 into swagger-api:master Dec 24, 2016
@fehguy fehguy modified the milestone: v1.0.25 Dec 27, 2016
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

Successfully merging this pull request may close these issues.

3 participants