Skip to content

Kotlinx polymorphism with custom discriminator support #21531

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

Merged
merged 6 commits into from
Jul 31, 2025

Conversation

some00
Copy link
Contributor

@some00 some00 commented Jul 8, 2025

  • added new config with kotlinx, discriminator (/w custom name) and kotlinx_serialization
  • remove discriminator properties from the generator in both base and derived classes
  • set discriminatorValue in additionalProperties of derived classes
  • add JsonClassDiscriminator the base classes in the template
  • set SerialName to discriminatorValue in the template
  • change base classes to sealed class from interface
  • make variables in base classes abstract
  • added test and new sample for validation

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (For Windows users, please run the script in WSL)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.
    @dr4ke616 @karismann @Zomzog @andrewemery @4brunu @yutaka0m @stefankoppier @e5l

some00 added 4 commits July 8, 2025 13:08
- added new config with kotlinx, discriminator (/w custom name) and
  kotlinx_serialization
- remove discriminator properties from the generator in both base and
  derived classes
- set discriminatorValue in additionalProperties of derived classes
- add JsonClassDiscriminator the derived classes in the template
- set SerialName to discriminatorValue in the template
- change base classes to sealed class instead of interface
- make variables in base classes abstract
@geirsagberg
Copy link

Great work, this is exactly what we are missing! Hopefully this can be merged somewhat quickly 👍

@wing328
Copy link
Member

wing328 commented Jul 31, 2025

thanks for the pr

let's give it a try

@wing328 wing328 merged commit dba7aee into OpenAPITools:master Jul 31, 2025
61 checks passed
@wing328 wing328 added this to the 7.15.0 milestone Jul 31, 2025
animal:
title: An animal
type: object
properties:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this spec incomplete? According to https://swagger.io/docs/specification/v3_0/data-models/inheritance-and-polymorphism/#discriminator, the discriminator property should be part of the listed properties.

I know this PR is already merged, but I'm hoping to get some clarification since we're looking forward to using this, but our spec does contain the discriminator property in the list of properties so I'm worried it won't work for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@F43nd1r I don't know about the spec being incomplete. I'm no expert there.
I've modified the test resource, added the discriminator to the list of properties. The generator outputed the same exact code, so you should be able to use it with your spec.

@Override
public Map<String, ModelsMap> postProcessAllModels(Map<String, ModelsMap> objs) {
objs = super.postProcessAllModels(objs);
if (getSerializationLibrary() == SERIALIZATION_LIBRARY_TYPE.kotlinx_serialization) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@some00 thank you so much!!
the only thing, this condition is missing kotlin-multiplatform, || getLibrary().equals(MULTIPLATFORM)
could you please fix that?..:)

@katepol
Copy link

katepol commented Aug 15, 2025

@wing328 should this functionality somehow be related to SchemaSupportFeature.Union?
To be honest, it very unclear what is SchemaSupportFeature.Union desined for, as there is no support anywhere

@yshrsmz
Copy link

yshrsmz commented Aug 19, 2025

I've created multiplatform port of this PR: #21772

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants