[typescript-fetch] correct oneOf model to `or` not `concat` when handling enums by prince-chrismc · Pull Request #10017 · OpenAPITools/openapi-generator

prince-chrismc · 2021-07-22T19:31:03Z

This was report in 2019 #4387 (comment) shortly after the original PR the author noted it was not tested... I do not know how to add test code but if you point me in the right direction I can try!

still broken #5202
resolves #6513

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 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
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*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master, 5.3.x, 6.0.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.

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02)

This was report in 2019 #4387 (comment) shortly after the original PR the author noted it was not tested

prince-chrismc · 2021-07-22T20:24:30Z

samples/client/petstore/typescript-fetch/builds/enum/models/ObjectOneOfEnum.ts

+    }
+    return {
+
+        'combinedStringEnum': !exists(json, 'combined-string-enum') ? undefined : StringEnum | StringEnumTwoFromJSON(json['combined-string-enum']),


This is still incorrectly generated

prince-chrismc · 2021-07-22T20:24:45Z

samples/client/petstore/typescript-fetch/builds/enum/models/ObjectOneOfEnum.ts

+    }
+    return {
+
+        'combined-string-enum': StringEnum | StringEnumTwoToJSON(value.combinedStringEnum),


This is still incorrectly generated

prince-chrismc · 2021-07-22T20:29:32Z

I do not know how to split datatype when it's a combination

openapi-generator/modules/openapi-generator/src/main/resources/typescript-fetch/modelGeneric.mustache

Line 75 in fe9636e

    
           '{{name}}': {{^required}}!exists(json, '{{baseName}}') ? undefined : {{/required}}{{datatype}}FromJSON(json['{{baseName}}']),

openapi-generator/modules/openapi-generator/src/main/resources/typescript-fetch/modelGeneric.mustache

Line 124 in fe9636e

'{{baseName}}': {{datatype}}ToJSON(value.{{name}}),

prince-chrismc · 2021-07-22T20:32:57Z

samples/client/petstore/typescript-fetch/builds/enum/models/StringOneOf.ts

+    if ((json === undefined) || (json === null)) {
+        return json;
+    }
+    return StringEnumFromJSONTyped(json, true) || StringEnumTwoFromJSONTyped(json, true);


Previously this came out as a object spread of the wrong type {...FromJSONTyped(json, true), ...FromJSONTyped(json, true) }

is it valid for an enum to include an empty string ("") as a value? If so, we might want to be stricter here and use ?? instead of ||

however, apart from an empty string, this || will never work at all. StringEnumFromJSONTyped always returns some value (because a null check is done just before it - line 36), so the second part, StringEnumTwoFromJSONTyped will never be executed

"" are valid in OAS so ?? is required ... it's also a problem for boolean and numbers so good catch.

I wrongly assumed casting json as {{classname}} would return falsy but that's not the case!

amakhrov

@prince-chrismc nice work here!

Could you please add an example of oneOf with object types, not string enums? I believe it is a more common case, and will better surface potential problems, if any

amakhrov · 2021-07-28T16:15:34Z

samples/client/petstore/typescript-fetch/builds/enum/models/StringOneOf.ts

+    if (value === null) {
+        return null;
+    }
+    return StringEnumToJSON(value as StringEnum) || StringEnumTwoToJSON(value as StringEnumTwo);


the second part after || will never work.
It's not a big deal for string enums, which don't really convert values and only do type assertion. However, for more complex data types it's a potential problem - right?

It didn't cross my mind, our docs only use string enums. I test it out tomorrow and see!

This reverts commit 4b40ca2.

prince-chrismc · 2021-07-29T23:59:34Z

Could you please add an example of oneOf with object types, not string enums? I believe it is a more common case, and will better surface potential problems, if any

Can you take a look at my commit 4b40ca2 I got the following error

Root: Y:\repository\openapi-generator
[pool-1-thread-7] Generating typescript-fetch (outputs to Y:\repository\openapi-generator\samples\client\petstore\typescript-fetch\builds\sagas-and-records)▒
[pool-1-thread-9] Generating typescript-fetch (outputs to Y:\repository\openapi-generator\samples\client\petstore\typescript-fetch\builds\with-interfaces)▒
[pool-1-thread-2] Generating typescript-fetch (outputs to Y:\repository\openapi-generator\samples\client\petstore\typescript-fetch\builds\default)▒
[pool-1-thread-8] Generating typescript-fetch (outputs to Y:\repository\openapi-generator\samples\client\petstore\typescript-fetch\builds\typescript-three-plus)▒
[pool-1-thread-5] Generating typescript-fetch (outputs to Y:\repository\openapi-generator\samples\client\petstore\typescript-fetch\builds\multiple-parameters)▒
[pool-1-thread-6] Generating typescript-fetch (outputs to Y:\repository\openapi-generator\samples\client\petstore\typescript-fetch\builds\prefix-parameter-interfaces)▒
[pool-1-thread-3] Generating typescript-fetch (outputs to Y:\repository\openapi-generator\samples\client\petstore\typescript-fetch\builds\enum)▒
[pool-1-thread-1] Generating typescript-fetch (outputs to Y:\repository\openapi-generator\samples\client\petstore\typescript-fetch\builds\default-v3.0)▒
[pool-1-thread-11] Generating typescript-fetch (outputs to Y:\repository\openapi-generator\samples\client\petstore\typescript-fetch\builds\without-runtime-checks)▒
[pool-1-thread-4] Generating typescript-fetch (outputs to Y:\repository\openapi-generator\samples\client\petstore\typescript-fetch\builds\es6-target)▒
[pool-1-thread-10] Generating typescript-fetch (outputs to Y:\repository\openapi-generator\samples\client\petstore\typescript-fetch\builds\with-npm-version)▒
[pool-1-thread-3] Generation failed for typescript-fetch: (RuntimeException) Could not process model 'ObjectEnum'.Please make sure that your schema is correct!
java.lang.RuntimeException: Could not process model 'ObjectEnum'.Please make sure that your schema is correct!
        at org.openapitools.codegen.DefaultGenerator.generateModels(DefaultGenerator.java:499)
        at org.openapitools.codegen.DefaultGenerator.generate(DefaultGenerator.java:875)
        at org.openapitools.codegen.cmd.GenerateBatch$GenerationRunner.run(GenerateBatch.java:232)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
        at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: java.lang.NullPointerException
        at org.openapitools.codegen.utils.ModelUtils.isBinarySchema(ModelUtils.java:647)
        at org.openapitools.codegen.utils.ModelUtils.isFileSchema(ModelUtils.java:659)
        at org.openapitools.codegen.languages.TypeScriptFetchClientCodegen.getTypeDeclaration(TypeScriptFetchClientCodegen.java:295)
        at org.openapitools.codegen.languages.TypeScriptFetchClientCodegen.addAdditionPropertiesToCodeGenModel(TypeScriptFetchClientCodegen.java:305)
        at org.openapitools.codegen.DefaultCodegen.fromModel(DefaultCodegen.java:2659)
        at org.openapitools.codegen.languages.TypeScriptFetchClientCodegen.fromModel(TypeScriptFetchClientCodegen.java:449)
        at org.openapitools.codegen.languages.TypeScriptFetchClientCodegen.fromModel(TypeScriptFetchClientCodegen.java:37)
        at org.openapitools.codegen.DefaultGenerator.processModels(DefaultGenerator.java:1250)
        at org.openapitools.codegen.DefaultGenerator.generateModels(DefaultGenerator.java:494)

prince-chrismc · 2021-07-30T00:01:28Z

❤️ Thank you for the review, it's clear after 6 months I am just starting my typescript journey... 8yrs of C++ was easier 🤣

Greatly appreciate your help!

prince-chrismc · 2021-07-30T00:06:04Z

modules/openapi-generator/src/main/resources/typescript-fetch/modelEnum.mustache

+    if ((json === undefined) || (json === null)) {
+        return json;
+    }
+    if(!Object.values({{classname}}).includes(json as {{classname}})) {
+        return null;
+    }


Are these runtime checks allowed?

amakhrov · 2021-07-30T22:04:26Z

Can you take a look at my commit 4b40ca2 I got the following error

Values of enum cannot be objects.

What I meant was trying oneOf with regular object types, not with enums at all.

prince-chrismc · 2021-08-06T00:24:37Z

What I meant was trying oneOf with regular object types

They do not work sadly, they also fall victim to #10017 (comment)

amakhrov · 2021-08-06T00:55:00Z

hey do not work sadly, they also fall victim to #10017 (comment)

I feel it means the serialization/deserialization logic should be loosened (type-wise) - the code should try to (de-)serialize all possible properties from intersection (A & B), despite the fact the real type we work with is the union A | B.

prince-chrismc · 2021-08-06T01:53:27Z

I am a little out of my depth, but AFAIK It does that for properties... it looks like the "type" is not well parsed... but I did not look into that section of the tool.

However your suggestion makes sense... we originally user the angular generator and that had no issue... and the templates seem much less complicated https://github.com/OpenAPITools/openapi-generator/blob/bf89f40263c6a57a76697c551d2c30c651812f7a/modules/openapi-generator/src/main/resources/typescript-angular/modelOneOf.mustache

amakhrov · 2021-08-06T17:43:26Z

The main difference between typescript-fetch and typescript-angular (apart from using a different underlying http library, of course :) ) is that typescript-fetch produces code for run-time serializing/deserializing. When doing so for nested data structures, it needs to understand at run time which model it's handling at each point, in order to call a corresponding serializer.

