[TypeScript] Implement oneOf type resolution without discriminator#22201
Conversation
|
The PR contains breaking change to generated TypeScript code, |
modules/openapi-generator/src/main/resources/typescript/model/ObjectSerializer.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/typescript/model/OneOfClass.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/typescript/model/OneOfClass.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/typescript/model/OneOfClass.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/typescript/model/OneOfClass.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/typescript/model/modelOneOf.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/typescript/model/modelOneOf.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/typescript/model/modelOneOf.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/typescript/model/modelOneOf.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/typescript/model/modelOneOf.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/typescript/model/TypeMatcher.mustache
Outdated
Show resolved
Hide resolved
|
Could you please check the PR again? |
|
Any updates? |
|
sorry for the delay, will try to find some time, its quite a complex PR |
|
Hi @macjohnny , just checking in on this PR. |
|
Any updates? |
macjohnny
left a comment
There was a problem hiding this comment.
@ksvirkou-hubspot I still didnt find time to thoroughly review this, but if you tested this thoroughly, i am fine merging that, just waiting for your confirmation.
are the tests of the samples/openapi3/client/petstore/typescript/tests/one-of/pom.xml correctly executed as part of the CI? i guess they would need to be configured somewhere here no? https://github.com/OpenAPITools/openapi-generator/tree/master/.github/workflows
| return type.replace(" ", "").split("[|&<>]"); | ||
| } | ||
|
|
||
| public class ExtendedCodegenModel extends CodegenModel { |
There was a problem hiding this comment.
could we somehow do without an extended CodegenModel class? everytime the base class is updated (e.g. new properties are added), this would need to be updated here as well
There was a problem hiding this comment.
Alternatively, we could consider moving our additional properties to the base class itself.
I've extended the CodegenModel class because I need to add new properties specific to oneOf constructions. As far as I understand, extending the base class is a common approach — for example, the typescript-fetch generator uses the same pattern with its own ExtendedCodegenModel class.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/samples-typescript-client.yaml">
<violation number="1" location=".github/workflows/samples-typescript-client.yaml:124">
P2: CI wiring for the new oneOf sample excludes the `tests/one-of` suite, so test-only changes won't trigger or run in this workflow.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| - samples/openapi3/client/petstore/typescript/builds/browser/ | ||
| #- samples/openapi3/client/petstore/typescript/tests/browser/ | ||
| #- samples/openapi3/client/petstore/typescript/builds/nullable-enum/ | ||
| - samples/openapi3/client/petstore/typescript/builds/one-of/ |
There was a problem hiding this comment.
P2: CI wiring for the new oneOf sample excludes the tests/one-of suite, so test-only changes won't trigger or run in this workflow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/samples-typescript-client.yaml, line 124:
<comment>CI wiring for the new oneOf sample excludes the `tests/one-of` suite, so test-only changes won't trigger or run in this workflow.</comment>
<file context>
@@ -119,6 +121,7 @@ jobs:
- samples/openapi3/client/petstore/typescript/builds/browser/
#- samples/openapi3/client/petstore/typescript/tests/browser/
#- samples/openapi3/client/petstore/typescript/builds/nullable-enum/
+ - samples/openapi3/client/petstore/typescript/builds/one-of/
- samples/client/petstore/typescript-fetch/builds/default/
- samples/client/petstore/typescript-fetch/builds/es6-target/
</file context>
Summary
OneOfClasswithfindMatchingType()method to determine correct type at runtimeObjectSerializerto use type deduction when discriminator is unavailableWhy
Previously, the TypeScript generator could only resolve oneOf types when a discriminator property was defined in the OpenAPI spec. This limitation meant that valid oneOf schemas without discriminators would fail at runtime.
This change enables automatic type resolution by checking which schema matches the provided data based on required fields, making the generated TypeScript clients more robust and easier to use for specs that don't define discriminators.
Implementation Details
OneOfClassbase class withinstanceOf()method to validate objects against schema requirementsOneOfClassand includefindMatchingType()to iterate through possible typesObjectSerializerchecks forfindMatchingType()method when discriminator is undefined and uses it for type resolutionfindMatchingType()is availableTests
Added integration tests at
samples/openapi3/client/petstore/typescript/tests/one-of/:modules/openapi-generator/src/test/resources/3_0/typescript/oneOf.yamldefines two endpoints with oneOf responsesPetResponse(oneOf without discriminator) correctly deserializes toCatbased on required fields (name,petType)PetDiscriminatorResponse(oneOf with discriminator) correctly deserializes toDogusing discriminator propertynpm testin the test directoryRelated issue: #19868
PR checklist
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.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @topce @akehir @petejohansonxo @amakhrov @davidgamero @mkusaka @joscha