From bc03a8f3b4fc2ffa325dad1d796c70d65db5239c Mon Sep 17 00:00:00 2001
From: Shawn Pearce <sop@google.com>
Date: Fri, 19 Jul 2013 14:28:13 -0700
Subject: [PATCH] Use ETag and If-None-Match for /detail caching

Last-Modified is only second level precision and may allow a client to
fail to see subsequent updates within the same second window.  Improve
the caching check by using an ETag string that is computed from the
full timestamp and the row version fields of Change.

Change-Id: I58fc95b5396baa1d8afb676ff672c9f19b835f1c
---
 .../extensions/restapi/RestResource.java      |  9 ++++++++
 .../gerrit/httpd/restapi/RestApiServlet.java  | 21 ++++++++++++++++---
 .../google/gerrit/reviewdb/client/Change.java |  4 ++++
 .../gerrit/server/change/ChangeResource.java  | 11 +++++++++-
 4 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/RestResource.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/RestResource.java
index f245fac67c..8032531c55 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/RestResource.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/RestResource.java
@@ -26,6 +26,15 @@ public interface RestResource {
 
   /** A resource with a last modification date. */
   public interface HasLastModified {
+    /**
+     * @return time for the Last-Modified header. HTTP truncates the header
+     *         value to seconds.
+     */
     public Timestamp getLastModified();
   }
+
+  /** A resource with an ETag. */
+  public interface HasETag {
+    public String getETag();
+  }
 }
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 58ee02c479..f2fee0b0c7 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -360,9 +360,19 @@ public class RestApiServlet extends HttpServlet {
     }
   }
 
-  private boolean notModified(HttpServletRequest req, RestResource rsrc) {
-    if (rsrc instanceof RestResource.HasLastModified
-        && "GET".equals(req.getMethod())) {
+  private static boolean notModified(HttpServletRequest req, RestResource rsrc) {
+    if (!"GET".equals(req.getMethod())) {
+      return false;
+    }
+
+    if (rsrc instanceof RestResource.HasETag) {
+      String have = req.getHeader(HttpHeaders.IF_NONE_MATCH);
+      if (have != null) {
+        return have.equals(((RestResource.HasETag) rsrc).getETag());
+      }
+    }
+
+    if (rsrc instanceof RestResource.HasLastModified) {
       Timestamp m = ((RestResource.HasLastModified) rsrc).getLastModified();
       long d = req.getDateHeader(HttpHeaders.IF_MODIFIED_SINCE);
 
@@ -400,6 +410,11 @@ public class RestApiServlet extends HttpServlet {
 
   private static void addResourceStateHeaders(
       HttpServletResponse res, RestResource rsrc) {
+    if (rsrc instanceof RestResource.HasETag) {
+      res.setHeader(
+          HttpHeaders.ETAG,
+          ((RestResource.HasETag) rsrc).getETag());
+    }
     if (rsrc instanceof RestResource.HasLastModified) {
       res.setDateHeader(
           HttpHeaders.LAST_MODIFIED,
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
index 2a51872628..7dedf14f3a 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
@@ -431,6 +431,10 @@ public final class Change {
     lastUpdatedOn = now;
   }
 
+  public int getRowVersion() {
+    return rowVersion;
+  }
+
   public void resetLastUpdatedOn() {
     lastUpdatedOn = new Timestamp(System.currentTimeMillis());
   }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java
index e2d3a1563a..1623237d22 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java
@@ -15,6 +15,7 @@
 package com.google.gerrit.server.change;
 
 import com.google.gerrit.extensions.restapi.RestResource;
+import com.google.gerrit.extensions.restapi.RestResource.HasETag;
 import com.google.gerrit.extensions.restapi.RestResource.HasLastModified;
 import com.google.gerrit.extensions.restapi.RestView;
 import com.google.gerrit.reviewdb.client.Change;
@@ -23,7 +24,8 @@ import com.google.inject.TypeLiteral;
 
 import java.sql.Timestamp;
 
-public class ChangeResource implements RestResource, HasLastModified {
+public class ChangeResource implements RestResource,
+    HasLastModified, HasETag {
   public static final TypeLiteral<RestView<ChangeResource>> CHANGE_KIND =
       new TypeLiteral<RestView<ChangeResource>>() {};
 
@@ -49,4 +51,11 @@ public class ChangeResource implements RestResource, HasLastModified {
   public Timestamp getLastModified() {
     return getChange().getLastUpdatedOn();
   }
+
+  @Override
+  public String getETag() {
+    return String.format("%x-%x",
+        getChange().getLastUpdatedOn().getTime(),
+        getChange().getRowVersion());
+  }
 }