-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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] correct oneOf model to or
not concat
when handling enums
#10017
Conversation
This was report in 2019 #4387 (comment) shortly after the original PR the author noted it was not tested
} | ||
return { | ||
|
||
'combinedStringEnum': !exists(json, 'combined-string-enum') ? undefined : StringEnum | StringEnumTwoFromJSON(json['combined-string-enum']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still incorrectly generated
} | ||
return { | ||
|
||
'combined-string-enum': StringEnum | StringEnumTwoToJSON(value.combinedStringEnum), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still incorrectly generated
if ((json === undefined) || (json === null)) { | ||
return json; | ||
} | ||
return StringEnumFromJSONTyped(json, true) || StringEnumTwoFromJSONTyped(json, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously this came out as a object spread of the wrong type {...FromJSONTyped(json, true), ...FromJSONTyped(json, true) }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
""
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
if (value === null) { | ||
return null; | ||
} | ||
return StringEnumToJSON(value as StringEnum) || StringEnumTwoToJSON(value as StringEnumTwo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't cross my mind, our docs only use string enums. I test it out tomorrow and see!
This reverts commit 4b40ca2.
Can you take a look at my commit 4b40ca2 I got the following error
|
❤️ 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! |
if ((json === undefined) || (json === null)) { | ||
return json; | ||
} | ||
if(!Object.values({{classname}}).includes(json as {{classname}})) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these runtime checks allowed?
Values of What I meant was trying |
They 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 ( |
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 |
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. |
or
not concat
or
not concat
when handling enums
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 |
@@ -1,10 +1,16 @@ | |||
{{>modelEnumInterfaces}} | |||
|
|||
export function {{classname}}FromJSON(json: any): {{classname}} { | |||
export function {{classname}}FromJSON(json: any): {{classname}} | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typescript gave a me a warning about the implicit return null. I believe it can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's because this PR changed the code inside the function, narrowing down json
type to null
before returning it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, we trust the api response matches the spec. otherwise the code is gonna break anyway, since we don't do full shape verification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The challenge is older clients being used with newer backends which just weren't aware of the existence of such values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
if (value === null) { | ||
return null; | ||
} | ||
return PatternObjectToJSON(value as PatternObject) ?? PatternObjectTwoToJSON(value as PatternObjectTwo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
🤔 Hmm
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
Should continue down this rabbit hole? Or is this too aggressive of a change? |
Probably the most correct way to address your case is to handle primitive values differently from objects.
So object models will keep deserializing using an object spread ( |
Thanks for all you help. |
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
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.
master
,5.3.x
,6.0.x
@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)