Implement a redirector for gitweb-style URLs. Add a filter to the host index chain to redirect in the presence of gitweb-style query parameters. Only handles gitweb URLs using the query parameter flavor; see gitweb(1) for details. The intent of this filter is to provide compatibility for preexisting links to old gitweb URLs to be migrated to Gitiles; of course Gitiles itself does not create such URLs. For this reason, use HTTP 301 (Moved Permanently) for the redirect rather than 302 Found. Include simple but not exhaustive tests; the code block in question is far more concise than exhaustive tests would be. Change-Id: Ief7022b7cd419fc0380992071ad6e4a4428a6169
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/GitilesFilter.java b/gitiles-servlet/src/main/java/com/google/gitiles/GitilesFilter.java index 73bc426..57f9306 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/GitilesFilter.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/GitilesFilter.java
@@ -30,6 +30,7 @@ import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.http.server.RepositoryFilter; import org.eclipse.jgit.http.server.glue.MetaFilter; +import org.eclipse.jgit.http.server.glue.ServletBinder; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.transport.ServiceMayNotContinueException; @@ -161,6 +162,7 @@ private RepositoryResolver<HttpServletRequest> resolver; private VisibilityCache visibilityCache; private TimeCache timeCache; + private GitwebRedirectFilter gitwebRedirect; private boolean initialized; GitilesFilter() { @@ -173,13 +175,15 @@ GitilesAccess.Factory accessFactory, final RepositoryResolver<HttpServletRequest> resolver, VisibilityCache visibilityCache, - TimeCache timeCache) { + TimeCache timeCache, + GitwebRedirectFilter gitwebRedirect) { this.config = checkNotNull(config, "config"); this.renderer = renderer; this.urls = urls; this.accessFactory = accessFactory; this.visibilityCache = visibilityCache; this.timeCache = timeCache; + this.gitwebRedirect = gitwebRedirect; if (resolver != null) { this.resolver = wrapResolver(resolver); } @@ -200,9 +204,11 @@ Filter viewFilter = new ViewFilter(accessFactory, urls, visibilityCache); Filter dispatchFilter = new DispatchFilter(filters, servlets); - serveRegex(ROOT_REGEX) - .through(viewFilter) - .through(dispatchFilter); + ServletBinder root = serveRegex(ROOT_REGEX).through(viewFilter); + if (gitwebRedirect != null) { + root.through(gitwebRedirect); + } + root.through(dispatchFilter); serveRegex(REPO_REGEX) .through(repositoryFilter) @@ -281,10 +287,6 @@ } private void setDefaultFields(FilterConfig filterConfig) throws ServletException { - if (renderer != null && urls != null && accessFactory != null && resolver != null - && visibilityCache != null && timeCache != null) { - return; - } Config config; if (this.config != null) { config = this.config; @@ -357,6 +359,11 @@ timeCache = new TimeCache(); } } + if (gitwebRedirect == null) { + if (config.getBoolean("gitiles", null, "redirectGitweb", true)) { + gitwebRedirect = new GitwebRedirectFilter(); + } + } } private static String getBaseGitUrl(Config config) throws ServletException {
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/GitilesServlet.java b/gitiles-servlet/src/main/java/com/google/gitiles/GitilesServlet.java index 38fb295..472f0a8 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/GitilesServlet.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/GitilesServlet.java
@@ -51,9 +51,11 @@ @Nullable GitilesAccess.Factory accessFactory, @Nullable RepositoryResolver<HttpServletRequest> resolver, @Nullable VisibilityCache visibilityCache, - @Nullable TimeCache timeCache) { + @Nullable TimeCache timeCache, + @Nullable GitwebRedirectFilter gitwebRedirect) { super(new GitilesFilter( - config, renderer, urls, accessFactory, resolver, visibilityCache, timeCache)); + config, renderer, urls, accessFactory, resolver, visibilityCache, timeCache, + gitwebRedirect)); } public GitilesServlet() {
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/GitwebRedirectFilter.java b/gitiles-servlet/src/main/java/com/google/gitiles/GitwebRedirectFilter.java new file mode 100644 index 0000000..ff1718b --- /dev/null +++ b/gitiles-servlet/src/main/java/com/google/gitiles/GitwebRedirectFilter.java
@@ -0,0 +1,217 @@ +// Copyright 2012 Google Inc. All Rights Reserved. +// +// 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.gitiles; + +import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.net.HttpHeaders.LOCATION; + +import static javax.servlet.http.HttpServletResponse.SC_GONE; +import static javax.servlet.http.HttpServletResponse.SC_MOVED_PERMANENTLY; + +import com.google.common.base.Charsets; +import com.google.common.base.Objects; +import com.google.common.base.Splitter; +import com.google.common.base.Strings; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.common.collect.LinkedListMultimap; +import com.google.common.collect.ListMultimap; +import com.google.gitiles.GitilesView.InvalidViewException; + +import org.eclipse.jgit.lib.ObjectId; + +import java.io.IOException; +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; +import java.util.List; +import java.util.regex.Pattern; + +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +/** Filter to redirect Gitweb-style URLs to Gitiles-style URLs. */ +public class GitwebRedirectFilter extends AbstractHttpFilter { + public static class TooManyUriParametersException extends IllegalArgumentException { + private static final long serialVersionUID = 1L; + } + + private static final String ARG_SEP = "&|;|%3[Bb]"; + private static final Pattern IS_GITWEB_PATTERN = Pattern.compile("(^|" + ARG_SEP + ")[pa]="); + private static final Splitter ARG_SPLIT = Splitter.on(Pattern.compile(ARG_SEP)); + private static final Splitter VAR_SPLIT = Splitter.on(Pattern.compile("=|%3[Dd]")).limit(2); + private static final int MAX_ARGS = 512; + + private final boolean trimDotGit; + + public GitwebRedirectFilter() { + this(false); + } + + public GitwebRedirectFilter(boolean trimDotGit) { + this.trimDotGit = trimDotGit; + } + + @Override + public void doFilter(HttpServletRequest req, HttpServletResponse res, FilterChain chain) + throws IOException, ServletException { + GitilesView gitwebView = ViewFilter.getView(req); + if (!isGitwebStyleQuery(req)) { + chain.doFilter(req, res); + return; + } + + ListMultimap<String, String> params = parse(req.getQueryString()); + String action = getFirst(params, "a"); + String project = getFirst(params, "p"); + String path = getFirst(params, "f"); + + // According to gitweb's perl source code, the primary parameters are these + // short abbreviated names. When pointing to blob or subtree hash,hashParent + // are the blob or subtree SHA-1s and hashBase,hashParentBase are commits. + // When pointing to commits or tags, hash is the commit/tag. Its messy. + Revision hash = toRevision(getFirst(params, "h")); + Revision hashBase = toRevision(getFirst(params, "hb")); + Revision hashParent = toRevision(getFirst(params, "hp")); + Revision hashParentBase = toRevision(getFirst(params, "hpb")); + + GitilesView.Builder view; + if ("project_index".equals(action)) { + view = GitilesView.hostIndex(); + project = null; + } else if ("summary".equals(action) || "tags".equals(action)) { + view = GitilesView.repositoryIndex(); + } else if (("commit".equals(action) || "tag".equals(action)) && hash != null) { + view = GitilesView.revision().setRevision(hash); + } else if ("log".equals(action) || "shortlog".equals(action)) { + view = GitilesView.log() + .setRevision(Objects.firstNonNull(hash, Revision.HEAD)); + } else if ("tree".equals(action)) { + view = GitilesView.path() + .setRevision(Objects.firstNonNull(hashBase, Revision.HEAD)) + .setTreePath(path); + } else if (("blob".equals(action) || "blob_plain".equals(action)) + && hashBase != null && !path.isEmpty()) { + view = GitilesView.path() + .setRevision(hashBase) + .setTreePath(path); + } else if ("commitdiff".equals(action) && hash != null) { + view = GitilesView.diff() + .setOldRevision(Objects.firstNonNull(hashParent, Revision.NULL)) + .setRevision(hash) + .setTreePath(""); + } else if ("blobdiff".equals(action) && !path.isEmpty() + && hashParentBase != null && hashBase != null) { + view = GitilesView.diff() + .setOldRevision(hashParentBase) + .setRevision(hashBase) + .setTreePath(path); + } else if ("history".equals(action) && !path.isEmpty()) { + view = GitilesView.log() + .setRevision(Objects.firstNonNull(hashBase, Revision.HEAD)) + .setTreePath(path); + } else { + // Gitiles does not provide an RSS feed (a=rss,atom,opml) + // Any other URL is out of date and not valid anymore. + res.sendError(SC_GONE); + return; + } + + if (!Strings.isNullOrEmpty(project)) { + view.setRepositoryName(cleanProjectName(project)); + } + + String url; + try { + url = view.setHostName(gitwebView.getHostName()) + .setServletPath(gitwebView.getServletPath()) + .toUrl(); + } catch (InvalidViewException e) { + res.setStatus(SC_GONE); + return; + } + res.setStatus(SC_MOVED_PERMANENTLY); + res.setHeader(LOCATION, url); + } + + private static boolean isGitwebStyleQuery(HttpServletRequest req) { + String qs = req.getQueryString(); + return qs != null && IS_GITWEB_PATTERN.matcher(qs).find(); + } + + private static String getFirst(ListMultimap<String, String> params, String name) { + return Iterables.getFirst(params.get(checkNotNull(name)), null); + } + + private static Revision toRevision(String rev) { + if (Strings.isNullOrEmpty(rev)) { + return null; + } else if ("HEAD".equals(rev) || rev.startsWith("refs/")) { + return Revision.named(rev); + } else if (ObjectId.isId(rev)) { + return Revision.unpeeled(rev, ObjectId.fromString(rev)); + } else { + return Revision.named(rev); + } + } + + private String cleanProjectName(String p) { + if (p.startsWith("/")) { + p = p.substring(1); + } + if (p.endsWith("/")) { + p = p.substring(0, p.length() - 1); + } + if (trimDotGit && p.endsWith(".git")) { + p = p.substring(0, p.length() - ".git".length()); + } + if (p.endsWith("/")) { + p = p.substring(0, p.length() - 1); + } + return p; + } + + private static ListMultimap<String, String> parse(String query) { + // Parse a gitweb style query string which uses ";" rather than "&" between + // key=value pairs. Some user agents encode ";" as "%3B" and/or "=" as + // "%3D", making a real mess of the query string. Parsing here is + // approximate because ; shouldn't be the pair separator and %3B might have + // been a ; within a value. + // This is why people shouldn't use gitweb. + ListMultimap<String, String> map = LinkedListMultimap.create(); + for (String piece : ARG_SPLIT.split(query)) { + if (map.size() > MAX_ARGS) { + throw new TooManyUriParametersException(); + } + + List<String> pair = ImmutableList.copyOf(VAR_SPLIT.split(piece)); + if (pair.size() == 2) { + map.put(decode(pair.get(0)), decode(pair.get(1))); + } else { // no equals sign + map.put(piece, ""); + } + } + return map; + } + + private static String decode(String str) { + try { + return URLDecoder.decode(str, Charsets.UTF_8.name()); + } catch (UnsupportedEncodingException e) { + return str; + } + } +}
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/FakeHttpServletRequest.java b/gitiles-servlet/src/test/java/com/google/gitiles/FakeHttpServletRequest.java index 04d9ef9..7b70400 100644 --- a/gitiles-servlet/src/test/java/com/google/gitiles/FakeHttpServletRequest.java +++ b/gitiles-servlet/src/test/java/com/google/gitiles/FakeHttpServletRequest.java
@@ -318,7 +318,7 @@ @Override public String getQueryString() { - return null; + return GitilesView.paramsToString(parameters); } @Override
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/FakeHttpServletResponse.java b/gitiles-servlet/src/test/java/com/google/gitiles/FakeHttpServletResponse.java index 5249e41..a6c90e3 100644 --- a/gitiles-servlet/src/test/java/com/google/gitiles/FakeHttpServletResponse.java +++ b/gitiles-servlet/src/test/java/com/google/gitiles/FakeHttpServletResponse.java
@@ -16,9 +16,11 @@ import static com.google.common.base.Charsets.UTF_8; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import com.google.common.base.Charsets; +import com.google.common.collect.Iterables; import com.google.common.collect.LinkedListMultimap; import com.google.common.collect.ListMultimap; import com.google.common.net.HttpHeaders; @@ -252,4 +254,8 @@ public String getActualBodyString() { return RawParseUtils.decode(getActualBody()); } + + public String getHeader(String name) { + return Iterables.getFirst(headers.get(checkNotNull(name)), null); + } }
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/GitwebRedirectFilterTest.java b/gitiles-servlet/src/test/java/com/google/gitiles/GitwebRedirectFilterTest.java new file mode 100644 index 0000000..3f2645a --- /dev/null +++ b/gitiles-servlet/src/test/java/com/google/gitiles/GitwebRedirectFilterTest.java
@@ -0,0 +1,136 @@ +// Copyright 2012 Google Inc. All Rights Reserved. +// +// 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.gitiles; + +import static com.google.gitiles.FakeHttpServletRequest.SERVLET_PATH; +import static com.google.gitiles.TestGitilesUrls.HOST_NAME; + +import static javax.servlet.http.HttpServletResponse.SC_GONE; +import static javax.servlet.http.HttpServletResponse.SC_MOVED_PERMANENTLY; + +import com.google.common.net.HttpHeaders; + +import junit.framework.TestCase; + +import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.storage.dfs.DfsRepository; +import org.eclipse.jgit.storage.dfs.DfsRepositoryDescription; +import org.eclipse.jgit.storage.dfs.InMemoryRepository; + +import javax.servlet.http.HttpServletRequest; + +/** Tests for gitweb redirector. */ +public class GitwebRedirectFilterTest extends TestCase { + private TestRepository<DfsRepository> repo; + private GitilesServlet servlet; + + @Override + protected void setUp() throws Exception { + repo = new TestRepository<DfsRepository>( + new InMemoryRepository(new DfsRepositoryDescription("test"))); + servlet = TestGitilesServlet.create(repo); + } + + private void assertRedirectsTo(String expectedLocation, HttpServletRequest req) throws Exception { + FakeHttpServletResponse res = new FakeHttpServletResponse(); + servlet.service(req, res); + assertEquals(SC_MOVED_PERMANENTLY, res.getStatus()); + assertEquals(expectedLocation, res.getHeader(HttpHeaders.LOCATION)); + } + + private void assertGone(HttpServletRequest req) throws Exception { + FakeHttpServletResponse res = new FakeHttpServletResponse(); + servlet.service(req, res); + assertEquals(SC_GONE, res.getStatus()); + } + + private static FakeHttpServletRequest newRequest(String qs) { + FakeHttpServletRequest req = FakeHttpServletRequest.newRequest(); + req.setPathInfo("/"); + req.setQueryString(qs); + return req; + } + + public void testHostIndex() throws Exception { + assertRedirectsTo( + GitilesView.hostIndex() + .setHostName(HOST_NAME) + .setServletPath(SERVLET_PATH) + .toUrl(), + newRequest("a=project_index")); + } + + public void testRepositoryIndex() throws Exception { + assertGone(newRequest("a=summary")); + assertRedirectsTo( + GitilesView.repositoryIndex() + .setHostName(HOST_NAME) + .setServletPath(SERVLET_PATH) + .setRepositoryName("test") + .toUrl(), + newRequest("a=summary;p=test")); + } + + public void testShow() throws Exception { + assertGone(newRequest("a=commit")); + assertGone(newRequest("a=commit;p=test")); + RevCommit commit = repo.branch("refs/heads/master").commit().create(); + assertRedirectsTo( + GitilesView.revision() + .setHostName(HOST_NAME) + .setServletPath(SERVLET_PATH) + .setRepositoryName("test") + .setRevision(commit) + .toUrl(), + newRequest("a=commit;p=test&h=" + ObjectId.toString(commit))); + } + + public void testNoStripDotGit() throws Exception { + assertRedirectsTo( + GitilesView.repositoryIndex() + .setHostName(HOST_NAME) + .setServletPath(SERVLET_PATH) + .setRepositoryName("test.git") + .toUrl(), + newRequest("a=summary;p=test.git")); + assertRedirectsTo( + GitilesView.repositoryIndex() + .setHostName(HOST_NAME) + .setServletPath(SERVLET_PATH) + .setRepositoryName("test") + .toUrl(), + newRequest("a=summary;p=test")); + } + + public void testStripDotGit() throws Exception { + servlet = TestGitilesServlet.create(repo, new GitwebRedirectFilter(true)); + assertRedirectsTo( + GitilesView.repositoryIndex() + .setHostName(HOST_NAME) + .setServletPath(SERVLET_PATH) + .setRepositoryName("test") + .toUrl(), + newRequest("a=summary;p=test.git")); + assertRedirectsTo( + GitilesView.repositoryIndex() + .setHostName(HOST_NAME) + .setServletPath(SERVLET_PATH) + .setRepositoryName("test") + .toUrl(), + newRequest("a=summary;p=test")); + } +}
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/TestGitilesAccess.java b/gitiles-servlet/src/test/java/com/google/gitiles/TestGitilesAccess.java index 0b31e24..b9607ae 100644 --- a/gitiles-servlet/src/test/java/com/google/gitiles/TestGitilesAccess.java +++ b/gitiles-servlet/src/test/java/com/google/gitiles/TestGitilesAccess.java
@@ -16,6 +16,8 @@ import static com.google.common.base.Preconditions.checkNotNull; +import com.google.common.collect.ImmutableMap; + import org.eclipse.jgit.storage.dfs.DfsRepository; import java.util.Map; @@ -36,9 +38,13 @@ return new GitilesAccess() { @Override public Map<String, RepositoryDescription> listRepositories(Set<String> branches) { - // TODO(dborowitz): Implement this, using the DfsRepositoryDescriptions to - // get the repository names. - throw new UnsupportedOperationException(); + if (branches != null && !branches.isEmpty()) { + throw new UnsupportedOperationException("branches set not yet supported"); + } + RepositoryDescription desc = new RepositoryDescription(); + desc.name = repo.getDescription().getRepositoryName(); + desc.cloneUrl = TestGitilesUrls.URLS.getBaseGitUrl(req) + "/" + desc.name; + return ImmutableMap.of(desc.name, desc); } @Override
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/TestGitilesServlet.java b/gitiles-servlet/src/test/java/com/google/gitiles/TestGitilesServlet.java index b3cb72b..ee7b6ac 100644 --- a/gitiles-servlet/src/test/java/com/google/gitiles/TestGitilesServlet.java +++ b/gitiles-servlet/src/test/java/com/google/gitiles/TestGitilesServlet.java
@@ -33,6 +33,12 @@ /** Static utility methods for creating {@link GitilesServlet}s for testing. */ public class TestGitilesServlet { + /** @see #create(TestRepository) */ + public static GitilesServlet create(final TestRepository<DfsRepository> repo) + throws ServletException { + return create(repo, new GitwebRedirectFilter()); + } + /** * Create a servlet backed by a single test repository. * <p> @@ -43,10 +49,11 @@ * to the servlet's {@code service} method to test. * * @param repo the test repo backing the servlet. + * @param gitwebRedirect optional redirect filter for gitweb URLs. * @return a servlet. */ - public static GitilesServlet create(final TestRepository<DfsRepository> repo) - throws ServletException { + public static GitilesServlet create(final TestRepository<DfsRepository> repo, + GitwebRedirectFilter gitwebRedirect) throws ServletException { final String repoName = repo.getRepository().getDescription().getRepositoryName(); GitilesServlet servlet = @@ -62,7 +69,7 @@ } return repo.getRepository(); } - }, null, null); + }, null, null, gitwebRedirect); servlet.init(new ServletConfig() { @Override
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/TestGitilesUrls.java b/gitiles-servlet/src/test/java/com/google/gitiles/TestGitilesUrls.java index f8a0883..14ccef7 100644 --- a/gitiles-servlet/src/test/java/com/google/gitiles/TestGitilesUrls.java +++ b/gitiles-servlet/src/test/java/com/google/gitiles/TestGitilesUrls.java
@@ -19,20 +19,21 @@ /** {@link GitilesUrls} for testing. */ public class TestGitilesUrls implements GitilesUrls { public static final GitilesUrls URLS = new TestGitilesUrls(); + public static final String HOST_NAME = "test-host"; @Override public String getHostName(HttpServletRequest req) { - return "test-host"; + return HOST_NAME; } @Override public String getBaseGitUrl(HttpServletRequest req) { - return "git://test-host/foo"; + return "git://" + HOST_NAME + "/foo"; } @Override public String getBaseGerritUrl(HttpServletRequest req) { - return "http://test-host-review/foo/"; + return "http://" + HOST_NAME + "-review/foo/"; } private TestGitilesUrls() {