#185: Use discriminator fields only in codecs #200
Conversation
…to Coproduct creation logic
…-fields-only-in-codecs
| private def isDiscriminatorProperty( | ||
| property: ParameterRef, | ||
| coproducts: List[Coproduct] | ||
| ): Boolean = | ||
| coproducts | ||
| .flatMap(_.discriminator) | ||
| .exists { discriminator => | ||
| property.paramName.term.value == discriminator.fieldName | ||
| } |
There was a problem hiding this comment.
Every property that is a discriminator in any of the Product's parents is marked as a discriminator property and thus not created as part of the model.
There could be an edge case in which a Product is a child of multiple Coproducts that define different param names for their discriminators, in which case you might want to have all these params as part of the model.
I see this as an anti-pattern that should be avoided.
| .map { | ||
| case EnumValue.StringEv(v) => Lit.String(v) | ||
| case EnumValue.IntEv(v) => Lit.Int(v) | ||
| } |
There was a problem hiding this comment.
It might be nicer not to use the Literal here, but the EnumValue itself, however, this adds complexity and we chose not to.
There was a problem hiding this comment.
I think that is a good compromise. We are the ones who generate the enum so we should be fine. Do you mind adding a comment here about that decision?
| discriminator: | ||
| propertyName: name |
There was a problem hiding this comment.
Enum discriminator property without explicit mapping is not supported.
| mapping: | ||
| 1: "#/components/schemas/Person" | ||
| 2: "#/components/schemas/Organization" |
There was a problem hiding this comment.
Int discriminator property without explicit mapping is not supported. So we changed the test to have an explicit mapping.
|
Thank you, this is a great work!
👍
This project follows semantic versioning. According to the rules versions prior to 1.0.0 do not have to guarantee any compatibility (https://developerexperience.io/practices/semantic-versioning). As this is indeed a breaking change I would like to release it as |
|
I have no idea why the mill integration test is failing but it seems that this is some weird gh quirk. I tested it locally and it passes. It also passes on the CI, just on another branch:https://github.com/ghostbuster91/sttp-openapi-generator/commits/test-ci |
…explain decision to use the value instead of the Enum
It passed locally for me as well, and it is passing now, after my commit.
That's great. I didn't know this about Semantic version.
The test for the allOf discriminator is exactly the case I had in mind. It's such a surprise to me that it works without changes, I hadn't even considered to just make the test and see if it runs. |
We have chosen not to make this functionality configurable, to avoid complexity.
This change should be released as a new major version, as it contains breaking changes.