exegesis-js / exegesis

Tools for implementing server-side OpenAPI 3.0.0
MIT License
139 stars 36 forks source link

Array type query parameters are not being parsed correctly. #60

Closed pvince closed 5 years ago

pvince commented 5 years ago

I have a path (Please excuse any formatting errors, I am typing this up in the text box):

  '/items':
    get:
      x-exegesis-controller: get_Items
      description: Gets items
      operationId: getItems
      parameters:
      - name: fields
        in: query
        schema:
          type: array
          items:
            type: string
            enum:
              - field1
              - field2
              - field3
              - field4
      responses:
        '200':
          description: Success

I am expecting to be able to run the following query: GET /api/v0/items?field1,field2

However when I run that I get the following error:

{
    "message": "Validation errors",
    "errors": [
        {
            "message": "should be equal to one of the allowed values",
            "location": {
                "in": "query",
                "name": "fields",
                "docPath": "/paths/items/get/parameters/1/schema",
                "path": "/0"
            }
        }
    ]
}

I did some digging, and I think this issue is tied to this line: https://github.com/exegesis-js/exegesis/blob/d66784ff9e8c33d7ac99d3a5322549fe8410acdd/src/oas3/parameterParsers/structuredParser.ts#L79

I was able to hack around the problem by changing that line from return [decodeURIComponent(value)]; to return decodeURIComponent(value).split(',');

jwalton commented 5 years ago

I think if you set explode to false it will do what you're expecting. Should it do what you're expecting here anyways? That's something the OpenAPI spec is very hazy on - there's lots of details about how to serialize parameters in RFC 6570, but not so many on how to deserialize them.

In this case, explode defaults to true, so a client should serialize this request as:

GET /api/v0/items?fields=field1&fields=field2

So technically your client is at fault. :P If the client sent:

GET /api/v0/items?fields=field1,field2&fields=field3

(Ignoring for the fact that in this case it's an enum) I would definitely want to deserialize this as ['field1,field2', 'filed3'] - it seems likely the comma is supposed to be there (even thought really the client should have serialized it as a %2C, but then why is the client sending this as half exploded?). But then, in this case:

GET /api/v0/items?fields=field1,field2

It's more ambiguous. If explode were false, it would CLEARLY be an array of values, but with explode true...

That's the logic behind the decisions I made. I'm not entirely opposed to changing it, though. :)

pvince commented 5 years ago

Setting explode to false worked. I was basing my functionality off the swagger-connect project using a Swagger 2.0 based specification. It looks like maybe in OpenAPI 3.0 they either clarified or changed the default behavior?

Reading the spec a bit more carefully, it looks like the current implemented functionality matches the spec around serialization of parameters. Specifically looking at the Style Examples

The default style for query parameters is form. Further, when style is set to form, explode defaults to true.

Looking at that table, when style: form and explode: true the expected array serialization is color=blue&color=black&color=brown.

On a side note, I am experimenting with both upgrading our specification from Swagger 2.0 to OpenAPI 3.0 along with switching from swagger-node to exegesis-express. This projects extensive & accurate documentation is an incredible breath of fresh air after working with swagger-node for almost 2 years. Thanks for pointing me towards the explode setting.

jwalton commented 5 years ago

No problem. And glad to hear you're having a good time with Exegesis and the docs. This project started when I was writing a series of blog posts about how to get around some of the weird and undocumented parts in swagger-node, and then I started reading about OpenAPI 3.x and realized it would solve a few problems I had, so I decided to just roll my own library. I tried to focus pretty heavily on writing good documentation, exactly because node-swagger is... not as well documented as I'd like. Glad to hear it's paying off. :)

If you find anything confusing in the docs, please raise an issue!