[Feature][Angular] request param enum as literal unions #7366

macjohnny · 2018-01-11T14:07:31Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Allows request params of type enum to be represented as enum in the generated angular client code.

Before:

public findPetsByStatus(status: Array<string>, observe?: 'body', reportProgress?: boolean): Observable<Array<Pet>>;

after:

public findPetsByStatus(status: Array<'available' | 'pending' | 'sold'>, observe?: 'body', reportProgress?: boolean): Observable<Array<Pet>>;

fixes #7365

macjohnny · 2018-01-11T14:07:55Z

cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @wing328 @bedag-moo

macjohnny · 2018-01-11T14:08:20Z

@wing328 could you include this PR in 2.3.1 please?

macjohnny · 2018-01-11T14:39:42Z

the code of this PR could also be moved to AbstractTypeScriptClientCodegen to allow using this feature with any TypeScript client. what do you think?

macjohnny · 2018-01-11T15:59:48Z

cc @JohannesHoppe

kenisteward · 2018-01-11T16:38:43Z

@macjohnny i can agree with you there. I also approve of this change.

macjohnny · 2018-01-12T07:00:13Z

@kenisteward would you mind reviewing the code?

macjohnny · 2018-01-16T11:33:37Z

would anyone of the technical comitee mind please reviewing the code?

cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @wing328 @bedag-moo

bedag-moo · 2018-01-16T15:05:38Z

Nice improvement :-)

Might this be beneficial for all typescript generators? If so, the super class AbstractTypeScriptClientCodegen might be a better home for this code.

macjohnny · 2018-01-16T15:12:41Z

@bedag-moo this feature could easily be made available to all typescript generators by moving the code to AbstractTypeScriptClientCodegen as you suggested. However, I don't know whether this would be accepted. We could also first merge it as it is and in a second step discuss about moving it to the abstract typescript generator.

bedag-moo · 2018-01-16T15:59:51Z

I think we should be able to find out whether it would be accepted, since their authors are all in the technical committee :-)

Personally, I am happy either way, I just thought now we be a good time to decide where we put this code.

TiFu · 2018-01-16T17:08:11Z

Wasn't this already implemented in #6233?

Also note, that there was quite a long discussion about literal unions in this PR after it was merged.

kenisteward · 2018-01-16T17:48:19Z

@TiFu this is different.

#6233 was the typescript code representation of what the enum will be in model code.

This PR sets those would be values in the service code instead of things like String so we have strong types for calling service names.

Note: in the descirption it shows this

public findPetsByStatus(status: Array<string>, observe?: 'body', reportProgress?: boolean): Observable<Array<Pet>>;

becomes

public findPetsByStatus(status: Array<'available' | 'pending' | 'sold'>, observe?: 'body', reportProgress?: boolean): Observable<Array<Pet>>;

so now

service.findPetsByStatus('someString');

would throw a compiler error which helps the developer know the server won't accept that value.

I think all the clients would benefit from this. My only thing about the code was I believe enum values can't be anything but numbers or string so the check for Date/DateTime shouldn't be needed.

TiFu

@kenisteward ah totally overlooked that. I thought that was already added in the other PR.

PR looks good to me. Just one code style improvement and one question due to the version bump in the samples.

Moving this to all generators would be a good idea.

TiFu · 2018-01-16T17:54:09Z

...agger-codegen/src/main/java/io/swagger/codegen/languages/TypeScriptAngularClientCodegen.java

