Skip to content
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

[BUG] [JAVA] Java generation of 'extra' class with allOf #3100

Closed
renskesterrenburg opened this issue Jun 5, 2019 · 44 comments · May be fixed by #9834
Closed

[BUG] [JAVA] Java generation of 'extra' class with allOf #3100

renskesterrenburg opened this issue Jun 5, 2019 · 44 comments · May be fixed by #9834

Comments

@renskesterrenburg
Copy link

Description

I am using the pet store example to create java classes with inheritance. In the newest release (4.0.1) I found a bug. When I generate code with the OpenApi declaration file it generates an extra class.

image

The class 'CatAllOf' I did not expect. It looks the same as 'Cat' except that it does not extends Pet.

OpenAPI declaration file:

openapi: 3.0.1
info:
  title: Article Event
  description: 'Article Event schema'
  version: 1.0.0
paths:
  /:
    get:
      responses:
        200:
          description: OK
          content: {}
components:
  schemas:
    TextChunk:
      type: object
      discriminator:
        propertyName: textChunkType
      properties:
        parent:
          type: string
    Dog:
      allOf: 
      - $ref: '#/components/schemas/TextChunk'
      - type: object
    Cat:
      allOf:
      - $ref: '#/components/schemas/TextChunk'
      - type: object
        properties:
          bark:
            type: string

openapi-generator version
I used the gradle plugin with version 4.0.1.

older version
I tested the same thing in the version 4.0.0-beta3 and there I did net see the 'extra' class. In other words, I got the expected behavior. It seems the bug was introduced with the latest release.

@ghost
Copy link

ghost commented Jul 9, 2019

And it's not about presence of extra class only. The expected inheritance is missing as well.
It seems to me that the whole 4.x is affected

@gerardbosch
Copy link

gerardbosch commented Jul 16, 2019

I'm facing this issue as well. But when I have realized the version 4 of generator working this way I asked myself if this was intended or really a weird bug. Could someone clarify it? I was pretty happy with 3.3.4 so I will go back to it for now. I rely on Java inheritance.
@wing328

@yilinjuang
Copy link
Contributor

Also happen in csharp client and server #3338

@wing328
Copy link
Member

wing328 commented Aug 30, 2019

      - type: object
        properties:
          bark:
            type: string

That's treated as an inline schema (which can be replaced with $ref to avoid the additional model generation).

If users prefer the old way of not generating this inline schema, technically we can add an option to restore the old behaviour.

@valliman
Copy link

valliman commented Sep 3, 2019

The proposed solutions would lead to a file being generated for the inline schema again. That sounds a bit counter-intuitive. Why would I want to give the inline schema a dedicated name, if I didn't had the need before?

@valliman
Copy link

@wing328 any comment on my post above? Is it possible to add the option to add the old behaviour?

@roxspring
Copy link
Contributor

That's treated as an inline schema (which can be replaced with $ref to avoid the additional model generation).

This seems to miss the point entirely. Spec authors understand that these are inline schemas and are used precisely "to avoid the additional model generation". Replacing with a $ref necessitates the creation of the additional model that spec authors wish to avoid.

If users prefer the old way of not generating this inline schema, technically we can add an option to restore the old behaviour.

It seems clear from the number of bugs raised in this area that many users do "prefer the old way". What I'm really not clear on is:

  1. What benefits there are to "the new way" - i.e. whether it's worth keeping both ways as options
  2. Where in the codebase I should be looking to patch in order to address the underlying problem

@nexen505
Copy link

Still got this problem on 4.2.2.

@ahokkonen
Copy link

This is also a behavior for .net generators. Still an issue in 4.3.0

@papegaaij
Copy link

Even the examples have this issue. For example, the Java RestEasy example has several files with AllOf, like this one https://github.com/OpenAPITools/openapi-generator/blob/v4.3.0/samples/client/petstore/java/resteasy/src/main/java/org/openapitools/client/model/CatAllOf.java

These files are not used at all by the API. The only reference to this class is an unused import from Cat and the testcase CatAllOfTest. The same is true for the other AllOf classes.

@wing328
Copy link
Member

wing328 commented Apr 21, 2020

What about using .openapi-generator-ignore (in the output directory) to skip those files from being generated?

@papegaaij
Copy link

@wing328 That seems like a strange solution to me. Why would I need to ignore files that shouldn't have been generated in the first place? Also, as I said, the classes are referenced by an unused import from other classes. Deleting the files, breaks the build.

