diff --git a/gerrit-httpd/BUCK b/gerrit-httpd/BUCK index 3f81d86174..3085054087 100644 --- a/gerrit-httpd/BUCK +++ b/gerrit-httpd/BUCK @@ -59,6 +59,7 @@ java_test( '//gerrit-server:server', '//gerrit-util-http:http', '//gerrit-util-http:testutil', + '//lib:jimfs', '//lib:junit', '//lib:gson', '//lib:gwtorm', @@ -70,6 +71,7 @@ java_test( '//lib/guice:guice-servlet', '//lib/jgit:jgit', '//lib/jgit:junit', + '//lib/joda:joda-time', ], source_under_test = [':httpd'], # TODO(sop) Remove after Buck supports Eclipse diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java index 507bc59812..6f7ad519a9 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java @@ -26,6 +26,7 @@ import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND; import static javax.servlet.http.HttpServletResponse.SC_NOT_MODIFIED; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.CharMatcher; import com.google.common.cache.Cache; import com.google.common.collect.ImmutableMap; @@ -96,10 +97,18 @@ public abstract class ResourceServlet extends HttpServlet { private final Cache cache; private final boolean refresh; + private final int cacheFileSizeLimitBytes; protected ResourceServlet(Cache cache, boolean refresh) { + this(cache, refresh, CACHE_FILE_SIZE_LIMIT_BYTES); + } + + @VisibleForTesting + ResourceServlet(Cache cache, boolean refresh, + int cacheFileSizeLimitBytes) { this.cache = checkNotNull(cache, "cache"); this.refresh = refresh; + this.cacheFileSizeLimitBytes = cacheFileSizeLimitBytes; } /** @@ -214,7 +223,7 @@ public abstract class ResourceServlet extends HttpServlet { private boolean maybeStream(Path p, HttpServletRequest req, HttpServletResponse rsp) throws IOException { try { - if (Files.size(p) < CACHE_FILE_SIZE_LIMIT_BYTES) { + if (Files.size(p) < cacheFileSizeLimitBytes) { return false; } } catch (NoSuchFileException e) { @@ -294,7 +303,12 @@ public abstract class ResourceServlet extends HttpServlet { } boolean isStale(Path p, ResourceServlet rs) throws IOException { - FileTime t = rs.getLastModifiedTime(p); + FileTime t; + try { + t = rs.getLastModifiedTime(p); + } catch (NoSuchFileException e) { + return this != NOT_FOUND; + } return t.toMillis() == 0 || lastModified.toMillis() == 0 || !lastModified.equals(t); diff --git a/gerrit-httpd/src/test/java/com/google/gerrit/httpd/raw/ResourceServletTest.java b/gerrit-httpd/src/test/java/com/google/gerrit/httpd/raw/ResourceServletTest.java new file mode 100644 index 0000000000..2ddd4eef7a --- /dev/null +++ b/gerrit-httpd/src/test/java/com/google/gerrit/httpd/raw/ResourceServletTest.java @@ -0,0 +1,337 @@ +// Copyright (C) 2015 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.raw; + +import static com.google.common.truth.Truth.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; +import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND; +import static javax.servlet.http.HttpServletResponse.SC_OK; + +import com.google.common.base.CharMatcher; +import com.google.common.base.Strings; +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; +import com.google.common.collect.Lists; +import com.google.common.io.ByteStreams; +import com.google.common.jimfs.Configuration; +import com.google.common.jimfs.Jimfs; +import com.google.gerrit.httpd.raw.ResourceServlet.Resource; +import com.google.gerrit.util.http.testutil.FakeHttpServletRequest; +import com.google.gerrit.util.http.testutil.FakeHttpServletResponse; + +import org.joda.time.format.ISODateTimeFormat; +import org.junit.Before; +import org.junit.Test; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.FileSystem; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.FileTime; +import java.util.concurrent.atomic.AtomicLong; +import java.util.zip.GZIPInputStream; + +public class ResourceServletTest { + private static Cache newCache(int size) { + return CacheBuilder.newBuilder() + .maximumSize(size) + .recordStats() + .build(); + } + + private static class Servlet extends ResourceServlet { + private static final long serialVersionUID = 1L; + + private final FileSystem fs; + + private Servlet(FileSystem fs, Cache cache, + boolean refresh) { + super(cache, refresh); + this.fs = fs; + } + + private Servlet(FileSystem fs, Cache cache, + boolean refresh, int cacheFileSizeLimitBytes) { + super(cache, refresh, cacheFileSizeLimitBytes); + this.fs = fs; + } + + @Override + protected Path getResourcePath(String pathInfo) { + return fs.getPath("/" + CharMatcher.is('/').trimLeadingFrom(pathInfo)); + } + } + + private FileSystem fs; + private AtomicLong ts; + + @Before + public void setUp() { + fs = Jimfs.newFileSystem(Configuration.unix()); + ts = new AtomicLong(ISODateTimeFormat.dateTime().parseMillis( + "2010-01-30T12:00:00.000-08:00")); + } + + @Test + public void notFoundWithoutRefresh() throws Exception { + Cache cache = newCache(1); + Servlet servlet = new Servlet(fs, cache, false); + + FakeHttpServletResponse res = new FakeHttpServletResponse(); + servlet.doGet(request("/notfound"), res); + assertThat(res.getStatus()).isEqualTo(SC_NOT_FOUND); + assertNotCacheable(res); + assertCacheHits(cache, 0, 1); + + res = new FakeHttpServletResponse(); + servlet.doGet(request("/notfound"), res); + assertThat(res.getStatus()).isEqualTo(SC_NOT_FOUND); + assertNotCacheable(res); + assertCacheHits(cache, 1, 1); + } + + @Test + public void notFoundWithRefresh() throws Exception { + Cache cache = newCache(1); + Servlet servlet = new Servlet(fs, cache, true); + + FakeHttpServletResponse res = new FakeHttpServletResponse(); + servlet.doGet(request("/notfound"), res); + assertThat(res.getStatus()).isEqualTo(SC_NOT_FOUND); + assertNotCacheable(res); + assertCacheHits(cache, 0, 1); + + res = new FakeHttpServletResponse(); + servlet.doGet(request("/notfound"), res); + assertThat(res.getStatus()).isEqualTo(SC_NOT_FOUND); + assertNotCacheable(res); + assertCacheHits(cache, 1, 1); + } + + @Test + public void smallFileWithRefresh() throws Exception { + Cache cache = newCache(1); + Servlet servlet = new Servlet(fs, cache, true); + + writeFile("/foo", "foo1"); + FakeHttpServletResponse res = new FakeHttpServletResponse(); + servlet.doGet(request("/foo"), res); + assertThat(res.getStatus()).isEqualTo(SC_OK); + assertThat(res.getActualBodyString()).isEqualTo("foo1"); + assertCacheable(res, true); + assertHasETag(res); + // Miss on getIfPresent, miss on get. + assertCacheHits(cache, 0, 2); + + res = new FakeHttpServletResponse(); + servlet.doGet(request("/foo"), res); + assertThat(res.getStatus()).isEqualTo(SC_OK); + assertThat(res.getActualBodyString()).isEqualTo("foo1"); + assertCacheable(res, true); + assertHasETag(res); + assertCacheHits(cache, 1, 2); + + writeFile("/foo", "foo2"); + res = new FakeHttpServletResponse(); + servlet.doGet(request("/foo"), res); + assertThat(res.getStatus()).isEqualTo(SC_OK); + assertThat(res.getActualBodyString()).isEqualTo("foo2"); + assertCacheable(res, true); + assertHasETag(res); + // Hit, invalidate, miss. + assertCacheHits(cache, 2, 3); + } + + @Test + public void smallFileWithoutRefresh() throws Exception { + Cache cache = newCache(1); + Servlet servlet = new Servlet(fs, cache, false); + + writeFile("/foo", "foo1"); + FakeHttpServletResponse res = new FakeHttpServletResponse(); + servlet.doGet(request("/foo"), res); + assertThat(res.getStatus()).isEqualTo(SC_OK); + assertThat(res.getActualBodyString()).isEqualTo("foo1"); + assertCacheable(res, false); + assertHasETag(res); + // Miss on getIfPresent, miss on get. + assertCacheHits(cache, 0, 2); + + res = new FakeHttpServletResponse(); + servlet.doGet(request("/foo"), res); + assertThat(res.getStatus()).isEqualTo(SC_OK); + assertThat(res.getActualBodyString()).isEqualTo("foo1"); + assertCacheable(res, false); + assertHasETag(res); + assertCacheHits(cache, 1, 2); + + writeFile("/foo", "foo2"); + res = new FakeHttpServletResponse(); + servlet.doGet(request("/foo"), res); + assertThat(res.getStatus()).isEqualTo(SC_OK); + assertThat(res.getActualBodyString()).isEqualTo("foo1"); + assertCacheable(res, false); + assertHasETag(res); + assertCacheHits(cache, 2, 2); + } + + @Test + public void verySmallFileDoesntBotherWithGzip() throws Exception { + Cache cache = newCache(1); + Servlet servlet = new Servlet(fs, cache, true); + writeFile("/foo", "foo1"); + + FakeHttpServletRequest req = request("/foo") + .addHeader("Accept-Encoding", "gzip"); + FakeHttpServletResponse res = new FakeHttpServletResponse(); + servlet.doGet(req, res); + assertThat(res.getStatus()).isEqualTo(SC_OK); + assertThat(res.getHeader("Content-Encoding")).isNull(); + assertThat(res.getActualBodyString()).isEqualTo("foo1"); + assertHasETag(res); + assertCacheable(res, true); + } + + @Test + public void smallFileWithGzip() throws Exception { + Cache cache = newCache(1); + Servlet servlet = new Servlet(fs, cache, true); + String content = Strings.repeat("a", 100); + writeFile("/foo", content); + + FakeHttpServletRequest req = request("/foo") + .addHeader("Accept-Encoding", "gzip"); + FakeHttpServletResponse res = new FakeHttpServletResponse(); + servlet.doGet(req, res); + assertThat(res.getStatus()).isEqualTo(SC_OK); + assertThat(res.getHeader("Content-Encoding")).isEqualTo("gzip"); + assertThat(gunzip(res.getActualBody())).isEqualTo(content); + assertHasETag(res); + assertCacheable(res, true); + } + + @Test + public void largeFileBypassesCacheRegardlessOfRefreshParamter() + throws Exception { + for (boolean refresh : Lists.newArrayList(true, false)) { + Cache cache = newCache(1); + Servlet servlet = new Servlet(fs, cache, refresh, 3); + + writeFile("/foo", "foo1"); + FakeHttpServletResponse res = new FakeHttpServletResponse(); + servlet.doGet(request("/foo"), res); + assertThat(res.getStatus()).isEqualTo(SC_OK); + assertThat(res.getActualBodyString()).isEqualTo("foo1"); + assertThat(res.getHeader("Last-Modified")).isNotNull(); + assertCacheable(res, refresh); + assertHasLastModified(res); + assertCacheHits(cache, 0, 1); + + writeFile("/foo", "foo1"); + res = new FakeHttpServletResponse(); + servlet.doGet(request("/foo"), res); + assertThat(res.getStatus()).isEqualTo(SC_OK); + assertThat(res.getActualBodyString()).isEqualTo("foo1"); + assertThat(res.getHeader("Last-Modified")).isNotNull(); + assertCacheable(res, refresh); + assertHasLastModified(res); + assertCacheHits(cache, 0, 2); + + writeFile("/foo", "foo2"); + res = new FakeHttpServletResponse(); + servlet.doGet(request("/foo"), res); + assertThat(res.getStatus()).isEqualTo(SC_OK); + assertThat(res.getActualBodyString()).isEqualTo("foo2"); + assertThat(res.getHeader("Last-Modified")).isNotNull(); + assertCacheable(res, refresh); + assertHasLastModified(res); + assertCacheHits(cache, 0, 3); + } + } + + @Test + public void largeFileWithGzip() throws Exception { + Cache cache = newCache(1); + Servlet servlet = new Servlet(fs, cache, true, 3); + String content = Strings.repeat("a", 100); + writeFile("/foo", content); + + FakeHttpServletRequest req = request("/foo") + .addHeader("Accept-Encoding", "gzip"); + FakeHttpServletResponse res = new FakeHttpServletResponse(); + servlet.doGet(req, res); + assertThat(res.getStatus()).isEqualTo(SC_OK); + assertThat(res.getHeader("Content-Encoding")).isEqualTo("gzip"); + assertThat(gunzip(res.getActualBody())).isEqualTo(content); + assertHasLastModified(res); + assertCacheable(res, true); + } + + // TODO(dborowitz): Check MIME type. + // TODO(dborowitz): Test that JS is not gzipped. + // TODO(dborowitz): Test ?e parameter. + // TODO(dborowitz): Test If-None-Match behavior. + // TODO(dborowitz): Test If-Modified-Since behavior. + + private void writeFile(String path, String content) throws Exception { + Files.write(fs.getPath(path), content.getBytes(UTF_8)); + Files.setLastModifiedTime( + fs.getPath(path), FileTime.fromMillis(ts.getAndIncrement())); + } + + private static void assertCacheHits(Cache cache, int hits, int misses) { + assertThat(cache.stats().hitCount()).named("hits").isEqualTo(hits); + assertThat(cache.stats().missCount()).named("misses").isEqualTo(misses); + } + + private static void assertCacheable(FakeHttpServletResponse res, + boolean revalidate) { + String header = res.getHeader("Cache-Control").toLowerCase(); + assertThat(header).contains("public"); + if (revalidate) { + assertThat(header).contains("must-revalidate"); + } else { + assertThat(header).doesNotContain("must-revalidate"); + } + } + + private static void assertHasLastModified(FakeHttpServletResponse res) { + assertThat(res.getHeader("Last-Modified")).isNotNull(); + assertThat(res.getHeader("ETag")).isNull(); + } + + private static void assertHasETag(FakeHttpServletResponse res) { + assertThat(res.getHeader("ETag")).isNotNull(); + assertThat(res.getHeader("Last-Modified")).isNull(); + } + + private static void assertNotCacheable(FakeHttpServletResponse res) { + assertThat(res.getHeader("Cache-Control")).contains("no-cache"); + assertThat(res.getHeader("ETag")).isNull(); + assertThat(res.getHeader("Last-Modified")).isNull(); + } + + private static FakeHttpServletRequest request(String path) { + return new FakeHttpServletRequest().setPathInfo(path); + } + + private static String gunzip(byte[] data) throws Exception { + try (InputStream in = new GZIPInputStream(new ByteArrayInputStream(data))) { + return new String(ByteStreams.toByteArray(in), UTF_8); + } + } +} diff --git a/gerrit-util-http/BUCK b/gerrit-util-http/BUCK index 4c3816b46c..e9b2a3da7e 100644 --- a/gerrit-util-http/BUCK +++ b/gerrit-util-http/BUCK @@ -14,6 +14,7 @@ java_library( '//gerrit-extension-api:api', '//lib:guava', '//lib:servlet-api-3_1', + '//lib/httpcomponents:httpclient', '//lib/jgit:jgit', ], visibility = ['PUBLIC'], diff --git a/gerrit-util-http/src/test/java/com/google/gerrit/util/http/testutil/FakeHttpServletRequest.java b/gerrit-util-http/src/test/java/com/google/gerrit/util/http/testutil/FakeHttpServletRequest.java index 04c7b48637..3991b95051 100644 --- a/gerrit-util-http/src/test/java/com/google/gerrit/util/http/testutil/FakeHttpServletRequest.java +++ b/gerrit-util-http/src/test/java/com/google/gerrit/util/http/testutil/FakeHttpServletRequest.java @@ -26,6 +26,8 @@ import com.google.common.collect.ListMultimap; import com.google.common.collect.Maps; import com.google.gerrit.extensions.restapi.Url; +import org.apache.http.client.utils.DateUtils; + import java.io.BufferedReader; import java.io.UnsupportedEncodingException; import java.net.URLDecoder; @@ -267,7 +269,8 @@ public class FakeHttpServletRequest implements HttpServletRequest { @Override public long getDateHeader(String name) { - throw new UnsupportedOperationException(); + String v = getHeader(name); + return v != null ? DateUtils.parseDate(v).getTime() : 0; } @Override @@ -301,7 +304,7 @@ public class FakeHttpServletRequest implements HttpServletRequest { } public FakeHttpServletRequest setPathInfo(String path) { - this.path = checkNotNull(path); + this.path = path; return this; } @@ -477,4 +480,9 @@ public class FakeHttpServletRequest implements HttpServletRequest { Class httpUpgradeHandlerClass) { throw new UnsupportedOperationException(); } + + public FakeHttpServletRequest addHeader(String name, String value) { + headers.put(name, value); + return this; + } } diff --git a/gerrit-util-http/src/test/java/com/google/gerrit/util/http/testutil/FakeHttpServletResponse.java b/gerrit-util-http/src/test/java/com/google/gerrit/util/http/testutil/FakeHttpServletResponse.java index 6201bb0e48..442c4744b2 100644 --- a/gerrit-util-http/src/test/java/com/google/gerrit/util/http/testutil/FakeHttpServletResponse.java +++ b/gerrit-util-http/src/test/java/com/google/gerrit/util/http/testutil/FakeHttpServletResponse.java @@ -85,12 +85,10 @@ public class FakeHttpServletResponse implements HttpServletResponse { public synchronized ServletOutputStream getOutputStream() { checkState(writer == null, "getWriter() already called"); if (outputStream == null) { - final PrintWriter osWriter = new PrintWriter(actualBody); outputStream = new ServletOutputStream() { @Override public void write(int c) throws IOException { - osWriter.write(c); - osWriter.flush(); + actualBody.write(c); } @Override @@ -150,13 +148,13 @@ public class FakeHttpServletResponse implements HttpServletResponse { @Override public void setContentLengthLong(long length) { headers.removeAll(HttpHeaders.CONTENT_LENGTH); - headers.put(HttpHeaders.CONTENT_LENGTH, Long.toString(length)); + addHeader(HttpHeaders.CONTENT_LENGTH, Long.toString(length)); } @Override public void setContentType(String type) { headers.removeAll(HttpHeaders.CONTENT_TYPE); - headers.put(HttpHeaders.CONTENT_TYPE, type); + addHeader(HttpHeaders.CONTENT_TYPE, type); } @Override @@ -176,17 +174,17 @@ public class FakeHttpServletResponse implements HttpServletResponse { @Override public void addHeader(String name, String value) { - headers.put(name, value); + headers.put(name.toLowerCase(), value); } @Override public void addIntHeader(String name, int value) { - headers.put(name, Integer.toString(value)); + addHeader(name, Integer.toString(value)); } @Override public boolean containsHeader(String name) { - return headers.containsKey(name); + return headers.containsKey(name.toLowerCase()); } @Override @@ -237,13 +235,13 @@ public class FakeHttpServletResponse implements HttpServletResponse { @Override public void setHeader(String name, String value) { - headers.removeAll(name); + headers.removeAll(name.toLowerCase()); addHeader(name, value); } @Override public void setIntHeader(String name, int value) { - headers.removeAll(name); + headers.removeAll(name.toLowerCase()); addIntHeader(name, value); } @@ -267,7 +265,8 @@ public class FakeHttpServletResponse implements HttpServletResponse { @Override public String getHeader(String name) { - return Iterables.getFirst(headers.get(checkNotNull(name)), null); + return Iterables.getFirst( + headers.get(checkNotNull(name.toLowerCase())), null); } @Override @@ -277,7 +276,7 @@ public class FakeHttpServletResponse implements HttpServletResponse { @Override public Collection getHeaders(String name) { - return headers.get(checkNotNull(name)); + return headers.get(checkNotNull(name.toLowerCase())); } public byte[] getActualBody() { diff --git a/lib/BUCK b/lib/BUCK index a92f910117..c635bbb762 100644 --- a/lib/BUCK +++ b/lib/BUCK @@ -184,6 +184,16 @@ maven_jar( license = 'protobuf', ) +# Test-only dependencies below. + +maven_jar( + name = 'jimfs', + id = 'com.google.jimfs:jimfs:1.0', + sha1 = 'edd65a2b792755f58f11134e76485a928aab4c97', + license = 'DO_NOT_DISTRIBUTE', + deps = [':guava'], +) + maven_jar( name = 'junit', id = 'junit:junit:4.11',