+     */
+    private String numericEnumValuesToEnumTypeUnion(List<Number> values) {
+        List<String> stringValues = new ArrayList<>();
+        for (Object value: values) {


Code Style: Change Object to Number? This would make it clearer that it's used for numbers only.

TiFu · 2018-01-16T17:57:02Z

samples/client/petstore/typescript-angular-v2/default/.swagger-codegen/VERSION

@@ -1 +1 @@
-2.3.0
+2.3.1-SNAPSHOT


Is this because someone forgot to update the samples? Are you sure that your branch is based on current master?

This should be for 2.3.1 now shouldn't it @TiFu . I thought at some point I talked to @wing328 about a patch release but I can't remember exactly.

@kenisteward @TiFu the branch is based on the master of some 5 days ago, but I will rebase and generate the code again.

kenisteward

After doing more in depth review, I think I can confidently say this shouldn't negatively affect the clients if this is put into the base AbstractTypescript class. I would just ask that at least one more on the technical committee confirm with me that moving this up one level won't break other clients.

@Vrolijkx @TiFu @sebastianhaas @taxpon

kenisteward · 2018-01-16T18:01:18Z

samples/client/petstore/typescript-angular-v2/default/.swagger-codegen/VERSION

@@ -1 +1 @@
-2.3.0
+2.3.1-SNAPSHOT


This should be for 2.3.1 now shouldn't it @TiFu . I thought at some point I talked to @wing328 about a patch release but I can't remember exactly.

macjohnny · 2018-01-17T08:08:10Z

@kenisteward concerning the allowed types of enums:
I tested with the following modification of the petstore.yml

/pet/findByStatus:
    get:
      tags:
        - pet
      summary: Finds Pets by status
      description: Multiple status values can be provided with comma separated strings
      operationId: findPetsByStatus
      produces:
        - application/xml
        - application/json
      parameters:
        - name: status
          in: query
          description: Status values that need to be considered for filter
          required: true
          type: array
          items:
            type: string
            enum:
              - available
              - pending
              - sold
            default: available
          collectionFormat: csv
        - name: status2
          in: query
          description: Status values that need to be considered for filter
          required: true
          type: array
          items:
            type: integer
            format: int32
            enum:
              - 3
              - 4
              - 5
          collectionFormat: csv
        - name: status3
          in: query
          description: Status values that need to be considered for filter
          required: true
          type: string
          enum:
            - available
            - pending
            - sold
        - name: status4
          in: query
          description: Status values that need to be considered for filter
          required: true
          type: string
          format: date
          enum:
            - 2012-02-04
            - 2012-02-05
            - 2014-02-04
        - name: status5
          in: query
          description: Status values that need to be considered for filter
          required: true
          type: number
          format: float
          enum:
            - 5.4
            - 6.5
            - 7.9

      responses:
        '200':
          description: successful operation
          schema:
            type: array
            items:
              $ref: '#/definitions/Pet'
        '400':
          description: Invalid status value
      security:
        - petstore_auth:
            - 'write:pets'
            - 'read:pets'

It seems that float, date, etc. are valid enum types, too.
Looking at their corresponding property classes in https://github.com/swagger-api/swagger-core/tree/master/modules/swagger-models/src/main/java/io/swagger/models/properties this seems to work as intended.
Also, my code is based on the checks here:
https://github.com/swagger-api/swagger-codegen/blob/v2.3.0/modules/swagger-codegen/src/main/java/io/swagger/codegen/DefaultCodegen.java#L1612

So far I found one limitation of the enum implementation: When using

          items:
            type: integer
            enum:
              - 3
              - 4
              - 5

without specifying e.g. format: int32, the resulting type is a string without the enum values, since it will be an AbstractNumericProperty which does not define enum values.

macjohnny · 2018-01-17T15:56:37Z

there is an issue when generating models with

definitions:
  EnumArrays:
    type: object
    properties:
      just_symbol:
        type: string
        enum:
          - ">="
          - "$"
      array_enum:
        type: array
        items:
          type: string
          enum:
            - fish
            - crab

which results in

export interface EnumArrays {
    justSymbol?: EnumArrays.JustSymbolEnum;
    arrayEnum?: Array<'fish' | 'crab'>;
}
export namespace EnumArrays {
    export type JustSymbolEnum = '>=' | '$';
    export const JustSymbolEnum = {
        GreaterThanOrEqualTo: '>=' as JustSymbolEnum,
        Dollar: '$' as JustSymbolEnum
    }
    export type ArrayEnumEnum = 'fish' | 'crab';
    export const ArrayEnumEnum = {
        Fish: 'fish' as ArrayEnumEnum,
        Crab: 'crab' as ArrayEnumEnum
    }
}

The problem is that

    arrayEnum?: Array<'fish' | 'crab'>;

should be

    arrayEnum?: Array<ArrayEnumEnum>;

in order to use the generated enum types instead of directly typing it as a literal union of the allowed values.

Somehow this needs to be fixed by allowing to distinguish between model properties and API client request parameters. Anyone's got an idea?

macjohnny · 2018-01-17T16:05:56Z

@kenisteward @Vrolijkx @TiFu @sebastianhaas @taxpon
I prepared a separate branch where I moved the code to the AbstractTypeScriptClientCodegen
https://github.com/macjohnny/swagger-codegen/tree/feature/7365-typescript-request-param-enum
I needed to disable two equality assertions in the unit tests of the TypeScriptFetchModelTest, because they failed due to the issue with model enums being replaced with literal unions as described in #7366 (comment)

macjohnny · 2018-01-17T16:13:48Z

@SamuelBeliveau do you have an idea for the issue described here #7366 (comment) ?

macjohnny · 2018-01-17T18:00:50Z

here is my proposal to resolve the issue:
we allow for a custom implementation of the generation of the datatype of the parameter here:
04d5e1d#diff-0c1b6d04d7c2c41f87f7c710a5610d88R2511
and can then implement it in the TypeScriptAngularCodegen or AbstractTypeScriptCodegen here:
04d5e1d#diff-ad60e100e050d5ef52943803a2adc5fdR175

 String parameterDataType = this.getParameterDataType(param, property);
 if (parameterDataType != null) {
      p.dataType = parameterDataType;
 } else {
    p.dataType = cp.datatype;
 }

I also updated the https://github.com/macjohnny/swagger-codegen/tree/feature/7365-typescript-request-param-enum branch, which should now also work correctly, allowing to have this feature for all typescript clients.

what do you guys think about it?

… is an enum

…on()

macjohnny · 2018-01-18T06:31:33Z

I filed a new PR #7433 for the general implementation of this feature for all TypeScript clients

macjohnny · 2018-01-20T07:59:21Z

@wing328 this PR can be closed if #7433 is merged, enabling this feature for all typescript clients.

macjohnny · 2018-01-23T06:46:53Z

closing this one in favor of #7433

macjohnny changed the title ~~Feature/7365 angular request param enum~~ [Feature][Angular] request param enum as literal unions Jan 11, 2018

svenhuber approved these changes Jan 16, 2018

View reviewed changes

TiFu reviewed Jan 16, 2018

View reviewed changes

kenisteward reviewed Jan 16, 2018

View reviewed changes

macjohnny added 7 commits January 17, 2018 19:21

#7365: use enums string / number literals for request parameter if it…

efe97f2

… is an enum

#7365: use enums string / number literals for request parameter if it…

ed0cece

… is an enum

#7365: use enums string / number literals for request parameter if it…

3f8435f

… is an enum

#7365: improve docs

94edd39

#7365: change type Object to Number in numericEnumValuesToEnumTypeUni…

0ac41b2

…on()

#7365: allow custom data type for parameters

04d5e1d

#7365: generate angular samples after rebasing onto master

478f5e3

macjohnny mentioned this pull request Jan 18, 2018

[Feature][TypeScript] request param enum as literal unions #7433

Merged

4 tasks

wing328 added Client: TypeScript Enhancement: General labels Jan 20, 2018

wing328 added this to the v2.4.0 milestone Jan 20, 2018

Merge branch 'master' into feature/7365-angular-request-param-enum

941dbed

macjohnny closed this Jan 23, 2018

		@@ -1 +1 @@
		2.3.0 No newline at end of file
		2.3.1-SNAPSHOT No newline at end of file

[Feature][Angular] request param enum as literal unions #7366

[Feature][Angular] request param enum as literal unions #7366

Uh oh!

Conversation

macjohnny commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Description of the PR

Uh oh!

macjohnny commented Jan 11, 2018

Uh oh!

macjohnny commented Jan 11, 2018

Uh oh!

macjohnny commented Jan 11, 2018

Uh oh!

macjohnny commented Jan 11, 2018

Uh oh!

kenisteward commented Jan 11, 2018

Uh oh!

macjohnny commented Jan 12, 2018

Uh oh!

macjohnny commented Jan 16, 2018

Uh oh!

bedag-moo commented Jan 16, 2018

Uh oh!

macjohnny commented Jan 16, 2018

Uh oh!

bedag-moo commented Jan 16, 2018

Uh oh!

TiFu commented Jan 16, 2018

Uh oh!

kenisteward commented Jan 16, 2018

Uh oh!

TiFu left a comment • 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!

TiFu Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

macjohnny Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

TiFu Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

kenisteward Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

macjohnny Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

kenisteward left a comment

Choose a reason for hiding this comment

Uh oh!

kenisteward Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

macjohnny commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

macjohnny commented Jan 17, 2018

Uh oh!

macjohnny commented Jan 17, 2018

Uh oh!

macjohnny commented Jan 17, 2018

Uh oh!

macjohnny commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

macjohnny commented Jan 18, 2018

Uh oh!

macjohnny commented Jan 20, 2018

Uh oh!

macjohnny commented Jan 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

macjohnny commented Jan 11, 2018 •

edited

Loading

TiFu left a comment •

edited

Loading

macjohnny commented Jan 17, 2018 •

edited

Loading

macjohnny commented Jan 17, 2018 •

edited

Loading