Skip to content
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

Fix typescript angular inheritance by enabling supportsMultipleInheritance flag #3812

Merged

Conversation

mnahkies
Copy link
Contributor

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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @nicokoenig (2018/09) @topce (2018/10) @akehir (2019/07)

Description of the PR

It appears that the typescript-angular template was re-written to expect the supportsMultipleInheritance flag to be enabled in 8c599eb but that the flag wasn't actually enabled.

By enabling this flag, parents are correctly added to interfaces as expected.

Also ran ./bin/openapi3/typescript-angular-petstore-all.sh before and after making changes. Seems this hadn't been run on master so there is some noise from that. Unfortunately the sample yaml used doesn't seem to exercise the allOf, anyOf, oneOf keywords and so no change occurred after the fix.

Fixes #2845

@mnahkies mnahkies mentioned this pull request Aug 31, 2019
5 tasks
Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@macjohnny
Copy link
Member

Please re-generate the samples with the
./bin/typescript-angular-petstore-all.sh
Command only

@macjohnny
Copy link
Member

Should the flag be added for typescript-fetch, too?

*/
public addPet(body: Pet, extraHttpRequestParams?: RequestOptionsArgs): Observable<{}> {
return this.addPetWithHttpInfo(body, extraHttpRequestParams)
public addPet(pet: Pet, extraHttpRequestParams?: RequestOptionsArgs): Observable<{}> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnahkies thanks for the PR. Looks like you're using an old version of OpenAPI Generator as the body name in OAS v2 is not preserved. Can you please pull the latest master into your branch and regenerate the samples again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, seems like this script actually generates to the same directory as the openapi2 script, is that intentional?

$ mvn clean package && ./bin/openapi3/typescript-angular-petstore-all.sh

Results in no changes (based on latest master)

Running

$ mvn clean package && ./bin/typescript-angular-petstore-all.sh

Reverts all changes to samples to current master. I'll push this, but seems like some work needs to be done around the typescript client samples with respect to openapi3

@wing328
Copy link
Member

wing328 commented Sep 3, 2019

Unfortunately the sample yaml used doesn't seem to exercise the allOf, anyOf, oneOf keywords and so no change occurred after the fix.

We've some test spec (OAS v3) for allOf, anyOf, oneOf under https://github.com/OpenAPITools/openapi-generator/tree/master/modules/openapi-generator/src/test/resources/3_0 and you're, right that the default petstore spec doesn't contain tests for different cases of allOf, anyOf, oneOf

…script angular client codegen

- note I reran ./bin/openapi3/typescript-angular-petstore-all.sh and no changes occurred.
  this suggests to me that the petstore.yaml sample should be improved to make use of the
  anyOf / allOf / oneOf keywords, in order to better show the effects of changes on generated code.
@mnahkies mnahkies force-pushed the bug-2845-typescript-angular-inheritance branch from 93a06f3 to 232f844 Compare September 3, 2019 12:50
@wing328
Copy link
Member

wing328 commented Sep 3, 2019

@mnahkies do you have a spec to reproduce the error? sorry if I miss it in your previous conversation. Just want to perform a bit more tests before merging this PR.

Thanks again for the PR.

@mnahkies
Copy link
Contributor Author

mnahkies commented Sep 3, 2019

@wing328 this is the spec I was testing with. Also I missed deleting some files that were generating when I ran the openapi3 script but shouldn't exist after running the openapi2 script.

openapi: "3.0.1"
info:
  version: 1.0.0
  title: Inheritance test
paths:
  /pets:
    patch:
      requestBody:
        content:
          application/json:
            schema:
              oneOf:
                - $ref: '#/components/schemas/Cat'
                - $ref: '#/components/schemas/Dog'
              discriminator:
                propertyName: pet_type
      responses:
        '200':
          description: Updated
components:
  schemas:
    Pet:
      type: object
      required:
        - pet_type
      properties:
        pet_type:
          type: string
      discriminator:
        propertyName: pet_type
    Dog:     # "Dog" is a value for the pet_type property (the discriminator value)
      allOf: # Combines the main `Pet` schema with `Dog`-specific properties 
        - $ref: '#/components/schemas/Pet'
        - type: object
          # all other properties specific to a `Dog`
          properties:
            bark:
              type: boolean
            breed:
              type: string
              enum: [Dingo, Husky, Retriever, Shepherd]
    Cat:     # "Cat" is a value for the pet_type property (the discriminator value)
      allOf: # Combines the main `Pet` schema with `Cat`-specific properties 
        - $ref: '#/components/schemas/Pet'
        - type: object
          # all other properties specific to a `Cat`
          properties:
            hunts:
              type: boolean
            age:
              type: integer

example.txt

@wing328
Copy link
Member

wing328 commented Sep 5, 2019

Did some tests and notice the following changes when compared with the master:

 import { Pet } from './pet';
 
-export interface Cat { 
+export interface Cat extends Pet { 
     hunts?: boolean;
     age?: number;
 }

Let's go with this fix and I'll merge it once the CircleCI tests pass.

@wing328 wing328 merged commit c63cf7e into OpenAPITools:master Sep 5, 2019
hokamoto pushed a commit to hokamoto/openapi-generator that referenced this pull request Sep 12, 2019
* issue OpenAPITools#2845: enable 'supportsMultipleInheritance' on typescript angular client codegen

- note I reran ./bin/openapi3/typescript-angular-petstore-all.sh and no changes occurred.
  this suggests to me that the petstore.yaml sample should be improved to make use of the
  anyOf / allOf / oneOf keywords, in order to better show the effects of changes on generated code.

* issue OpenAPITools#2845: run ./bin/openapi3/typescript-angular-petstore-all.sh

* run `mvn clean package && ./bin/typescript-angular-petstore-all.sh`

* revert extranous files
@wing328
Copy link
Member

wing328 commented Sep 12, 2019

@mnahkies thanks for the PR, which has been included in the v4.1.2 release: https://twitter.com/oas_generator/status/1172068944355528705

@wing328 wing328 changed the title Bug #2845 typescript angular inheritance Fix typescript angular inheritance by enabling supportsMultipleInheritance flag Sep 12, 2019
wing328 pushed a commit that referenced this pull request Sep 13, 2019
* First version of Nim Client

* Add some codes

* Add some codes

* Add some codes

* Add some codes

* Add some codes

* First version of Nim Client

* Add some codes

* Add some codes

* [Dart] Fix README template and update testing doco (#3809)

* [Dart] Fix README template and update testing doco

- deleted redundant shell script
- fixed and updated README template
- updated test package and moved to a dev_dependency
- removed old unused dev_dependency packages
- updated testing documentation in petstore sample

* Remove references to dart-flutter-petstore.sh

* Fix typos

* Fix typo

* Support custom git repository (#3757)

* add gitHost param to GeneratorSettings and related

* parameterize gitHost in READMEs

* parameterize gitHost in go.mod

* parameterize gitHost in git_push

* update petstore samples

* run ./bin/utils/export_docs_generators.sh

* run meta-codehen.sh

* Revert "run meta-codehen.sh"

This reverts commit d6d579f.

* Revert "run ./bin/utils/export_docs_generators.sh"

This reverts commit 1b81538.

* Revert "update petstore samples"

This reverts commit f513add.

* run ensure-up-to-date

* Add links to article and video (#3820)

* Better Go code format (#3819)

* better varible naming

* better comments

* better code format for go experimental client

* better comment, update samples

* Add some codes

* Add some codes

* Add some codes

* Add gRPC Protobuf schema generator (#3818)

* add grpc protobuf generator

* update doc

* add new doc

* add windows batch, comment out root proto

* 1792 fix remote spec handling and hash calculation (#3440)

* fixed bug where nullApi.java would be generated.  Instead, generated DefaultApi.java to match the default path /{pathParam} (#3821)

* Revert "1792 fix remote spec handling and hash calculation (#3440)"

This reverts commit 2a2eefe.

* Add  nickmeinhold to Dart technical committee (#3830)

* Bug #2845 typescript angular inheritance (#3812)

* issue #2845: enable 'supportsMultipleInheritance' on typescript angular client codegen

- note I reran ./bin/openapi3/typescript-angular-petstore-all.sh and no changes occurred.
  this suggests to me that the petstore.yaml sample should be improved to make use of the
  anyOf / allOf / oneOf keywords, in order to better show the effects of changes on generated code.

* issue #2845: run ./bin/openapi3/typescript-angular-petstore-all.sh

* run `mvn clean package && ./bin/typescript-angular-petstore-all.sh`

* revert extranous files

* fix warnings in csharp-netcore client (#3831)

* Add missing files to the form request (#3834)

* [client][go] avoid duplicated reflect imports (#3847)

* Following up for #3440 (1792 fix remote spec handling and hash calculation) (#3826)

* This patch fixes the bug that we cannot access to remote files when checking file updates.
Following up #3440, supporting auth.

* 1792 fix remote spec handling and hash calculation (#3440)

(cherry picked from commit 2a2eefe)

* fix detecting remote file / local file logic while finding the hash file, taking care of IllegalArgumentException for local files.

* add testcase

* Add a link (#3850)

* Add Element AI to the list (#3856)

* maven-plugin-plugin 3.6.0 (#3854)

*  [Java][okhttp-gson] fix failure to deserialize floats (#3846)

* fixed bug where nullApi.java would be generated.  Instead, generated DefaultApi.java to match the default path /{pathParam}

* fix to bug #3157

* update samples

* Adds Http Info To Dart Api (#3851)

* [C++][Pistache] Add missing setter for arrays (#3837)

* [C++][Pistache] Add missing setter for arrays

Fixes #3769

* [C++][Pistache] Update Petstore sample

* typescript-inversify: improve check for required parameters, support multiple media types (#3849)

* [typescript-inversify] Allow falsy parameters

A required parameter to an api method must not be `null` or `undefined`.
It can be any other falsy value, e.g. `""`, `0` or `false` though. This
change makes sure an error is only thrown in the former case and not in
the latter.

* [typescript-inversify] Handle multiple media types

The Accept and Content-Type HTTP headers can contain a list of media
types. Previously all but the first media type in the api definition
were ignored. Now the headers are properly generated.

* [typescript-inversify] Fix http client interface

The api service methods allow the `body` parameter to be optional. The
parameter is then passed to an `IHttpClient`. So it needs to be optional
there as well.
Also fixed the sample implementation `HttpClient`.

Fixes #3618.

* [typescript-inversify] Regenerate Petstore sample

* [typescript-inversify] Use more explicit null check

This does not change the semantic of the generated code, but makes it more explicit.

Co-Authored-By: Esteban Gehring <esteban.gehring@gmail.com>

* [typescript-angular] allow empty string basePath (#3489)

* [typescript-angular] Fixing #2731 - empty string basePath

* typescript-angular: refactor base path configuration

* typescript-angular: refactor base path configuration

* Fix/r/serialization fix and minor 3xx resp fix (#3817)

* fix(qlik): fix for minor serialization bug

* fix(r): add petsore generated classes

* fix(r): indendation fixes

* typescript-axios: Fix baseoptions (#3866)

* Fixed missing baseOptions of typescript-axios.

The typescript-axios template was missing the baseOptions setting when building an API Configuration. Set it.

* update sample.

* re-generate typescript axios samples

* Rename gRPC generator to "protobuf-schema" (#3864)

* rename grpc generator to protobuf-schema

* update doc

* Prepare v4.1.2 release (#3873)

* update samples

* update date

* fix version in readme

* BugFix #2053 Spring Boot fails to parse LocalDate query parameter (#3860)

Adds the format annotation so that Spring is able to serialize OpenApi date/date-time format into LocalDate/OffsetDateTime.

* update doc, samples (#3875)

* update stable release

* Update the batch for Windows

* Add a test snippet

* Update ensure-up-to-date

* Add Nim to README.md

* Ran ensure-up-to-date to pass CircleCI tests
@mnahkies mnahkies deleted the bug-2845-typescript-angular-inheritance branch September 13, 2019 12:19
@mnahkies
Copy link
Contributor Author

Thanks @wing328 - will this release also go to maven? https://mvnrepository.com/artifact/org.openapitools/openapi-generator-maven-plugin

@wing328
Copy link
Member

wing328 commented Sep 13, 2019

https://search.maven.org/artifact/org.openapitools/openapi-generator-maven-plugin/4.1.2/maven-plugin

The link you provided seems not up-to-date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Inheritance is broken
3 participants