fix(execute): fix encoding of complex nested type values in OpenAPI 3.x request builders

glowcloud commented 3 months ago

Refs https://github.com/swagger-api/swagger-ui/issues/9755

From the test example, when encoding these parameters:

query: {
  a: {
    b: {
      c: 'd',
    },
  },
},
pathPartial: {
  foo: {
    bar: { baz: 'qux' },
  },
  a: {
    b: {
      c: 'd',
    },
  },
}

we will now get the following URL:

/foo=%7B%22bar%22%3A%7B%22baz%22%3A%22qux%22%7D%7D,a=%7B%22b%22%3A%7B%22c%22%3A%22d%22%7D%7D?a=%7B%22b%22%3A%7B%22c%22%3A%22d%22%7D%7D

which will decode to:

/foo={"bar":{"baz":"qux"}},a={"b":{"c":"d"}}?a={"b":{"c":"d"}}

The cURL result for the 2.0 specification is:

curl -X 'GET' \
'https://petstore.swagger.io/v2/pet/findByStatus/%7B%22test%22%3A%20%7B%22test2%22%3A%20%22test3%22%20%7D%20%7D?queryParam=%7B%22test%22%3A%20%7B%22test2%22%3A%20%22test3%22%20%7D%20%7D' \
  -H 'accept: application/xml' \
  -H 'headerParam: {"test": {"test2": "test3" } }'

and the URL decodes to:

https://petstore.swagger.io/v2/pet/findByStatus/{"test": {"test2": "test3" } }?queryParam={"test": {"test2": "test3" } }

so it seems to me like it doesn't need any changes.

char0n commented 3 months ago

One additional question: how is it possible that it worked for Parameter Objects with content field, but didn't work for ones with schema field? Do we know?

This test guarantees that it works for Parameter Objects with content field: https://github.com/swagger-api/swagger-js/blob/e943f35aa94745f9b7e89bec5a029a89e1789951/test/oas3/execute/main.js#L666

glowcloud commented 3 months ago

One additional question: how is it possible that it worked for Parameter Objects with content field, but didn't work for ones with schema field? Do we know?

This test guarantees that it works for Parameter Objects with content field:

https://github.com/swagger-api/swagger-js/blob/e943f35aa94745f9b7e89bec5a029a89e1789951/test/oas3/execute/main.js#L666

In this case, we use the serialize function https://github.com/swagger-api/swagger-js/blob/12f249aee1a372bae9de0d69c00b50712835ae61/src/execute/oas3/parameter-builders.js#L31-L33 which stringifies the value https://github.com/swagger-api/swagger-js/blob/12f249aee1a372bae9de0d69c00b50712835ae61/src/execute/oas3/content-serializer.js#L6-L16

For schema we just use stylize https://github.com/swagger-api/swagger-js/blob/12f249aee1a372bae9de0d69c00b50712835ae61/src/execute/oas3/style-serializer.js#L46-L56

In case of query we go into https://github.com/swagger-api/swagger-js/blob/12f249aee1a372bae9de0d69c00b50712835ae61/src/http/index.js#L424 which then goes through: https://github.com/swagger-api/swagger-js/blob/12f249aee1a372bae9de0d69c00b50712835ae61/src/http/index.js#L400 and https://github.com/swagger-api/swagger-js/blob/12f249aee1a372bae9de0d69c00b50712835ae61/src/http/index.js#L223 and finally into: https://github.com/swagger-api/swagger-js/blob/12f249aee1a372bae9de0d69c00b50712835ae61/src/http/index.js#L310 where it uses encodeDisallowedCharacters https://github.com/swagger-api/swagger-js/blob/12f249aee1a372bae9de0d69c00b50712835ae61/src/http/index.js#L322

glowcloud commented 3 months ago

With our current findings, we decided not to apply the styling properties from OpenAPI specification to the nested objects. They will be processed by using JSON.stringify function.

We should do that before using encodeDisallowedCharacters for objects and arrays in https://github.com/swagger-api/swagger-js/blob/12f249aee1a372bae9de0d69c00b50712835ae61/src/execute/oas3/style-serializer.js#L59-L62 and accordingly in the other valueEncoder functions, and in https://github.com/swagger-api/swagger-js/blob/12f249aee1a372bae9de0d69c00b50712835ae61/src/http/index.js#L322

encodeDisallowedCharacters should also be refactored (in a separate issue/PR) so that it actually only encodes given strings, instead of having additional logic like this: https://github.com/swagger-api/swagger-js/blob/12f249aee1a372bae9de0d69c00b50712835ae61/src/execute/oas3/style-serializer.js#L6-L8

We should handle these cases in the valueEncoder functions and encodeDisallowedCharacters should only be getting strings.

It also looks like the parse parameter is not set anywhere when using this function, so this logic might be removed with the refactor: https://github.com/swagger-api/swagger-js/blob/12f249aee1a372bae9de0d69c00b50712835ae61/src/execute/oas3/style-serializer.js#L17-L19

