Swagger-codegen: [java] discriminator not defined when inside an allOf: inline object
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 #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
https://github.com/swagger-api/swagger-codegen/issues/2449
https://github.com/swagger-api/swagger-codegen/pull/4085
https://github.com/OAI/OpenAPI-Specification/issues/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;
jeff9finger
All 17 comments
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
on 22 Nov 2016
Also note that your example spec defines 2 discriminators for Object which is forbidden.
cbornet
on 22 Nov 2016
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
on 22 Nov 2016
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 https://github.com/swagger-api/swagger-codegen/pull/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
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"
cbornet
on 22 Nov 2016
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?
jeff9finger
on 22 Nov 2016
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...
cbornet
on 22 Nov 2016
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
on 23 Nov 2016
4251
jeff9finger
on 29 Nov 2016
@wing328 Please take a look at this pr for this issue.
jeff9finger
on 3 Dec 2016
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?
sreeshas
on 4 Dec 2016
"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.
jeff9finger
on 6 Dec 2016
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...
mtsvetanov
on 7 Dec 2016
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
on 7 Dec 2016
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. :-)
jeff9finger
on 19 Dec 2016
@jeff9finger I'll review by this week and let you know if I've any question.
wing328
on 20 Dec 2016
@wing328 Any chance this can be merged soon? I need this feature to be available in the release.
jeff9finger
on 4 Jan 2017
Related issues
aliemrahpekesen
路
3Comments
dylan-colaco
路
3Comments
PavelPolyakov
路
3Comments
jeusdi
路
3Comments
wing328
路
3Comments