Azure / autorest

OpenAPI (f.k.a Swagger) Specification code generator. Supports C#, PowerShell, Go, Java, Node.js, TypeScript, Python
MIT License
4.62k stars 739 forks source link

Optional path needs to be supported #729

Closed pomortaz closed 8 years ago

pomortaz commented 8 years ago

In Hyak when a part of the path is set to be optional, if the optional argument in the path is not provided, it would not include the optional path. This is not supported with AutoRest, which is needed by Key Vault service.

For example for /subscriptions/{subscriptionId}/[resourceGroups/{resourceGroupName}]/resources if {resourceGroupName} is provided, the base URL should be /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/resources and when {resourceGroupName} is not provided, the base URL should be /subscriptions/{subscriptionId}/resources

devigned commented 8 years ago

Path params are not optional per the OpenAPI spec.

See: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#parameterObject

stankovski commented 8 years ago

This request comes again and again from partners so I'd like to have this issue open.

devigned commented 8 years ago

@stankovski then this is one that we should propose to the next rev of the OpenAPI spec. I understand that people ask for it, but it's totally against the spec.

stankovski commented 8 years ago

One proposed solution can be to define these 2 methods as 2 different paths with 2 different operationIds (e.g. List and ListForGroup) so that the swagger is still valid. Then we can introduce an extension x-ms-operationId that would allow specifying overloading method names (if supported by the language). The x-ms-operationId value would be used instead of operationId to generate a method name and we will validate the method signature to ensure uniqueness.

In C# and Java, the client will end up with two List() methods - List() and List(string group), while in NodeJS we will still generate List() and ListForGroup().

stankovski commented 8 years ago

@devigned I'm all for proposing it to OpenAPI as well. Can you do that?

devigned commented 8 years ago

@stankovski I see what you are doing.... Yeah, I'll start that conversation.

azuresdkci commented 8 years ago

we'll add, but it'll go into the x-ms-paths extension space

amarzavery commented 8 years ago

@devigned @markcowl - Please find the corresponding issue on open API spec OAI/OpenAPI-Specification#622

amarzavery commented 8 years ago

Adding the open api issue, which has lot of similar requests for optional path parameters: https://github.com/OAI/OpenAPI-Specification/issues/574

darrelmiller commented 8 years ago

Optional path parameters are supported in URI Templates (RFC 6570) , but they don't work the way the original example suggested.
Consider this template:

 /subscriptions/{subscriptionId}/resourceGroups{/resourceGroupName}/resources 

Note the / is inside the curly braces. When {resourceGroupName} is not provided, the URL would be,

/subscriptions/{subscriptionId}/resourceGroups/resources 

I realize that doesn't help much, but if OpenAPI are going to support optional parameters then it is likely going to be the 6570 way. I doubt we could reach consensus on the idea that the parent path segment is "attached" to the parameter segment.

fearthecowboy commented 8 years ago

Howdy!

In our planning for driving towards a stable '1.0' release, I'm marking this issue as 'deferred' :zzz: and we're going to review it during the post-1.0 planning cycle.

It's not to say that we're not going to work on it, or that this isn't not important, but at the moment, we're picking and choosing the stuff we must do before 1.0. :horse_racing: :horse_racing: :horse_racing:

We'll make sure we pick this back up at that point. :tada: