swagger-api / swagger-js

Javascript library to connect to swagger-enabled APIs via browser or nodejs
http://swagger.io
Apache License 2.0
2.6k stars 754 forks source link

[3.27.4 Regression] Multiple path parameters are not all applied #3508

Closed kevinoid closed 2 months ago

kevinoid commented 2 months ago

Q&A (please complete the following information)

Content & configuration

Swagger/OpenAPI definition:

OpenAPI 3.0 example (saved as openapi.yaml) ```yaml openapi: 3.0.1 info: title: Example OpenAPI 3.0 spec with multiple path parameters version: 1.0.0 servers: - url: https://example.com/ paths: /{owner}/{pet}/age: get: description: Get the age of a given pet for a given owner. operationId: getAge parameters: - name: owner in: path required: true schema: type: string - name: pet in: path required: true schema: type: string responses: "200": description: "Age of the pet, in years" content: application/json: schema: type: integer ```
Swagger 2.0 example (saved as swagger.yaml) ```yaml swagger: "2.0" info: version: 1.0.0 title: Example Swagger 2.0 spec with multiple path parameters host: example.com produces: - application/json consumes: - application/json paths: /{owner}/{pet}/age: parameters: - name: owner in: path type: string required: true - name: pet in: path type: string required: true get: description: Get the age of a given pet for a given owner. responses: "200": description: Age of the pet, in years schema: type: integer ```

Describe the bug you're encountering

When an operation contains multiple path parameters, SwaggerClient makes a request to a URL where only one of the path parameters is applied and the other parameter names remain unchanged from the URL template in the request URL.

To reproduce...

Run the following code

import SwaggerClient from 'swagger-client';

function testFetch(url) {
  console.log(`fetch('${url}')`);
  process.exit(url === 'https://example.com/joe/fido/age' ? 0 : 1);
}

const client = await new SwaggerClient({
  spec: {
    swagger: '2.0',
    info: {
      version: '1.0.0',
      title: 'Example Swagger 2.0 spec with multiple path parameters',
    },
    schemes: ['https'],
    host: 'example.com',
    produces: ['application/json'],
    consumes: ['application/json'],
    paths: {
      '/{owner}/{pet}/age': {
        parameters: [
          {
            name: 'owner',
            in: 'path',
            type: 'string',
            required: true,
          },
          {
            name: 'pet',
            in: 'path',
            type: 'string',
            required: true,
          },
        ],
        get: {
          description: 'Get the age of a given pet for a given owner.',
          operationId: 'getAge',
          responses: {
            200: {
              description: 'Age of the pet, in years',
              schema: {
                type: 'integer',
              },
            },
          },
        },
      },
    },
  },
  userFetch: testFetch,
});

await client.apis.default.getAge({
  owner: 'joe',
  pet: 'fido',
});

Expected behavior

The script produces:

fetch('https://example.com/joe/fido/age')

This occurs with swagger-client 3.27.3 and earlier.

Actual behavior

The script produces:

fetch('https://example.com/joe/{pet}/age')

This occurs with swagger-client 3.27.4 and later.

Additional context or thoughts

It appears that this was broken by #3504

swati410 commented 2 months ago

When path has more than one placeholder like tasks/v1/lists/{tasklist}/tasks/{task}/move after replacing first instance swagger-client@3.27.4 is not replacing the other instance.

Code bug here : pathBuilder is called in loop here with the same pathName tasks/v1/lists/{tasklist}/tasks/{task}/move for each iteration https://github.com/swagger-api/swagger-js/blob/7be663681f7da2db7f20beba6a4eec108efee14d/src/execute/index.js#L215

So after 1st iteration req.url changes to http://tasks/v1/lists/123/tasks/{task}/move here it is unable to find `tasks/v1/lists/{tasklist}/tasks/{task}/move in req.url req.url = req.url.replace(pathName, resolvedPathname); https://github.com/swagger-api/swagger-js/blob/7be663681f7da2db7f20beba6a4eec108efee14d/src/execute/oas3/parameter-builders.js#L38

swati410 commented 2 months ago

@char0n have a look at this PR https://github.com/swagger-api/swagger-js/pull/3510

char0n commented 2 months ago

Hi @swati410,

Thanks for the detailed description of the issue. We're looking at possible vector of remediation + your PR.

char0n commented 2 months ago

Option 1

@glowcloud we can return back to using new URL to deconstruct the req.url and use decodeURIComponent for the pathname component. In pathname context, bot new URL and decodeURIComponent are compatible. The downside of this is that we are decoding and encoding already resolved pathname components repeatedly because of https://github.com/swagger-api/swagger-js/blob/7be663681f7da2db7f20beba6a4eec108efee14d/src/execute/index.js#L215'

Option 2

We will fake path template for openapi-path-templating library

Absolute URLs will be prepended by /. The only side effect is that when contextURL contains {variables} they will be resolved as well.

/https://example.com/1.0.0/execute/oas3/parameter-builders.js?query={param}

Option 3

We will pass baseURL to parameter builds and use it to compute current path template for resolution from req.url. Then we will concatenate the baseURL with resolved path template.


As discussed, we decided to go for Option 3 as it the cleanest and most explicit one.

char0n commented 2 months ago

Addressed in https://github.com/swagger-api/swagger-js/pull/3511