Redirect /123 to /project/+/123

This commit changes the servlet we put behind the single-number regex
to rewrite the URL to include the project name in the URL.

It also adds tests to previously untested behavior.

Change-Id: Ide63d5d4d1a8d8db316b1ee87d64856a7fef2bf5

java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -269,6 +269,7 @@ public abstract class AbstractDaemonTest {
   protected Project.NameKey project;
   protected RestSession adminRestSession;
   protected RestSession userRestSession;
+  protected RestSession anonymousRestSession;
   protected ReviewDb db;
   protected SshSession adminSshSession;
   protected SshSession userSshSession;
@@ -445,6 +446,7 @@ public abstract class AbstractDaemonTest {
 
     adminRestSession = new RestSession(server, admin);
     userRestSession = new RestSession(server, user);
+    anonymousRestSession = new RestSession(server, null);
 
     initSsh();
 

java/com/google/gerrit/acceptance/RestResponse.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.acceptance;
 
+import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.gerrit.httpd.restapi.RestApiServlet.JSON_MAGIC;
 import static java.nio.charset.StandardCharsets.UTF_8;
@@ -21,6 +22,7 @@ import static java.nio.charset.StandardCharsets.UTF_8;
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.Reader;
+import java.net.URI;
 import org.apache.http.HttpStatus;
 
 public class RestResponse extends HttpResponse {
@@ -83,4 +85,9 @@ public class RestResponse extends HttpResponse {
   public void assertPreconditionFailed() throws Exception {
     assertStatus(HttpStatus.SC_PRECONDITION_FAILED);
   }
+
+  public void assertTemporaryRedirect(String path) throws Exception {
+    assertStatus(HttpStatus.SC_MOVED_TEMPORARILY);
+    assertThat(URI.create(getHeader("Location")).getPath()).isEqualTo(path);
+  }
 }

java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java

@@ -0,0 +1,71 @@
+// Copyright (C) 2018 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.httpd;
+
+import com.google.gerrit.common.PageLinks;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.restapi.change.ChangesCollection;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+/** Redirects {@code domain.tld/123} to {@code domain.tld/c/project/+/123}. */
+@Singleton
+public class NumericChangeIdRedirectServlet extends HttpServlet {
+  private static final long serialVersionUID = 1L;
+
+  private final ChangesCollection changesCollection;
+
+  @Inject
+  NumericChangeIdRedirectServlet(ChangesCollection changesCollection) {
+    this.changesCollection = changesCollection;
+  }
+
+  @Override
+  protected void doGet(HttpServletRequest req, HttpServletResponse rsp) throws IOException {
+    String idString = req.getPathInfo();
+    if (idString.endsWith("/")) {
+      idString = idString.substring(0, idString.length() - 1);
+    }
+    Change.Id id;
+    try {
+      id = Change.Id.parse(idString);
+    } catch (IllegalArgumentException e) {
+      rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
+      return;
+    }
+
+    ChangeResource changeResource;
+    try {
+      changeResource = changesCollection.parse(id);
+    } catch (ResourceConflictException | ResourceNotFoundException e) {
+      rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
+      return;
+    } catch (OrmException | PermissionBackendException e) {
+      throw new IOException("Unable to lookup change " + id.id, e);
+    }
+    String path =
+        PageLinks.toChange(changeResource.getProject(), changeResource.getChange().getId());
+    UrlModule.toGerrit(path, req, rsp);
+  }
+}

java/com/google/gerrit/httpd/UrlModule.java

@@ -87,7 +87,7 @@ class UrlModule extends ServletModule {
     serveRegex("^/settings/?$").with(screen(PageLinks.SETTINGS));
     serveRegex("^/register$").with(registerScreen(false));
     serveRegex("^/register/(.+)$").with(registerScreen(true));
-    serveRegex("^/([1-9][0-9]*)/?$").with(directChangeById());
+    serveRegex("^/([1-9][0-9]*)/?$").with(NumericChangeIdRedirectServlet.class);
     serveRegex("^/p/(.*)$").with(queryProjectNew());
     serveRegex("^/r/(.+)/?$").with(DirectChangeByCommit.class);
 
@@ -162,30 +162,6 @@ class UrlModule extends ServletModule {
         });
   }
 
-  private Key<HttpServlet> directChangeById() {
-    return key(
-        new HttpServlet() {
-          private static final long serialVersionUID = 1L;
-
-          @Override
-          protected void doGet(HttpServletRequest req, HttpServletResponse rsp) throws IOException {
-            try {
-              String idString = req.getPathInfo();
-              if (idString.endsWith("/")) {
-                idString = idString.substring(0, idString.length() - 1);
-              }
-              Change.Id id = Change.Id.parse(idString);
-              // User accessed Gerrit with /1234, so we have no project yet.
-              // TODO(hiesel) Replace with a preflight request to obtain project before we deprecate
-              // the numeric change id.
-              toGerrit(PageLinks.toChange(null, id), req, rsp);
-            } catch (IllegalArgumentException err) {
-              rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
-            }
-          }
-        });
-  }
-
   private Key<HttpServlet> queryProjectNew() {
     return key(
         new HttpServlet() {

java/com/google/gerrit/server/restapi/change/ChangesCollection.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.restapi.change;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestCollection;
@@ -101,7 +102,8 @@ public class ChangesCollection implements RestCollection<TopLevelResource, Chang
   }
 
   public ChangeResource parse(Change.Id id)
-      throws RestApiException, OrmException, PermissionBackendException, IOException {
+      throws ResourceConflictException, ResourceNotFoundException, OrmException,
+          PermissionBackendException, IOException {
     List<ChangeNotes> notes = changeFinder.find(id);
     if (notes.isEmpty()) {
       throw new ResourceNotFoundException(toIdString(id));
@@ -139,7 +141,7 @@ public class ChangesCollection implements RestCollection<TopLevelResource, Chang
   }
 
   private void checkProjectStatePermitsRead(Project.NameKey project)
-      throws IOException, RestApiException {
+      throws IOException, ResourceNotFoundException, ResourceConflictException {
     ProjectState projectState = projectCache.checkedGet(project);
     if (projectState == null) {
       throw new ResourceNotFoundException("project not found: " + project.get());

javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java

@@ -18,6 +18,7 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.RestResponse;
 import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.reviewdb.client.Change;
 import org.junit.Test;
 
 public class ChangeIdIT extends AbstractDaemonTest {
@@ -92,6 +93,43 @@ public class ChangeIdIT extends AbstractDaemonTest {
     res.assertNotFound();
   }
 
+  @Test
+  public void changeNumberRedirects() throws Exception {
+    // This test tests a redirect that is primarily intended for the UI (though the backend doesn't
+    // really care who the caller is). The redirect rewrites a shorthand change number URL (/123) to
+    // it's canonical long form (/c/project/+/123).
+    int changeId = createChange().getChange().getId().id;
+    RestResponse res = anonymousRestSession.get("/" + changeId);
+    res.assertTemporaryRedirect("/c/" + project.get() + "/+/" + changeId + "/");
+  }
+
+  @Test
+  public void changeNumberRedirectsWithTrailingSlash() throws Exception {
+    int changeId = createChange().getChange().getId().id;
+    RestResponse res = anonymousRestSession.get("/" + changeId + "/");
+    res.assertTemporaryRedirect("/c/" + project.get() + "/+/" + changeId + "/");
+  }
+
+  @Test
+  public void changeNumberOverflowNotFound() throws Exception {
+    RestResponse res = anonymousRestSession.get("/9" + Long.MAX_VALUE);
+    res.assertNotFound();
+  }
+
+  @Test
+  public void unknownChangeNumberNotFound() throws Exception {
+    RestResponse res = anonymousRestSession.get("/10");
+    res.assertNotFound();
+  }
+
+  @Test
+  public void hiddenChangeNotFound() throws Exception {
+    Change.Id changeId = createChange().getChange().getId();
+    gApi.changes().id(changeId.id).setPrivate(true, null);
+    RestResponse res = anonymousRestSession.get("/" + changeId.id);
+    res.assertNotFound();
+  }
+
   private static String changeDetail(String changeId) {
     return "/changes/" + changeId + "/detail";
   }