Skip to content

[Java] Useless generation of fields when using allOf #1348

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.

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

Closed
cbornet opened this issue Oct 7, 2015 · 9 comments
Closed

[Java] Useless generation of fields when using allOf #1348

cbornet opened this issue Oct 7, 2015 · 9 comments

Comments

@cbornet
Copy link
Contributor

cbornet commented Oct 7, 2015

When using composition with allOf, the child class contains un-needed attributes which are already in the parent class.

@cbornet
Copy link
Contributor Author

cbornet commented Oct 7, 2015

It even leads to an exception : class io.swagger.client.model.Bar declares multiple JSON fields named foo when using Gson (in okhttp-gson and retrofit libraries)

@wing328
Copy link
Contributor

wing328 commented Oct 7, 2015

@cbornet my understanding of allOf (without discriminator) is that child should "copy" all the parent properties to itself (child) since child does not inherit (extend) parent.

Ref: https://github.com/swagger-api/swagger-spec/blob/master/versions/2.0.md#composition-and-inheritance-polymorphism

Do you have a sample spec so that I can repeat the issue you mentioned above?

@cbornet
Copy link
Contributor Author

cbornet commented Oct 7, 2015

@wing328 Well, it seems to me that implementing swagger composition with class extension would be fine if there was only one object possible in the composition since the classes are just mere containers for the Json mapping. But since there can be several objects to compose, it looks preferable to just copy the attributes for clarity and remove the class inheritance in the generated code.

@cbornet
Copy link
Contributor Author

cbornet commented Oct 7, 2015

A spec to reproduce here: https://raw.githubusercontent.com/swagger-api/swagger-spec/07cce43535e90815737e7258bb4d51c0c76999d6/examples/v2.0/json/petstore-expanded.json

public class Pet extends NewPet  {

  @SerializedName("name")
  private String name = null;

  @SerializedName("tag")
  private String tag = null;

  @SerializedName("id")
  private Long id = null;
public class NewPet   {

  @SerializedName("name")
  private String name = null;

  @SerializedName("tag")
  private String tag = null;

@cbornet
Copy link
Contributor Author

cbornet commented Oct 10, 2015

The issue is also debated in #1120 . We need to have class extension or copy but not both as it fails with gson.

cbornet added a commit to cbornet/swagger-codegen that referenced this issue Oct 10, 2015
@fehguy fehguy added this to the v2.1.5 milestone Oct 26, 2015
@cbornet
Copy link
Contributor Author

cbornet commented Nov 9, 2015

I have tested that a definition with multiple allOf gets the copies of all the elements so composition is working fine if we remove the class extension. As per #1120, there seems to be an issue when the parent holds an enum. I think the solution there would be to also copy the enum to the child class (or import the one from the parent class if it is visible).
I agree with @wing328 that class extension should be reserved for when we have discriminator support.
So what should be done ? I have removed the class extension on my fork and I can pull a PR.

@fehguy fehguy modified the milestones: v2.1.5, v2.1.6 Jan 6, 2016
@hiveship
Copy link
Contributor

hiveship commented Feb 8, 2016

@wing328 I'm facing the problem. What is the final decision about it ?

@ethlo
Copy link

ethlo commented May 4, 2016

I was going to open a separate issue (and I still can if you find that more appropriate), but I just wanted mention other issues with model generation. It seems that model generation where inheritance/composition is involved is severely broken.

Basically:
It should support the same inheritance hierarchy as specified in the Swagger model.

Also, this generates no extra properties at all (it is the same as Page class, with no extra properties):

PageBatchOrder:
    allOf: 
    - $ref: '#/definitions/Page'
    type: object
    properties: 
      content: 
        type: array
        items: 
          $ref: '#/definitions/BatchOrder'

So my question is, is this an issue that people are actively working on, or should I have a look?

@wing328
Copy link
Contributor

wing328 commented Jul 19, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants