[Java] Generated code does not compile when using enums in inherited models

[who]

Given this spec snippet

{
    "SuperObject": {
        "type": "object",
        "properties": {
            "thingType": {
                "type": "string",
                "enum": [
                    "FOO",
                    "BAR"
                ]
            }
        }
    },
    "SubObject": {
        "allOf": [
            {
                "$ref": "#/definitions/SuperObject"
            },
            {
                "type": "object",
                "properties": {
                    "thingType": {
                        "type": "string",
                        "enum": [
                            "FOO",
                            "BAR"
                        ]
                    },
                    "anotherProperty": {
                        "type": "string"
                    }
                }
            }
        ]
    }
}

The swagger-codegen output for java will not compile. It generates an enum for both the parent and the child class, and generates a getter&setter for each. However, this will not compile in java, because the return type of the enum getter in the subtype is not equal to the enum type in the parent class, and therefore the override is not proper.

[who]

Here's a repro:

me@box:/tmp/tmp.SUB54PNhX$ cd `mktemp -d /tmp/tmp.XXXXXXXXX`
me@box:/tmp/tmp.SUB54PNhX$ git clone https://github.com/swagger-api/swagger-codegen.git -q .
me@box:/tmp/tmp.SUB54PNhX$ git checkout develop_2.0
me@box:/tmp/tmp.SUB54PNhX$ mvn clean install 
me@box:/tmp/tmp.SUB54PNhX$ java -jar ./modules/swagger-codegen-cli/target/swagger-codegen-cli.jar generate -i https://gist.githubusercontent.com/who/2ad48a523ce91b646ba7/raw/e9cda73c4bd523fa04bfb59484d6fbfebf75baa2/repro_1051.json -l java -o output
me@box:/tmp/tmp.SUB54PNhX$ cd output; mvn compile;

Repro output:

[INFO] Scanning for projects...
[WARNING] 
[WARNING] Some problems were encountered while building the effective model for io.swagger:swagger-java-client:jar:1.0.0
[WARNING] 'build.plugins.plugin.version' for org.codehaus.mojo:build-helper-maven-plugin is missing. @ line 68, column 15
[WARNING] 
[WARNING] It is highly recommended to fix these problems because they threaten the stability of your build.
[WARNING] 
[WARNING] For this reason, future Maven versions might no longer support building such malformed projects.
[WARNING] 
[INFO]                                                                         
[INFO] ------------------------------------------------------------------------
[INFO] Building swagger-java-client 1.0.0
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] --- build-helper-maven-plugin:1.9.1:add-source (add_sources) @ swagger-java-client ---
[INFO] Source directory: /tmp/tmp.SUB54PNhX/output/src/main/java added.
[INFO] 
[INFO] --- maven-resources-plugin:2.6:resources (default-resources) @ swagger-java-client ---
[WARNING] Using platform encoding (UTF-8 actually) to copy filtered resources, i.e. build is platform dependent!
[INFO] skip non existing resourceDirectory /tmp/tmp.SUB54PNhX/output/src/main/resources
[INFO] 
[INFO] --- maven-compiler-plugin:2.3.2:compile (default-compile) @ swagger-java-client ---
[WARNING] File encoding has not been set, using platform encoding UTF-8, i.e. build is platform dependent!
[INFO] Compiling 2 source files to /tmp/tmp.SUB54PNhX/output/target/classes
[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /tmp/tmp.SUB54PNhX/output/src/main/java/io/swagger/client/model/SubObject.java:[35,23] error: getThingType() in SubObject cannot override getThingType() in SuperObject
[INFO] 1 error
[INFO] -------------------------------------------------------------
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1.321 s
[INFO] Finished at: 2015-08-05T21:29:17-07:00
[INFO] Final Memory: 12M/107M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:2.3.2:compile (default-compile) on project swagger-java-client: Compilation failure
[ERROR] /tmp/tmp.SUB54PNhX/output/src/main/java/io/swagger/client/model/SubObject.java:[35,23] error: getThingType() in SubObject cannot override getThingType() in SuperObject
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException

[xhh]

Below is how we could get rid of this issue in another way (IMHO):

1. In the above example, both SuperObject and SubObject have the thingType property defined, it would be best practice (or required) that duplicate properties are remove from child object since they will be inherited from parent object.
2. Java client supports inheritance, e.g. class SubObject extends SuperObject, so we should not copy properties from parent object to child object, i.e. set supportsInheritance to false for Java client codegen. Please refer to Conditional copy properties from parent #1015 for details.

With the above two steps, the issue will be gone.

@who, it's cool that your PR (#1051) gets the issue fixed regardless whether the above two approaches are taken. That said, I'm not sure if it's worth the effort (although you've done it, thanks!) or we should keep it simple by using the two approaches above to avoid the issue from the beginning.
What do you think?

[who]

Thanks for the feedback, @xhh

would be best practice (or required) that duplicate properties are remove from child object since they will be inherited from parent object.

The output spec is the default output generated by swagger-core, so we'd have to modify swagger-core to remove the duplicated properties. I think this probably would be a bigger change, so it may be best to fix this from the swagger-codegen side.

so we should not copy properties from parent object to child object, i.e. set supportsInheritance to false for Java client codegen

Currently, supportsInheritance is already set to false in DefaultCodegen, and it is not over-ridden to true in JavaClientCodegen.

[who]

@xhh @fehguy @webron

It looks like there are two ways to fix this: prevention or post-processing.

Option: Prevention
In the process of populating properties into a CodegenModel, if the model is a child of a parent which has an enum, do not populate an identical enum into the child class. The problem with this approach is that no properties are being looped over by the property population mechanism; they are copied en masse.

Option: Post-processing
After populating properties into a CodegenModel, if the model is a child of a parent which has an enum,
look for duplicate enums in the child and remove them. This post-processing approach has been implemented in PR #1051

[xhh]

Currently, supportsInheritance is already set to false in DefaultCodegen, and it is not over-ridden to true in JavaClientCodegen.

Sorry for the typo, I meant "set supportsInheritance to true for Java client codegen" so that parent properties are NOT copied into child object (like in AbstractTypeScriptClientCodegen.java).
However, setting supportsInheritance to true does not prevent from copying properties of intermediate models (as you mentioned in the "Option: Prevention" section). We could ensure this by filtering all those properties.

[ahgittin]

@who @xhh i've opened #1094 to at least set supportsInheritance=true. while this may not be a perfect fix it fixes my models. (my models work with master so i'm guessing this restores behaviour to what it was, fixing at least the regression? the intermediate models problem was probably also present in master and so can be addressed as a further improvement?)

[who]

PR is merged - closing now