[Java] Fix SubClass annotations missing from the base class

[szakrewsky]

PR checklist

• Read the contribution guildelines.
• Ran the shell/batch 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)
• Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

#2449

[wing328]

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/swagger-api/swagger-codegen/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/swagger-api/swagger-codegen/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

[wing328]

Please also run ./bin/java-petstore-all.sh to update Java Petstore samples if you've not done so.

[szakrewsky]

@wing328 I did run ./bin/java-petstore-all.sh I guess I was confused whether I was supposed to commit those file or if it was just running some tests.

[wing328]

@szakrewsky please commit the update Java Petstore files as well so that CIs can test the change.

[szakrewsky]

@wing328 Thanks. I think I found some issues, this commit only effects jersey but it seems that the other frameworks are adding the annotations as well. I will fix first.

[szakrewsky]

@wing328 I believe this is ready now. Let me know if there is anything more I need to do.

[wing328]

@szakrewsky I'll review and let you know if I've any questions.

[wing328]

@szakrewsky Can we use 2-space instead of 2 tabs?

[szakrewsky]

Done!

[jeff9finger]

The PR looks to only place the annotations on the first level of inheritance. For example:

definitions:
BaseObj:
    type: object
    discriminator: object_type
    required:
      - id
      - object_type
    properties:
      id:
        type: integer
        format: int64
      object_type:
        type: string
        readOnly: true
  SubObjType:
    type: string
    enum: 
    - daily
    - monthly
    - quarterly
    - yearly

  SubObj:
    allOf:
      - $ref: '#/definitions/BaseObj'
      - type: object
        discriminator: sub_obj_type
        required:
          - sub_obj_type
        properties:          
          sub_obj_type:
            $ref: '#/definitions/SubObjType'
          name:
            type: string
            readOnly: true
  DailySubObj:
    allOf:
      - $ref: '#/definitions/SubObj'
      - type: object
         properties:
           day_of_month:
             type: integer
             format: int32
             readOnly: true

In this case, the Jackson annotations are not placed on the SubObj class

[jeff9finger]

Adding the following to DefaultCodeGen seems to fix the issue for me. However, this assumes that allOf: can have no more than one inline element that contains a discriminator. (My experience also shows that allOf: understands no more than 2 inline elements. If more than to exist, only last one is recognized).

@@ -1240,6 +1251,12 @@ public class DefaultCodegen {
                 allProperties = new LinkedHashMap<String, Property>();
                 allRequired = new ArrayList<String>();
                 m.allVars = new ArrayList<CodegenProperty>();
+                for (Model innerModel: composed.getAllOf()) {
+                    if (innerModel instanceof ModelImpl) {
+                        m.discriminator = ((ModelImpl) innerModel).getDiscriminator();
+                        break; // only one discriminator allowed at the moment.
+                    }
+                }
             } else {
                 allProperties = null;
                 allRequired = null;

[willgorman]

I'm interested in this same feature but for generated JAX-RS server code as well as Java clients. It looks like that works by applying the same changes to the templates in the JavaJaxRS resource directory. Would it be possible to add that here or should I submit a separate PR after this is merged?

[szakrewsky]

@jeff9finger I just fixed one issue which I had already documented before, #2449 (comment)

In your example the following code would have been generated,

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "object_type")
@JsonSubTypes({ @Type(value = SubObj.class, name = "SubObj") })
public class BaseObj {
}

now you will get

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "object_type")
@JsonSubTypes({ @Type(value = SubObj.class, name = "SubObj"),  @Type(value = DailySubObj.class, name = "DailySubObj")})
public class BaseObj {
}

The annotations will be added as long as the discriminator value is set, why that only happens for ModelImpl and not ComposedModel I don't know, but I would probably say that is a different issue.

[jeff9finger]

The generated code above is not correct. It might work, but does not represent the actual structure of the inheritance. I need the annotations to be on the SubObj class. Like so:

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "object_type")
@JsonSubTypes({ @Type(value = SubObj.class, name = "SubObj") })
public class BaseObj {
}

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "sub_obj_type")
@JsonSubTypes({ @Type(value = DailySubObj.class, name = "DailySubObj") })
public class SubObj {
}

Notice that there is a different discriminator on SubObj - "sub_obj_type". This is completely lost unless the @JsonTypeInfo annotations are on the SubObj class.

"discriminator" is only available on ModelImpl, so until the swagger spec is updated to handle discriminator in more than one object, it has to live there. That is why the discriminator field needs to be populated for any object that has it declared in the swagger spec.

Without the code above, or something similar, any sub objects that declare a discriminator, will not have the discriminator set in the ModelImpl object in the code that represents the swagger definition. And this is why the @JsonTypeInfo annotations are not placed on the sub class.

@willgorman You should be able to simply add the mustache templates to the other Java based templates to get these annotations. I'll need them eventually in a jaxrs server and in inflector.

[jeff9finger]

