[BUG][JAVA|SPRING] Properties duplicated in inheritance chain

goatfryed commented 1 month ago

Bug Report Checklist

[x] Have you provided a -full-/minimal spec to reproduce the issue?
[x] Have you validated the input using an OpenAPI validator (example)?
[x] Have you tested with the latest master to confirm the issue still exists?
[x] Have you searched for related issues/PRs?
[x] What's the actual output vs expected output?
[ ] [Optional] Sponsorship to speed up the bug fix or feature request (example)

Description

Properties are duplicated instead of inherited, if inheritance is redeclared.

openapi-generator version

7.7.0

OpenAPI declaration file content

openapi: 3.0.1
info:
  title: "BugSample"
  version: SNAPSHOT-1
paths: {}
components:
  schemas:
    Extensible:
      type: object
      properties:
        '@type':
          type: string
        'baseType':
          type: string
    ErrorMessage:
      allOf:
        - $ref: "#/components/schemas/Extensible"
        - type: object
          properties:
            chuck:
              type: string
      discriminator:
        propertyName: '@type'
        mapping:
          ErrorMessage: '#/components/schemas/ErrorMessage'
          ProductOrderErrorMessage: '#/components/schemas/SpecialErrorMessage'
    SpecialErrorMessage:
      allOf:
        - $ref: "#/components/schemas/Extensible"
        - $ref: "#/components/schemas/ErrorMessage"
        - type: object
          properties:
            norris:
              type: string

Generation Details

docker run --rm -v $PWD:/local openapitools/openapi-generator-cli generate -i /local/bugSample.yaml -g spring -o /local/out

Steps to reproduce

Create a sample with at least 3 levels of inheritance A > B > C
Declare B as inheritance root via discriminator mapping
Declare C as allOf A and B explicitly
Generate Java or Spring sources. (I assume this is similar for other typed languages,. not tested)

Expectation

For my example, I expect SpecialErrorMessage to only declare property norris.

Actual

SpecialErrorMessage redeclares atType, atBaseType

Workaround

Don't duplicate composition in allOf. Patch the specs.

On the downside, this is sometimes not obvious in more complicated specs and not ideal, if you're not the owner of the spec. I work with an external party that applies the Design guideline to require re-declaration of Extensible Schema explicitly, because they use it to communicate all Schemas that support further extension.

Related issues/PRs

https://github.com/OpenAPITools/openapi-generator/issues/6117 is similar, but only covers flat, simple inheritance.

Suggest a fix

If you point me in the right direction, I'd try to create a PR

jpfinne commented 1 month ago

@goatfryed composition is the default.

if you want inheritance, add x-parent: true to Extensible. Or ask the generator to perform that for you with the REF_AS_PARENT_IN_ALLOF normalizer

Btw: could you change the title of your issue with a better "description"

goatfryed commented 1 month ago

Sorry for the oversight. Fixed the title.

The inheritance is working as expected for me. My issue is with the redeclared properties that break tooling.

In my case, I want to store the model in MongoDB, but this doesn't work with redeclared private properties. This also leads to confusion, if a debugger is used.

jpfinne commented 1 month ago

I don't have redeclared properties while using REF_AS_PARENT_IN_ALLOFF

public class ErrorMessage extends Extensible {

  private String chuck;

public class SpecialErrorMessage extends ErrorMessage {

