[typescript][axios] Handle boolean enum correctly #9025

haedaal · 2021-03-20T10:05:02Z

with following schema

    "schemas": {
      "BooleanEnum": {
        "enum": ["true", "false"],
        "type": "boolean"
      },

https://gist.github.com/haedaal/a771d6f3df337b624ae8a2c2d176c0db

typescript-fetch currently generates

export enum BooleanEnum {
    True = 'true',
    False = 'false'
}

with this pr

export type BooleanEnum = true | false

by using 'type' rather than 'enum' we breaks consistency, but TS18033 does not allow this

export enum BooleanEnum {
    True = true, // Only numeric enums can have computed members, but this expression has type 'true' ...
    False = false
}

this pr has 2 changes.

by adding isBoolean, generate model correctly
fix typescript-axios modelEnum.mustache

accidentally typescript-angular is fixed without template changes, but many others need their own fixes on template.

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.1.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.

cc @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)

can't use enum because of TS18033

auto-labeler · 2021-03-20T10:05:07Z

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

tusharchutani

This looks good

macjohnny · 2021-04-06T05:39:47Z

What is the use case to have an enum with two boolean values?

haedaal · 2021-04-07T02:27:07Z

I think it's main use case is boolean literal type, not an enum with two boolean value (though I took the example above)

type FooResponse = FooSucessResponse | FooFailureResponse
type FooSuccessResponse = {
  success: true,
  body: {
    foo: string
  }
}
type FooFailureResponse = {
  success: false,
  body: {
    reason: string
  }
}

this case needs boolean enum with single boolean value.
by handling this case correctly, typescript can infer body type if value of success is given

haedaal · 2021-05-06T23:39:01Z

@tusharchutani any plan to merge it..?

amakhrov · 2021-05-07T00:34:54Z

Is it even a valid or reasonable schema in the first place?
I know it satisfies the validator - but the following one also does.

type: boolean
enum: [true, false, fuzzy]

So it tells us more about strictness of the validator, rather than correctness of the spec.

type: boolean already unambiguously indicates that the only two possible values are true and false (but not "true" or "false"!)

In this case we just need to decide whether we give priority to type attribute or enum attribute. In the former case, the generated code should not emit an additional type at all - just use boolean for the model property. In the latter case a string enum, as it's generated today, sounds correct.

kenisteward · 2021-05-07T00:36:36Z

This is true the original schema is not valid. If you want to use the enum properly it should not be 'true' but just true

…

On Thu, May 6, 2021, 7:35 PM Alexey Makhrov ***@***.***> wrote: Is it even a valid or reasonable schema in the first place? I know it satisfies the validator - but the following one also does. type: boolean enum: [true, false, fuzzy] So it tells us more about strictness of the validator, rather than correctness of the spec. type: boolean already unambiguously indicates that the only two possible values are true and false (but not "true" or "false"!) In this case we just need to decide whether we give priority to type attribute or enum attribute. In the former case, the generated code should not emit an additional type at all - just use boolean for the model property. In the latter case a string enum, as it's generated today, sounds correct. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9025 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADB4XNJKG6HFNHAPAMVVBPTTMMYT7ANCNFSM4ZQIZCGA> .

kenisteward · 2021-05-07T00:38:02Z

Technically the base validation is invalid due to the fact that the items 'true' and 'false' are not actually Boolean anyway.

…

On Thu, May 6, 2021, 7:36 PM Kenneth Steward ***@***.***> wrote: This is true the original schema is not valid. If you want to use the enum properly it should not be 'true' but just true On Thu, May 6, 2021, 7:35 PM Alexey Makhrov ***@***.***> wrote: > Is it even a valid or reasonable schema in the first place? > I know it satisfies the validator - but the following one also does. > > type: boolean > enum: [true, false, fuzzy] > > So it tells us more about strictness of the validator, rather than > correctness of the spec. > > type: boolean already unambiguously indicates that the only two possible > values are true and false (but not "true" or "false"!) > > In this case we just need to decide whether we give priority to type > attribute or enum attribute. In the former case, the generated code > should not emit an additional type at all - just use boolean for the > model property. In the latter case a string enum, as it's generated today, > sounds correct. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#9025 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ADB4XNJKG6HFNHAPAMVVBPTTMMYT7ANCNFSM4ZQIZCGA> > . >

haedaal · 2021-05-07T00:55:38Z

as a result of assigning string value to the value of boolean enum,
following pattern is required for now

if (response.success === 'true') { }

even though you've specified the type {success: boolean}

haedaal · 2021-06-07T17:31:12Z

@amakhrov does it make sense? I'm wondering what's blocking point.

amakhrov · 2021-06-07T17:42:56Z

@haedaal I still struggle to understand the use case, honestly. Why define a boolean value in the spec as an enum? Why are the enum values are defined as strings in the spec - even though you clearly want them to be boolean in the generated code.

haedaal · 2021-06-08T16:50:37Z

@haedaal I still struggle to understand the use case, honestly. Why define a boolean value in the spec as an enum? Why are the enum values are defined as strings in the spec - even though you clearly want them to be boolean in the generated code.

@amakhrov

Why define a boolean value in the spec as an enum?

type GetNextOrderResponse = {success: true, data: Order} | {success: false, data?: unknown} // or any other FailReason type for data in fail case

Why are the enum values are defined as strings in the spec - even though you clearly want them to be boolean in the generated code.

that's the whole point of this pr. now codegen is generating string enum value rather than boolean enum value even though I clearly specify them as a boolean! this pr is a bugfix (if not a hotfix)

I've made it an issue in #9024. I've also put a sample schema that generates wrong result.

haedaal · 2021-06-08T17:06:28Z

