[JAVA] Manage List<Integer> datatype for enum #75

Zomzog · 2018-05-16T18:46:44Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
Filed the PR against the correct branch: Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Add method call for manage datatype List<> when creating enum value in java.

Must fix #49.

Technical committee:
@bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01)

jmini

Thank you a lot for this contribution.

In general I do not think that re-parsing the type to get the type defined in the generic is a good approach. We have all the information we need, we just need to see how we can pass it to the method.

jmini · 2018-05-16T19:13:35Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java

+     * @return the contained datatype if datatype is List
+     */
+    private String sanitizeForEnum(String datatype){
+        Pattern p = Pattern.compile("List<(.*)>");


This will not work with array of array.

jmini · 2018-05-16T19:13:38Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java

@@ -1036,19 +1037,34 @@ public String toEnumVarName(String value, String datatype) {

    @Override
    public String toEnumValue(String value, String datatype) {
-        if ("Integer".equals(datatype) || "Double".equals(datatype)) {
+        String sanitzedDataType = sanitizeForEnum(datatype);


Who is calling this method?

I did not check it yet, I think we could add a parameter and provide the mostInnerItems type (will be added with #66)

The method is called into updateCodegenPropertyEnum.
I've done a second try where I search the most inner datatype on this method, so I think you're right, I can replace my call to findMostInnerDatatype method with a simple var.mostInnerItems.datatype with #66 .

jmini · 2018-05-17T14:25:17Z

This is a very nice contribution. Thank you a lot.

I have used this spec to test it:

openapi: 3.0.1
info:
  title: OpenAPI Tree Pots
  description: Example spec
  version: 1.0.0
servers:
  - url: 'http://api.company.xyz/v2'
paths:
  /pull75:
    get:
      operationId: op
      responses:
        '200':
          description: Ok
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/ObjWithEnums'
components:
  schemas:
    ObjWithEnums:
      type: object
      properties:
        IProp:
            $ref: "#/components/schemas/IntEnum"
        LProp:
            $ref: "#/components/schemas/LongEnum"
        SProp:
            $ref: "#/components/schemas/StringEnum"

    IntEnum:
      type: integer
      format: int32
      enum:
        - 1
        - 2
        - 3

    LongEnum:
      type: integer
      format: int64
      enum:
        - 20
        - 30
        - 40
        
    StringEnum:
      type: string
      enum:
        - "c"
        - "b"
        - "a"

With the java generator (directly no CLI or maven):

JavaClientCodegen config = new org.openapitools.codegen.languages.JavaClientCodegen();
config.setArtifactId(artifactId);
config.setJava8Mode(true);

Path outputDirPath = Paths.get(outputDir);

config.setHideGenerationTimestamp(true);
config.setOutputDir(outputDir);

final OpenAPIParser openApiParser = new OpenAPIParser();
final ParseOptions options = new ParseOptions();
final OpenAPI openAPI = openApiParser.readLocation(inputSpec, null, options).getOpenAPI();

final ClientOptInput opts = new ClientOptInput();
opts.setConfig(config);
opts.setOpenAPI(openAPI);
opts.setOpts(new ClientOpts());
new DefaultGenerator().opts(opts).generate();

And I get this compile error:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:compile (default-compile) on project ***: Compilation failure
[ERROR] /___/src/main/java/org/openapitools/client/model/IntEnum.java:[70,33] cannot find symbol
[ERROR]   symbol:   method nextInteger()
[ERROR]   location: variable jsonReader of type com.google.gson.stream.JsonReader

Corresponding code:

  public static class Adapter extends TypeAdapter<IntEnum> {
    @Override
    public void write(final JsonWriter jsonWriter, final IntEnum enumeration) throws IOException {
      jsonWriter.value(enumeration.getValue());
    }

    @Override
    public IntEnum read(final JsonReader jsonReader) throws IOException {
      Integer value = jsonReader.nextInteger(); //HERE ERROR, SHOULD BE "nextInt()"
      return IntEnum.fromValue(String.valueOf(value));
    }
  }

jmini

This is a nice contribution. I am looking forward being able to merge it.

We need to find a solution for the compilation error that is created (see previous comment)
I also found an import that now can be removed.

If you need help, let us know

jmini · 2018-05-18T15:15:20Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java

@@ -50,6 +50,7 @@
 import java.util.List;
 import java.util.ListIterator;
 import java.util.Map;
+import java.util.regex.Matcher;


I think this is an unused import

Zomzog · 2018-05-19T21:49:37Z

I've added the missing "isInteger" to the CodegenModel, but I'm not sure it's the right solution because it adds a java enum specific requirement into a generic model.

Test for OpenAPITools/openapi-generator#75

jmini · 2018-05-22T09:53:08Z

I've added the missing "isInteger" to the CodegenModel, but I'm not sure it's the right solution because it adds a java enum specific requirement into a generic model.

This is not a problem, in my opinion we should rework the Codegen classes to have more stuff in common (CodegenModel and CodegenProperty) => See #20

But I am not sure that the way you have implemented it will work. You should be aware that each language is allowed to change the value in dataType (it is mutable). By looking at the CodegenModel#dataType usage, some language are putting other values than "Integer" in that member.
I guess it would be safer to add a isInteger property, similar to CodegenProperty#isInteger. In this case, I think that you will not need a getter.

Zomzog · 2018-05-27T12:58:12Z

Ok, I've had it as property.

Maybe dataType must be wrap into a valueObject during #20 to simplify the model and avoid having the possibility of an Integer that respond "false" to isInteger.

jmini · 2018-05-30T05:01:22Z

Thank you for the changes.

I tested it with OAS3 spec mentioned in my comment, and I got several problems.

Java client (library okhttp-gson [default], `rest-assured):

  public static class Adapter extends TypeAdapter<LongEnum> {
    @Override
    public void write(final JsonWriter jsonWriter, final LongEnum enumeration) throws IOException {
      jsonWriter.value(enumeration.getValue());
    }

    @Override
    public LongEnum read(final JsonReader jsonReader) throws IOException {
      Long value = jsonReader.nextInt(); //COMPILE ERROR
      return LongEnum.fromValue(String.valueOf(value));
    }
  }

I did not look into the code, but maybe you will need isLong similar to isInteger.

Server (generator name jaxrs-cxf-cdi)

I got a lot of compile error for IntEnum.java and LongEnum.java. Here the file, with some comments (I did not analyze it in detail, feel free to do other changes in order for the files to compile):

@XmlType(name="")
@XmlEnum(Long.class)
public enum LongEnum {

    @XmlEnumValue(20l) NUMBER_20(Long.valueOf(20l)), @XmlEnumValue(30l) NUMBER_30(Long.valueOf(30l)), @XmlEnumValue(40l) NUMBER_40(Long.valueOf(40l));


    private Long value;

    LongEnum(Long v) {
        value = v;
    }

    public String value() { //RETURN TYPE SHOULD BE 'Long'
        return value;
    }

    @Override
    public String toString() {
        return String.valueOf(value);
    }

    public static LongEnum fromValue(String v) { //PARAMETER TYPE SHOULD BE 'Long'
        for (LongEnum b : LongEnum.values()) {
            if (String.valueOf(b.value).equals(v)) { //I THINK, SHOULD BE 'Long.valueOf(..)' ?

                return b;
            }
        }
        return null;
    }
}

Test for OpenAPITools/openapi-generator#75

Zomzog · 2018-06-12T21:54:18Z

Fixed for java, jaxrs-cxf-cdi and kotlin.

jmini · 2018-06-13T06:34:08Z

I will have a look at your changes later today. Thank you a lot.

jmini · 2018-06-13T13:12:13Z

@wing328, @jimschubert:

An addition on CodegenModel like this:

CodegenModel.isString
CodegenModel.isInteger

See:
https://github.com/Zomzog/openapi-generator/blob/e56a708531e375bec66c901ae41908f3192a2a9b/modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenModel.java#L47

Is that allowed for a patch release (3.0.x) or should we contribute this to 3.1.x because of the API change?

In my opinion it can be added, but I am not sure about the change policy.

jimschubert · 2018-06-13T15:41:56Z

@jmini I wouldn't consider this a breaking change unless it would cause custom templates with by users to break (as far as I can tell, that's not the case).

I do think it's weird adding Java specific code to the shared CodegenModel. When I've had to do things like this in Kotlin and C#, I've done it on those generators base types as a vendor extension. Putting it on CodegenModel means all templates can consume it and making changes will be difficult.

I would recommend that to keep this change, the public fields should be getter/setters.

wing328 · 2018-06-13T15:49:12Z

@Zomzog can you share a minimal spec to reproduce the issue? I may have a better approach to address the problem

jmini · 2018-06-13T17:39:38Z

@wing328 : what is wrong with the one I have published as comment in this PR: #75 (comment)

You have an enum for String (to ensure no regression), for Long and and for Integer.

I did not checked the last changes here, but each time I look at the changes @Zomzog is getting closer to the result.

Let me perform some tests before I can approve this PR, but the work so far looks good to me.

@jimschubert:

I do think it's weird adding Java specific code to the shared CodegenModel. When I've had to do things like this in Kotlin and C#, I've done it on those generators base types as a vendor extension. Putting it on CodegenModel means all templates can consume it and making changes will be difficult.

This is not java specific at all... It is valid OAS and needs potentially to be implemented everywhere. @Zomzog did a great job by implementing:

java
kotlin

Other language maintainers interested in such a feature will need to pick the new exposed stuff in their templates.

@jimschubert: thank you for your definition of a breaking change. As this is an addition only, we will be able to merge it on master.

jimschubert · 2018-06-13T20:15:30Z

@jmini looking at the generated code, I'd consider this a breaking change as users will no longer be able to generate Kotlin code without modifying existing code.

wing328 · 2018-06-14T05:49:34Z

@jmini thanks for the pointer. I'll use that to repeat the issue.

jmini · 2018-06-25T16:42:18Z

I have verified this change, I think it now works well. Thank you a lot for this contribution.

As discussed earlier here, I propose to merge it on 3.1.x.

@wing328:

I may have a better approach to address the problem

This change is well covered with unit tests, you can still improve the implementation later.

Conflicts: modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java modules/openapi-generator/src/test/java/org/openapitools/codegen/java/JavaClientCodegenTest.java

jmini · 2018-06-25T17:19:43Z

@Zomzog : In order to prepare the merge, I took the liberty to merge 3.1.x on your branch feature/49 as commit 6998a6a

Test for OpenAPITools/openapi-generator#75

jmini reviewed May 16, 2018

View reviewed changes

Zomzog force-pushed the feature/49 branch from dd7b88c to 64a9b30 Compare May 16, 2018 21:36

jmini requested changes May 18, 2018

View reviewed changes

Zomzog force-pushed the feature/49 branch from 64a9b30 to 45f3df3 Compare May 19, 2018 21:38

jmini added a commit to jmini/openapi-experiments that referenced this pull request May 20, 2018

Treepots: add enums of other types

a61b8de

Test for OpenAPITools/openapi-generator#75

Zomzog force-pushed the feature/49 branch from 45f3df3 to a24347a Compare May 20, 2018 20:18

jmini mentioned this pull request May 22, 2018

Code clean-up: refactor CodegenResponse, CodegenProperty and CodegenParameter #20

Open

Zomzog force-pushed the feature/49 branch from a24347a to c2735d2 Compare May 27, 2018 12:51

jmini added a commit to jmini/openapi-experiments that referenced this pull request May 30, 2018

Treepots: add enums of other types

3b328c7

Test for OpenAPITools/openapi-generator#75

Zomzog added 6 commits June 12, 2018 20:21

Sanitize enum datatype for List<Integer>

5d5dc94

Revert previous

d8f8a9d

Use most inner datatype for enum

93e54fd

Use mostInnerItems property + fix codegen missing isInteger

2ea9fbe

isInteger as property

c671370

Fix generation for java, jaxrs-cxf-cdi and kotlin

e56a708

Zomzog force-pushed the feature/49 branch from c2735d2 to e56a708 Compare June 12, 2018 21:52

wing328 added Feature: Enum Issue: Bug labels Jun 13, 2018

jmini changed the base branch from master to 3.1.x June 25, 2018 16:39

jmini approved these changes Jun 25, 2018

View reviewed changes

Merge remote-tracking branch 'origin/3.1.x' into pull_75

6998a6a

Conflicts: modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java modules/openapi-generator/src/test/java/org/openapitools/codegen/java/JavaClientCodegenTest.java

jmini merged commit b6717a5 into OpenAPITools:3.1.x Jun 27, 2018

etherealjoy mentioned this pull request Jun 27, 2018

[java] enum for int array initialized with strings #49

Closed

jmini added this to the 3.1.0 milestone Jul 2, 2018

jmini mentioned this pull request Jul 4, 2018

[JAVA] enums with number type are not supported #447

Closed

wing328 added Client: Java Client: Kotlin Server: Java labels Jul 6, 2018

jmini mentioned this pull request Jul 9, 2018

[Discussion] Generator Compatibility Check and Table #503

Open

jmini added a commit to jmini/openapi-experiments that referenced this pull request Jul 16, 2018

Treepots: add enums of other types

0c11589

Test for OpenAPITools/openapi-generator#75

Zomzog deleted the feature/49 branch January 27, 2021 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JAVA] Manage List<Integer> datatype for enum #75

[JAVA] Manage List<Integer> datatype for enum #75

Zomzog commented May 16, 2018

jmini left a comment

jmini May 16, 2018

jmini May 16, 2018

Zomzog May 16, 2018

jmini commented May 17, 2018

jmini left a comment

jmini May 18, 2018

Zomzog commented May 19, 2018

jmini commented May 22, 2018

Zomzog commented May 27, 2018

jmini commented May 30, 2018

Zomzog commented Jun 12, 2018

jmini commented Jun 13, 2018

jmini commented Jun 13, 2018

jimschubert commented Jun 13, 2018

wing328 commented Jun 13, 2018

jmini commented Jun 13, 2018

jimschubert commented Jun 13, 2018

wing328 commented Jun 14, 2018

jmini commented Jun 25, 2018

jmini commented Jun 25, 2018

[JAVA] Manage List<Integer> datatype for enum #75

[JAVA] Manage List<Integer> datatype for enum #75

Conversation

Zomzog commented May 16, 2018

PR checklist

Description of the PR

jmini left a comment

Choose a reason for hiding this comment

jmini May 16, 2018

Choose a reason for hiding this comment

jmini May 16, 2018

Choose a reason for hiding this comment

Zomzog May 16, 2018

Choose a reason for hiding this comment

jmini commented May 17, 2018

jmini left a comment

Choose a reason for hiding this comment

jmini May 18, 2018

Choose a reason for hiding this comment

Zomzog commented May 19, 2018

jmini commented May 22, 2018

Zomzog commented May 27, 2018

jmini commented May 30, 2018

Zomzog commented Jun 12, 2018

jmini commented Jun 13, 2018

jmini commented Jun 13, 2018

jimschubert commented Jun 13, 2018

wing328 commented Jun 13, 2018

jmini commented Jun 13, 2018

jimschubert commented Jun 13, 2018

wing328 commented Jun 14, 2018

jmini commented Jun 25, 2018

jmini commented Jun 25, 2018