On the other hand, code generated by typescript-angular passes the api data through as is, so it doesn't face the same challenge with oneOf schemas.

prince-chrismc · 2021-08-10T19:39:46Z

Is it possible to move ahead with this PR? 🙏 Sadly if the work gets anymore complicated I wouldn't be able to help fix the issue

amakhrov · 2021-08-10T20:06:55Z

modules/openapi-generator/src/main/resources/typescript-fetch/modelEnum.mustache

 {{>modelEnumInterfaces}}

-export function {{classname}}FromJSON(json: any): {{classname}} {
+export function {{classname}}FromJSON(json: any): {{classname}} | null {


this is gonna be a breaking change. if an enum property is marked as required / non-nullable in the spec, all consumers expect that it's defined.
after this change some apps will likely stop compiling.

why do yo need it anyway? you can keep the original code here - return the enum value as is, without any processing

Typescript gave a me a warning about the implicit return null. I believe it can be removed

that's because this PR changed the code inside the function, narrowing down json type to null before returning it

Playing around with this more... it previously returned a value that was not valid. so if you passed 7 and your enum was [0, 1, 2] you would get back 7 which is not the right type.

well, we trust the api response matches the spec. otherwise the code is gonna break anyway, since we don't do full shape verification

The challenge is older clients being used with newer backends which just weren't aware of the existence of such values

I would say, null is as unexpected as a bogus enum value (if the property is not marked as nullable).
Again, i honestly believe it's way out of scope of the generated api client.

modules/openapi-generator/src/main/resources/typescript-fetch/modelOneOf.mustache

prince-chrismc · 2021-08-13T18:08:26Z

samples/client/petstore/typescript-fetch/builds/one-of/models/ObjectCombinedOneOf.ts

+    if (value === null) {
+        return null;
+    }
+    return PatternObjectToJSON(value as PatternObject) ?? PatternObjectTwoToJSON(value as PatternObjectTwo);


Finally 🎉 Got that trouble code generated!

Before it was { ...TypeOneFromJson(value), ...TypeTwoFromJson(value) } which was actually an all of? 🤔

AFAIK, With these changes we now have "its was completely parsed as Type one" or "its not TypeOne and completely parsed as TypeTwo"

we now have "its was completely parsed as Type one" or "its not TypeOne and completely parsed as TypeTwo"

erhm, not exactly. TypeOneFromJson only returns null if input is null, otherwise it returns an object So the second parsing (PatternObjectTwoToJSON) will never happen

prince-chrismc · 2021-08-14T02:45:29Z

🤔 Hmm

since we don't do full shape verification

It seems like that would be required for this to work out as you point out in #10017 (comment) other wise I've made wierd code worse 🤣

There's the fact

typescript-fetch produces code for run-time serializing/deserializing #10017 (comment)

Should continue down this rabbit hole? Or is this too aggressive of a change?

amakhrov · 2021-08-14T04:53:24Z

Probably the most correct way to address your case is to handle primitive values differently from objects.

perhaps need some condition here to process enums and objects differently. might need to create a flag for that in the java model, if it's not available yet

So object models will keep deserializing using an object spread ({...oneOfFirstFromJson(data), ...oneOfSecondFromJson(data)}), while enums will just pass the value through as is

prince-chrismc · 2021-08-15T14:09:00Z

Thanks for all you help.

prince-chrismc and others added 8 commits July 22, 2021 14:05

[typescript-fetch] correct one of model

505ceaa

This was report in 2019 #4387 (comment) shortly after the original PR the author noted it was not tested

following build steps

eee43b4

following build steps

39b6299

Update modelOneOf.mustache

cf1b7b1

following build steps

59c117e

missing file

9df3375

exmaple with bad generation

200a6c0

revert bad change

45b537a

prince-chrismc commented Jul 22, 2021

View reviewed changes

proposed work around

3503ef7

prince-chrismc mentioned this pull request Jul 22, 2021

[BUG][Typescript-Fetch] Incorrect toJSON & fromJSONTyped functions when using oneOf #5202

Open

5 tasks

amakhrov reviewed Jul 28, 2021

View reviewed changes

cmcarthu added 3 commits July 29, 2021 19:56

make sure one of works with falsy

b45cf8f

test object enum

4b40ca2

Revert "test object enum"

2f5fb8d

This reverts commit 4b40ca2.

following build steps

e68fe47

prince-chrismc commented Jul 30, 2021

View reviewed changes

following build steps

bf89f40

prince-chrismc changed the title ~~[typescript-fetch] correct oneOf model to or not concat~~ [typescript-fetch] correct oneOf model to or not concat when handling enums Aug 6, 2021

amakhrov reviewed Aug 10, 2021

View reviewed changes

modules/openapi-generator/src/main/resources/typescript-fetch/modelOneOf.mustache Outdated Show resolved Hide resolved

sarumont mentioned this pull request Aug 12, 2021

[BUG] [typescript-node] Models not generated properly using oneOf #10145

Open

6 tasks

prince-chrismc and others added 2 commits August 13, 2021 13:59

Update one-of.yaml

5f90822

following build steps

27abd20

prince-chrismc commented Aug 13, 2021

View reviewed changes

better null/truthy handling

3fb4b52

prince-chrismc closed this Aug 15, 2021

amakhrov mentioned this pull request Oct 14, 2021

Issue #10593 Fix nullable property implemented with oneOf construct #10602

Closed

5 tasks

Uh oh!

Conversation

prince-chrismc commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prince-chrismc commented Jul 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amakhrov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prince-chrismc commented Jul 29, 2021

Uh oh!

prince-chrismc commented Jul 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amakhrov commented Jul 30, 2021

Uh oh!

prince-chrismc commented Aug 6, 2021

Uh oh!

amakhrov commented Aug 6, 2021

Uh oh!

prince-chrismc commented Aug 6, 2021

Uh oh!

amakhrov commented Aug 6, 2021

Uh oh!

prince-chrismc commented Aug 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prince-chrismc Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prince-chrismc commented Aug 14, 2021

Uh oh!

amakhrov commented Aug 14, 2021

Uh oh!

prince-chrismc commented Aug 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

prince-chrismc commented Jul 22, 2021 •

edited

Loading

prince-chrismc Aug 13, 2021 •

edited

Loading