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

Guess the scheme if r.URL.Scheme is unset #474

Merged
merged 5 commits into from Oct 18, 2019

Conversation

euank
Copy link
Contributor

@euank euank commented May 15, 2019

It's not expected that the request's URL is fully populated when used on
the server-side (it's more of a client-side field), so we shouldn't
expect it to be present.

In practice, it's only rarely set at all on the server, making mux's
Schemes matcher tricky to use as it is.

This commit adds a test which would have failed before demonstrating the
problem, as well as a fix which I think makes .Schemes match what
users expect.

This will technically break anyone who did the following:

router.Schemes("") // match http and https

@euank euank force-pushed the fix-scheme-matchers branch 2 times, most recently from a1058ab to 578eac8 Compare May 15, 2019
route.go Outdated Show resolved Hide resolved
@fharding1
Copy link
Member

@fharding1 fharding1 commented May 27, 2019

To back up it's only rarely set at all on the server:

https://golang.org/pkg/net/http/#Request

        // URL specifies either the URI being requested (for server
        // requests) or the URL to access (for client requests).
        //
        // For server requests, the URL is parsed from the URI
        // supplied on the Request-Line as stored in RequestURI.  For
        // most requests, fields other than Path and RawQuery will be
        // empty. (See RFC 7230, Section 5.3)
        //
        // For client requests, the URL's Host specifies the server to
        // connect to, while the Request's Host field optionally
        // specifies the Host header value to send in the HTTP
        // request.
        URL *url.URL

Copy link
Member

@fharding1 fharding1 left a comment

This could benefit from some documentation on the Schemes methods of Router and Route

@euank
Copy link
Contributor Author

@euank euank commented May 28, 2019

I added some docs to Route.Schemes. Since Router just references the Route docs, it seems like that's the only place docs should be updated.

route.go Outdated Show resolved Hide resolved
@euank euank force-pushed the fix-scheme-matchers branch from ca364df to 8af972a Compare Jul 3, 2019
euank added 4 commits Jul 3, 2019
It's not expected that the request's URL is fully populated when used on
the server-side (it's more of a client-side field), so we shouldn't
expect it to be present.

In practice, it's only rarely set at all on the server, making mux's
`Schemes` matcher tricky to use as it is.

This commit adds a test which would have failed before demonstrating the
problem, as well as a fix which I think makes `.Schemes` match what
users expect.
@euank euank force-pushed the fix-scheme-matchers branch from 8981322 to 7b0a1dc Compare Jul 3, 2019
@euank
Copy link
Contributor Author

@euank euank commented Jul 3, 2019

I updated the test so it would have caught that typo and rebased it on master to pickup the CI config changes.

@elithrar
Copy link
Member

@elithrar elithrar commented Oct 12, 2019

@fharding1 - PTAL.

Copy link
Member

@fharding1 fharding1 left a comment

I would say that the test case should be updated, but otherwise it looks good to me.

"testing"
)

func TestSchemeMatchers(t *testing.T) {
Copy link
Member

@fharding1 fharding1 Oct 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense for this test to have different responses for the different schemes so we can use that to differentiate? I believe this test would pass if the httpsServer subtest was matching on the http route.

Copy link
Contributor Author

@euank euank Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right. I made it so there's one router that has both matchers, and that each matcher has a different response. I think that should better test for what we care about.
Thanks for the review!

@elithrar
Copy link
Member

@elithrar elithrar commented Oct 13, 2019

@fharding1
Copy link
Member

@fharding1 fharding1 commented Oct 17, 2019

@euank I'll take another look at this today

Copy link
Member

@fharding1 fharding1 left a comment

This looks good to me

@elithrar elithrar merged commit ff4e71f into gorilla:master Oct 18, 2019
1 check passed
@elithrar
Copy link
Member

@elithrar elithrar commented Oct 18, 2019

Thanks both!

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

Successfully merging this pull request may close these issues.

None yet

5 participants