path portion of 'origin_url' being ignored in build_api_serving_url()

[DStape]

When an origin_url of http://localhost/x is specified, /x is expected to be present in all URLs that the function returns.

This was the behaviour until https://github.com/Yelp/bravado-core/commit/108363636068fc6b1234bad2a9139550e256af3a was added.

The message in that commit indicates the behaviour change was made because, "According to the Swagger spec we should not use the path part of the spec URL".

However, I'm not sure I agree. Looking at https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#fixed-fields, the only restriction I see is, "The basePath does not support path templating.".

In our use case, the basePath is a variable and we've been using origin_url in our bravado-based tests which are now broken with the change. So for now, we've pinned bravado-core to a lower version.

I'd like to see the original behaviour (full origin_url being honoured) re-instated, especially since I don't see any reason why this would violate the swagger 2.0 spec (although I may be mis-reading it, so please correct me :)).

@mulmschneider , are you able to expand on why the behaviour change was made?

Thanks for your time, David

[mulmschneider]

Hi there,

I was afraid that my change would create fallout for somebody, sorry it hit you.

I was referring to the following sentence in the API spec: "If it is not included, the API is served directly under the host". Which I interpreted to mean that if basePath is not specified in the swagger.json you should indeed just use the host part of URL.

After re-reading the spec I'm not sure though if this still applies if you also don't specify the host field in your API spec (it is not a required field).

My problem that caused me to propose the change was the following piece of code:

from bravado.client import SwaggerClient                                                                                                                                                                                                                                              
client = SwaggerClient.from_url('https://172.30.251.16/swagger.json')  

Which, since my swagger.json didn't include a basePath, caused bravado to build API urls like https://172.30.251.16/swagger.json/getpets instead of https://172.30.251.16/getpets.

Note that you can use the following to overwrite the auto generated URL:

client.swagger_spec.api_url = 'http://172.30.251.16/'

(Meta: I used the term API spec interchangeably for either the Swagger specification or the specification of the Swagger specification :/ - are there any better terms that clearly differentiate between the two?)

[DStape]

No probs.

We also don't specify basePath in our docs. This is because we have a use-case where the base path contains a variable (/<name>), but Swagger 2.0 does not support this. Therefore, to get around this, we download the API doc using the requests library, load this into the Swagger client and set origin_url to a specific :

# example name = test
client = SwaggerClient.from_spec(requests.get('http://localhost/swagger').json(), origin_url='/test)

This results in URLs being built that look like http://localhost/test/getPets. Doing it this way also means we have a swagger client per <name>, which works for our use-case.

I understand why the change that's been made works for your case, however it unfortunately breaks others.

To fix this, I see 3 other potential solutions you could use:

1) specify a basePath in your doc (this wouldn't work in our case due to the lack of template support)

2) update your code to instantiate your client using SwaggerClient.from_spec() instead (as shown above)

3) we modify the bravado code to handle missing basePaths when a client is built using from_url() - if basePath is missing from the spec, then it fallbacks to /, otherwise it should continue to use whatever basePath is set (as is the case now).

EDIT: w.r.t. 3) from_url() eventually calls from_spec(), so I guess it's more for convenience than anything else. I think the code change should be made in the from_url() method, which means a PR to yelp/bravado (not yelp/bravado-core). Working on it just now.

(I think your usage of API spec is fine, was all clear to me :))

In summary, I'd still like to see a code change for this issue, be it a reversion of https://github.com/Yelp/bravado-core/commit/108363636068fc6b1234bad2a9139550e256af3a (due to changing your implementation to 1 or 2) or I'd also be happy to write a solution that addresses my third suggestion.

Let me know your thoughts :).

Thanks, David

[DStape]

After studying the code for a bit, seems like the best place for the change is in bravado-core (and not bravado).

https://github.com/Yelp/bravado-core/pull/300

[mulmschneider]

No objections from me, though my bravado knowledge is limited. Let's see what the maintainers think.

[DStape]

300 merged, so I'm happy to close this now.