Guess the scheme if r.URL.Scheme is unset#474
Conversation
a1058ab to
578eac8
Compare
578eac8 to
16409f3
Compare
|
To back up https://golang.org/pkg/net/http/#Request |
fharding1
left a comment
There was a problem hiding this comment.
This could benefit from some documentation on the Schemes methods of Router and Route
|
I added some docs to Route.Schemes. Since |
ca364df to
8af972a
Compare
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.
8981322 to
7b0a1dc
Compare
|
I updated the test so it would have caught that typo and rebased it on master to pickup the CI config changes. |
|
@fharding1 - PTAL. |
fharding1
left a comment
There was a problem hiding this comment.
I would say that the test case should be updated, but otherwise it looks good to me.
| "testing" | ||
| ) | ||
|
|
||
| func TestSchemeMatchers(t *testing.T) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
|
Thanks for taking a look - agree that the test cases should be split out
here.
…On Sun, Oct 13, 2019 at 10:56 AM Franklin Harding ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I would say that the test case should be updated, but otherwise it looks
good to me.
------------------------------
In mux_httpserver_test.go
<#474 (comment)>:
> @@ -0,0 +1,50 @@
+// +build go1.9
+
+package mux
+
+import (
+ "bytes"
+ "io/ioutil"
+ "net/http"
+ "net/http/httptest"
+ "testing"
+)
+
+func TestSchemeMatchers(t *testing.T) {
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.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#474?email_source=notifications&email_token=AAAEQ4EK5NJT5U6Y5LUSQ4DQONOK7A5CNFSM4HNGM662YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHY4UPA#pullrequestreview-301058620>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEQ4HU6XK2B72RWSJK2NTQONOK7ANCNFSM4HNGM66Q>
.
|
ac8a272 to
6a87310
Compare
|
@euank I'll take another look at this today |
|
Thanks both! |
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
Schemesmatcher 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
.Schemesmatch whatusers expect.
This will technically break anyone who did the following: