[release-branch.go1.11] godoc/redirect: display Gerrit/Rietveld CL disambiguation page when needed

dmitshur · dmitshur · commit 2646b7dc2e7f · 2019-01-10T01:59:24.000Z
For CL numbers that are determined to be Rietveld CLs, instead of immediately redirecting, check whether a Gerrit CL with the same number also exists. Do so by querying the Gerrit API and caching the existing CLs. If both exist, display a very simple disambiguation HTML page. Cache Gerrit CLs that exist, to avoid querying the remote API server more than once per CL. We can't cache Gerrit CLs that don't exist, since they might get created in the future. Fixes golang/go#29645 Change-Id: I08c32dc82a0136788337c5c32028e87428e8d81e Reviewed-on: https://go-review.googlesource.com/c/157237 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
diff --git a/godoc/redirect/redirect.go b/godoc/redirect/redirect.go
@@ -8,12 +8,18 @@
 package redirect // import "golang.org/x/tools/godoc/redirect"
 
 import (
+	"context"
 	"fmt"
+	"html/template"
 	"net/http"
 	"os"
 	"regexp"
 	"strconv"
 	"strings"
+	"sync"
+	"time"
+
+	"golang.org/x/net/context/ctxhttp"
 )
 
 // Register registers HTTP handlers that redirect old godoc paths to their new
@@ -193,18 +199,82 @@ func clHandler(w http.ResponseWriter, r *http.Request) {
 	target := ""
 
 	if n, err := strconv.Atoi(id); err == nil && isRietveldCL(n) {
-		// TODO: Issue 28836: if this Rietveld CL happens to
+		// Issue 28836: if this Rietveld CL happens to
 		// also be a Gerrit CL, render a disambiguation HTML
-		// page with two links instead. We'll need to make an
-		// RPC (to maintner?) to figure that out. For now just
-		// redirect to rietveld.
+		// page with two links instead. We need to make a
+		// Gerrit API call to figure that out, but we cache
+		// known Gerrit CLs so it's done at most once per CL.
+		if ok, err := isGerritCL(r.Context(), n); err == nil && ok {
+			w.Header().Set("Content-Type", "text/html; charset=utf-8")
+			clDisambiguationHTML.Execute(w, n)
+			return
+		}
+
 		target = "https://codereview.appspot.com/" + id
 	} else {
 		target = "https://go-review.googlesource.com/" + id
 	}
 	http.Redirect(w, r, target, http.StatusFound)
 }
 
+var clDisambiguationHTML = template.Must(template.New("").Parse(`<!DOCTYPE html>
+<html lang="en">
+	<head>
+		<title>Go CL {{.}} Disambiguation</title>
+		<meta name="viewport" content="width=device-width">
+	</head>
+	<body>
+		CL number {{.}} exists in both Gerrit (the current code review system)
+		and Rietveld (the previous code review system). Please make a choice:
+
+		<ul>
+			<li><a href="https://go-review.googlesource.com/{{.}}">Gerrit CL {{.}}</a></li>
+			<li><a href="https://codereview.appspot.com/{{.}}">Rietveld CL {{.}}</a></li>
+		</ul>
+	</body>
+</html>`))
+
+// isGerritCL reports whether a Gerrit CL with the specified numeric change ID (e.g., "4247")
+// is known to exist by querying the Gerrit API at https://go-review.googlesource.com.
+// isGerritCL uses gerritCLCache as a cache of Gerrit CL IDs that exist.
+func isGerritCL(ctx context.Context, id int) (bool, error) {
+	// Check cache first.
+	gerritCLCache.Lock()
+	ok := gerritCLCache.exist[id]
+	gerritCLCache.Unlock()
+	if ok {
+		return true, nil
+	}
+
+	// Query the Gerrit API Get Change endpoint, as documented at
+	// https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-change.
+	ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
+	defer cancel()
+	resp, err := ctxhttp.Get(ctx, nil, fmt.Sprintf("https://go-review.googlesource.com/changes/%d", id))
+	if err != nil {
+		return false, err
+	}
+	resp.Body.Close()
+	switch resp.StatusCode {
+	case http.StatusOK:
+		// A Gerrit CL with this ID exists. Add it to cache.
+		gerritCLCache.Lock()
+		gerritCLCache.exist[id] = true
+		gerritCLCache.Unlock()
+		return true, nil
+	case http.StatusNotFound:
+		// A Gerrit CL with this ID doesn't exist. It may get created in the future.
+		return false, nil
+	default:
+		return false, fmt.Errorf("unexpected status code: %v", resp.Status)
+	}
+}
+
+var gerritCLCache = struct {
+	sync.Mutex
+	exist map[int]bool // exist is a set of Gerrit CL IDs that are known to exist.
+}{exist: make(map[int]bool)}
+
 var changeMap *hashMap
 
 // LoadChangeMap loads the specified map of Mercurial to Git revisions,
