Merge pull request #362 from authchir/master

jknack · jknack · commit 452e6dfdb8a0 · 2016-05-03T21:52:38.000-03:00
Refactor `AssetHandler` according to issue #356
diff --git a/coverage-report/src/test/java/org/jooby/issues/Issue356.java b/coverage-report/src/test/java/org/jooby/issues/Issue356.java
@@ -0,0 +1,65 @@
+package org.jooby.issues;
+
+import java.time.Duration;
+
+import org.jooby.handlers.AssetHandler;
+import org.jooby.test.ServerFeature;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class Issue356 extends ServerFeature {
+  {
+    assets("/assets/file.css", new AssetHandler("/")
+      .etag(false).lastModified(false).maxAge(Duration.ofDays(1)));
+
+    assets("/assets/favicon.ico", new AssetHandler("/")
+      .etag(false).lastModified(true).maxAge(Duration.ofDays(14)));
+
+    assets("/assets/file.js", new AssetHandler("/")
+      .etag(true).lastModified(false).maxAge(Duration.ofDays(2)));
+
+    assets("/assets/empty.css", new AssetHandler("/")
+      .etag(true).lastModified(true).maxAge(Duration.ofDays(7)));
+  }
+
+  @Test
+  public void etag() throws Exception {
+    request().get("/assets/file.css").execute().header("ETag", Assert::assertNull);
+    request().get("/assets/favicon.ico").execute().header("ETag", Assert::assertNull);
+    request().get("/assets/file.js").execute().header("ETag", Assert::assertNotNull);
+    request().get("/assets/empty.css").execute().header("ETag", Assert::assertNotNull);
+  }
+
+  @Test
+  public void lastModified() throws Exception {
+    request().get("/assets/file.css").execute().header("Last-Modified", Assert::assertNull);
+    request().get("/assets/favicon.ico").execute().header("Last-Modified", Assert::assertNotNull);
+    request().get("/assets/file.js").execute().header("Last-Modified", Assert::assertNull);
+    request().get("/assets/empty.css").execute().header("Last-Modified", Assert::assertNotNull);
+  }
+
+  @Test
+  public void maxAge() throws Exception {
+    request().get("/assets/file.css").execute().header("Cache-Control", value -> {
+      assertNotNull(value);
+      assertTrue(value.contains("max-age=86400"));
+    });
+    request().get("/assets/favicon.ico").execute().header("Cache-Control", value -> {
+      assertNotNull(value);
+      assertTrue(value.contains("max-age=1209600"));
+    });
+    request().get("/assets/file.js").execute().header("Cache-Control", value -> {
+      assertNotNull(value);
+      assertTrue(value.contains("max-age=172800"));
+    });
+    request().get("/assets/empty.css").execute().header("Cache-Control", value -> {
+      assertNotNull(value);
+      assertTrue(value.contains("max-age=604800"));
+    });
+  }
+}
diff --git a/jooby-assets/src/main/java/org/jooby/assets/Assets.java b/jooby-assets/src/main/java/org/jooby/assets/Assets.java
@@ -18,7 +18,8 @@
  */
 package org.jooby.assets;
 
-import java.util.concurrent.TimeUnit;
+import java.time.Duration;
+import java.util.Optional;
 
 import org.jooby.Env;
 import org.jooby.Jooby;
