OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
21.85k stars 6.59k forks source link

[BUG][TYPESCRIPT] ObservableAPI methods don't take care of `_options.middleware` property. #14549

Open marcomontalbano opened 1 year ago

marcomontalbano commented 1 year ago

Bug Report Checklist

Description

ObservableAPI generated methods don't take care of _options.middleware property.

openapi-generator version

v6.2.1

OpenAPI declaration file content or url

I'm able to reproduce the bug using the official petstore.json file available at: https://raw.githubusercontent.com/openapitools/openapi-generator/master/modules/openapi-generator/src/test/resources/3_0/petstore.json

Generation Details

I generated the code by running:

openapi-generator-cli generate -i https://raw.githubusercontent.com/openapitools/openapi-generator/master/modules/openapi-generator/src/test/resources/3_0/petstore.json -g typescript -o .
Steps to reproduce

I installed all dependencies and created a bug.ts file to test and show the issue.

npm install
npm install -D ts-node

I uploaded all sources to StackBitz, this is the link https://stackblitz.com/edit/node-4amp2b?file=README.md,bug.ts

You just need to run:

npx ts-node bug.ts

Inside the bug.ts file, I'm defining two middleware;

The first one is at the configuration level.

const configuration = createConfiguration({
  middleware: [
    {
      pre: (context) => {
        // set a Query Param from here (it works!)
        context.setQueryParam('fromConfiguration', 'got-it');

        console.log('###### URL:', context.getUrl());
        console.log('');

        return of(context);
      },
      post: (context) => of(context),
    },
  ],
});

The second one is at the getPetById level.

petApi
  .getPetById(1, {
    authMethods: configuration.authMethods,
    baseServer: configuration.baseServer,
    httpApi: configuration.httpApi,

    // middleware from here are not applied
    middleware: [
      {
        pre: (context) => {
          // set a Query Param from here (it doesn't work)
          context.setQueryParam('fromGetPet', 'so-sad');
          return of(context);
        },
        post: (context) => of(context),
      },
    ],
  })

My expectation is to see a console.log with:

URL: http://petstore.swagger.io/v2/pet/1?fromConfiguration=1&fromGetPet=1

but instead, I see:

URL: http://petstore.swagger.io/v2/pet/1?fromConfiguration=1

Suggest a fix

When methods are auto-generated, they are not taking care of the property _options.middleware but they rely only on this.configuration.middleware.

https://github.com/OpenAPITools/openapi-generator/blob/ac5134acf3db00171f8ee4689d52c404284a9128/modules/openapi-generator/src/main/resources/typescript/types/ObservableAPI.mustache#L67-L71

I think that we should concat these two arrays.

I'll open a PR as soon as possible with the proposed solution.

What do you think? Is there a different way to apply query parameters (usually for filtering data) when the OpenApi Schema doesn't provide an - in: query?

maxpain commented 8 months ago

Any updates on this?

davidgamero commented 2 months ago

I think that we should concat these two arrays.

this solution sounds good to me. likely appending the operation-level middleware after the api-level middleware would be in line with the expected use case (overriding/adding something to a request, but allowing api-level configurations to persist if not overridden)

the RequestFactory replaces the full Configuration if _options has one, which leans towards only applying the operation-level middleware if it exists. I suspect that isn't in line with user expectations based on the linked issues in this thread for patching headers.

An alternative approach: passing _options as either a Configuration or an array of Middleware.

In this scenario when passing a Configuration it would override the existing configuration fully, including _options.middleware which is how the api is intended to work now.

Adding a check for array-type check would let us indicate that the middleware is to be appended to the existing, api-level configuraiton. This would allow us to avoid the problem of having to define a full merge strategy for every part of a Configuration while covering this issue.

This would maintain fully backward compatibility with uses of the _options: Configuration param as it is now, while allowing users to easily do a one-off override of a header/query param which appears to be a missing ergonomic use case at the moment.