There's also the issue of using the escape parameter where sometimes it's a string and sometimes a boolean - we need to research its usage and whether it should stay in its current form.

glowcloud commented 3 months ago

For the parameters in the new test:

arrayOfObjects: [
  {
    a: {
      b: 'c',
    },
  },
  {
    d: {
      e: 'f',
    },
  },
],
arrayOfArrays: [
  [
    {
      a: {
        b: 'c',
      },
    },
  ],
]

we will get the following URL:

/?arrayOfObjects=%7B%22a%22%3A%7B%22b%22%3A%22c%22%7D%7D,%7B%22d%22%3A%7B%22e%22%3A%22f%22%7D%7D&arrayOfArrays=%5B%7B%22a%22%3A%7B%22b%22%3A%22c%22%7D%7D%5D

which should decode to:

/?arrayOfObjects={"a":{"b":"c"}},{"d":{"e":"f"}}&arrayOfArrays=[{"a":{"b":"c"}}]

glowcloud commented 3 months ago

More on deepObject style for query parameters:

For this spec:

{
  openapi: '3.0.1',
  info: {
    title: 'Test',
    version: 'v0',
  },
  servers: [
    {
      url: 'http://localhost:8080',
      description: 'Test',
    },
  ],
  paths: {
    '/': {
      post: {
        operationId: 'params',
        parameters: [
          {
            name: 'queryParam',
            in: 'query',
            schema: {
              type: 'object',
            },
            style: 'deepObject',
            explode: true,
          },
        ],
      },
    },
  },
}

with these parameters:

{
  queryParam: {
    first: {
      firstKey: 'firstValue',
    },
    second: {
      secondKey: 'secondValue',
    },
    third: {
      thirdKey: 'thirdValue',
    },
  },
}

With the current changes, the parameters will be encoded as:

http://localhost:8080/?queryParam%5Bfirst%5D=%7B%22firstKey%22%3A%22firstValue%22%7D&queryParam%5Bsecond%5D=%7B%22secondKey%22%3A%22secondValue%22%7D&queryParam%5Bthird%5D=%7B%22thirdKey%22%3A%22thirdValue%22%7D

which should decode to:

http://localhost:8080/?queryParam[first]={"firstKey":"firstValue"}&queryParam[second]={"secondKey":"secondValue"}&queryParam[third]={"thirdKey":"thirdValue"}

so only the first nested key/value pair would be in the deepObject style.

Previously we would have this url:

http://localhost:8080/?queryParam%5Bfirst%5D[firstKey]=firstValue&queryParam%5Bsecond%5D[secondKey]=secondValue&queryParam%5Bthird%5D[thirdKey]=thirdValue

which would be decoded to:

http://localhost:8080/?queryParam[first][firstKey]=firstValue&queryParam[second][secondKey]=secondValue&queryParam[third][thirdKey]=thirdValue

so we had the style but not the encoding for nested values.

I did further research and the URL for nested object parameters if we didn't use deepObject style would be:

http://localhost:8080/?first[firstKey]=firstValue&second[secondKey]=secondValue&third[thirdKey]=thirdValue

In case of an array of parameters:

http://localhost:8080/?queryParam[firstKey]=firstValue&queryParam[secondKey]=secondValue&queryParam[thirdKey]=thirdValue

so there wasn't an issue with getting [object Object] in query but we didn't do any encoding and we forced the deepObject style.

As it turns out, it's because we're using the qs library here https://github.com/swagger-api/swagger-js/blob/8128a0903f9cb397fa1581bd6a6ef6c176abb9cb/src/http/index.js#L419 which has support for the deepObject style:

it can even do its own encoding, which we set to false.

From what I'm seeing, this is all because there's no specified way of treating complex parameters in the OpenAPI specification. In the Swagger OpenAPI guide, we're recommending to use content instead of schema:

It looks like we would have to decide on the behaviour for these complex parameters when we get them with schema. In my opinion we can get rid of these [object Object] cases as well as not encoding everything in query by stringifying nested objects like in the PR. We would not support any styling / serialization options for them, as the behaviour is undefined in that case.

Also, for the deepObject style, I'm not sure why we have this https://github.com/swagger-api/swagger-js/blob/a560dd3b7862703b9d2ac1b5875ff8e451e40e1a/src/execute/oas3/style-serializer.js#L180-L182 as it looks to me like this option should only apply to query:

Query is encoded separately and directly uses encodeDisallowedCharacters, skipping these functions: https://github.com/swagger-api/swagger-js/blob/a560dd3b7862703b9d2ac1b5875ff8e451e40e1a/src/http/index.js#L323-L332

Additionally, I'm wondering if we could perhaps support the nested parameters in case of deepObject by using the encoding option from qs.

swagger-bot commented 3 months ago

:tada: This PR is included in version 3.26.6 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

swagger-api / swagger-js

fix(execute): fix encoding of complex nested type values in OpenAPI 3.x request builders #3446