[java] discriminator not defined when inside an allOf: inline object #4226

jeff9finger · 2016-11-20T22:40:14Z

Description

When a discriminator is defined in a spec in an allOf inline object, it is ignored. This prevents the inheritance in the generated code from being generated correctly.

I have been studying the open api spec and swagger-codegen projects for the constraints on allOf:

It appears that swagger-codegen (language is java) only recognizes the last inline element of an allOf: array. I have not found a statement in the open api spec or code in swagger-codegen that specifies this constraint.

How are inline objects supposed to be handled when there are multiple inline objects defined within an allOf array?
How are discriminators inside the objects from Adds the missing install-ivy build script #1 supposed to be represented in the generated code (java in this case)?

Currently, it appears that only the last inline object is recognized.

FYI: This came up in trying to define a discriminator for the inline object.

Swagger-codegen version

2.2.2-SNAPSHOT + pr 4085

Swagger declaration file content or url

definitions:

  Object:
    allOf:
      - $ref: '#/definitions/SomeOtherObject'
      - type: object
        discriminator: object_type0
        required:
          - object_type0
        properties:          
          object_type0:
            type: string
          name:
            type: string
      - type: object
        discriminator: object_type1
        required:
          - object_type1
        properties:          
          object_type1:
            type: string
          name:
            type: string

Command line used for generation

Maven

                    <plugin>
                        <groupId>io.swagger</groupId>
                        <artifactId>swagger-codegen-maven-plugin</artifactId>
                        <version>${swagger-codegen.version}</version>
                        <executions>
                            <execution>
                                <id>xxx</id>
                                <goals>
                                    <goal>generate</goal>
                                </goals>
                                <phase>generate-test-sources</phase>
                                <configuration>
                                    <verbose>false</verbose>
                                    <environmentVariables>
                                    </environmentVariables>
                                    <inputSpec>${project.build.directory}/${api.spec.file}</inputSpec>
                                    <output>${project.build.directory}/generated/java-client</output>
                                    <language>java</language>
                                    <library>jersey2</library>
                                    <configOptions>
                                        <sourceFolder>.</sourceFolder>
                                        <dateLibrary>java8</dateLibrary>
                                        <hideGenerationTimestamp>false</hideGenerationTimestamp>
                                    </configOptions>
                                    <addCompileSourceRoot>true</addCompileSourceRoot>
                                    <bigDecimalAsString>true</bigDecimalAsString>
                                    <generateApiTests>true</generateApiTests>
                                    <generateModelTests>true</generateModelTests>
                                    <modelPackage>com.acme.api.model</modelPackage>
                                    <apiPackage>com.acme.api</apiPackage>
                                    <invokerPackage>com.acme.api.client</invokerPackage>
                                </configuration>
                            </execution>
                        </executions>
                    </plugin>

Related issues

#2449
#4085
OAI/OpenAPI-Specification#848

Suggest a Fix

Adding the following to DefaultCodeGen seems to fix the issue for me. However, this assumes that allOf: can have no more than one inline element that contains a discriminator. (My experience also shows that allOf: understands no more than one inline elements. If more than one exists, only last one is recognized).

