Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trailing slash removed from root requests #101

Open
stevecookform3 opened this issue Apr 30, 2018 · 5 comments
Open

Trailing slash removed from root requests #101

stevecookform3 opened this issue Apr 30, 2018 · 5 comments

Comments

@stevecookform3
Copy link

Given the example swagger:

swagger: '2.0'
basePath: "/myservice"
paths:
  /:
    get:

the following code in request.go doesnt create the correct path:

	// create http request
	var reinstateSlash bool
	if r.pathPattern != "" && r.pathPattern != "/" && r.pathPattern[len(r.pathPattern)-1] == '/' {
		reinstateSlash = true
	}
	urlPath := path.Join(basePath, r.pathPattern)
	for k, v := range r.pathParams {
		urlPath = strings.Replace(urlPath, "{"+k+"}", url.PathEscape(v), -1)
	}
	if reinstateSlash {
		urlPath = urlPath + "/"
	}

So in this example:

  • r.pathPatterm is "/"
  • basePath is "/myservice"

request.buildHTTP currently bins off the trailing slash to set the urlPath to: "/myservice" instead of the correct "/myservice/". Changing basePath to "/myservice/" doesnt make any difference.

Couple of options:

  1. Remove && r.pathPattern != "/" - not sure what this is trying to achieve
  2. Swap out path.Join for something that is URL aware (e.g. as suggested by https://stackoverflow.com/questions/34668012/combine-url-paths-with-path-join)
@stevecookform3
Copy link
Author

This seems to be the commit that introduced the change: de2e648

however I dont see any specific test cases that relate to the root path.

@casualjim @e-zhang - do you know what the expected behaviour is with the case above? Any issue with just removing the r.pathPattern != "/" from the expression?

@e-zhang
Copy link
Contributor

e-zhang commented May 9, 2018

I think the original change was introduced in 20675b2

It looks like that change was for the case where basePath == "".

Here's the difference in behavior from path.Join in a playground.

@casualjim
Copy link
Member

there is nothing in the spec that says a trailing / is significant.
Servers sometimes choose to implement one or the other and not both.
If you depend on this behavior it might be better to reconsider the server you're using because url normalizers often just remove insignificant chars.

@stevecookform3
Copy link
Author

@casualjim unfortunately this is a third-party service, so we cant change their implementation. Like you say, its not clear from the spec how to deal with this case, so i'll probably open a ticket over there.

I did find a workaround though, if you specify the endpoint like this:

paths:
  //:
    get:

then it resolves to the intended destination /myservice/

@casualjim
Copy link
Member

we could add a method to the runtime that allows you to hook into or augment the path normalization?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants