[BUG][Java] Generated API class has wrong dataType and does not compile

[sebastien-rosset]

Bug Report Checklist

• Have you provided a full/minimal spec to reproduce the issue?
• Have you validated the input using an OpenAPI validator (example)?
• What's the version of OpenAPI Generator used?
• Have you search for related issues/PRs?
• What's the actual output vs expected output?
• [Optional] Bounty to sponsor the fix (example)

Description

Using OpenAPI v3 document to generate a client-side Java SDK with the default library (okhttp-gson). The generated classes under the "api" directory cause a compilation failure.

For example, a generated API Java class api/SyslogApi.java has the following createSyslogPolicyCall() method. The requestBody argument is a templatized PolicyAbstractPolicy<String, Object>.

  public okhttp3.Call
  createSyslogPolicyCall(PolicyAbstractPolicy<String, Object> requestBody,
                         final ApiCallback _callback) throws ApiException {
    Object localVarPostBody = requestBody;

However, the generated model/PolicyAbstractPolicy.java class is not a generic, hence the compilation fails. There are two problems:

1. The argument uses a generic
2. The argument type is the parent class

I was expecting the generated method signature to be something like:

public okhttp3.Call createSyslogPolicyCall(SyslogPolicy syslogPolicy, final ApiCallback _callback)

The mustache templates that are used to generate these classes are:

1. API: Java/libraries/native/api.mustache
2. Model: modules/openapi-generator/src/main/resources/Java/pojo.mustache

It looks like the problem is the value of dataType in the API template does not match the class name of the POJO

{{{dataType}}} {{paramName}}

I am using the default Java library, which is okhttp-gson.

openapi-generator version

master February 14th 2020

OpenAPI declaration file content or url

Operations:

  /syslog/Policies:
    post:
      tags:
      - syslog
      summary: Create a 'syslog.Policy' resource.
      operationId: CreateSyslogPolicy
      requestBody:
        description: The 'syslog.Policy' resource to create.
        required: true
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/syslog.Policy'
      responses:
        '200':
          description: The 'syslog.Policy'
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/syslog.Policy'

Schema:

    mo.BaseMo:
      type: object
      discriminator:
        propertyName: objectType
      required:
      - objectType
      properties:
        objectType:
          type: string

    policy.AbstractPolicy:
      allOf:
      - $ref: '#/components/schemas/mo.BaseMo'
      - type: object
        required:
        - objectType
        properties:
          description:
            type: string

    syslog.Policy:
      additionalProperties: {}
      allOf:
      - $ref: '#/components/schemas/policy.AbstractPolicy'
      - type: object
        required:
        - objectType
        properties:
          localClients:
            type: array
            items:
              type: string

Command line used for generation

Steps to reproduce

Edit modules/openapi-generator/src/test/resources/3_0/petstore-with-fake-endpoints-models-for-testing.yaml and add the YAML snippets listed in this issue.

Invoke bin/java-petstore-native.sh

Run mvn package in sample directory.

Open samples/......./openapitools/client/api/SyslogApi.java in file editor.

Related issues/PRs

Suggest a fix

[sebastien-rosset]

I have determined this issue occurs both with the "okhttp-gson" and "native" libraries; probably with all libraries.

[sebastien-rosset]

Narrowing the issue to https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java#L740

If a composed schema 'allOf' has the additionalAttribute set to "true", ModelUtils.isMapSchema() returns true

[sebastien-rosset]

@wing328, @jimschubert , what is the rationale for toAllOfName() to return the name of the first element in the 'allOf' list?
https://github.com/OpenAPITools/openapi-generator/blame/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L1754

The reason I ask is so I can submit a PR that addresses the use cases below. One option is I could do something similar to what has been done for "allOf", i.e. introducing a "x-allOf-name" vendor extension.

Class inheritance
For example, with the schema below, there is a class inheritance hierarchy. I know strictly speaking the JSON schema and OpenAPI are about schema validation, not code generation, but from the OpenAPITools we need to be able to control the output, i.e. whether the templates generate class inheritance, composition or some other language construct.

    syslog.Policy:
      additionalProperties: {}
      allOf:
      - $ref: '#/components/schemas/policy.AbstractPolicy'
      - type: object
        required:
        - objectType
        properties:
          localClients:
            type: array
            items:
              type: string

In this example, I find it surprising that toAllOfName returns the name of the parent class "policy.AbstractPolicy". I was expecting the return value to be "syslog.Policy". Wouldn't this be a better default value? It's more likely that the name of the outer schema captures the complete semantics of the collection of the inner schemas.

A tangential problem is that one OAS author may decide it makes more sense to put the "parent" class first in the allOf children, and another author may think otherwise, that it makes more sense to put it last (or why not somewhere in the middle). I tend to think it makes more sense to put it first, but there is no such requirement in the OAS spec (since the spec doesn't deal with class inheritance, just validation), and I can see how somebody would argue differently.

Composition
As another example, the OAS author may want to express composition instead of inheritance, such as the "Sailboat" schema below. In that case I also think it's really strange for toAllOfName() to return "Hull". "Sailboat would be a good name.

Sailboat:
  allOf:
    - $ref: '#/components/schemas/Hull'
    - $ref: '#/components/schemas/Rigging'
    - $ref: '#/components/schemas/Cabin'
    - $ref: '#/components/schemas/Sails'
    - $ref: '#/components/schemas/Electronics'

[sebastien-rosset]

There might be a problem at this line: https://github.com/OpenAPITools/openapi-generator/blame/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L5251. I don't think it handles the case of a ComposedSchema which also defines additionalproperties: true

[jimschubert]

@sebastien-rosset Thanks for bring this conversation!

Unfortunately, the logic to grab the first item in the array was an original implementation bridging the gap between our 2.3.x branch (when we forked from Swagger Codegen) and the initial implementation of OpenAPI Generator which included minimal OAS 3.0 support.

The method you've linked to would allow generators to override the logic specific to the language's support for composition/inheritance and variance. It was originally added in #2046 and is called for a model via getSchemaType.

I think we'll have better support for inheritance once Justin's PR #4906 is merged.

It still leads the question open about the broken composition use case (see #5097, #1095, and #2080 - probably a few others as well).

The issue between these two arises in user intentions. For instance, OpenAPI Specification claims that discriminator is intended to represent inheritance in a specification document. In practical usage, many specification documents are written with a parent in mind but no property used as a type discriminator. You can see this in your first example which appears to suggest that you want inheritance (via AbstractPolicy). But, according to the specification you'd have composition since you've defined no discriminator. It's possible that you're leaving out the objectType and the discriminator is defined on AbstractPolicy… but the point is that many users write specification documents like this without discriminators.

I made a recommendation here to introduce x-codegen-prefer-polymorphism: true (or false) for allOf implementation without discriminator but with all the inheritance magic. You made a good point about some authors preferring the parent at the first or last indexes, and I see that your reference to subclassOf would solve this and would be used to define the parent schema for inheritance.

Right now, I'm thinking x-codegen-parent might make things much clearer for users who have control over specification documents, allowing them to explicitly define the parent schema. We'd still need to address the use case for those who can't modify the documentation. Maybe that's where we could use the "prefer inheritance" option and make it a CLI/Configurator option?

cc @OpenAPITools/generator-core-team for discussion

[sebastien-rosset]

@sebastien-rosset Thanks for bring this conversation!

Unfortunately, the logic to grab the first item in the array was an original implementation bridging the gap between our 2.3.x branch (when we forked from Swagger Codegen) and the initial implementation of OpenAPI Generator which included minimal OAS 3.0 support.

The method you've linked to would allow generators to override the logic specific to the language's support for composition/inheritance and variance. It was originally added in #2046 and is called for a model via getSchemaType.

I think we'll have better support for inheritance once Justin's PR #4906 is merged.

It still leads the question open about the broken composition use case (see #5097, #1095, and #2080 - probably a few others as well).

I will read these issues. I was taking notes for myself for the various options to generate code:

1. Get all inner schemas, both inline and referenced, flatten all properties into single class/struct
2. Create inheritance or composition model. External properties and vendor extensions may be used to control the implementation output.

a. Flatten properties from all inline schemas into single class/struct.
b. Single class inheritance. Use first referenced inner schema and assume it is the parent class. Fail codegen if there is more than one referenced schema. This would be a limitation of the generator hence it should be avoided
c. Same as above, but vendor extension specifies which inner schema is the parent.
d. Multiple class inheritance. All inner referenced schemas are assumed to be a parent class
e. Create one class per inline schema ( ie schemas that are not referenced) One challenge is there is nowhere in the spec to get a meaningful name.
f. Composition. The generated outer class/struct contains class/structs for each inner referenced schema. One challenge is the same property may be listed more than once across the inner schemas. There must be a single setter and a single getter method.
g. Embedded (it's really a variant of composition, but using language specific construct and specifically golang embedded struct, which has the interesting property of inlining the properties which serializing/deserializing).

The issue between these two arises in user intentions. For instance, OpenAPI Specification claims that discriminator is intended to represent inheritance in a specification document. In practical usage, many specification documents are written with a parent in mind but no property used as a type discriminator. You can see this in your first example which appears to suggest that you want inheritance (via AbstractPolicy).

Sorry I forgot to mention in our case the discriminator for class inheritance is defined in the most abstract class from which all sub-class are descendants.

But, according to the specification you'd have composition since you've defined no discriminator. It's possible that you're leaving out the objectType and the discriminator is defined on AbstractPolicy… but the point is that many users write specification documents like this without discriminators.

I made a recommendation here to introduce x-codegen-prefer-polymorphism: true (or false) for allOf implementation without discriminator but with all the inheritance magic. You made a good point about some authors preferring the parent at the first or last indexes, and I see that your reference to subclassOf would solve this and would be used to define the parent schema for inheritance.

I think x-codegen-prefer-polymorphism could lead to some ambiguities. See my notes above. For example, should the code generate a class for inlined schema? Should it flatten inlined schema (like it does today)? There are many ways that people can think about how the code is going to be generated. It would be nice to also provide properties to control the output. In my case, I don't mind adding vendor extensions because I'm both the author and also using the codegenerator, so can tweak both sides, but sometimes people use an existing OAS document and they don't really have the choice to modify it (or it's too cumbersome).

Right now, I'm thinking x-codegen-parent might make things much clearer for users who have control over specification documents, allowing them to explicitly define the parent schema. We'd still need to address the use case for those who can't modify the documentation. Maybe that's where we could use the "prefer inheritance" option and make it a CLI/Configurator option?

cc @OpenAPITools/generator-core-team for discussion

[jimschubert]

@sebastien-rosset for your use case with the inline schema and $ref merge, OpenAPI Specification specifically says:

When using the discriminator, inline schemas will not be considered.

Seems like we'd need an option or vendor extension to allow this behavior as well.

[sebastien-rosset]

@sebastien-rosset for your use case with the inline schema and $ref merge, OpenAPI Specification specifically says:

When using the discriminator, inline schemas will not be considered.

Seems like we'd need an option or vendor extension to allow this behavior as well.

Like you wrote before, the OAS spec is not clear. Unfortunately the spec mentions both validation and code generation in the introduction, but it never seem to state which sections or sentences apply to code generation. My understanding is the OAS spec is only about validation, and it's completely outside the scope of the spec to dictate how to generate code (?)

So when the spec mentions composition and polymorphism, it's just for validation purpose, correct? Code may be generated in any shape or form, as long as the generated code serializes, deserializes and validates the payload as specified in the spec. Of course in practice, it makes sense for the tooling to use implementation constructs that map the spec, but that is not a requirement. For example, somebody can write code using composition at the implementation level to model the OAS inheritance model, and vice versa.

[sebastien-rosset]

This issue is resolved if using the jersey2 library.