[typescript] fix: nullable enums should not serialize a null value to a string
#19540
Conversation
| @@ -3947,7 +3947,9 @@ public CodegenProperty fromProperty(String name, Schema p, boolean required, boo | |||
| List<Object> _enum = p.getEnum(); | |||
| property._enum = new ArrayList<>(); | |||
| for (Object i : _enum) { | |||
| property._enum.add(String.valueOf(i)); | |||
| if(i != null) { | |||
There was a problem hiding this comment.
I believe this can be rolled back, as isValueNull is sufficient.
The current model before this change is:
"_enum" : [ "affinity-data", "dealroom", null ],
"allowableValues" : {
"enumVars" : [ {
"name" : "AffinityData",
"isString" : false,
"value" : "'affinity-data'"
}, {
"name" : "Dealroom",
"isString" : false,
"value" : "'dealroom'"
}, {
"name" : "Null",
"isString" : false,
"value" : "'null'"
} ],
"values" : [ "affinity-data", "dealroom", null ]
},
and after:
"_enum" : [ "affinity-data", "dealroom" ],
"allowableValues" : {
"enumVars" : [ {
"name" : "AffinityData",
"isString" : false,
"isNullValue": false,
"value" : "'affinity-data'"
}, {
"name" : "Dealroom",
"isString" : false,
"isNullValue": false,
"value" : "'dealroom'"
}, {
"name" : "Null",
"isString" : false,
"isNullValue": true,
"value" : "'null'"
} ],
"values" : [ "affinity-data", "dealroom", null ]
},
you can see there is an oddity in allowableValues, where the value is already serialized to 'null', however, #19027 didn't change this I believe, as it only touched modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptClientCodegen.java not the DefaultCodegen.
The 'null' value is set here:
maybe the surrounding loop:
needs the guard as well and we just remove the value from the possible enum values altogether? WDYT?
There was a problem hiding this comment.
@wing328 whats the core guidance here?
There was a problem hiding this comment.
I am just trying to guard the second part:
diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
index f27e4a400e4..9215daee503 100644
--- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
+++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
@@ -6630,6 +6630,9 @@ public class DefaultCodegen implements CodegenConfig {
: 0;
for (Object value : values) {
+ if(value == null) {
+ continue;
+ }
Map<String, Object> enumVar = new HashMap<>();
String enumName = truncateIdx == 0
? String.valueOf(value)and see how it will affect the samples. My guess is that it might be the better solution, as we don't introduce another template variable, keep the model more sane and it will fix all future generators as well.
There was a problem hiding this comment.
yes that would be nice, and it would work for all generators without modifying the templates.
There was a problem hiding this comment.
Okay, I will make the change later.
Just OOI: is there a better way to crate fixtures for a very specific part of the generation? It feels like quite the overkill to generate a whole scaffold with 10+ files just to assert one tiny part of it.
For the model change in this PR I could write a unit test, but generally, is there a way to create a partial scaffold in any way?
There was a problem hiding this comment.
just pushed 01209d5 with the changes; seems cleaner (have a look at the diff)?
There was a problem hiding this comment.
Just OOI: is there a better way to crate fixtures for a very specific part of the generation? It feels like quite the overkill to generate a whole scaffold with 10+ files just to assert one tiny part of it.
For the model change in this PR I could write a unit test, but generally, is there a way to create a partial scaffold in any way?
yeah unit tests are the more compact/preferred option for these specific aspects
There was a problem hiding this comment.
yeah unit tests are the more compact/preferred option for these specific aspects
I can change this PR to use unit tests if/when @wing328 approves and we merge it.
| @@ -384,10 +384,12 @@ public void enumArrayModelTest() { | |||
| fish.put("name", "Fish"); | |||
There was a problem hiding this comment.
this test seems to be identical to modules/openapi-generator/src/test/java/org/openapitools/codegen/php/PhpModelTest.java - it seems maybe these files / tests were duplicated at some point. Might be worth moving this up onto a higher level and remove the duplication?
| enum: | ||
| - "affinity-data" | ||
| - "dealroom" | ||
| - null |
There was a problem hiding this comment.
this is the line of the fixture that shows the issue
samples/openapi3/client/petstore/typescript/builds/nullable-enum/models/Response.ts
Outdated
Show resolved
Hide resolved
| /** | ||
| * The source of the data in this Field (if it is enriched) | ||
| */ | ||
| 'enrichmentSource'?: ResponseEnrichmentSourceEnum | null; |
There was a problem hiding this comment.
nullability (e.g. the possible value null) is declared here already.
|
|
||
| export enum ResponseEnrichmentSourceEnum { | ||
| AffinityData = 'affinity-data', | ||
| Dealroom = 'dealroom' |
There was a problem hiding this comment.
Before the change in this PR, there is an additional line
Null = 'null'
joscha
changed the title
nullable enums should not serialize a null value to a stringnullable enums should not serialize a null value to a string
|
@macjohnny @wing328 is there any more changes needed for this one to land, please? |
|
we just need approval from the core team |
@wing328 could I get your 👀 on this one, please? |
modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
Show resolved
Hide resolved
…en/DefaultCodegen.java Co-authored-by: Joscha Feth <[email protected]>
|
🎉 all green @wing328 |
| @@ -0,0 +1,4 @@ | |||
| generatorName: typescript | |||
| outputDir: samples/openapi3/client/petstore/typescript/builds/nullable-enum | |||
There was a problem hiding this comment.
for new TS sample folder, do we need to add it to the Circle CI script to ensure the output compiles without issues?
https://github.com/OpenAPITools/openapi-generator/blob/master/CI/circle_parallel.sh#L76
There was a problem hiding this comment.
I guess, yes, would it make sense to determine this:
openapi-generator/CI/circle_parallel.sh
Lines 75 to 104 in 5732e27
automatically?
There was a problem hiding this comment.
added bbe6d34 for now
There was a problem hiding this comment.
not against automating anything
maybe we should take this opportunity to move some tests (which don't require a local petstore server) to github workflows instead
cc @macjohnny
nullable enums with a value of
null(e.g.['a', 'b', null]) currently receive an additional (incorrect) valueNull = 'null'.This issue was introduced in #19027
Sample is here: #19027 (comment)
PR checklist
master(upcoming 7.6.0 minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)