Skip to content

#6545: Fixing several bugs for Java model generation for enums #6579

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

#6545: Fixing several bugs for Java model generation for enums #6579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
be86ef1
#6545: add extensive enum definitions into fake petstore
sdoeringNew Sep 27, 2017
faae261
#6545: set correct values for enums on DefaultCodegen and AbstractJav…
sdoeringNew Sep 27, 2017
464e93d
#6545: updated mustaches to changed codegen values
sdoeringNew Sep 27, 2017
d11df6f
#6545: updated jersey1 and okhttp-gson samples, adding tests for expe…
sdoeringNew Sep 27, 2017
04b61a0
#6545: used correct swagger-core version for changed tests
sdoeringNew Sep 27, 2017
7864eee
fixed typo to trigger randomly failed build again
sdoeringNew Sep 27, 2017
5f82d88
#6545: updated all other java-petstore samples
sdoeringNew Sep 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
7 changes: 7 additions & 0 deletions modules/swagger-codegen/src/main/java/io/swagger/codegen/CodegenModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public class CodegenModel {

public Set<String> imports = new TreeSet<String>();
public boolean hasVars, emptyVars, hasMoreModels, hasEnums, isEnum, hasRequired, hasOptional, isArrayModel, hasChildren;
public boolean isInteger, isFloat;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag names a bit misleading for me (same names exists in CodegenProperty), may be isFloatEnum/isIntegerEnum?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wing328 plz advice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public boolean hasOnlyReadOnly = true; // true if all properties are read-only
public ExternalDocs externalDocs;

Expand Down Expand Up @@ -132,6 +133,10 @@ public boolean equals(Object o) {
return false;
if (isEnum != that.isEnum)
return false;
if (isInteger != that.isInteger)
return false;
if (isFloat != that.isFloat)
return false;
if (externalDocs != null ? !externalDocs.equals(that.externalDocs) : that.externalDocs != null)
return false;
if (!Objects.equals(hasOnlyReadOnly, that.hasOnlyReadOnly))
Expand Down Expand Up @@ -178,6 +183,8 @@ public int hashCode() {
result = 31 * result + (hasMoreModels ? 13:31);
result = 31 * result + (hasEnums ? 13:31);
result = 31 * result + (isEnum ? 13:31);
result = 31 * result + (isInteger ? 13:31);
result = 31 * result + (isFloat ? 13:31);
result = 31 * result + (externalDocs != null ? externalDocs.hashCode() : 0);
result = 31 * result + (vendorExtensions != null ? vendorExtensions.hashCode() : 0);
result = 31 * result + Objects.hash(hasOnlyReadOnly);
Expand Down
2 changes: 1 addition & 1 deletion modules/swagger-codegen/src/main/java/io/swagger/codegen/CodegenProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public class CodegenProperty implements Cloneable {
public boolean exclusiveMinimum;
public boolean exclusiveMaximum;
public boolean hasMore, required, secondaryParam;
public boolean hasMoreNonReadOnly; // for model constructor, true if next properyt is not readonly
public boolean hasMoreNonReadOnly; // for model constructor, true if next property is not readonly
public boolean isPrimitiveType, isContainer, isNotContainer;
public boolean isString, isNumeric, isInteger, isLong, isFloat, isDouble, isByteArray, isBinary, isFile, isBoolean, isDate, isDateTime, isUuid;
public boolean isListContainer, isMapContainer;
Expand Down
5 changes: 4 additions & 1 deletion modules/swagger-codegen/src/main/java/io/swagger/codegen/DefaultCodegen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1448,7 +1448,10 @@ public CodegenModel fromModel(String name, Model model, Map<String, Model> allDe
typeAliases.put(name, impl.getType());
m.isAlias = true;
}
m.dataType = getSwaggerType(p);
final String dataType = getSwaggerType(p);
m.dataType = dataType;
m.isInteger = "Integer".equalsIgnoreCase(dataType);
m.isFloat = "Float".equalsIgnoreCase(dataType);
}
if(impl.getEnum() != null && impl.getEnum().size() > 0) {
m.isEnum = true;
Expand Down
64 changes: 51 additions & 13 deletions modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/AbstractJavaCodegen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,7 @@ public String toDefaultValue(Property p) {
} else if (p instanceof LongProperty) {
LongProperty dp = (LongProperty) p;
if (dp.getDefault() != null) {
return dp.getDefault().toString()+"l";
return dp.getDefault().toString()+"L";
}
return "null";
} else if (p instanceof DoubleProperty) {
Expand Down Expand Up @@ -980,9 +980,9 @@ public String toEnumVarName(String value, String datatype) {
}

// number
if ("Integer".equals(datatype) || "Long".equals(datatype) ||
"Float".equals(datatype) || "Double".equals(datatype)) {
String varName = "NUMBER_" + value;
if (representsInteger(datatype) || representsLong(datatype) ||
representsFloat(datatype) || representsDouble(datatype)) {
String varName = "NUMBER_" + forceDotOnDecimal(value, datatype);
varName = varName.replaceAll("-", "MINUS_");
varName = varName.replaceAll("\\+", "PLUS_");
varName = varName.replaceAll("\\.", "_DOT_");
Expand All @@ -999,17 +999,21 @@ public String toEnumVarName(String value, String datatype) {
}

@Override
public String toEnumValue(String value, String datatype) {
if ("Integer".equals(datatype) || "Double".equals(datatype)) {
return value;
} else if ("Long".equals(datatype)) {
// add l to number, e.g. 2048 => 2048l
return value + "l";
} else if ("Float".equals(datatype)) {
public String toEnumValue(final String value, final String datatype) {
final String varValue = forceDotOnDecimal(value, datatype);
if (representsInteger(datatype)) {
return varValue;
} else if (representsLong(datatype)) {
// add L to number, e.g. 2048 => 2048L
return varValue + "L";
} else if (representsDouble(datatype)) {
// add d to number, e.g. 3.14 => 3.14d
return varValue + "d";
} else if (representsFloat(datatype)) {
// add f to number, e.g. 3.14 => 3.14f
return value + "f";
return varValue + "f";
} else {
return "\"" + escapeText(value) + "\"";
return "\"" + escapeText(varValue) + "\"";
}
}

Expand All @@ -1020,6 +1024,40 @@ public CodegenOperation fromOperation(String path, String httpMethod, Operation
return op;
}

/**
* Depending on a property or model the values for enums will be differently read.
* To ensure equality between models, which are enums, and inner enum as properties this
* method will force a decimal point on decimal numbers if it doesn't contain it, yet.
* <p>
* So decimal enums will always look the same in the generated model:
* 1 => NUMBER_1_DOT_0(1.0d)
* 1.0 => NUMBER_1_DOT_0(1.0d)
*/
private static String forceDotOnDecimal(final String value, final String datatype) {
if (representsFloat(datatype) || representsDouble(datatype)) {
if (!value.contains(".")) {
return value + ".0";
}
}
return value;
}

private static boolean representsInteger(final String datatype) {
return "Integer".equals(datatype) || "List<Integer>".equals(datatype);
}

private static boolean representsDouble(final String datatype) {
return "Double".equals(datatype) || "List<Double>".equals(datatype);
}

private static boolean representsFloat(final String datatype) {
return "Float".equals(datatype) || "List<Float>".equals(datatype);
}

private static boolean representsLong(final String datatype) {
return "Long".equals(datatype) || "List<Long>".equals(datatype);
}

private static CodegenModel reconcileInlineEnums(CodegenModel codegenModel, CodegenModel parentCodegenModel) {
// This generator uses inline classes to define enums, which breaks when
// dealing with models that have subTypes. To clean this up, we will analyze
Expand Down
2 changes: 1 addition & 1 deletion modules/swagger-codegen/src/main/resources/Java/modelEnum.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we overflow here while converting double => float?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this seems like overkill.

ok, seems reasonable.

return {{#datatypeWithEnum}}{{{.}}}{{/datatypeWithEnum}}{{^datatypeWithEnum}}{{{classname}}}{{/datatypeWithEnum}}.fromValue(String.valueOf(value));
}
}
Expand Down
2 changes: 1 addition & 1 deletion modules/swagger-codegen/src/main/resources/Java/modelInnerEnum.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@

@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}}
return {{#datatypeWithEnum}}{{{.}}}{{/datatypeWithEnum}}{{^datatypeWithEnum}}{{classname}}{{/datatypeWithEnum}}.fromValue(String.valueOf(value));
}
}
Expand Down
127 changes: 121 additions & 6 deletions modules/swagger-codegen/src/test/java/io/swagger/codegen/CodegenTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,18 @@
import io.swagger.parser.SwaggerParser;

import org.testng.Assert;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

import java.lang.reflect.Field;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Sets;

public class CodegenTest {

Expand Down Expand Up @@ -217,7 +226,7 @@ public void fileResponeseTest() {
Assert.assertTrue(op.responses.get(0).isFile);
Assert.assertTrue(op.isResponseFile);
}

@Test(description = "discriminator is present")
public void discriminatorTest() {
final Swagger model = parseAndPrepareSwagger("src/test/resources/2_0/discriminatorTest.json");
Expand Down Expand Up @@ -359,39 +368,39 @@ public void localConsumesAndProducesTest() {
final String path = "/tests/localConsumesAndProduces";
final Operation p = model.getPaths().get(path).getGet();
CodegenOperation op = codegen.fromOperation(path, "get", p, model.getDefinitions(), model);

Assert.assertTrue(op.hasConsumes);
Assert.assertEquals(op.consumes.size(), 1);
Assert.assertEquals(op.consumes.get(0).get("mediaType"), "application/json");
Assert.assertTrue(op.hasProduces);
Assert.assertEquals(op.produces.size(), 1);
Assert.assertEquals(op.produces.get(0).get("mediaType"), "application/json");
}

@Test(description = "use spec consumes and produces")
public void globalConsumesAndProducesTest() {
final Swagger model = parseAndPrepareSwagger("src/test/resources/2_0/globalConsumesAndProduces.json");
final DefaultCodegen codegen = new DefaultCodegen();
final String path = "/tests/globalConsumesAndProduces";
final Operation p = model.getPaths().get(path).getGet();
CodegenOperation op = codegen.fromOperation(path, "get", p, model.getDefinitions(), model);

Assert.assertTrue(op.hasConsumes);
Assert.assertEquals(op.consumes.size(), 1);
Assert.assertEquals(op.consumes.get(0).get("mediaType"), "application/global_consumes");
Assert.assertTrue(op.hasProduces);
Assert.assertEquals(op.produces.size(), 1);
Assert.assertEquals(op.produces.get(0).get("mediaType"), "application/global_produces");
}

@Test(description = "use operation consumes and produces (reset in operation with empty array)")
public void localResetConsumesAndProducesTest() {
final Swagger model = parseAndPrepareSwagger("src/test/resources/2_0/globalConsumesAndProduces.json");
final DefaultCodegen codegen = new DefaultCodegen();
final String path = "/tests/localResetConsumesAndProduces";
final Operation p = model.getPaths().get(path).getGet();
CodegenOperation op = codegen.fromOperation(path, "get", p, model.getDefinitions(), model);

Assert.assertNotNull(op);
Assert.assertFalse(op.hasConsumes);
Assert.assertNull(op.consumes);
Expand All @@ -417,4 +426,110 @@ public void deprecatedParamTest() {

Assert.assertTrue(op.isDeprecated);
}

@Test(description = "Setting the values for defined object")
public void fromModelObjectTest() throws NoSuchFieldException, IllegalAccessException {
final Swagger swagger = parseAndPrepareSwagger("src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml");
final Model model = swagger.getDefinitions().get("Category");
final CodegenModel codegenModel = new DefaultCodegen().fromModel("Category", model, swagger.getDefinitions());

// test the not set fields
for (final String fieldName : Arrays.asList("parent", "parentSchema", "interfaces", "parentModel", "interfaceModels",
"children", "title", "description", "xmlPrefix", "xmlNamespace", "unescapedDescription", "discriminator",
"defaultValue", "arrayModelType", "allowableValues", "externalDocs", "additionalPropertiesType")) {
final Field field = CodegenModel.class.getField(fieldName);
Assert.assertNull(field.get(codegenModel), "Field: " + fieldName);
}
// test the boolean fields
for (final String fieldName : Arrays.asList("isAlias", "emptyVars", "hasMoreModels", "hasEnums", "isEnum",
"hasRequired", "isArrayModel", "hasChildren", "isInteger", "isFloat", "hasOnlyReadOnly")) {
final Field field = CodegenModel.class.getField(fieldName);
Assert.assertFalse(field.getBoolean(codegenModel), "Field: " + fieldName);
}
// test the variables fields - they have all the same content
for (final String varsFieldName : Arrays.asList("vars", "optionalVars", "readWriteVars", "allVars")) {
final Field field = CodegenModel.class.getField(varsFieldName);
final List<CodegenProperty> vars = (List<CodegenProperty>) field.get(codegenModel);

final Set<List<String>> variables = new HashSet<>();
for (final CodegenProperty codegenProperty : vars) {
variables.add(Arrays.asList(codegenProperty.baseName, codegenProperty.datatype));
}
Assert.assertEquals(variables, new HashSet<>(
Arrays.asList(Arrays.asList("id", "Long"), Arrays.asList("name", "String"))
), "Field: " + varsFieldName);
}
// test the class names which shall all be "Category"
for (final String fieldName : Arrays.asList("name", "classname", "classVarName", "xmlName", "classFilename")) {
final Field field = CodegenModel.class.getField(fieldName);
Assert.assertEquals(field.get(codegenModel), "Category", "Field: " + fieldName);
}
// test the empty collections
for (final String fieldName : Arrays.asList("requiredVars", "readOnlyVars", "parentVars", "mandatory", "allMandatory")) {
final Field field = CodegenModel.class.getField(fieldName);
final Collection collection = (Collection) field.get(codegenModel);
Assert.assertTrue(collection.isEmpty(), "Field: " + fieldName);
}

Assert.assertEquals(codegenModel.dataType, "object");
Assert.assertTrue(codegenModel.hasVars);
Assert.assertTrue(codegenModel.hasOptional);
Assert.assertTrue(codegenModel.vendorExtensions.isEmpty());
Assert.assertEquals(codegenModel.imports, Sets.newHashSet("string"));
Assert.assertNotNull(codegenModel.modelJson);
}

@DataProvider(name = "fromModelEnumTest")
public static Object[][] fromModelEnumTest() {
return new Object[][]{
{"IntegerEnum", true, false, "integer", Arrays.asList("1", "2", "3") /* @see: https://github.com/swagger-api/swagger-core/issues/2449 */},
Copy link
Contributor

@lukoyanov lukoyanov Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure that we need those links in comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{"LongEnum", false, false, "long", Arrays.asList("1", "2", "3") /* @see: https://github.com/swagger-api/swagger-core/issues/2449 */},
{"FloatEnum", false, true, "float", Arrays.asList("1", "2", "3") /* @see: https://github.com/swagger-api/swagger-core/issues/2449 */},
{"DoubleEnum", false, false, "double", Arrays.asList("1", "2", "3") /* @see: https://github.com/swagger-api/swagger-core/issues/2449 */},
};
}

@Test(description = "Setting the values for defined enum", dataProvider = "fromModelEnumTest")
public void fromModelEnumTest(final String modelName, final boolean isInteger, final boolean isFloat,
final String datatype, final List allowableValues) throws NoSuchFieldException, IllegalAccessException {
final Swagger swagger = parseAndPrepareSwagger("src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml");
final Model model = swagger.getDefinitions().get(modelName);
final CodegenModel codegenModel = new DefaultCodegen().fromModel(modelName, model, swagger.getDefinitions());

// test the not set fields
for (final String fieldName : Arrays.asList("parent", "parentSchema", "interfaces", "parentModel", "interfaceModels",
"children", "title", "description", "xmlPrefix", "xmlNamespace", "unescapedDescription", "discriminator",
"defaultValue", "arrayModelType", "externalDocs", "additionalPropertiesType", "xmlName")) {
final Field field = CodegenModel.class.getField(fieldName);
Assert.assertNull(field.get(codegenModel), "Field: " + fieldName);
}
// test the boolean fields
for (final String fieldName : Arrays.asList("isAlias", "hasMoreModels", "hasEnums",
"hasRequired", "isArrayModel", "hasChildren", "hasVars", "hasOptional")) {
final Field field = CodegenModel.class.getField(fieldName);
Assert.assertFalse(field.getBoolean(codegenModel), "Field: " + fieldName);
}
// test the class names which shall all be "${modelName}"
for (final String fieldName : Arrays.asList("name", "classname", "classVarName", "classFilename")) {
final Field field = CodegenModel.class.getField(fieldName);
Assert.assertEquals(field.get(codegenModel), modelName, "Field: " + fieldName);
}
// test the empty collections
for (final String fieldName : Arrays.asList("vars", "optionalVars", "readWriteVars", "allVars", "requiredVars",
"readOnlyVars", "parentVars", "mandatory", "allMandatory", "imports")) {
final Field field = CodegenModel.class.getField(fieldName);
final Collection collection = (Collection) field.get(codegenModel);
Assert.assertTrue(collection.isEmpty(), "Field: " + fieldName);
}

Assert.assertEquals(codegenModel.dataType, datatype);
Assert.assertEquals(codegenModel.isInteger, isInteger);
Assert.assertEquals(codegenModel.isFloat, isFloat);
Assert.assertTrue(codegenModel.isEnum);
Assert.assertTrue(codegenModel.emptyVars);
Assert.assertTrue(codegenModel.hasOnlyReadOnly);
Assert.assertTrue(codegenModel.vendorExtensions.isEmpty());
Assert.assertEquals(codegenModel.allowableValues, ImmutableMap.of("values", allowableValues));
Assert.assertNotNull(codegenModel.modelJson);
}
}
Loading