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

[typescript-axios] Implement useSingleRequestParameter option #6288

Conversation

codeserk
Copy link
Contributor

This is a follow-up story that completes this PR proposed before. This PR adds a new parameter that has default value to false to maintain retro-compatibility.

Problem

The current implementation expands all the parameters in the API methods:

        /**
         * 
         * @summary Logs user into the system
         * @param {string} username The user name for login
         * @param {string} password The password for login in clear text
         * @param {*} [options] Override http request option.
         * @throws {RequiredError}
         */
        loginUser(username: string, password: string, options?: any): AxiosPromise<string> {

This is especially problematic when adding more fields to existing endpoints, since the order of the parameters might change. This leads to a breaking changes in the generated client, even if the API introduced a non-breaking change (a new optional parameter).

Proposal

Adding a new parameter useSingleRequestParameter to combine all the request parameters in a single object. This is not a new solution, it has been implemented in other generators and there is already a proposed solution using this mechanism. This new option is disabled by default, to keep compatibility with previous versions

Interfaces for the requests

/**
 * Request parameters for loginUser operation in UserApi.
 * @export
 * @interface UserApiLoginUserRequest
 */
export interface UserApiLoginUserRequest {
    /**
     * The user name for login
     * @type {string}
     * @memberof UserApiLoginUser
     */
    readonly username: string

    /**
     * The password for login in clear text
     * @type {string}
     * @memberof UserApiLoginUser
     */
    readonly password: string
}

Changes in the methods to use the request interfaces

    /**
     * 
     * @summary Logs user into the system
     * @param {UserApiLoginUserRequest} requestParameters Request parameters.
     * @param {*} [options] Override http request option.
     * @throws {RequiredError}
     * @memberof UserApi
     */
    public loginUser(requestParameters: UserApiLoginUserRequest, options?: any) {
        return UserApiFp(this.configuration).loginUser(requestParameters.username, requestParameters.password, options).then((request) => request(this.axios, this.basePath));
    }

Review

I've split the changes in different commits to ease the review process. The last commit contains the highest number of changes: I've regenerated all the samples.

@macjohnny I believe you were the reviewer of the previous PR and asked for some changes in the previous proposal. Please let me know if there's something missing (I'm pretty new with mustache and this is my first contribution to this project so I might have messed up something :D)

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @nicokoenig @topce @akehir @petejohansonxo @amakhrov

- New options `useSingleRequestParameter` to use a single param for the request, instead of one param per request param.
Include new parameter `useSingleRequestParameter`.
Default value = `false` to keep compatibility with previous versions.
Also included script in the script that runs all
- Previous samples have a minor change (one line is deleted)
- New sample generated with the new parameter set to true
@auto-labeler
Copy link

auto-labeler bot commented May 13, 2020

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

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
thanks a lot for your PR. Please add the windows scripts and we should be good to go.

@@ -4,5 +4,6 @@
./bin/typescript-axios-petstore-with-npm-version.sh
./bin/typescript-axios-petstore-with-npm-version-and-separate-models-and-api.sh
./bin/typescript-axios-petstore-with-complex-headers.sh
./bin/typescript-axios-petstore-with-single-request-parameters.sh
Copy link
Member

Choose a reason for hiding this comment

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

please also add a windows version to the corresponding bin/windows/typescript-axios-petstore-all.bat

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.

somehow the changes to the allowed parameters are still missing:
see e.g. https://github.com/OpenAPITools/openapi-generator/pull/4479/files#diff-0c001a8f6a2f4306967815c6f6979681R62

@codeserk
Copy link
Contributor Author

@macjohnny I've pushed two commits for your comments,

I'm not 100% sure about the changes about the allowed parameters though:
Looks like there's no equivalent for the changes you pointed out (that seem to be related to a test they have in angular client). But I saw some changes that were missing in our end.
I think the approach for that client is a bit different and that we don't need to create getters/settings, nor set the value explicitly; only adding the new command to the CLI options.

I guess adding those lines only affects the message shown when using the CLI, right? Since the new variable was being read in mustache before them :D

@macjohnny
Copy link
Member

@codeserk please update the doc file according to the ci:
https://circleci.com/gh/OpenAPITools/openapi-generator/15600#tests/containers/2

@macjohnny macjohnny added this to the 5.0.0 milestone May 15, 2020
@macjohnny macjohnny merged commit 311ca78 into OpenAPITools:master May 15, 2020
@codeserk
Copy link
Contributor Author

@macjohnny Thanks for the quick review, and for helping maintaining this awesome repository :)
I have one question: When will these changes be available? I've seen it's added to 5.0.0 milestone, but I can't see how to use that using docker, for instance.

Would it make sense if I made another PR but for the current latest version 4.3.1 I guess?

