From 4adf83cb433b4a63cae0fcb8f5af6d16801336fd Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Fri, 19 Oct 2018 15:16:21 +0200 Subject: [PATCH] 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 --- .../gerrit/acceptance/AbstractDaemonTest.java | 2 + .../gerrit/acceptance/RestResponse.java | 7 ++ .../httpd/NumericChangeIdRedirectServlet.java | 71 +++++++++++++++++++ java/com/google/gerrit/httpd/UrlModule.java | 26 +------ .../restapi/change/ChangesCollection.java | 6 +- .../acceptance/rest/change/ChangeIdIT.java | 38 ++++++++++ 6 files changed, 123 insertions(+), 27 deletions(-) create mode 100644 java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 6332188454..6eaf46339b 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/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(); diff --git a/java/com/google/gerrit/acceptance/RestResponse.java b/java/com/google/gerrit/acceptance/RestResponse.java index da082150e6..e8de5c6220 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 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); + } } diff --git a/java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java b/java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java new file mode 100644 index 0000000000..164f957455 --- /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 f337718c24..6620c443ba 100644 --- a/java/com/google/gerrit/httpd/UrlModule.java +++ b/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 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 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 561c27c306..30d93be82c 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 @@ 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 notes = changeFinder.find(id); if (notes.isEmpty()) { throw new ResourceNotFoundException(toIdString(id)); @@ -139,7 +141,7 @@ public class ChangesCollection implements RestCollection