openapi-generator icon indicating copy to clipboard operation
openapi-generator copied to clipboard

fix: [typescript-angular] duplicate export of request params when `useSingleRequestParameter`

Open snebjorn opened this issue 3 years ago • 15 comments

Fixes #13200 see bug report for details about the issue

To validate the work run the OpenApi spec provided in the bug report. Remember to set useSingleRequestParameter as described in #13200. Then check that there are no duplicate export errors in /output-folder/api/api.ts

The fix was to prefix the RequestParameter interface with the classname which it looks like the option prefixParameterInterfaces controls. However the generator typescript-angular doesn't have a prefixParameterInterfaces option. It appears to be bad copy/paste.

PR checklist

  • [x] Read the contribution guidelines.
  • [x] 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.
  • [x] 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.
  • [x] File the PR against the correct branch: master (6.1.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
  • [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.

@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) @davidgamero (2022/03) @mkusaka (2022/04)

snebjorn avatar Aug 23 '22 20:08 snebjorn

I ran these with Git BASH (I'm on windows)

./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh

But it turned all / into \ and other issues. Seems there's an issue with the scripts on windows.

snebjorn avatar Aug 23 '22 20:08 snebjorn

@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) @davidgamero (2022/03) @mkusaka (2022/04)

Sorry to pester, but can I do something to help this getting reviewed/merged?

snebjorn avatar Sep 02 '22 07:09 snebjorn

@snebjorn thanks for your contribution

can you also add a config in https://github.com/OpenAPITools/openapi-generator/tree/master/bin/configs with the useSingleRequestParameter and some test run similar to https://github.com/OpenAPITools/openapi-generator/blob/2a8ea162d72fee839580bb75a08874fa26f92d5d/pom.xml#L1214? this would prevent future issues.

moreover, can you think of a backwards-compatible way of fixing this issue?

macjohnny avatar Sep 05 '22 11:09 macjohnny

Think I got a test added. Not sure of what to call the files or folders.

I'll have to get back to you on the non-backwards-compatible solution.

snebjorn avatar Sep 06 '22 16:09 snebjorn

@macjohnny while investigating a backwards compatible solution I found this piece of code that is supposed to ensure unique operationIds

https://github.com/OpenAPITools/openapi-generator/blob/6665d069830c2e71c12b447cfb26a7eeb70c9ce6/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L5338-L5346

however it only ensures unique operation ids within the same tag. According to the OpenAPI Spec 2.0+ the operationId must be unique among all operations described in the API.

operationId

Unique string used to identify the operation. The id MUST be unique among all operations described in the API. Tools and libraries MAY use the operationId to uniquely identify an operation, therefore, it is recommended to follow common programming naming conventions.

The duplicate export problem is caused by operationId not being unique in the generated CodegenOperation objects. https://github.com/OpenAPITools/openapi-generator/blob/6665d069830c2e71c12b447cfb26a7eeb70c9ce6/modules/openapi-generator/src/main/resources/typescript-angular/api.service.mustache#L32 However operationId is not duplicated in the source yaml specification.

I'm a bit out of my depth here. Is the correct fix to ensure all CodegenOperation objects have a unique operationId? If so where would be the appropriate place to do so? addOperationToGroup doesn't seem like it's the right place as it seems to be grouping operations.

snebjorn avatar Sep 07 '22 14:09 snebjorn

@snebjorn thanks for your analysis

@wing328 what is the general approach to ensure unique operation ids even if multiple tags are used?

macjohnny avatar Sep 07 '22 17:09 macjohnny

May I asked when will this bug be fixed ?

SarochaL avatar Sep 12 '22 08:09 SarochaL

We're waiting for feedback from @wing328. If there's a bug with operationId I guess a change can be labeled a bug fix and be released in the current v6.1. Otherwise it'll have to go into v7 as the current fix is a breaking change.

Furthermore... @macjohnny what's going on with the failing CircleCI Pipeline?

Could not find a usable config.yml, you may have revoked the CircleCI OAuth app.

I don't think I broke that.

snebjorn avatar Sep 15 '22 11:09 snebjorn

@wing328 ping

@snebjorn can you merge the most recent master? Maybe it will work again

macjohnny avatar Sep 15 '22 16:09 macjohnny

can you merge the most recent master? Maybe it will work again

@macjohnny still fails. do I need a CircleCI account? I seem to get this error on all my PRs.

snebjorn avatar Sep 16 '22 14:09 snebjorn

do I need a CircleCI account? I seem to get this error on all my PRs.

I don't know. @wing328 do you know?

macjohnny avatar Sep 16 '22 16:09 macjohnny

Found another PR with the same issue https://github.com/OpenAPITools/openapi-generator/pull/13381

snebjorn avatar Sep 16 '22 18:09 snebjorn

@macjohnny @wing328 found the solution to image

I had to sign in to Circle CI using my GitHub account. This isn't explained anywhere. It's unfortunate that an account on Circle CI have to be created for PR checks to run. Can't GitHub workflows run everything? It certainly would lower barrier to entry.

snebjorn avatar Sep 16 '22 19:09 snebjorn

@snebjorn Works for me without signing in image

Anyhow, looks to me like the issue is the following line in pom.xml added in thes PR:

<module>samples/client/others/typescript-angular/typescript-angular-v14-use-single-request-parameter</module>

as the error on CirceCi suggests that this module does not exist (see below). I would suggest to double check that the relevant XML is also added in this PR.

ERROR] The build could not read 1 project -> [Help 1]
[ERROR]   
[ERROR]   The project org.openapitools:openapi-generator-project:6.1.1-SNAPSHOT (/home/circleci/OpenAPITools/openapi-generator/pom.xml) has 1 error
[ERROR]     Child module /home/circleci/OpenAPITools/openapi-generator/samples/client/others/typescript-angular/typescript-angular-v14-use-single-request-parameter/pom.xml of /home/circleci/OpenAPITools/openapi-generator/pom.xml does not exist
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:

TiFu avatar Oct 15 '22 21:10 TiFu

@TiFu thanks for taking a look. However the failing test isn't the issue. I just haven't fixed it as I'm waiting for feedback on https://github.com/OpenAPITools/openapi-generator/pull/13262#issuecomment-1239494946 Once I have the needed info I can proceed to fix the code and tests accordingly 😄

snebjorn avatar Oct 15 '22 23:10 snebjorn

Is the correct fix to ensure all CodegenOperation objects have a unique operationId? If so where would be the appropriate place to do so? addOperationToGroup doesn't seem like it's the right place as it seems to be grouping operations.

can you please PM me via Slack on this when you've time?

https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g

wing328 avatar Oct 19 '22 02:10 wing328

Closed via https://github.com/OpenAPITools/openapi-generator/issues/13200#issuecomment-1684844228

wing328 avatar Aug 19 '23 06:08 wing328