@ahokkonen
Copy link

ahokkonen commented Apr 21, 2020

What about using .openapi-generator-ignore (in the output directory) to skip those files from being generated?

Great! Using .openapi-generator-ignore including "**/*AllOf.cs" for .net generation solves this.
Yes, this is a workaround, but still valid solution for my needs.

@wing328
Copy link
Member

wing328 commented Apr 21, 2020

Understood that not all users want these allOf classes. The original PR is try to make the behavior consistent (generating a model for inline schema) but clearly not everyone likes it.

I'll think about this more and see what I can do, probably in the upcoming 5.0.x release in May/Jun in which breaking changes are expected.

@gerardbosch
Copy link

gerardbosch commented Apr 22, 2020

Good news to listen about restoring to the original behavior. We rely upon inheritance that the v3.x provided, i.e. class Cat extends Animal (or as per OP example Cat extends TextChunk), which seems missing in v4.x.

@wing328
Copy link
Member

wing328 commented Apr 22, 2020

We rely upon inheritance that the v3.x provided

That's not going to be restored.

You will need to use discriminator for inheritance: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#models-with-polymorphism-support

@sko-ob
Copy link

sko-ob commented May 1, 2020

@wing328 That seems like a strange solution to me. Why would I need to ignore files that shouldn't have been generated in the first place? Also, as I said, the classes are referenced by an unused import from other classes. Deleting the files, breaks the build.

Facing the same issue here, where configuring the generator to ignore these AllOf files causes the classes to not compile due to these AllOf classes being imported (even though they're unused). Using Java here.

@jharmelink
Copy link
Contributor

Can someone explain why these *AllOf.java files are being generated in the first place?

@gerardbosch
Copy link

gerardbosch commented Jun 5, 2020

We rely upon inheritance that the v3.x provided

That's not going to be restored.

You will need to use discriminator for inheritance: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#models-with-polymorphism-support

(Sorry for the bit delay...)

Sure @wing328 we use the discriminator to allow inheritance on model classes be generated (at least in the generator v3.x). But the whole point is that the v4.x does not generate the inheritance in the Java classes. So this is the behavior that was broken and that we would like to recover back.

Aside of that, and as others state, the new generated classes *AllOf.java in v4.x seems not useful at all and pollutes the generated artifact.

Is there any news for the v5.x you mentioned regarding the inheritance issue? Thanks!! 🙏 😃

@gerardbosch
Copy link

I've just realized that I have been confused on all my previous assertions regarding inheritance. The inheritance is not actually broken and seems never was (I've tried again with several generator versions 4.0, 4.02, 4.0.3, 4.1, 4.3.1) and Java inheritance is still fine. So I apologize if I introduced noise. I think other commenter stated the inheritance was broken as well.

So the actual problem is only about the extra unuseful AllOf generated classes (this ones are the ones that has no inheritance, but for the sake of the issue are the ones we want to remove).

The main point is that this pollutes the generated JAR with unnecessary, unused classes, so breaks the point of a sane API artifact.

I've just tried using the .openapi-generator-ignore file with **/*AllOf.java in output directory, and as others already said, classes are not generated, BUT the unused imports in other classes (say Cat.java) prevents the code to compile.

Maybe I could hack the POM of my generator (here using openapi-generator-maven-plugin) to remove all ^import.*AllOf;$ using sed or something similar but this is not the right way to go. Could work as a patch at most.

@gmcelhoe
Copy link

gmcelhoe commented Aug 26, 2020

the unused imports in other classes (say Cat.java) prevents the code to compile.

That seems like the most solvable problem -- if the generated file only imported classes it used, then the .openapi-generator-ignore trick would work.

@surmabck
Copy link

As a workaround I can suggest adding additionalProperties: true to child schema.
For example, below would not generate additional CatAllOf.java

Cat:
   allOf:
   - $ref: '#/components/schemas/TextChunk'
   - type: object
     additionalProperties: true
     properties:
       bark:
         type: string

That works because composed schema (allOf, oneOf, anyOf) that also defines additionalProperties are considered to be a MapSchema and models are not generated for MapSchemas.
I haven't encountered any problems so far with this solution so far, but I have not tested other generators

@gerlacdt
Copy link

Are there any news about this? Is it on the roadmap for the upcoming 5.x release?

@milczarekIT
Copy link

When you change from:

Cat:
      allOf:
      - $ref: '#/components/schemas/TextChunk'
      - type: object
        properties:
          bark:
            type: string

