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

Revert supports for delete body in typescript-angular client #10976

Merged
merged 4 commits into from
Jan 31, 2022

Conversation

cghislai
Copy link
Contributor

@cghislai cghislai commented Nov 27, 2021

  • Updates angular version in generated typescript-angular 12 client. (first commit.It produces an uncompilable client)
  • Revert the commit introducing the support for delete body in the client. (second commit).

Fixes #10975

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.3.0), 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.

@cghislai
Copy link
Contributor Author

@topce see also discussion in the issue

@cghislai cghislai changed the title Angular delete body Revert supports for delete body in typescript-angular client Nov 27, 2021
Copy link
Contributor

@amakhrov amakhrov left a comment

Choose a reason for hiding this comment

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

Instead of reverting - can we actually fix the generated code?
Perhaps it can be as simple as using lower level httpClient.request, instead of trying to adjust to inconsistent http client interface (I realize that inconsistency has a very good reason, though - the angular team didn't want to break all existing apps by making a backword incompatible change in .delete method :) )

@cghislai
Copy link
Contributor Author

cghislai commented Dec 3, 2021

We can. But I think we first need the spec to account for this (i couldn't generate any client when specifying a delete body because it produced an incorrect spec).
Once it is supported, I think we need to alter client to account for different spec versions and generate the body param for the delete call when using an openapi spec >=1.3 and angular >=12.2.
As it stands now, this is broken dead code.

@maxs-rose
Copy link

Any updates on when this will be fixed?

@maestrocoder
Copy link

hit same issue, was working okay on 5.3.0 - looking for how to revert

Angular CLI: 12.1.4
Node: 14.18.1
Package Manager: npm 6.14.15
OS: win32 x64

have exact issue as - #10864

so ran this command to go back to previous @openapitools/openapi-generator-cli from 2.4.23 to 2.4.13

npm uninstall @openapitools/openapi-generator-cli
npm i --save-dev @openapitools/[email protected]

then re-run the code gen

openapi-generator-cli generate -g typescript-angular -i swagger.json -o output/typescript-angular

but it still uses version in .openapi/generator/VERSION = 5.3.1

looking to just download the jar file and run the java without @openapitools/openapi-generator-cli as a work-around?

so i made this - hope it helps while the patch/spec/issues are resolved.

#!/bin/bash
rm -rf output
mkdir output
FILE="`pwd`/openapi-generator-cli-5.3.0.jar"
if [ -f "$FILE" ]; then
    echo "$FILE exists."
else
  echo "openapi-generator-cli-5.3.0.jar file not found, downloading..."
  curl "https://repo1.maven.org/maven2/org/openapitools/openapi-generator-cli/5.3.0/openapi-generator-cli-5.3.0.jar" --output "$FILE"
fi

java -jar "`pwd`/openapi-generator-cli-5.3.0.jar" generate -g typescript-angular -i swagger.json -o output/typescript-angular

Copy link
Contributor

@amakhrov amakhrov left a comment

Choose a reason for hiding this comment

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

As it stands now, this is broken dead code.

Agree.
Sounds like reverting it is the minimal-effort approach to make the generator code compile again.
And after that we can consider properly supporting DELETE body (if someone is up to filing a corresponding PR).

@cghislai looks like there are some merge conflicts in this branch - would you be able to resolve those?

@falk-stefan
Copy link

I'm facing the same issue with 5.3.x and 5.2.x. Do we know that the latest working version is?

@iangabrielsanchez
Copy link

I'm facing the same issue with 5.3.x and 5.2.x. Do we know that the latest working version is?

5.1.0 seems to be working properly

@cghislai
Copy link
Contributor Author

@amakhrov rebased, conflict resolved.

Copy link
Contributor

@amakhrov amakhrov left a comment

Choose a reason for hiding this comment

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

Thanks!

@EE1234EE
Copy link

EE1234EE commented Jan 30, 2022

Hello guys,

I'm not quite sure about the "requestBody" problem. My specification:

#YAML
 /server/{id}:
    parameters:
      - in: path
        name: id
        required: true
        schema:
          type: integer
          format: int64
    delete:
      tags:
        - Server
      summary: Remove server from check list
      responses:
          '200':
            description: Server successfully removed

and I'm still getting: error TS2554: Expected 1-2 arguments, but got 3.

@amakhrov
Copy link
Contributor

@macjohnny can we merge this?

@macjohnny macjohnny merged commit 95c996f into OpenAPITools:master Jan 31, 2022
@cghislai cghislai deleted the angular-delete-body branch January 31, 2022 17:25
@gong4soft
Copy link

Hello guys,
this ticket is not included in 5.4.0 release, is a 5.4.1 in planning?

thanks.

@macjohnny
Copy link
Member

@gong4soft looks like the next upcoming release is 6.0.0:
https://github.com/OpenAPITools/openapi-generator/milestones

But zou can already use it by installing the 6.0.0-SNAPSHOT version

@wing328 wing328 added this to the 6.0.0 milestone Feb 28, 2022
@mgol
Copy link

mgol commented Mar 11, 2022

@macjohnny is there a way to use it when you use the @openapitools/openapi-generator-cli?

@macjohnny
Copy link
Member

@mgol I don‘t think the npm package supports snapshot versions, but it would be a great contribution if you implemented that

@A77AY
Copy link

A77AY commented Mar 31, 2022

@mgol You can set downloadUrl in the openapitools.json:

{
  "$schema": "node_modules/@openapitools/openapi-generator-cli/config.schema.json",
  "spaces": 2,
  "generator-cli": {
    "version": "6.0.0-20220330.230127-109",
    "repository": {
      "downloadUrl": "https://oss.sonatype.org/content/repositories/snapshots/org/openapitools/openapi-generator-cli/6.0.0-SNAPSHOT/openapi-generator-cli-${versionName}.jar"
    }
  }
}

@mgol
Copy link

mgol commented Apr 6, 2022

@KrickRay Thanks, that's very useful! Although, currently, all that's enough to resolve this issue is to set version to 6.0.0-beta and no downloadUrl needs to be set.

@angelaki
Copy link
Contributor

angelaki commented Sep 5, 2022

I'm thinking about filing a PR for this issue since I need to generate POST methods with a payload. For not making the same mistakes again / reinventing the wheel: is someone already about to fix this? And what exactly was wrong with this previous PR?

Why not always rely on the request method (instead of post / put / patch etc.)? This way the generated code would be less dependent on Angulars interfaces I guess?

@macjohnny
Copy link
Member

@angelaki I am not aware of anybody actively working on this, but #10976 (review) sums it up.

@angelaki
Copy link
Contributor

angelaki commented Sep 5, 2022

@macjohnny so there's probably nothing wrong with relying totally on HttpClient.request? So this fix would be pretty easy ...

@maxs-rose
Copy link

Would using HttpClient.request possibly ignore any http interceptors, not entirely sure where they get applied.

@angelaki
Copy link
Contributor

angelaki commented Sep 5, 2022

@maxs-rose Nope, actually all the HttpClient methods are just overloads for the request-method, e.g. https://github.com/angular/angular/blob/main/packages/common/http/src/client.ts#L2565. Is someone knowing the OpenAPICodeGen code able & willing to quickly fix this? I've never contributed to this project and imho it seams pretty easy if you just know what & where to do.

@maxs-rose
Copy link

Interesting.

I believe you would want to end it the templates it uses to generate the angular code

https://github.com/OpenAPITools/openapi-generator/tree/master/modules%2Fopenapi-generator%2Fsrc%2Fmain%2Fresources%2Ftypescript-angular

I know I have made a few edits to one of them, a long time ago, then you can tell the generator to use your edited templates. Unfortunately I can't remember how you actually tell the generator to use the custom templates.

@angelaki
Copy link
Contributor

angelaki commented Sep 5, 2022

@maxs-rose wow, this looks pretty cryptic to me ... I was hoping that I'd just need to replace some post / put / ... by the corresponding request-method. I haven't event found the HttpClient calls...

@maxs-rose
Copy link

Yea it's a bit wild, thankfully when I did have to make a change for work it was something obvious. I believe the HttpClient calls are in the api.service.mustache

return this.httpClient.{{httpMethod}}{{^isResponseFile}}<{{#returnType}}{{{returnType}}}{{#isResponseTypeFile}}|undefined{{/isResponseTypeFile}}{{/returnType}}{{^returnType}}any{{/returnType}}>{{/isResponseFile}}(`${this.configuration.basePath}${localVarPath}`,{{#isBodyAllowed}}

@angelaki
Copy link
Contributor

angelaki commented Sep 6, 2022

Oh wow. Now I know why noone is willing / able to make this small fix. I still don't get why request wasn't used from the beginning. All the other methods are just overloads to make it more handy. Never the less it seams like one has to do it. Can you tell me where the variables come from? And is there an easy way to test my changes? Thank you!

@maxs-rose
Copy link

Its probably better to move this into its own issue/discussion

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

Successfully merging this pull request may close these issues.

[BUG] Broken delete request body in typescript-angular generator