@szakrewosky (I'm new to git) How can I merge your code into my local master repository? I don't see a branch that the commits for this issue are in. Thanks

[wing328]

@jeff9finger the PR has been merged into master. Please pull the latest to give it a try.

@szakrewsky thanks for your contribution.

[jeff9finger]

The implementation of the sub-sub class does not work in my case.

See my comment above: #4085 (comment)

In my data model (and probably other people's as well), this approach is wrong. I don't want to have all the @JsonSubType.@Type annotations on the base class because I need to discriminate sub-sub classes differently that the sub class

I think my comment referenced explains this. It seems to me that making an assumption about sub-sub (and deeper) classes must have more options on the approach than simply assuming that all the sub-sub classes will be serialized as one type of object.

[szakrewsky]

@jeff9finger As far as I know, if Jackson is deserializing BaseObj and you want it to instantiate the proper subclass to have working polymorphism, "My" solution is correct. DailySubObj is indeed a subclass of BaseObj even if it is a few levels down in the hierarchy. If you also want Jackson to deserialize the SubObj with proper polymorphism, then "Your" solution is also correct, however at the current time I think it depends on the separate issue of the discriminator not being set #4226. Fix that, and you will get what you want, if I understand your problem correctly.

[jeff9finger]

@szakrewsky Thanks for your comments. Let me amend my statement from above.
Instead of "In my data model (and probably other people's as well), this approach is wrong." I should have said "In my data model (and probably other people's as well), this approach is not desired."

So, I agree with you, that the solution is correct, but with a caveat. In my case, I have the annotation regarding DailySubObj on BaseObj, but it is not strictly needed for serialization (as long as it is defined on the SubObj) - though I don't think it causes any harm. I'll be looking into this to verify.

The annotation for the sub-sub class is not desired because in some cases (mine to be specific) there are many sub-sub classes and I prefer that they not pollute the base class when they are not strictly required.

I am suggesting that there be an option to specifying the @JsonSubType.@Type annotation for sub-sub (and deeper) classes on the base object. This can definitely be handled in a new issue though, so this one can be closed. What do you think about that?

[jeff9finger]

@szakrewsky What does the spec look like when there are sub sub classes in your case? Doesn't there always need to be a discriminator for inheritance? Just wondering why the cm is always added to the parent.parent.children. The parent would need to have a discriminator defined right?

In other words, how can a sub sub class be defined in a swagger wprc without the sub class specifying a discriminator? (The only way I can think that it would make sense is if the sub class defined the same discriminator property as the base class.

[szakrewsky]

@jeff9finger The spec that exhibits this problem is the same as your spec just without the discriminator on the SubObj. Now what does this mean? It could mean that DailySubObj is actually composed of SubObj and therefore SubObj doesn't exist as a class at all and there is only one level of inheritance. However, it appears that swagger-code-gen will generate a class for SubObj that extends BaseObj, and a class for DailySubObj that extends SubObj.

I did a little more research and it appears that the JsonSubTypes annotation is recursive. So you could put the JsonTypeInfo annotation at the top level of the inheritance hierarchy, and have the JsonSubTypes annotation only include direct subclasses. The JsonSubType annotation would then be included recursively down the inheritance tree for subclasses that themselves have subclasses. Would this solve your problem? What exactly is your problem by the way? Is it just a preference over annotation placement or do you get a stacktrace while deserializing something?

[jeff9finger]

The open api spec specifies that discriminator is required for inheritance.

Here is the spec that shows SubObj without a discriminator.

definitions:
BaseObj:
    type: object
    discriminator: object_type
    required:
      - id
      - object_type
    properties:
      id:
        type: integer
        format: int64
      object_type:
        type: string
        readOnly: true
  SubObjType:
    type: string
    enum: 
    - daily
    - monthly
    - quarterly
    - yearly

  SubObj:
    allOf:
      - $ref: '#/definitions/BaseObj'
      - type: object
        properties:          
          name:
            type: string
  DailySubObj:
    allOf:
      - $ref: '#/definitions/SubObj'
      - type: object
         properties:
           day_of_month:
             type: integer
             format: int32
             readOnly: true

You state "... it appears that swagger-code-gen will generate a class for SubObj that extends BaseObj, and a class for DailySubObj that extends SubObj."

This indeed appears to be the case.

However, this is not the behavior that I am specifying in my original spec. I've included it below for clarity.

definitions:
BaseObj:
    type: object
    discriminator: object_type
    required:
      - id
      - object_type
    properties:
      id:
        type: integer
        format: int64
      object_type:
        type: string
        readOnly: true
  SubObjType:
    type: string
    enum: 
    - daily
    - monthly
    - quarterly
    - yearly

  SubObj:
    allOf:
      - $ref: '#/definitions/BaseObj'
      - type: object
        discriminator: sub_obj_type
        required:
          - sub_obj_type
        properties:          
          sub_obj_type:
            $ref: '#/definitions/SubObjType'
          name:
            type: string
            readOnly: true
  DailySubObj:
    allOf:
      - $ref: '#/definitions/SubObj'
      - type: object
         properties:
           day_of_month:
             type: integer
             format: int32
             readOnly: true

The annotations on SubObj require a different discriminator property. Therefore it must be represented on that class, otherwise the desired inheritance is lost - i.e The discriminator property would be completely lost if the the Jackson annotations are placed on BaseObj.

Notice that the discriminator for BaseObj is "object_type", but the discriminator for SubObj is "sub_obj_type". These are distinctions that are required in my data model.

So, if all the annotations for DailySubObj are placed on BaseObj (using the discriminator property "object_type"), I'd lose the discriminator ("sub_obj_type") and related inheritance for DailySubObj.

While Jackson handles the annotations recursively, doing so in this case does not accurately represent the semantics of the open api spec.

See issue #4226 for more detail. My suggestion there is to only add the children of an object to the parent.parent.children if there is no discriminator.

[jeff9finger]

@wing328 Please take a look at this pr as well. It is related to #4252

[wing328]

@jeff9finger this PR was merged into master back on Nov 21, 2016 via the commit 7696559

[jeff9finger]

OK. I have another change required to handle the issues discussed here. Will open a pr for that. Is a new issue created?