  private String norris;

goatfryed-holos commented 4 weeks ago

In my case the normalizer solution doesn't work, because this normalizer introduces to many other issues in the rest of the spec and it's generated code. I'm also not sure if I want this normalization even.

The x-parent solution doesn't work for me. The proeprties are not an ErrorMessage anymore, but still on SpecialErrorMessage, still duplicated. Let me check, if I changed something in my example. I'm surprised that it works for you.

I'm fine that ErrorMessage does not extend Extensible and declares it's property. I actually prefer that way. But the SpecialErrorMessage should not redeclare the properties of the parent that it extends. I haven't tried it yet, but wouldn't this lead to other issues with regards to Composition of multiple refs?

In any case, I think the generator could check which refs are declared on the parent (chain) and exclude them from generation. Don't you agree that this should be the behavior without any normalizers or vendor extensions?

jpfinne commented 3 weeks ago

I have no duplication with x-parent on Extensible

components:
  schemas:
    Extensible:
      type: object
      x-parent: true
      properties:
        '@type':
          type: string
        'baseType':
          type: string

goatfryed commented 3 weeks ago

Hey @jpfinne I've updated the sample in the description. This version causes the issue for me, even if i add x-parent: true like so

openapi: 3.0.1
info:
  title: "BugSample"
  version: SNAPSHOT-1
paths: {}
components:
  schemas:
    Extensible:
      type: object
      x-parent: true
      properties:
        '@type':
          type: string
        'baseType':
          type: string
    ErrorMessage:
      allOf:
        - $ref: "#/components/schemas/Extensible"
        - type: object
          properties:
            chuck:
              type: string
      discriminator:
        propertyName: '@type'
        mapping:
          ErrorMessage: '#/components/schemas/ErrorMessage'
          ProductOrderErrorMessage: '#/components/schemas/SpecialErrorMessage'
    SpecialErrorMessage:
      allOf:
        - $ref: "#/components/schemas/Extensible"
        - $ref: "#/components/schemas/ErrorMessage"
        - type: object
          properties:
            norris:
              type: string

docker run --rm -v "${PWD}:/local" openapitools/openapi-generator-cli:latest generate \
    -i /local/BugSample.openapi.yaml \
    -g java \
    -o /local/out/java

I'm sorry for wasting some of your time. While I mentioned that the issue occurs, if the same schema is composed on multiple levels, the sample didn't include that. It seems I copied the wrong version

jpfinne commented 3 weeks ago

I use the spring generator (as mentioned in your first post)

<plugin>
    <groupId>org.openapitools</groupId>
    <artifactId>openapi-generator-maven-plugin</artifactId>
    <version>7.7.0</version>
    <executions>
        <execution>
            <id>generate-api-spring</id>
            <goals>
                <goal>generate</goal>
            </goals>
            <phase>generate-sources</phase>
            <configuration>
                <inputSpec>sample.yaml</inputSpec>
                <output>target</output>
                <generatorName>spring</generatorName>

                <apiPackage>com.sample</apiPackage>
                <modelPackage>sample</modelPackage>
                <modelNameSuffix/>
                <skip>false</skip>
                <generateApis>true</generateApis>
                <generateModelDocumentation>false</generateModelDocumentation>
                <generateModelTests>false</generateModelTests>
                <configOptions>
                    <oas3>true</oas3>
                    <unhandledException>true</unhandledException>
                    <requestMappingMode>api_interface</requestMappingMode>
                    <useJakartaEe>true</useJakartaEe>
                    <useBeanValidation>true</useBeanValidation>
                    <bigDecimalAsString>true</bigDecimalAsString>
                    <ignoreAnyOfInEnum>true</ignoreAnyOfInEnum>
                    <interfaceOnly>true</interfaceOnly>
                    <dateLibrary>java8</dateLibrary>
                </configOptions>
            </configuration>
        </execution>
    </executions>
</plugin>

goatfryed commented 3 weeks ago

The issue is present in both java and spring. it is also present in kotlin generator, which generates

/**
 *
 * Please note:
 * This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).
 * Do not edit this file manually.
 *
 */

@file:Suppress(
    "ArrayInDataClass",
    "EnumEntryName",
    "RemoveRedundantQualifierName",
    "UnusedImport"
)

package org.openapitools.client.models

import org.openapitools.client.models.ErrorMessage
import org.openapitools.client.models.Extensible

import com.squareup.moshi.Json
import com.squareup.moshi.JsonClass

/**
 * 
 *
 * @param atType 
 * @param baseType 
 * @param chuck 
 * @param norris 
 */

data class SpecialErrorMessage (

    @Json(name = "@type")
    val atType: kotlin.String? = null,

    @Json(name = "baseType")
    val baseType: kotlin.String? = null,

    @Json(name = "chuck")
    val chuck: kotlin.String? = null,

    @Json(name = "norris")
    val norris: kotlin.String? = null

) : Extensible {

}

To me, this seems like some core issue independent of the generator. Also this should be default behavior and not fixed with some normalizers or extra settings in my opinion. If I redeclare something in allOf that is already declared in my parent hierachy, I'd expect the property to be declared only once as far up the hierarchy as possible.

I'm not yet found with the inner workings, but I assume there is some normalization of the model hierarchy going on that is then provided to the generators that translate these definitions into language specific structures. So I'd assume that this is an issue in the normalization step. If you agree with my expectation and if you could give me some guidance, I'd be happy to take a look at this.

OpenAPITools / openapi-generator