This is true the original schema is not valid. If you want to use the enum properly it should not be 'true' but just true
…

that's exactly what I thought is a problem, but if you try it, making schema like this

    "schemas": {
      "BooleanEnum": {
        "enum": [true, false],
        "type": "boolean"
      },
    }

does not fix the wrong result.

it's because we check only for number type and unconditionally wrap it's value with double quote as you can see here.
And this pr add "boolean" type to the cases where you have to use value as it is.

openapi-generator/modules/swagger-codegen/src/main/java/io/swagger/codegen/DefaultCodegen.java

Lines 249 to 255 in 11deb43

    
           public String toEnumValue(String value, String datatype) { 
        
               if ("number".equalsIgnoreCase(datatype)) { 
        
                   return value; 
        
               } else { 
        
                   return "\"" + escapeText(value) + "\""; 
        
               } 
        
           }

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

Lines 719 to 723 in 5d58afe

    
           if ("number".equalsIgnoreCase(datatype) || "boolean".equalsIgnoreCase(datatype)) { 
        
               return value; 
        
           } else { 
        
               return "\"" + escapeText(value) + "\""; 
        
           }

amakhrov · 2021-06-08T18:22:05Z

Ok, I think now I see what you're trying to do (sorry, it took me a while :) ).

You don't actually need BooleanEnum = true | false (since it's redundant). You want smth like TrueBooleanEnum = true and FalseBooleanEnum = false - is that correct?

If so, I would rather try to avoid generating an enum in the first place. The ideal generated model would be exactly as in your example

{success: true, data: Order}

(no enum whatsoever)

However, I admit this would probably be a bigger change in the templates. So as a quick solution your PR makes total sense to me

haedaal · 2021-06-09T05:30:39Z

You don't actually need BooleanEnum = true | false (since it's redundant). You want smth like TrueBooleanEnum = true and FalseBooleanEnum = false - is that correct?

Yes! Exactly! :)

wing328 · 2021-06-18T08:30:38Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenModel.java

@@ -64,7 +64,7 @@
    public String defaultValue;
    public String arrayModelType;
    public boolean isAlias; // Is this effectively an alias of another simple type
-    public boolean isString, isInteger, isLong, isNumber, isNumeric, isFloat, isDouble, isDate, isDateTime, isShort, isUnboundedInteger;
+    public boolean isString, isInteger, isLong, isNumber, isNumeric, isFloat, isDouble, isDate, isDateTime, isShort, isUnboundedInteger, isBoolean;


The following function also needs to be updated as well:

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenModel.java

Line 764 in 5d58afe

public boolean equals(Object o) {

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenModel.java

Line 854 in 5d58afe

public int hashCode() {

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenModel.java

Line 872 in 5d58afe

public String toString() {

I'll update these in another PR after merging this one.

Filed #9802

wing328 · 2021-06-18T08:31:21Z

cc @OpenAPITools/generator-core-team as the change impacts the codegen model class.

Handle boolean enum correctly in typescript-axios

1d29f6f

can't use enum because of TS18033

auto-labeler bot added the Client: TypeScript label Mar 20, 2021

tusharchutani approved these changes Mar 26, 2021

View reviewed changes

Merge branch 'master' into fix-boolean-enum

5d58afe

amakhrov approved these changes Jun 8, 2021

View reviewed changes

macjohnny requested a review from jimschubert June 8, 2021 20:09

wing328 reviewed Jun 18, 2021

View reviewed changes

wing328 added Enhancement: General Feature: Generator labels Jun 18, 2021

wing328 added this to the 5.2.0 milestone Jun 18, 2021

wing328 merged commit 41afcd0 into OpenAPITools:master Jun 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[typescript][axios] Handle boolean enum correctly #9025

[typescript][axios] Handle boolean enum correctly #9025

haedaal commented Mar 20, 2021

auto-labeler bot commented Mar 20, 2021

tusharchutani left a comment

macjohnny commented Apr 6, 2021

haedaal commented Apr 7, 2021 •

edited

haedaal commented May 6, 2021

amakhrov commented May 7, 2021

kenisteward commented May 7, 2021 via email

kenisteward commented May 7, 2021 via email

haedaal commented May 7, 2021 •

edited

haedaal commented Jun 7, 2021

amakhrov commented Jun 7, 2021

haedaal commented Jun 8, 2021 •

edited

haedaal commented Jun 8, 2021

amakhrov commented Jun 8, 2021

haedaal commented Jun 9, 2021

wing328 Jun 18, 2021

wing328 Jun 18, 2021

wing328 commented Jun 18, 2021

[typescript][axios] Handle boolean enum correctly #9025

[typescript][axios] Handle boolean enum correctly #9025

Conversation

haedaal commented Mar 20, 2021

PR checklist

auto-labeler bot commented Mar 20, 2021

tusharchutani left a comment

Choose a reason for hiding this comment

macjohnny commented Apr 6, 2021

haedaal commented Apr 7, 2021 • edited

haedaal commented May 6, 2021

amakhrov commented May 7, 2021

kenisteward commented May 7, 2021 via email

kenisteward commented May 7, 2021 via email

haedaal commented May 7, 2021 • edited

haedaal commented Jun 7, 2021

amakhrov commented Jun 7, 2021

haedaal commented Jun 8, 2021 • edited

haedaal commented Jun 8, 2021

amakhrov commented Jun 8, 2021

haedaal commented Jun 9, 2021

wing328 Jun 18, 2021

Choose a reason for hiding this comment

wing328 Jun 18, 2021

Choose a reason for hiding this comment

wing328 commented Jun 18, 2021

haedaal commented Apr 7, 2021 •

edited

haedaal commented May 7, 2021 •

edited

haedaal commented Jun 8, 2021 •

edited