swagger-api / swagger-ui

Swagger UI is a collection of HTML, JavaScript, and CSS assets that dynamically generate beautiful documentation from a Swagger-compliant API.
https://swagger.io
Apache License 2.0
26.01k stars 8.86k forks source link

Optional path parameter throwing error when empty #380

Closed timwhitlock closed 9 years ago

timwhitlock commented 10 years ago

I have an optional parameter in my path, something like this:

/{mandatory}{.optional}

Using Guzzle to consume my API and using the same format in my service description, it's quite happy to have the parameter omitted, and will also omit the dot along with it. This is exactly what I want.

However when this field is empty and I submit the "Try it out" form in swagger-ui I get "Uncaught optional is a required path param."

When I do put something in this field, the path parameter doesn't get populated, it stays as "{.optional}" in the request path. I'm guessing Swagger doesn't support the same templating as Guzzle? I'd really like to use them in combination if possible.

fehguy commented 10 years ago

You are correct, the path params are explicitly required by the spec, however, there is definitely a possibility that a part of the path param is not.

I think revisiting the path param as being required is in order, or in this case, a special indicator that this is a portion of the path param makes sense.

timwhitlock commented 10 years ago

Understood. Unfortunately. my optional parameter must be optional and the API is already written and in use. I'm also already using Guzzle for my SDK. Are there any plans to work this URI template spec into the Swagger spec?

StefanGla commented 10 years ago

I have the same problem with optional params in path's. I'm using php slim framework with swagger-php and swagger-ui. My API implements an path like this: /list/infos/{page}/{categoryId}

How can I tell swagger(-php/-ui) that these params are optional?

webron commented 10 years ago

Tony's answer still stands. The spec currently does not allow for optional path params. Feel free to open an issue on https://github.com/wordnik/swagger-spec/about it with an explanation of your use case.

On Mon, Mar 31, 2014 at 5:26 PM, StefanGla notifications@github.com wrote:

I have the same problem with optional params in path's. I'm using php slim framework with swagger-php and swagger-ui. My API implements an path like this: /list/infos/{page}/{categoryId}

How can I tell swagger(-php/-ui) that these params are optional?

— Reply to this email directly or view it on GitHubhttps://github.com/wordnik/swagger-ui/issues/380#issuecomment-39094164 .

netizen commented 10 years ago

Just because Swagger UI cannot support optional path parameters, I am not able to present Swagger UI based API documentation for the REST API that I built using Zend Framework to my team... Is there any ETA on when Swagger UI will start supporting optional path parameters in the GET requests?

fehguy commented 10 years ago

Hi @netizen, it's not that swagger-ui doesn't support it. It's that the spec doesn't support it, intentionally. So the ETA is not the concern, it's simply outside of the design of swagger and the spec itself.

That doesn't mean you can't hack this on your own, you could easily modify swagger.js and swagger-ui.js to skip the path param check.

netizen commented 10 years ago

Thanks for the clarification. Just out of curiosity, is there a reason for the Swagger Spec to not support optional path parameters intentionally?

fehguy commented 10 years ago

I think this is covered in other tickets, but as path parameters become optional, the response type from the calls can become indeterminate, and therefore impossible to describe.

If for example this returns a single user object: http://api.foo.com/users/{id}

And this API returns an array: http://api.foo.com/users/

What is the response type if {id} is optional? And what if we have multiple path parameters: http://api.foo.com/users/{id}/{contacts}

Would it become: http://api.foo.com/users/ ?

There are of course perfectly valid and great use cases for the above, and it's easy to justify optional path parameters as valid use cases, and acceptable REST practices. For the ecosystem of client code generation, and limited knowledge of the back-end's logic, it is very difficult to support and can end up being confusing.

donoseven commented 10 years ago

Same issue with Restler generated API. I understand the concept to single item with a parameter and array without. It cannot be acceptable to simply say you cannot document those APIs. They exist, they work and they are acceptable REST practices. Therefore, is stands to reason, the API will need documentation. The desire to have Swagger-ui be that documentation should be seen as a good thing.

So I ask, how then do you document it. Does Swagger recommend to list these as two different paths? One for the Array without the param and then an item when the "now required" param is sent? I can see the Docs getting a bit longer, but very clearly documented. I understand it is a manual step to move from the generated documentation that does not fully represent the API flexibility to one that at least documents the possibility.

Before I take those steps, I was wondering if anyone else found a workaround. I don't like the make it a query parameter always option. Another API documentation tool is also an option. Thanks.

fehguy commented 10 years ago

Hi @donoseven I'm not going to argue that it's uncommon to do this. But it's simply not in the swagger specification to have optional path parameters. To make the ecosystem work, there are some use cases that simply do not fit inside the spec. This can be a feature or a bug, depending where you stand.

The solution is actually extremely simple. You fork swagger-ui and add that support. It's easy to do--in fact I'll show you were (bottom of this comment). But I cannot add support that breaks the spirit of the spec, it will only cause other issues downstream.

Many people have found use cases outside what the default swagger does, and luckily it is completely OSS and very fork-able.

In swagger.js, change line 912, which is this:

      else
        throw "" + param.name + " is a required path param.";

to something like this:

      else {
        var reg = new RegExp('\{' + param.name + '[^\}]*\}', 'gi');
        url = url.replace(reg, "");
        delete args[param.name];
      }

It's that easy.

donoseven commented 10 years ago

Thank you for the change and agree with your sentiment. In the case where a parameter qualifies or filters the first parameter, this change makes sense.

In your example where the return type changes, it might not. Thinking the same as other coding languages and optional parameters. You should not change the return data type and make it generic Object. In those cases two functions makes more sense.

Thanks

Sent from my iPad

On May 1, 2014, at 6:37 PM, Tony Tam notifications@github.com wrote:

Hi @donoseven I'm not going to argue that it's uncommon to do this. But it's simply not in the swagger specification to have optional path parameters. To make the ecosystem work, there are some use cases that simply do not fit inside the spec. This can be a feature or a bug, depending where you stand.

The solution is actually extremely simple. You fork swagger-ui and add that support. It's easy to do--in fact I'll show you were (bottom of this comment). But I cannot add support that breaks the spirit of the spec, it will only cause other issues downstream.

Many people have found use cases outside what the default swagger does, and luckily it is completely OSS and very fork-able.

In swagger.js, change line 912, which is this:

  else
    throw "" + param.name + " is a required path param.";

to something like this:

  else {
    var reg = new RegExp('\{' + param.name + '[^\}]*\}', 'gi');
    url = url.replace(reg, "");
    delete args[param.name];
  }

It's that easy.

— Reply to this email directly or view it on GitHub.

fehguy commented 9 years ago

Please reopen if this is still unclear. Some of this will be handled in the 2.0 spec version.