Merge pull request #482 from alphagov/remove-temporary-redirects

Remove temporary redirection

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"
 }
 ```
 

handlers/metrics.go

@@ -11,7 +11,6 @@ var (
 			Help: "Number of redirects served by redirect handlers",
 		},
 		[]string{
-			"redirect_code",
 			"redirect_type",
 		},
 	)

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()
 }

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))

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,23 +67,17 @@ 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)
 		})
 
-		It("should redirect permanently to the destination", func() {
+		It("should redirect to the destination", func() {
 			resp := routerRequest(routerPort, "/foo")
 			Expect(resp.StatusCode).To(Equal(301))
 			Expect(resp.Header.Get("Location")).To(Equal("/bar"))
 		})
 
-		It("should redirect temporarily to the destination when asked to", func() {
-			resp := routerRequest(routerPort, "/foo-temp")
-			Expect(resp.StatusCode).To(Equal(302))
-			Expect(resp.Header.Get("Location")).To(Equal("/bar-temp"))
-		})
-
 		It("should preserve extra path sections when redirecting by default", func() {
 			resp := routerRequest(routerPort, "/foo/baz")
 			Expect(resp.Header.Get("Location")).To(Equal("/bar/baz"))
@@ -135,9 +124,9 @@ var _ = Describe("Redirection", func() {
 	Describe("external redirects", func() {
 		BeforeEach(func() {
 			addRoute("/foo", NewRedirectRoute("http://foo.example.com/foo"))
-			addRoute("/baz", NewRedirectRoute("http://foo.example.com/baz", "exact", "permanent", "preserve"))
+			addRoute("/baz", NewRedirectRoute("http://foo.example.com/baz", "exact", "preserve"))
 			addRoute("/bar", NewRedirectRoute("http://bar.example.com/bar", "prefix"))
-			addRoute("/qux", NewRedirectRoute("http://bar.example.com/qux", "prefix", "permanent", "ignore"))
+			addRoute("/qux", NewRedirectRoute("http://bar.example.com/qux", "prefix", "ignore"))
 			reloadRoutes(apiPort)
 		})
 
@@ -183,12 +172,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)
 		})
 

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

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))