jimschubert added a commit that referenced this pull request May 16, 2020
* master:
  Update username (arun-nalla) (#6319)
  [typescript-axios] Implement useSingleRequestParameter option (#6288)
  [typescript] Remove "v4-compat" value of enumSuffix (#6308)
  Mark swift4 generator as deprecated (#6311)
  Remove @nickmeinhold from Dart technical committee (#6309)
  Migrate Erlang samples to use OAS 3 spec (#6297)
  update dart samples
  Improve parameter documentation (#6092)
  Minor improvements to `plantuml` doc generator (#6298)
  undo changes to petstore.yaml oas3.0 (#6299)
  Allow passing progress callbacks through client methods. (#6261)
  Create method to json (#6111)
@macjohnny
Copy link
Member

@codeserk it will be released with version 5.0.0, due in june. If you want to use it now, simply use the 5.0.0-SNAPSHOT version

@codeserk codeserk deleted the feature/5549-added-named-params-to-typescript-axios branch May 16, 2020 21:47
@standayweb
Copy link

Did you manage to start using it? If so how? @codeserk

Maybe you can elaborate @macjohnny would be much appreciated.

I am unable to get it working, currently using a script in package.json like this
"generate": "OPENAPI_GENERATOR_VERSION=5.0.0-SNAPSHOT openapi-generator generate -i orchd/oas3-api.yaml -g typescript-axios -o client --additional-properties supportsES6=true,npmName=orchd,useSingleRequestParameter=true",

@codeserk
Copy link
Contributor Author

codeserk commented May 19, 2020

@standayweb
Hello!

I was unable to make it work with npm or with the current docker images, I had to build a new image with the latest code, not sure if there's any easier way to do it :D

  • Checkout the repository, latest master
  • Generate the new image:
docker build -f ./Dockerfile -t <name of your image> .
  • Use your image
docker run --rm -v ${PWD}:/local <name of your image> generate -i <location for you schema> -g typescript-axios -o /local/src/generated --additional-properties=useSingleRequestParameter=true

Of course, replace <name of your image> and <location for your schema> :)

As said, not sure if this is the best solution, but I couldn't find version 5 in the docker registry

@macjohnny
Copy link
Member

you can download the snapshot version here:
https://oss.sonatype.org/content/repositories/snapshots/org/openapitools/openapi-generator-cli/5.0.0-SNAPSHOT/

@darrenjennings
Copy link

thanks for your work @codeserk!

@benwiley4000
Copy link

benwiley4000 commented May 25, 2020

For those who were like me and still confused by the instructions for using the 5.0.0-SNASHOT version with Docker, here's what I did:

  1. The Dockerfile relies on a single jar file so you can replace it. I scrolled to the bottom of the page @macjohnny provided and grabbed the link to the .jar file posted this morning.
  2. I created a new Dockerfile that looks like this:
    FROM openapitools/openapi-generator-cli:v4.3.0
    ARG CLI_RELEASE_DIR=5.0.0-SNAPSHOT
    ARG CLI_VERSION=5.0.0-20200524.091815-150
    
    RUN apk add --no-cache curl
    RUN curl \
      https://oss.sonatype.org/content/repositories/snapshots/org/openapitools/openapi-generator-cli/$CLI_RELEASE_DIR/openapi-generator-cli-$CLI_VERSION.jar \
      > /opt/openapi-generator/modules/openapi-generator-cli/target/openapi-generator-cli.jar
    RUN apk del --purge curl
    
    ENTRYPOINT ["docker-entrypoint.sh"]
    CMD ["help"]
  3. My build script looks like this:
    docker build . -t open_api_image_local
    docker run -v "$pwd":/app open_api_image_local generate \
    	-i /app/swagger.json \
    	-c /app/openapi.yml \
    	-o /app \
    	-g typescript-axios \
    	--additional-properties useSingleRequestParameter=true

However... the option doesn't seem to be doing anything. All my output params are still listed as separate arguments.

Am I missing something? @codeserk

Thanks in advance! I hope I can get this to work, because I was really considering ditching openapi-generator for typescript before I found that this option had been added. 🙂

EDIT: oops I was wrong, it all works! #6288 (comment)

@wing328
Copy link
Member

wing328 commented May 25, 2020

@benwiley4000 Please try

docker pull openapitools/openapi-generator-cli:latest
docker run --rm -v ${PWD}:/local openapitools/openapi-generator-cli generate \
    -i https://raw.githubusercontent.com/openapitools/openapi-generator/master/modules/openapi-generator/src/test/resources/2_0/petstore.yaml \
    -g go \
    -o /local/out/go

then verify the .openapi-generator/VERSION in the output to ensure it's 5.0.0-SNAPSHOT

@benwiley4000
Copy link

@wing328 I'm sorry, I actually re-verified my output and it was correct. I was switching over from a different generator and didn't realize there would be the private internal function with ordered parameters as well as the public version with the single request parameter. Thanks and sorry for the confusion! 🙂

@wing328
Copy link
Member

wing328 commented May 25, 2020

@benwiley4000 no problem at all.

@mushan0x0
Copy link

After openapi.json is updated, the InlineObject number has changed, causing all interfaces to be modified😢.

@macjohnny
Copy link
Member

@mushan0x0 can you please open a new issue and give a short example?

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.

None yet

7 participants