Route.Host -matching will ignore any provided port from getHost(), if…

[cognusion]

… a port isn't specified in the template

In lieu of checking the template pattern on every Match request, a bool is added to the routeRegexp, and set if the routeRegexp is a host AND there is no ":" in the template. I dislike extending the type, but I'd dislike doing a string match on every single Match, even more.

All existing mux tests pass. Additionally my application-level (Convey) positive and negative tests pass. Feel free to comment: You know your code better than I do.

=== RUN   TestPathHostPortUnset

  When a request is made, with a host and port specified, the correct match is found (no port specified) ✔✔✔✔✔


16 total assertions

--- PASS: TestPathHostPortUnset (0.00s)
=== RUN   TestPathHostPortSet

  When a request is made, with a host and port specified, and the port does match, a match is found ✔✔✔✔✔


21 total assertions

--- PASS: TestPathHostPortSet (0.00s)
=== RUN   TestPathHostPortSetNope

  When a request is made, with a host and port specified, and the port does not match, a match is NOT found ✔✔✔


24 total assertions

--- PASS: TestPathHostPortSetNope (0.00s)

[yogo1212]

works for me

[stale]

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

[elithrar]

Sorry, I missed this: can you show application code before/after that explains this better?

I understand what the change does, but not why.

[yogo1212]

@elithrar
Without this change, r.Host("test.host.com") won't match requests with the Host: test.host.com:1234.
In order to make that case work, this PR assumes that, unless a port specified (e.g. r.Host("test.host.com:1234")), any port is allowed to match.

[cognusion]

@elithrar ,

As @yogo1212 properly described, a previous PR #383 horrifically broke a good swath of the Gorilla-using Internet by deciding that if you didn't specify a port in the route, you can't specify a port on the Request and have it match. This PR allows that PR to exist in-spirit, without the awful requirement of specifying ports for every route. Without either reverting #383 or implementing this patch, if your route is r.Host("a.b.c") then http://a.b.c:80/ won't work but http://a.b.c/ will. With this patch, unless you specify the port on the route, it assumes you don't give a flying leap what port the request tacks on.

[elithrar]

Understood - #383 broke more than we anticipated (users should pin + test deps, but we also need to do a better job of checking impact).

Reviewing the PR today and can (hopefully) merge + tag a new version tonight.

[elithrar]

@cognusion See https://github.com/gorilla/mux/releases/tag/v1.7.2

[yogo1212]

@elithrar thank you for the notice <3

[harshavardhana]

This PR actually caused a regression one more change needed is missing in this PR

git diff
diff --git a/regexp.go b/regexp.go
index 96dd94a..0144842 100644
--- a/regexp.go
+++ b/regexp.go
@@ -325,6 +325,12 @@ func (v routeRegexpGroup) setMatch(req *http.Request, m *RouteMatch, r *Route) {
        // Store host variables.
        if v.host != nil {
                host := getHost(req)
+               if v.host.wildcardHostPort {
+                       // Don't be strict on the port match
+                       if i := strings.Index(host, ":"); i != -1 {
+                               host = host[:i]
+                       }
+               }
                matches := v.host.regexp.FindStringSubmatchIndex(host)
                if len(matches) > 0 {
                        extractVars(host, matches, v.host.varsN, m.Vars)