swagger-api / swagger-js

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

Support deep objects for query parameters with deepObject style #1385

Open bajtos opened 6 years ago

bajtos commented 6 years ago

Content & configuration

Swagger/OpenAPI definition:

parameters:
  filterParam:
    in: query
    name: filter
    schema:
      type: object
    style: deepObject
    explode: true
    description: Options for filtering the results
    required: false

Swagger-Client usage:

 SwaggerClient({
   // no special configuration is needed
 })

Is your feature request related to a problem?

Many applications expect deeply nested objects in input parameters, see the discussion in swagger-ui starting from this comment: https://github.com/swagger-api/swagger-ui/issues/4064#issuecomment-357417513 In LoopBack, we are running into this problem too, see https://github.com/strongloop/loopback-next/pull/1679.

Consider the filter parameter described above (in:query style:deepObject explode:true).

Let's say the user provides the following value, for example by entering a JSON into the text area rendered by swagger-ui:

{
  "name": "ivan",
  "birth-date": {
    "gte": "1970-01-01"
  }
}

At the moment, swagger-js (and swagger-ui) silently ignores the value, produces an empty query and does not even let the user know that something went wrong.

Describe the solution you'd like

The following query string should be created by swagger-js client for the input value shown above.

filter[name]=ivan&filter[birth-date][qte]=1970-01-01

Describe alternatives you've considered

There are no alternatives as far as I can tell.

In the past, when Swagger 2.x did not support deepObject style, we were describing our filter argument as string and allowing clients to send the deep object in a JSON string. This was our custom extension that was of course not supported by swagger-js and swagger-ui, creating a suboptimal user experience.

Additional context

We are using swagger-ui-dist@3.17.0, which is the latest version published to npm. Unfortunately I am not able to tell which version of swagger-js is used by this version of swagger-ui-dist :(

The proposed serialization style is supported by https://www.npmjs.com/package/qs, which is used by http://expressjs.com as the default query parser, which means that a vast number of Node.js API servers are already expecting this serialization style.

I found the following two older comments that may be relevant:

https://github.com/swagger-api/swagger-js/pull/1140

Limitations: deepObject does not handle nested objects. The specification and swagger.io documentation does not provide an example for serializing deep objects. Flat objects will be serialized into the deepObject style just fine.

https://github.com/swagger-api/swagger-js/pull/1140#issuecomment-333017825

As for deepObject and nested objects - that was explicitly left out of the spec, and it's ok to just Not Support It™.

@shockey @webron What's your opinion on this? If we contributed a patch to swagger-js that will serialize deeply-nested properties using the style described above, would you accept such change, despite the fact that OpenAPI specification does not explicitly describe how to serialize deeply-nested properties?

webron commented 5 years ago

@bajtos when we defined deepObject in the spec, we explicitly chose to not mention what happens when the object has several levels in it, but in our conversations we went with 'not supported'. If you're looking for supporting that use case, it needs to be addressed by the spec first.

bajtos commented 5 years ago

@webron thank you for the response.

when we defined deepObject in the spec, we explicitly chose to not mention what happens when the object has several levels in it, but in our conversations we went with 'not supported'.

Fair enough. Can swagger-js reject deeply nested objects with a descriptive error then? Right now, swagger-ui is silently ignoring parameter values set to a deep object. I'd like it to display an error explaining the problem to the user.

Would you accept a patch adding such validation logic?

If you're looking for supporting that use case, it needs to be addressed by the spec first.

Makes sense. I have very little experience with the standardization process. What is your opinion - how likely will such proposal get accepted? How long would the process take? Is it a matter of weeks, months or years?

webron commented 5 years ago

Providing a descriptive error makes sense to me, and we'd appreciate a patch for that logic.

To start a process with affecting the spec, you can file a ticket at https://github.com/OAI/OpenAPI-Specification/ with the details. Releases of the spec are not frequent, so it can take some time. Assuming it's a non-breaking change, it can get into a minor version (but cannot get into a patch version) - that would be a few months.

bajtos commented 5 years ago

Thank you @webron. I opened a new ticket to enhance the spec, see https://github.com/OAI/OpenAPI-Specification/issues/1706.

I'll try to find some time to fix swagger-js in the next few weeks.

Adhalianna commented 1 year ago

Since the discussion on the OAS side seems to have stopped on "use extensions", maybe Swagger could support one that implements this? Some suggestions have been proposed here in the comment under the discussion started by @bajtos .