[Java] Subclass with attribute allOf issue

[hiveship]

i'm facing a strange behaviour with Swagger Codegen while generating Java code (JAXRS / JAXRS-CXF and Java).

Considering the Swagger definitions:

  event:
    properties:
      mediumUUID:
        type: string
        format: uuid

  messageEvent:
    allOf:  
      - $ref: '#/definitions/event'
      - type: object
        required:
          - messageId
          - message
        properties:
          messageId:
            type: integer
            format: int32
          message:
            type: string

I exepect to get a class "Event":

public class Event   {
  private UUID mediumUUID = null;

and a subclass "MessageEvent" like this:

public class MessageEvent extends Event  {
  // get access to "mediumUUID" from super.getMediumUUID()
  private Integer messageId = null;
  private String message = null;

But with the current Swagger codegen, I got:

public class Event   {

  private UUID mediumUUID = null;

public class InputEvent extends Event  {

  private String name = null;
  private String value = null;

  private UUID mediumUUID = null; // PROBLEM !

This behaviour can cause issue. MessageEvent is a subclass of Event, so in my code I can do:

MessageEvent messageEvent = new MessageEvent();
messageEvent.setMediumUUID(foo);
// messageEvent.getMediumUUID() -> "foo"
Event event = messageEvent;
//event.getMediumUUID() -> null ! Because not calling the right getter/setter

And then I will got 'null' !

I can help to solve this issue but I need some help to point me where I need to do changes.

What we want is "don't put the parent attributes in a sub class".

In the .mustache template file I can found:

public class {{classname}} {{#parent}}extends {{{parent}}}{{/parent}}  {

So with that, we can know if the class 'classname' has a parent or not. We now need something to get the list of all attributes of the parent. And then, don't generate them.

[fehguy]

Yes, this is an issue in the composition aspect of the codegen. The generator doesn't really know if it's extending a base class or if it should be implementing an interface.

For example:

messageEvent:
  allOf:
  - $ref: '#/definitions/ModelA'
  - $ref: '#/definitions/ModelB'

could in fact be an extends construct. But

messageEvent:
  allOf:
  - $ref: '#/definitions/ModelA'
  - $ref: '#/definitions/ModelB'
  - $ref: '#/definitions/ModelC'

The generator isn't capable of knowing that it cannot extend more than one class and therefore must use interfaces.

I think the right thing to do is to first decide how we can detect these situations. I think it's entirely fair to use vendor extensions to hint to the codegen that "all should be interfaces", or ModelA is a class.

Next, fixing the issue is actually quite simple. There is a mechanism inside CodegenConfig to postProcessModels. This gives you the opportunity to add some additional logic in the Java codegen. I would suggest looking for duplicate variables between the model defined by parent in CodegenModel and removing them from the appropriate subclass.

https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/java/io/swagger/codegen/CodegenConfig.java

[hiveship]

@fehguy Is there any way to iterate over all the models of my swagger spec in the postProcessModels ? This method is called for each model and only with 1 model in the 'models' object.

[fehguy]

@hiveship it looks like it's not available. If you'd like to send a PR we can review and merge it.

[hiveship]

I was wrong, my fix was not correct. I don't tink I would be able to do something if I can't access all the models at the same time in the postProcessModels method

[wing328]

@hiveship I'll give it a try tomorrow to see if I can give you a method for processing all models.

[wing328]

@hiveship I filed #2170 to add a function for processing all models before generating the files. Would that meet your need?

[hiveship]

Yes, It's exactly what I need :)

[wing328]

Merged into master. Please give it a try by pulling the latest master.

[hiveship]

I'm on something else now, I'll do it as fast as possible :) If it's OK for me, I'll send a PR to fix that in Java, JAXRS (jersey) & JAXRS-CXF codegen :)

[hiveship]

I think there is 2 points:
Find a way to decide whether to extends or not the parent. Now, I don't see any element in the Swagger file to indicate whether we want to set a model extending another or not (same problem for abstract class in Java).

Plus, if we a model A extends model B and also duplicate all the model A attributes, then we can get Java issues at runtime (calling setters method would modify model B attribute, but if we cast it at model A, the commons properties won't have the good values).

The current behaviour is always extending the parent of the allOf attribute. I think the first step could be to remove all the duplicated properties like I suggested in my previous mail ? And in a second time, when we'll have a good way to decide if we want or not extends the parent, then implement it. Also, if you have an idea to indicate that we want to generate an abstract class, I can implement it (Java).

[fehguy]

I really think this has to be a vendor extension. There is literally no way to know if we want an interface, an abstract class, a base class, etc.

I do think that with the code that @wing328 added--the ability to iterate over all models, we can take the extensions into account and remove variables as needed.

If you need help on this, please make a test case with a sample spec and let's work together on it.

[hiveship]

@fehguy Ok, so I don't really understand what I first have to do.
My suggestion is to check if there is duplicate properties between a model and his parent (if exist). If there is duplication, then just remove the duplicated properties from the model (and leave it to the parent). If there is no duplication, don't do anything.

Having duplicated properties between a parent and a subclass can lead to Java runtime issues.

This behaviour can change if we add some vendor extension (let's do it in a special method that can be override to use interface, abstract class, etc.)

[cbornet]

Note : duplicate properties between model and its parent causes errors with gson : Gson: [Class] declares multiple JSON fields named [property].

[cbornet]

Any news on this ? I still can't use allOf because of the failures with Gson ...

[lukashaertel]

@cbornet I just checked out the code generator and set supportsInheritance = true. Running that version made Gson work (basically what pull request #1120 did).

[cbornet]

@lukashaertel it works if you import only one model but not if you import several models

[cbornet]

A lot of issues are related to this one (#1120, #1348, #1050, ...). So I will try to sum up and give my opinion:

1. We can not use class extension to implement allOf composition. This is because it doesn't work if you import more than one model with allOf (in Java, we shouldn't use extends, we shouldn't set supportsInheritance = true)
2. All attributes for the imported models will be copied including nested enums

So IMO, the only thing to do is to remove the parent/child relationship when using allOf without discriminator and all should work well. The only case where we should use class inheritance is when one (and no more than one or else we should throw an error) of the imported models has a discriminator field.
I don't think it is an issue that the "extended" model has its own enum as it will not inherit the one from a parent class.
@fehguy @wing328 @xhh @ahgittin

[cbornet]

Actually we should set supportsInheritance = true in java but that should only be used for the model which holds the discriminator field.

[wing328]

@cbornet thanks for the summary. I'll review and share my view this weekend... (sorry very busy these days)

[jeff9finger]

Thanks @cbornet for your description. It seems like that approach may work.

Any more thoughts from the community on how to implement this?

This is a major issue for my project.

[cbornet]

If you agree with this, you can support swagger-api/swagger-parser#246

[wing328]

@cbornet PR (#3393) has been merged into master. Please pull the latest to give it a try.