To:

Cat:
   type: object
   properties:
      bark:
         type: string   
   allOf:
      - $ref: '#/components/schemas/TextChunk'
      

So, if additional properties for Cat are added to the root of Cat, then there is no additional class CatAllOf

@skchande
Copy link

skchande commented Oct 6, 2020

Very nice Workaround @milczarekIT
I have tested it with openapi-generator version 5.0.0-beta2.

Does anyone know if this behavior is intended? I could not find any documentation for this.

@MattLichter
Copy link

@milczarekIT Nice workaround. It allowed me to generate the intended client code without the dummy classes (I'm using csharp-netcore 5.0.0-SNAPSHOT).

Does anyone know if this is still valid OpenAPI syntax though? I haven't found any official examples with this structure.

@thobiaskarlsson
Copy link

That is a nice workaround. Sadly for Java the "builder" methods does not work since they return the super type, e.g.

Dog dog = new Dog().bark(true); // Ok since "bark" belongs to Dog
Pet dog = new Dog().bark(true).name("Doggie"); // Returns Pet (since "name" belongs to Pet), should return Dog

I've tried so many different patterns but none of them work for Java inheritance, this was the closest though, but I can't live with:

Dog dog = (Dog) new Dog().bark(true).name("Doggie");

nor...

Dog dog = new Dog().bark(true);
dog.name("Doggie"); // <- Pet

nor...

Dog dog = new Dog();
dog.setBark(true);
dog.setName("Doggie");

A simple override of the builder methods would suffice I believe, e.g.

@Override
public Dog name(String name) {
  return (Dog) super.name(name);
}

Then this workaround would be very acceptable imo.

@stapel
Copy link
Contributor

stapel commented Mar 3, 2021

There's the same problem with kotlin and kotlin-server (in 5.0.1)

@mizolight
Copy link

@wing328 why can't you just have the old behavior? I think a lot of folks are disappointed with the new behavior. We are supposed to make things better not worse.

Thanks!

@phirzel
Copy link

phirzel commented Apr 11, 2021

Since I annotate the @Schema directly in my java code and even generate openapi.yml out of it, I cannot influence the positioning of :allOf ref much (as in the nice hack above ;-)
Therefore I created another silly hack class CatAllOf to please the non-understandable import without being used in the generated class code at all, just like:

package pet.model;
/**
 * Just an unnecessary generated "import" compiler workaround class
 * @see <a href="https://github.com/OpenAPITools/openapi-generator/issues/3100">*AllOf imports</a>
 * @see Cat
 */
@Deprecated
public class CatPlaceAllOf {}

To OpenApi developers:

  • I suggest you clean up generated code within the plugin (for e.g. like IntellJ's feature to remove unneccessary imports), that would do it in that case (as a quick fix)
  • of course such usage for continuous quality comparison of generated code, would be more handy within your unit-tests
  • I can follow the missunderstandings above, but I did not find any explanation, whether it is ok to remove the "silly import" or if there is another fault in my annotation or in the generated code

Thanks for fixing soon and appreciate your work!


I am not sure if I understood the inheritance spec well, but my Java implementation looks as follows:

io.swagger.v3.oas.annotations.media.Schema;

@Schema(name = "Pet", 
        discriminatorProperty = "petType",
        subTypes = {Cat.class, Dog.class})
public /*TODO make abstract */ class  Pet {
  @Schema(type = "string", required = true)
  String petType;
}
@Schema(name = "Cat",allOf = Pet.class)
public class Cat extends Pet {..}
@Schema(name = "Dog",allOf = Pet.class)
public class Dog extends Pet {..}

Besides I am using:

<plugin>
                <groupId>org.openapitools</groupId>
                <!-- see https://openapi-generator.tech/docs/generators/java/ -->
                <artifactId>openapi-generator-maven-plugin</artifactId>
                <version>5.1.0</version>
..

@rgoers
Copy link

rgoers commented Jun 13, 2021

I am not sure what the status is of this. I have specified

 OrganizationPage:
  type: object
  allOf:
    - $ref: '#/components/schemas/BasePage'
  properties:
    data:
      type: array
      items:
        $ref: '#/components/schemas/Organization'

OrganizationPage must be an instance of BasePage but it is not. instead all the properties from BasePage are included in OrganizationPage. If this syntax no longer causes inheritance what is the new spec format that does?

@gerardbosch
Copy link

@rgoers the corrent form should be something like this #3100 (comment)

@rgoers
Copy link

rgoers commented Jun 13, 2021

I tried

OrganizationPage:
  allOf:
    - $ref: '#/components/schemas/BasePage'
    - type: object
      properties:
        data:
          type: array
          items:
            $ref: '#/components/schemas/Organization'

and

OrganizationPage:
  type: object
  properties:
    data:
      type: array
      items:
        $ref: '#/components/schemas/Organization'
  allOf:
    - $ref: '#/components/schemas/BasePage'

they both ended up with

public class OrganizationPage  implements Serializable {

instead of

public class OrganizationPage extends BasePage implements Serializable {

which is what I really need. However, the format that has everything under the allof element also generates a class named OrganizationPageAllOf as

public class OrganizationPageAllOf  implements Serializable 

but this class has none of the attributes from BasePage and doesn't extend it so it is unusable for anything.

So what do I need to do to get OrganizationPage to extend BasePage?

@ntroutman
Copy link

Still seeing this as an issue. Interestingly the swagger-codegen doesn't produce these extra files.

@efriandika
Copy link

It's been 3 years, this issue is still open. the swagger-codegen doesn't produce this extra files. But why openapi-generator still produce this extra files?

Could you please give us explanation / insight wether these extra files useful or no @wing328 ?

@wing328
Copy link
Member

wing328 commented Sep 22, 2022

I've filed #13498 to improve how the allOf inline schemas are created. Please give it a try as follows:

git clone -b inline-schema-improve3 https://github.com/OpenAPITools/openapi-generator
cd openapi-generator
mvn clean package
java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g java  -i /var/tmp/your_spec.yaml -o /var/tmp/java/

(if you're using mvn wrapper that comes with the project, just replace mvn with mvnw (Mac/Linux) or mvnw.cmd (Windows))

I tested with java, csharp-netcore and the result is good - no longer seeing those *AllOf files created.

@wing328
Copy link
Member

wing328 commented Sep 24, 2022

Tagging @msymonov @gerardbosch @yilinjuang @valliman @roxspring @nexen505 @ahokkonen @papegaaij @sko-ob @jharmelink @gmcelhoe @surmabck @gerlacdt @milczarekIT @MattLichter @thobiaskarlsson @stapel @mizolight @phirzel @rgoers @ntroutman @efriandika to review the PR 🙏

Have a nice weekend.

@pfconrey
Copy link

pfconrey commented Feb 27, 2023

Is there any hope of getting the fix into the main release any time soon? This parser is unusable for any openapi document that uses "allOf".

@pfconrey
Copy link

Guys, I'd be happy to help if there's any way I can, but this is a huge blocker for me. Any hope of getting a fix soon?

@pfconrey
Copy link

I've filed #13498 to improve how the allOf inline schemas are created. Please give it a try as follows:

git clone -b inline-schema-improve3 https://github.com/OpenAPITools/openapi-generator
cd openapi-generator
mvn clean package
java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g java  -i /var/tmp/your_spec.yaml -o /var/tmp/java/

(if you're using mvn wrapper that comes with the project, just replace mvn with mvnw (Mac/Linux) or mvnw.cmd (Windows))

I tested with java, csharp-netcore and the result is good - no longer seeing those *AllOf files created.

The problem still occurs in the plugin. When merging together 2 YAML files, one of which has an allOf object, the resulting YAML file still has the 3 objects (Object1, Object2, Object2_allOf)

@wing328 wing328 closed this as completed Jun 11, 2023
@wing328
Copy link
Member

wing328 commented Jun 11, 2023

Closed via #15682

Please pull the latest master to give it a try.

@phirzel
Copy link

phirzel commented Jun 12, 2023

(sorry for my testing delay, after SpringBoot v3.0.6 migration and upgrade of openapi-generator-maven-plugin to v6.5.0 I do not see those unnecessary *AllOf imports in the generated ApiClient code any more - we use Java and @Schema annotation in code as illustrated above in my ancient post). Whoever solved the issue, THANKS a lot!)

@wing328
Copy link
Member

wing328 commented Jun 12, 2023

@phirzel no need to sorry. Glad that you no longer encounter the issue.

@icarusfire
Copy link

icarusfire commented Nov 6, 2023

@wing328 Hi William. Currently using v7.0. I have a field with type "Object" and this field is generated as a new class "...AllOfType". I want it to stay as Object, and do not want the new AllOfType class. Anything I can do?

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

Successfully merging a pull request may close this issue.