Skip to content
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

[TYPESCRIPT-FETCH] Subclassing components using discriminators fails to convert subclasses to JSON #19524

Conversation

hagis
Copy link
Contributor

@hagis hagis commented Sep 4, 2024

…minators fails to convert subclasses to JSON.

@macjohnny, for your kind attention:

  • Added similar discriminator handling to ToJSON as was already in place for FromJSON.
  • The actual files changed are typescript-fetch/modelGeneric.mustache and typescript-fetch/modelEnum.mustache.
  • Also, adjusted FromJSON a bit in an attempt to support multiple hierarchical levels of discriminators.
  • ~~And fixed an issue with calling FromJSON from the map() function, which caused the index parameter getting inadvertently passed as the ignoreDiscriminator parameter. ~~
  • Additionally, fixed failing "mvn integration-test -f samples/client/petstore/typescript-fetch/builds/prefix-parameter-interfaces/pom.xml"
  • Moreover, added forceConsistentCasingInFileNames:false into tsconfig.json to make tests compile on OsX.

PR checklist

  • [x ] Read the contribution guidelines.
  • [ x] 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.
  • [ z] Run the following to build the project and update samples:
    ./mvnw clean package - did not run, as only changed templates
    ./bin/generate-samples.sh ./bin/configs/*.yaml - ran for typescript-fetch only, as my changes concerned only that
    ./bin/utils/export_docs_generators.sh - did not commit changes resulting from this, as they were not related to my changes, and committing them would probably result in conflicts.
    
  • [x ] File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • [x ] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

fixes #15736
closes #13260

…using discriminators fails to convert subclasses to JSON.

Added similar discriminator handling to ToJSON as was already in place for FromJSON.
The actual files changed are typescript-fetch/modelGeneric.mustache and typescript-fetch/apis.mustache.
Also, adjusted FromJSON a bit in an attempt to support multiple hierarchical levels of discriminators.
And fixed an issue with calling FromJSON from the map() function, which caused the index parameter getting inadvertently passed as the ignoreDiscriminator parameter.
Additionally, fixed failing "mvn integration-test -f samples/client/petstore/typescript-fetch/builds/prefix-parameter-interfaces/pom.xml"
Moreover, added forceConsistentCasingInFileNames:false into tsconfig.json to make tests compile on OsX.
@macjohnny
Copy link
Member

thanks for your contribution!

is this similar to #13260?

cc @SimonErm

is this here related as well? #18740

cc @akira-furukawa-stb

can you all help me understand how we should reconcile all of this and move forward?

@hagis
Copy link
Contributor Author

hagis commented Sep 4, 2024

is this similar to #13260?

It seems to address the same issue, yes. I now realise that my changes to FromJSON being called from map() were unnecessary. And having separate ToJSON and ToJSONTyped functions would have required no changes to map() calls.

Not sure how that other PR handles the case were we have a 3-level hierarchy of objects A-discriminator->B-discriminator-C.

is this here related as well? #18740

I do not think so, as that talks about primitive types wrt. oneOf or anyOf.

can you all help me understand how we should reconcile all of this and move forward?

Merge 13260 or this one, if they are acceptable. Possibly with changes.

@macjohnny
Copy link
Member

@hagis without having looked in more detail at #19524 or #13260, which one would you recommend to review/merge?

@hagis
Copy link
Contributor Author

hagis commented Sep 4, 2024

@hagis without having looked in more detail at #19524 or #13260, which one would you recommend to review/merge?

I would probably ask me to drop the map() changes from #19524 and add a separate ToJSONTyped function instead.

@macjohnny
Copy link
Member

@hagis thanks, can you do your proposed changes and let me know once the PR is ready to review? so we would close #13260 in favor of #19524

…of using ToJSONTyped instead, as that is in line with how FromJSON is already implemented.
@hagis
Copy link
Contributor Author

hagis commented Sep 5, 2024

@hagis thanks, can you do your proposed changes and let me know once the PR is ready to review? so we would close #13260 in favor of #19524

Changes done, PR is ready for review.

Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

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

thanks!

@macjohnny macjohnny merged commit 2f54a2f into OpenAPITools:master Sep 5, 2024
15 checks passed
@wing328 wing328 added this to the 7.9.0 milestone Oct 7, 2024
@wing328 wing328 changed the title Fix for #15736 [TYPESCRIPT-FETCH] Subclassing components using discri… [TYPESCRIPT-FETCH] Subclassing components using discriminators fails to convert subclasses to JSON Oct 7, 2024
@guizmaii
Copy link
Contributor

guizmaii commented Nov 7, 2024

This PR introduces the following issue: #19858

Fix: #20046

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.

[BUG] [TYPESCRIPT-FETCH] Subclassing components using discriminators fails to convert subclasses to JSON
4 participants