-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add support for generating aggregated inlined objects #264
Conversation
@@ -222,26 +224,6 @@ object KaizenParserExtensions { | |||
fun Schema.isOneOfSuperInterface() = | |||
discriminator != null && discriminator.propertyName != null && oneOfSchemas.isNotEmpty() | |||
|
|||
private fun Schema.isPropertyWithAllOfSingleType() = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acanda It appears as though your contribution below was attempting to work around the lack of support. Hopefully this PR improves things further
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of my PR #247 was to treat allOf
schemas with a single $ref
(and nothing else that changes the type) as non-aggregated schemas and keep the type name of the referenced type, i.e.
Dog:
type: object
properties:
owner:
description: "The registered owner of the dog."
allOf:
- $ref: "#/components/schemas/Person"
walker:
description: "The person walking the dog."
allOf:
- $ref: "#/components/schemas/Person"
results in
public data class Dog(
@param:JsonProperty("owner")
@get:JsonProperty("owner")
@get:Valid
public val owner: Person? = null, // not type DogOwner
@param:JsonProperty("owner")
@get:JsonProperty("owner")
@get:Valid
public val walker: Person? = null, // not type DogWalker
)
From looking a the test case, this PR reverts those changes again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior to your change, there was no support for inlined aggregated schemas. Your change added support for inlined allOf schemas with exactly one reference. This PR expands support for inlined schemas to include allOf anyOf with any number of referenced schemas.
The sacrifice is that if there is only one referenced schema in the allOf definition, it no longer short-cuts to reference that directly. But, I would say that the generated data classes are a more accurate representation of the OpenApi modelling this way. If you want to reference the shared Person object you do:
Dog:
type: object
properties:
owner:
$ref: "#/components/schemas/Person"
walker:
$ref: "#/components/schemas/Person"
But if you use an aggregator, you get a new object each time, DogOwner
and DogWalker
in this example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically you are right, aggregations, even with only one reference, should generate a new type. But OpenAPI 3.0 has one major drawback: it ignores any sibling elements of $ref
, so you can't directly document a property with a $ref
. The following is unfortunately not a valid 3.0 spec:
Dog:
type: object
properties:
owner:
description: "The owner of the dog."
$ref: "#/components/schemas/Person"
walker:
description: "The person walking the dog."
$ref: "#/components/schemas/Person"
People who want to document the properties in their API have to use the workaround with description
, allOf
and a single $ref
.
In my case I have to use a third party spec that uses this pattern a lot. There a re a lot of animal to person relations that need to be documented as the property name is not enough to explain the relationship. Generating the code for this spec would end in a lot of DogOwner, DogWalker, CatGroomer, PonyRider, AligatorWrangler, LionTamer, DolphinTrainer, and DinosaurResearcher classes that should just be a single Person class.
Having to deal with all those synthetic classes would make life for people in my shoes considerably harder. I'd probably have to preprocess the spec to remove all the description
and allOf
elements so I end up with only one Person class.
In the end it's your call. I hope Fabrikt keeps supporting a (I think quite common) pattern people use to work around a limitation of OpenAPI 3.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acanda if you think it is beneficial for fabrkit to have the special single-value case that you original added, we can still support that:
https://github.com/cjbooms/fabrikt/pull/266/files
Let me know what you decide, I am happy with or without it. It's not too much extra complexity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we just posted at the same time 😄. As stated above, I'd be happier with support for the single-value case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice. That's a decent justification for the extra complexity
Fabrikt now supports inlined definitions of aggregated objects. For example:
Support is preserved for the special case where the aggregation is over a single schema.
Highlighted by the API supplied in Issue #262