@@ -266,8 +267,11 @@ public void configure(final Env env, final Config config, final Binder binder) {
           : new AssetHandler("/")
               .etag(conf.getBoolean("assets.etag"))
               .cdn(conf.getString("assets.cdn"))
-              .lastModified(conf.getBoolean("assets.lastModified"))
-              .maxAge(conf.getDuration("assets.cache.maxAge", TimeUnit.SECONDS));
+              .lastModified(conf.getBoolean("assets.lastModified"));
+
+      if (conf.hasPath("assets.cache.maxAge")) {
+        handler.maxAge(Duration.parse(conf.getString("assets.cache.maxAge")));
+      }
 
       compiler.patterns().forEach(
           pattern -> routes.addBinding()
diff --git a/jooby-assets/src/test/java/org/jooby/assets/AssetsTest.java b/jooby-assets/src/test/java/org/jooby/assets/AssetsTest.java
@@ -182,7 +182,7 @@ public void configuredist() throws Exception {
         .withValue("assets.etag", ConfigValueFactory.fromAnyRef(true))
         .withValue("application.path", ConfigValueFactory.fromAnyRef("/"))
         .withValue("assets.cdn", ConfigValueFactory.fromAnyRef(""))
-        .withValue("assets.cache.maxAge", ConfigValueFactory.fromAnyRef("365d"))
+        .withValue("assets.cache.maxAge", ConfigValueFactory.fromAnyRef("P365D"))
         .withValue("assets.lastModified", ConfigValueFactory.fromAnyRef(true))
         .withValue("assets.watch", ConfigValueFactory.fromAnyRef(false));
     new MockUnit(Env.class, Config.class, Binder.class, Request.class, Response.class,
diff --git a/jooby/src/main/java/org/jooby/Jooby.java b/jooby/src/main/java/org/jooby/Jooby.java
@@ -91,7 +91,6 @@
 import org.jooby.Session.Store;
 import org.jooby.handlers.AssetHandler;
 import org.jooby.internal.AppPrinter;
-import org.jooby.internal.AssetProxy;
 import org.jooby.internal.BuiltinParser;
 import org.jooby.internal.BuiltinRenderer;
 import org.jooby.internal.DefaulErrRenderer;
@@ -3033,7 +3032,14 @@ public Route.Definition assets(final String path) {
    */
   @Override
   public Route.Definition assets(final String path, final String location) {
-    return assets(path, new AssetHandler(location));
+    AssetHandler handler = new AssetHandler(location);
+    on("*", conf -> {
+      handler
+        .cdn(conf.getString("assets.cdn"))
+        .lastModified(conf.getBoolean("assets.lastModified"))
+        .etag(conf.getBoolean("assets.etag"));
+    });
+    return assets(path, handler);
   }
 
   /**
@@ -3080,16 +3086,7 @@ public Route.Definition assets(final String path, final String location) {
    */
   @Override
   public Route.Definition assets(final String path, final AssetHandler handler) {
-
-    AssetProxy router = new AssetProxy();
-    Route.Definition asset = new Route.Definition("GET", path, router);
-    on("*", conf -> {
-      router.fwd(handler
-          .cdn(conf.getString("assets.cdn"))
-          .lastModified(conf.getBoolean("assets.lastModified"))
-          .etag(conf.getBoolean("assets.etag")));
-    });
-    return appendDefinition(asset);
+    return appendDefinition(new Route.Definition("GET", path, handler));
   }
 
   /**
diff --git a/jooby/src/main/java/org/jooby/Route.java b/jooby/src/main/java/org/jooby/Route.java
@@ -36,7 +36,6 @@
 import java.util.Set;
 import java.util.stream.Collectors;
 
-import org.jooby.internal.AssetProxy;
 import org.jooby.internal.DeferredExecution;
 import org.jooby.internal.RouteImpl;
 import org.jooby.internal.RouteMatcher;
@@ -1333,11 +1332,8 @@ public String toString() {
      */
     private Route asRoute(final String method, final RouteMatcher matcher,
         final List<MediaType> produces) {
-      Route.Filter filter = this.filter;
-      if (filter instanceof AssetProxy) {
-        filter = ((AssetProxy) filter).delegate();
-      }
-      return new RouteImpl(filter, this, method, matcher.path(), produces, matcher.vars(), mapper);
+      return new RouteImpl(filter, this, method, matcher.path(), produces,
+        matcher.vars(), mapper);
     }
 
   }
diff --git a/jooby/src/main/java/org/jooby/handlers/AssetHandler.java b/jooby/src/main/java/org/jooby/handlers/AssetHandler.java
@@ -25,8 +25,10 @@
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.text.MessageFormat;
+import java.time.Duration;
 import java.util.Date;
 import java.util.Map;
+import java.util.Optional;
 
 import org.jooby.Asset;
 import org.jooby.Jooby;
@@ -92,7 +94,7 @@ public class AssetHandler implements Route.Handler {
 
   private boolean etag = true;
 
-  private long maxAge = -1;
+  private Optional<Duration> maxAgeOpt = Optional.empty();
 
   private boolean lastModified = true;
 
@@ -184,12 +186,21 @@ public AssetHandler cdn(final String cdn) {
     return this;
   }
 
+  /**
+   * @param maxAge Set the cache header max-age value.
+   * @return This handler.
+   */
+  public AssetHandler maxAge(final Duration maxAge) {
+    this.maxAgeOpt = Optional.of(maxAge);
+    return this;
+  }
+
   /**
    * @param maxAge Set the cache header max-age value in seconds.
    * @return This handler.
    */
   public AssetHandler maxAge(final long maxAge) {
-    this.maxAge = maxAge;
+    this.maxAgeOpt = Optional.of(Duration.ofSeconds(maxAge));
     return this;
   }
 
@@ -253,9 +264,9 @@ private void doHandle(final Request req, final Response rsp, final Asset asset)
     }
 
     // cache max-age
-    if (maxAge > 0) {
-      rsp.header("Cache-Control", "max-age=" + maxAge);
-    }
+    maxAgeOpt.ifPresent(d -> {
+      rsp.header("Cache-Control", "max-age=" + d.getSeconds());
+    });
 
     send(req, rsp, asset);
   }
diff --git a/jooby/src/main/java/org/jooby/internal/AssetProxy.java b/jooby/src/main/java/org/jooby/internal/AssetProxy.java
diff --git a/jooby/src/main/resources/org/jooby/jooby.conf b/jooby/src/main/resources/org/jooby/jooby.conf
@@ -153,7 +153,8 @@ assets {
 
   charset = ${application.charset}
 
-  cache.maxAge = -1
+  # Either null or a string in the ISO 8601 format according to `java.time.Duration.parse(CharSequence)`.
+  cache.maxAge = null
 
 }
 
diff --git a/jooby/src/test/java/org/jooby/internal/AssetProxyTest.java b/jooby/src/test/java/org/jooby/internal/AssetProxyTest.java
diff --git a/jooby/src/test/java/org/jooby/test/Client.java b/jooby/src/test/java/org/jooby/test/Client.java
@@ -72,7 +72,7 @@ public Request(final Client server, final Executor executor,
       this.req = req;
     }
 
-    private Response execute() throws Exception {
+    public Response execute() throws Exception {
       this.rsp = executor.execute(req).returnResponse();
       return new Response(server, rsp);
     }
@@ -252,7 +252,10 @@ public Response header(final String headerName, final Optional<Object> headerVal
     }
 
     public Response header(final String headerName, final Callback callback) throws Exception {
-      callback.execute(rsp.getFirstHeader(headerName).getValue());
+      callback.execute(
+        Optional.ofNullable(rsp.getFirstHeader(headerName))
+          .map(Header::getValue)
+          .orElse(null));
       return this;
     }
 

Original file line number	Diff line number	Diff line change
`@@ -153,7 +153,8 @@ assets {`
`153`	`153`
`154`	`154`	`charset = ${application.charset}`
`155`	`155`
`156`		`- cache.maxAge = -1`
	`156`	+ # Either null or a string in the ISO 8601 format according to `java.time.Duration.parse(CharSequence)`.
	`157`	`+ cache.maxAge = null`
`157`	`158`
`158`	`159`	`}`
`159`	`160`
Original file line number	Diff line number	Diff line change
`@@ -72,7 +72,7 @@ public Request(final Client server, final Executor executor,`
`72`	`72`	`this.req = req;`
`73`	`73`	`}`
`74`	`74`
`75`		`- private Response execute() throws Exception {`
	`75`	`+ public Response execute() throws Exception {`
`76`	`76`	`this.rsp = executor.execute(req).returnResponse();`
`77`	`77`	`return new Response(server, rsp);`
`78`	`78`	`}`
`@@ -252,7 +252,10 @@ public Response header(final String headerName, final Optional<Object> headerVal`
`252`	`252`	`}`
`253`	`253`
`254`	`254`	`public Response header(final String headerName, final Callback callback) throws Exception {`
`255`		`- callback.execute(rsp.getFirstHeader(headerName).getValue());`
	`255`	`+ callback.execute(`
	`256`	`+ Optional.ofNullable(rsp.getFirstHeader(headerName))`
	`257`	`+ .map(Header::getValue)`
	`258`	`+ .orElse(null));`
`256`	`259`	`return this;`
`257`	`260`	`}`
`258`	`261`