@@ -1240,6 +1251,12 @@ public class DefaultCodegen {
                 allProperties = new LinkedHashMap<String, Property>();
                 allRequired = new ArrayList<String>();
                 m.allVars = new ArrayList<CodegenProperty>();
+                for (Model innerModel: composed.getAllOf()) {
+                    if (innerModel instanceof ModelImpl) {
+                        m.discriminator = ((ModelImpl) innerModel).getDiscriminator();
+                        break; // only one discriminator allowed at the moment.
+                    }
+                }
             } else {
                 allProperties = null;
                 allRequired = null;

The text was updated successfully, but these errors were encountered:

cbornet · 2016-11-22T21:33:41Z

~~I don't understand: what is the point of putting a discriminator in an in-line object ? Since it won't have any child...~~
Edit: Actually, if you have only one in-line object, the codegen will add the properties to the enclosing object (as expected) I don't think it's possible to have several in-line objects in an allOf. What do you expect to generate ?

cbornet · 2016-11-22T21:40:48Z

Also note that your example spec defines 2 discriminators for Object which is forbidden.

jeff9finger · 2016-11-22T21:54:34Z

The spec that I wrote above is not realistic. It was created to get clarity on whether more than one inline object is allowed. @webron indicated (IIUC) that the OpenAPI spec is silent on the issue and the decision is left to the tools. So, my issue is not that I am trying to create 2 inline objects each with a discriminator. I want to know what swagger-codegen's behavior should be when there are more than one inline objects. Currently, the behavior is to ignore all except the last one.

The reason I am asking this has to do with the suggested fix. In the swagger-codeven code, adding the "discriminator" to the CodegenModel for the sub class works, but it assumes that only one inline object can exist.

jeff9finger · 2016-11-22T21:58:34Z

It is good to know that "2 discriminators for Object which is forbidden." Where is that documented?

But also, please clarify what you mean by "2 discriminators for Object". Are you saying that having a discriminator in BaseObj and one in SubObj is forbidden?

That does not seem to be the case in practice. Here is an example that illustrates the problem I am trying to resolve. (taken from #4085)

In order to create the inheritance from the SubObj, one inline object with a discriminator is required. This is the specified way to accomplish this according to other issues in this project.

definitions:
BaseObj:
    type: object
    discriminator: object_type
    required:
      - id
      - object_type
    properties:
      id:
        type: integer
        format: int64
      object_type:
        type: string
        readOnly: true
  SubObjType:
    type: string
    enum: 
    - daily
    - monthly
    - quarterly
    - yearly

  SubObj:
    allOf:
      - $ref: '#/definitions/BaseObj'
      - type: object
        discriminator: sub_obj_type
        required:
          - sub_obj_type
        properties:          
          sub_obj_type:
            $ref: '#/definitions/SubObjType'
          name:
            type: string
            readOnly: true
  DailySubObj:
    allOf:
      - $ref: '#/definitions/SubObj'
      - type: object
         properties:
           day_of_month:
             type: integer
             format: int32

cbornet · 2016-11-22T22:33:33Z

The discriminator is the schema property name that is used to differentiate between other schema that inherit this schema

I agree that the fact that the discriminator is unique is not explicit but how would you choose which Model to apply if the JSON you get has two discriminator fields ?

As for your BaseObj/SubObj spec, it looks valid. Just keep in mind that if SubObj didn't have a discriminator, people would still expect DailySubObj to extend SubObj and be discriminated by object_type (see #3474). So the rule would be something like "if an object holds a discriminator (unique), it becomes the new discriminator for the sub-hierarchy"

jeff9finger · 2016-11-22T22:43:54Z

The rule you stated ("if an object holds a discriminator (unique), it becomes the new discriminator for the sub-hierarchy") is what my understanding is and is how I plan on implementing this. I do have one question though...

When you say "if an object holds a discriminator (unique)", does that mean that the discriminator property name is unique? That is the way i understood it at first, but after some thought, it seems like swagger-codegen does not care whether the discriminator name is unique from its parent discriminator property. So, I think that even if the discriminator property name is the same as its parent, it should still be considered as a distinct discriminator.

For example:

definitions:
BaseObj:
    type: object
    discriminator: object_type
    required:
      - id
      - object_type
    properties:
      id:
        type: integer
        format: int64
      object_type:
        type: string

  SubObj:
    allOf:
      - $ref: '#/definitions/BaseObj'
      - type: object
        discriminator: object_type
        required:
          - object_type
        properties:          
          object_type:
            type: string
          name:
            type: string
  DailySubObj:
    allOf:
      - $ref: '#/definitions/SubObj'
      - type: object
         properties:
           day_of_month:
             type: integer
             format: int32

Even though the discriminator property names are the same, the fact that one exists on SubObj suggests that it has a purpose. Should this be an error, a warning, or accepted as a distinct discriminator property?

cbornet · 2016-11-22T22:53:10Z

That's a good question. I think that it shouldn't be possible to have properties with the same name in a parent and a child (imagine one is string type and the other number). But the discriminator is kinda special...

jeff9finger · 2016-11-23T21:47:37Z

Implementing this issue by only allowing one inline object and issuing a warning. And if the parent has a discriminator != null, the child is not added to the grandparent's children.

jeff9finger · 2016-11-29T15:45:53Z

#4251

jeff9finger · 2016-12-02T23:54:11Z

@wing328 Please take a look at this pr for this issue.

sreeshas · 2016-12-04T22:02:18Z

It is good to know that "2 discriminators for Object which is forbidden." Where is that documented?

It is not explicit as mentioned by @cbornet but aren't discriminators by definition unique? If you have two discriminators for an Object, the server/client would have to look at two fields to decide the behavior.

I couldn't find the documentation. I believe the discussion here https://gist.github.com/leedm777/5730877 should be moved to wiki.

But also, please clarify what you mean by "2 discriminators for Object". Are you saying that having a discriminator in BaseObj and one in SubObj is forbidden?

if it is not forbidden does it mean DailySubObj must be discriminated based on the context in which it is used? i.e
Consider two operations. Operation1 accepts BaseObj and Operation 2 accepts SubObj.
If DailySubObj is passed to Operation 1, the discriminator would be object_type.
If DailySubObj is passed to Operation 2, the discriminator would be sub_obj_type.

@cbornet

That's a good question. I think that it shouldn't be possible to have properties with the same name in a parent and a child (imagine one is string type and the other number). But the discriminator is kinda special...

Can you explain what you meant when you said discriminator is kinda special?

jeff9finger · 2016-12-06T16:07:36Z

"If you have two discriminators for an Object, the server/client would have to look at two fields to decide the behavior."

Just to be clear, there are not 2 discriminators defined in the same object definition is the swagger specification. But an object that is at least 2 levels "deep" could have a discriminator on its immediate base class, and that class could have a discriminator on its base class. (this is the example specified)

In Java, this is handled by the Jackson annotations. In practice, only the "closest" discriminator is really required though. For example, the payload for DailySubObj can only specify sub_obj_type and it is deserialized correctly (with this pr).

"But also, please clarify what you mean by "2 discriminators for Object". Are you saying that having a discriminator in BaseObj and one in SubObj is forbidden?"

If BaseObj and SubObj both specified object_type as the discriminator respectively, there would be no distinction between a subclass of BaseObj and a subclass of SubObj. So, the each discriminator property name in a class hierarchy must be unique.

"if it is not forbidden does it mean DailySubObj must be discriminated based on the context in which it is used? i.e
Consider two operations. Operation1 accepts BaseObj and Operation 2 accepts SubObj.
If DailySubObj is passed to Operation 1, the discriminator would be object_type.
If DailySubObj is passed to Operation 2, the discriminator would be sub_obj_type."

Yes, DailySubObj is discriminated based upon the context. The example you provide illustrates the use case well. Operation 1 sees the DailySubObj as a SubObj (because it only cares about the object from the SubObj perspective). And Operation 2 sees the DailySubObj as DailySubObj.

mtsvetanov · 2016-12-07T17:33:27Z

Yes, DailySubObj is discriminated based upon the context. The example you provide illustrates the use case well. Operation 1 sees the DailySubObj as a SubObj (because it only cares about the object from the SubObj perspective). And Operation 2 sees the DailySubObj as DailySubObj.

This is some possible interpretation, but the OpenAPI spec, doesn't say anything about this case, it doesn't enforce this (or any other) interpretation. It seems that the multiple levels of allOf-based composition or the multiple discriminators possibility was just not taken into account when designing/describing the discriminator semantics.

This seems like a huge gap in the specification to me which MUST be fixed.

BTW, this possible interpretation seems to assume that "discriminator" declaration is valid only one level down (or until another discriminator is declared). Which one of these are you suggesting? Again, the specification says nothing about any of these...

jeff9finger · 2016-12-07T17:59:37Z

See "related issues" section for the question asked in the OpenAPI project. OpenAPI Issue 848

I think the whole issue of inheritance this will be addressed in the next version of the OpenAPI spec.

I am assuming that a discriminator is valid until another one is declared. This allows the behavior in Issue 2449 to continue to work.

jeff9finger · 2016-12-19T17:12:32Z

What is the status of this issue being merged? I'd really like to go back to using the standard swagger-codegen release for my project again. :-)

wing328 · 2016-12-20T13:49:51Z

@jeff9finger I'll review by this week and let you know if I've any question.

jeff9finger · 2017-01-04T21:08:21Z

@wing328 Any chance this can be merged soon? I need this feature to be available in the release.

szakrewsky mentioned this issue Nov 21, 2016

[Java] Fix SubClass annotations missing from the base class #4085

Merged

3 tasks

jeff9finger mentioned this issue Nov 23, 2016

Add a vendor extension to specify the discriminator value for a model definition #4244

Open

jeff9finger mentioned this issue Nov 29, 2016

Fixes for issue 4226. Detects and warns about more than one inline ob… #4251

Closed

3 tasks

wing328 added Feature: Composition / Inheritance Issue: Bug labels Dec 9, 2016

jeff9finger mentioned this issue Feb 3, 2017

Java support for discriminator attribute when reading objects #1253

Closed

jeff9finger mentioned this issue Jul 23, 2018

Don't use visible = true in Java model classes OpenAPITools/openapi-generator#613

Closed

4 tasks

jdart mentioned this issue Apr 10, 2020

PHP Client doesn't seem to support anyOf with discriminator #10168

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[java] discriminator not defined when inside an allOf: inline object #4226

[java] discriminator not defined when inside an allOf: inline object #4226

jeff9finger commented Nov 20, 2016

cbornet commented Nov 22, 2016 •

edited

Loading

cbornet commented Nov 22, 2016

jeff9finger commented Nov 22, 2016

jeff9finger commented Nov 22, 2016

cbornet commented Nov 22, 2016

jeff9finger commented Nov 22, 2016 •

edited

Loading

cbornet commented Nov 22, 2016 •

edited

Loading

jeff9finger commented Nov 23, 2016

jeff9finger commented Nov 29, 2016

jeff9finger commented Dec 2, 2016

sreeshas commented Dec 4, 2016 •

edited

Loading

jeff9finger commented Dec 6, 2016

mtsvetanov commented Dec 7, 2016

jeff9finger commented Dec 7, 2016 •

edited

Loading

jeff9finger commented Dec 19, 2016

wing328 commented Dec 20, 2016

jeff9finger commented Jan 4, 2017

[java] discriminator not defined when inside an allOf: inline object #4226

[java] discriminator not defined when inside an allOf: inline object #4226

Comments

jeff9finger commented Nov 20, 2016

Description

Swagger-codegen version

Swagger declaration file content or url

Command line used for generation

Related issues

Suggest a Fix

cbornet commented Nov 22, 2016 • edited Loading

cbornet commented Nov 22, 2016

jeff9finger commented Nov 22, 2016

jeff9finger commented Nov 22, 2016

cbornet commented Nov 22, 2016

jeff9finger commented Nov 22, 2016 • edited Loading

cbornet commented Nov 22, 2016 • edited Loading

jeff9finger commented Nov 23, 2016

jeff9finger commented Nov 29, 2016

jeff9finger commented Dec 2, 2016

sreeshas commented Dec 4, 2016 • edited Loading

jeff9finger commented Dec 6, 2016

mtsvetanov commented Dec 7, 2016

jeff9finger commented Dec 7, 2016 • edited Loading

jeff9finger commented Dec 19, 2016

wing328 commented Dec 20, 2016

jeff9finger commented Jan 4, 2017

cbornet commented Nov 22, 2016 •

edited

Loading

jeff9finger commented Nov 22, 2016 •

edited

Loading

cbornet commented Nov 22, 2016 •

edited

Loading

sreeshas commented Dec 4, 2016 •

edited

Loading

jeff9finger commented Dec 7, 2016 •

edited

Loading