#6545: Fixing several bugs for Java model generation for enums #6579
Conversation
…AbstractJavaCodegen
…ts for expected enum methods
|
Something is wrong with the Travis build. It's always timeouting. |
| {"LongEnum", false, false, "long", Arrays.asList(1, 2, 3) /* this seems wrong, the SwaggerParser should read the enum values as Long not as Integer */}, | ||
| {"FloatEnum", false, true, "float", Arrays.asList("1", "2", "3") /* this seems wrong, the SwaggerParser should read the enum values as Float not as String */}, | ||
| {"DoubleEnum", false, false, "double", Arrays.asList("1", "2", "3") /* this seems wrong, the SwaggerParser should read the enum values as Double not as String */}, | ||
| {"IntegerEnum", true, false, "integer", Arrays.asList("1", "2", "3") /* @see: https://github.com/swagger-api/swagger-core/issues/2449 */}, |
There was a problem hiding this comment.
i'm not sure that we need those links in comments
There was a problem hiding this comment.
If this test fails it is probably because the mentioned bug was solved and the swagger-core was updated in the swagger-codegen.
This link shall help to understand that there was a bug which was solved and because of that the test is now failing and can be corrected.
Without that link the contributor can only guess how the tests might be.
But I can remove the link and write a big fat comment explaining the whole situation and that test might fail in the future and what to do if that happens. I thought a link might shorter and is more capable of explaining the test situation.
| @@ -60,7 +60,7 @@ public enum {{#datatypeWithEnum}}{{{.}}}{{/datatypeWithEnum}}{{^datatypeWithEnum | |||
|
|
|||
| @Override | |||
| public {{#datatypeWithEnum}}{{{.}}}{{/datatypeWithEnum}}{{^datatypeWithEnum}}{{{classname}}}{{/datatypeWithEnum}} read(final JsonReader jsonReader) throws IOException { | |||
| {{{dataType}}} value = jsonReader.{{#isInteger}}nextInt(){{/isInteger}}{{^isInteger}}next{{{dataType}}}(){{/isInteger}}; | |||
| {{{dataType}}} value = {{#isFloat}}new Float(jsonReader.nextDouble()); // floats shall be restricted so this cast is safe{{/isFloat}}{{^isFloat}}jsonReader.{{#isInteger}}nextInt(){{/isInteger}}{{^isInteger}}next{{{dataType}}}(){{/isInteger}};{{/isFloat}} | |||
There was a problem hiding this comment.
Can we overflow here while converting double => float?
There was a problem hiding this comment.
Yes. new Float(Double.MAX_VALUE) will result in Infinity.
That is not the best solution.
To make it better the JsonReader functionality has to be approximately reimplemented. Something like this:
final double nextDouble = jsonReader.nextDouble();
float result = (float) nextDouble;
if (Float.isInfinite(result)) {
throw new MalformedJsonException("JSON forbids infinities: " + result + " for " + nextDouble);
}(Compare with: https://github.com/google/gson/blob/b1fb9ca9a1bea5440bc6a5b506ccf67236b08243/gson/src/main/java/com/google/gson/stream/JsonReader.java#L909)
But this seems like overkill.
My thoughts were:
In case a float value is restricted by enumerations this method will result in null if the enum value can't be matched. If a decimal number is to big for float, leading to Infinity, the value can't be matched and the result will be null, too.
There was a problem hiding this comment.
But this seems like overkill.
ok, seems reasonable.
| @@ -41,6 +41,7 @@ | |||
|
|
|||
| public Set<String> imports = new TreeSet<String>(); | |||
| public boolean hasVars, emptyVars, hasMoreModels, hasEnums, isEnum, hasRequired, hasOptional, isArrayModel, hasChildren; | |||
| public boolean isInteger, isFloat; | |||
There was a problem hiding this comment.
This flag names a bit misleading for me (same names exists in CodegenProperty), may be isFloatEnum/isIntegerEnum?
There was a problem hiding this comment.
Yes, this flag also exists in CodegenProperty. And it has the same name as it has the same meaning.
Imaging this definition:
definitions:
IntegerValue:
type: integer
format: int32
Object:
type: object
properties:
IntegerValue:
type: integer
format: int32
The outer IntegerValue is an integer, so the flag isInteger == true.
The inner IntegerValue is an integer, so the flag isInteger == true.
Why should I call the flag differently?
There was a problem hiding this comment.
Previously we encounter the use case with array of array of enum. To make it easier to check whether the inner type is an integer, we use isInteger. If we need to ensure it's not an enum, we can {{^isEnum}} .. {{/isEnum}}
I agree it may be a bit misleading and it's tradeoff to make it easier to handle 2D or 3D array of enum
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates).2.3.0branch for changes related to OpenAPI spec v?. Default:masterFiled the PR against the correct branch:3.0.0branch for changes related to OpenAPI spec 3.0. Default:master.Description of the PR
fixes #6545