diff --git a/docs/data-structure.md b/docs/data-structure.md index d62ed422..3ba5619d 100644 --- a/docs/data-structure.md +++ b/docs/data-structure.md @@ -40,8 +40,7 @@ extra fields are supported: ```json { - "redirect_to" : "/target-of-redirect", - "redirect_type" : ["permanent", "temporary"] + "redirect_to" : "/target-of-redirect" } ``` diff --git a/handlers/metrics.go b/handlers/metrics.go index a2f3331a..75c69b65 100644 --- a/handlers/metrics.go +++ b/handlers/metrics.go @@ -11,7 +11,6 @@ var ( Help: "Number of redirects served by redirect handlers", }, []string{ - "redirect_code", "redirect_type", }, ) diff --git a/handlers/redirect_handler.go b/handlers/redirect_handler.go index 125fb60c..6d5795bd 100644 --- a/handlers/redirect_handler.go +++ b/handlers/redirect_handler.go @@ -20,15 +20,12 @@ const ( downcaseRedirectHandlerType = "downcase-redirect-handler" ) -func NewRedirectHandler(source, target string, preserve bool, temporary bool) http.Handler { - statusMoved := http.StatusMovedPermanently - if temporary { - statusMoved = http.StatusFound - } +func NewRedirectHandler(source, target string, preserve bool) http.Handler { + status := http.StatusMovedPermanently if preserve { - return &pathPreservingRedirectHandler{source, target, statusMoved} + return &pathPreservingRedirectHandler{source, target, status} } - return &redirectHandler{target, statusMoved} + return &redirectHandler{target, status} } func addCacheHeaders(w http.ResponseWriter) { @@ -63,7 +60,6 @@ func (handler *redirectHandler) ServeHTTP(w http.ResponseWriter, r *http.Request http.Redirect(w, r, target, handler.code) redirectCountMetric.With(prometheus.Labels{ - "redirect_code": fmt.Sprintf("%d", handler.code), "redirect_type": redirectHandlerType, }).Inc() } @@ -84,7 +80,6 @@ func (handler *pathPreservingRedirectHandler) ServeHTTP(w http.ResponseWriter, r http.Redirect(w, r, target, handler.code) redirectCountMetric.With(prometheus.Labels{ - "redirect_code": fmt.Sprintf("%d", handler.code), "redirect_type": pathPreservingRedirectHandlerType, }).Inc() } @@ -107,7 +102,6 @@ func (handler *downcaseRedirectHandler) ServeHTTP(w http.ResponseWriter, r *http http.Redirect(w, r, target, status) redirectCountMetric.With(prometheus.Labels{ - "redirect_code": fmt.Sprintf("%d", status), "redirect_type": downcaseRedirectHandlerType, }).Inc() } diff --git a/handlers/redirect_handler_test.go b/handlers/redirect_handler_test.go index 7a6c6dc1..a8828e32 100644 --- a/handlers/redirect_handler_test.go +++ b/handlers/redirect_handler_test.go @@ -24,34 +24,32 @@ var _ = Describe("A redirect handler", func() { // These behaviours apply to all combinations of both NewRedirectHandler flags. for _, preserve := range []bool{true, false} { - for _, temporary := range []bool{true, false} { - Context(fmt.Sprintf("where preserve=%t, temporary=%t", preserve, temporary), func() { - BeforeEach(func() { - handler = NewRedirectHandler("/source", "/target", preserve, temporary) - handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, url, nil)) - }) - - It("allows its response to be cached publicly for 30m", func() { - Expect(rr.Result().Header.Get("Cache-Control")).To( - SatisfyAll(ContainSubstring("public"), ContainSubstring("max-age=1800"))) - }) - - It("returns an expires header with an RFC1123 datetime 30m in the future", func() { - Expect(rr.Result().Header.Get("Expires")).To(WithTransform( - func(s string) time.Time { - t, err := time.Parse(time.RFC1123, s) - Expect(err).NotTo(HaveOccurred()) - return t - }, - BeTemporally("~", time.Now().Add(30*time.Minute), time.Minute))) - }) + Context(fmt.Sprintf("where preserve=%t", preserve), func() { + BeforeEach(func() { + handler = NewRedirectHandler("/source", "/target", preserve) + handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, url, nil)) }) - } + + It("allows its response to be cached publicly for 30m", func() { + Expect(rr.Result().Header.Get("Cache-Control")).To( + SatisfyAll(ContainSubstring("public"), ContainSubstring("max-age=1800"))) + }) + + It("returns an expires header with an RFC1123 datetime 30m in the future", func() { + Expect(rr.Result().Header.Get("Expires")).To(WithTransform( + func(s string) time.Time { + t, err := time.Parse(time.RFC1123, s) + Expect(err).NotTo(HaveOccurred()) + return t + }, + BeTemporally("~", time.Now().Add(30*time.Minute), time.Minute))) + }) + }) } Context("where preserve=true", func() { BeforeEach(func() { - handler = NewRedirectHandler("/source", "/target", true, false) + handler = NewRedirectHandler("/source", "/target", true) handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, url, nil)) }) @@ -62,7 +60,7 @@ var _ = Describe("A redirect handler", func() { Context("where preserve=false", func() { BeforeEach(func() { - handler = NewRedirectHandler("/source", "/target", false, false) + handler = NewRedirectHandler("/source", "/target", false) }) It("returns only the configured path in the location header", func() { @@ -78,28 +76,26 @@ var _ = Describe("A redirect handler", func() { }) DescribeTable("responds with the right HTTP status", - EntryDescription("preserve=%t, temporary=%t -> HTTP %d"), - Entry(nil, false, false, http.StatusMovedPermanently), - Entry(nil, false, true, http.StatusFound), - Entry(nil, true, false, http.StatusMovedPermanently), - Entry(nil, true, true, http.StatusFound), - func(preserve, temporary bool, expectedStatus int) { - handler = NewRedirectHandler("/source", "/target", preserve, temporary) + EntryDescription("preserve=%t -> HTTP %d"), + Entry(nil, false, http.StatusMovedPermanently), + Entry(nil, true, http.StatusMovedPermanently), + func(preserve bool, expectedStatus int) { + handler = NewRedirectHandler("/source", "/target", preserve) handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, url, nil)) Expect(rr.Result().StatusCode).To(Equal(expectedStatus)) }) DescribeTable("increments the redirect-count metric with the right labels", - EntryDescription("preserve=%t, temporary=%t -> {redirect_code=%s,redirect_type=%s}"), - Entry(nil, false, false, "301", "redirect-handler"), - Entry(nil, false, true, "302", "redirect-handler"), - Entry(nil, true, false, "301", "path-preserving-redirect-handler"), - Entry(nil, true, true, "302", "path-preserving-redirect-handler"), - func(preserve, temporary bool, codeLabel, typeLabel string) { - lbls := prometheus.Labels{"redirect_code": codeLabel, "redirect_type": typeLabel} + EntryDescription("preserve=%t -> {redirect_type=%s}"), + Entry(nil, false, "redirect-handler"), + Entry(nil, false, "redirect-handler"), + Entry(nil, true, "path-preserving-redirect-handler"), + Entry(nil, true, "path-preserving-redirect-handler"), + func(preserve bool, typeLabel string) { + lbls := prometheus.Labels{"redirect_type": typeLabel} before := promtest.ToFloat64(redirectCountMetric.With(lbls)) - handler = NewRedirectHandler("/source", "/target", preserve, temporary) + handler = NewRedirectHandler("/source", "/target", preserve) handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, url, nil)) after := promtest.ToFloat64(redirectCountMetric.With(lbls)) diff --git a/integration_tests/redirect_test.go b/integration_tests/redirect_test.go index 9bd20637..4622ef8a 100644 --- a/integration_tests/redirect_test.go +++ b/integration_tests/redirect_test.go @@ -13,23 +13,18 @@ var _ = Describe("Redirection", func() { Describe("exact redirects", func() { BeforeEach(func() { addRoute("/foo", NewRedirectRoute("/bar")) - addRoute("/foo-temp", NewRedirectRoute("/bar", "exact", "temporary")) + addRoute("/foo-temp", NewRedirectRoute("/bar", "exact")) addRoute("/query-temp", NewRedirectRoute("/bar?query=true", "exact")) addRoute("/fragment", NewRedirectRoute("/bar#section", "exact")) - addRoute("/preserve-query", NewRedirectRoute("/qux", "exact", "permanent", "preserve")) + addRoute("/preserve-query", NewRedirectRoute("/qux", "exact", "preserve")) reloadRoutes(apiPort) }) - It("should redirect permanently by default", func() { + It("should redirect", func() { resp := routerRequest(routerPort, "/foo") Expect(resp.StatusCode).To(Equal(301)) }) - It("should redirect temporarily when asked to", func() { - resp := routerRequest(routerPort, "/foo-temp") - Expect(resp.StatusCode).To(Equal(302)) - }) - It("should contain the redirect location", func() { resp := routerRequest(routerPort, "/foo") Expect(resp.Header.Get("Location")).To(Equal("/bar")) @@ -72,8 +67,8 @@ var _ = Describe("Redirection", func() { Describe("prefix redirects", func() { BeforeEach(func() { addRoute("/foo", NewRedirectRoute("/bar", "prefix")) - addRoute("/foo-temp", NewRedirectRoute("/bar-temp", "prefix", "temporary")) - addRoute("/qux", NewRedirectRoute("/baz", "prefix", "temporary", "ignore")) + addRoute("/foo-temp", NewRedirectRoute("/bar-temp", "prefix")) + addRoute("/qux", NewRedirectRoute("/baz", "prefix", "ignore")) reloadRoutes(apiPort) }) @@ -183,12 +178,12 @@ var _ = Describe("Redirection", func() { Describe("redirects with a _ga parameter", func() { BeforeEach(func() { - addRoute("/foo", NewRedirectRoute("https://hmrc.service.gov.uk/pay", "prefix", "permanent", "ignore")) - addRoute("/bar", NewRedirectRoute("https://bar.service.gov.uk/bar", "exact", "temporary", "preserve")) - addRoute("/baz", NewRedirectRoute("https://gov.uk/baz-luhrmann", "exact", "permanent", "ignore")) - addRoute("/pay-tax", NewRedirectRoute("https://tax.service.gov.uk/pay", "exact", "permanent", "ignore")) - addRoute("/biz-bank", NewRedirectRoute("https://british-business-bank.co.uk", "prefix", "permanent", "ignore")) - addRoute("/query-paramed", NewRedirectRoute("https://param.servicegov.uk?included-param=true", "exact", "permanent", "ignore")) + addRoute("/foo", NewRedirectRoute("https://hmrc.service.gov.uk/pay", "prefix", "ignore")) + addRoute("/bar", NewRedirectRoute("https://bar.service.gov.uk/bar", "exact", "preserve")) + addRoute("/baz", NewRedirectRoute("https://gov.uk/baz-luhrmann", "exact", "ignore")) + addRoute("/pay-tax", NewRedirectRoute("https://tax.service.gov.uk/pay", "exact", "ignore")) + addRoute("/biz-bank", NewRedirectRoute("https://british-business-bank.co.uk", "prefix", "ignore")) + addRoute("/query-paramed", NewRedirectRoute("https://param.servicegov.uk?included-param=true", "exact", "ignore")) reloadRoutes(apiPort) }) diff --git a/integration_tests/route_helpers.go b/integration_tests/route_helpers.go index 7ec34767..84ed1e53 100644 --- a/integration_tests/route_helpers.go +++ b/integration_tests/route_helpers.go @@ -28,7 +28,6 @@ type Route struct { Handler string `bson:"handler"` BackendID string `bson:"backend_id"` RedirectTo string `bson:"redirect_to"` - RedirectType string `bson:"redirect_type"` SegmentsMode string `bson:"segments_mode"` } @@ -47,20 +46,16 @@ func NewBackendRoute(backendID string, extraParams ...string) Route { func NewRedirectRoute(redirectTo string, extraParams ...string) Route { route := Route{ - Handler: "redirect", - RedirectTo: redirectTo, - RedirectType: "permanent", - RouteType: "exact", + Handler: "redirect", + RedirectTo: redirectTo, + RouteType: "exact", } if len(extraParams) > 0 { route.RouteType = extraParams[0] } if len(extraParams) > 1 { - route.RedirectType = extraParams[1] - } - if len(extraParams) > 2 { - route.SegmentsMode = extraParams[2] + route.SegmentsMode = extraParams[1] } return route diff --git a/lib/router.go b/lib/router.go index f651e5bd..5fbac7bd 100644 --- a/lib/router.go +++ b/lib/router.go @@ -78,7 +78,6 @@ type Route struct { Handler string `bson:"handler"` BackendID string `bson:"backend_id"` RedirectTo string `bson:"redirect_to"` - RedirectType string `bson:"redirect_type"` SegmentsMode string `bson:"segments_mode"` } @@ -354,8 +353,7 @@ func loadRoutes(c *mgo.Collection, mux *triemux.Mux, backends map[string]http.Ha logDebug(fmt.Sprintf("router: registered %s (prefix: %v) for %s", incomingURL.Path, prefix, route.BackendID)) case "redirect": - redirectTemporarily := (route.RedirectType == "temporary") - handler := handlers.NewRedirectHandler(incomingURL.Path, route.RedirectTo, shouldPreserveSegments(route), redirectTemporarily) + handler := handlers.NewRedirectHandler(incomingURL.Path, route.RedirectTo, shouldPreserveSegments(route)) mux.Handle(incomingURL.Path, prefix, handler) logDebug(fmt.Sprintf("router: registered %s (prefix: %v) -> %s", incomingURL.Path, prefix, route.RedirectTo))