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
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 63321884..6eaf463 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -269,6 +269,7 @@
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 @@
adminRestSession = new RestSession(server, admin);
userRestSession = new RestSession(server, user);
+ anonymousRestSession = new RestSession(server, null);
initSsh();
diff --git a/java/com/google/gerrit/acceptance/RestResponse.java b/java/com/google/gerrit/acceptance/RestResponse.java
index da08215..e8de5c6 100644
--- a/java/com/google/gerrit/acceptance/RestResponse.java
+++ b/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 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 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);
+ }
}
diff --git a/java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java b/java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java
new file mode 100644
index 0000000..164f957
--- /dev/null
+++ b/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);
+ }
+}
diff --git a/java/com/google/gerrit/httpd/UrlModule.java b/java/com/google/gerrit/httpd/UrlModule.java
index f337718..6620c44 100644
--- a/java/com/google/gerrit/httpd/UrlModule.java
+++ b/java/com/google/gerrit/httpd/UrlModule.java
@@ -87,7 +87,7 @@
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 @@
});
}
- 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() {
diff --git a/java/com/google/gerrit/server/restapi/change/ChangesCollection.java b/java/com/google/gerrit/server/restapi/change/ChangesCollection.java
index 561c27c..30d93be 100644
--- a/java/com/google/gerrit/server/restapi/change/ChangesCollection.java
+++ b/java/com/google/gerrit/server/restapi/change/ChangesCollection.java
@@ -17,6 +17,7 @@
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 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 @@
}
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());
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java
index a2ad7fc..3f1608c 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java
@@ -18,6 +18,7 @@
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 @@
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";
}
