BatchUpdate: Always bump rowVersion when dirty

When any Op returns true from updateChange, we need to bump the
rowVersion so we get a new ETag.

Add a regression test based on PostReviewers, which was the
manifestation reported by one of our internal users.

Change-Id: Ibe1e69430e2e24b5bcbb85cb24a032d820fdd10e

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -44,6 +44,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.ChangeFinder;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.OutputFormat;
@@ -175,6 +176,9 @@ public abstract class AbstractDaemonTest {
   @Inject
   protected PatchSetUtil psUtil;
 
+  @Inject
+  protected ChangeFinder changeFinder;
+
   protected TestRepository<InMemoryRepository> testRepo;
   protected GerritServer server;
   protected TestAccount admin;

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -60,10 +60,12 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.config.AnonymousCowardNameProvider;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.group.SystemGroupBackend;
 import com.google.gerrit.server.notedb.ChangeNoteUtil;
+import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.Util;
 
 import org.eclipse.jgit.lib.Constants;
@@ -73,6 +75,7 @@ import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.junit.Test;
 
+import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -396,6 +399,10 @@ public class ChangeIT extends AbstractDaemonTest {
   @Test
   public void addReviewer() throws Exception {
     PushOneCommit.Result r = createChange();
+    ChangeResource rsrc = parseResource(r);
+    String oldETag = rsrc.getETag();
+    Timestamp oldTs = rsrc.getChange().getLastUpdatedOn();
+
     AddReviewerInput in = new AddReviewerInput();
     in.reviewer = user.email;
     gApi.changes()
@@ -417,6 +424,11 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(reviewers).hasSize(1);
     assertThat(reviewers.iterator().next()._accountId)
         .isEqualTo(user.getId().get());
+
+    // Ensure ETag is updated but lastUpdatedOn isn't.
+    rsrc = parseResource(r);
+    assertThat(rsrc.getETag()).isNotEqualTo(oldETag);
+    assertThat(rsrc.getChange().getLastUpdatedOn()).isEqualTo(oldTs);
   }
 
   @Test
@@ -1023,4 +1035,12 @@ public class ChangeIT extends AbstractDaemonTest {
       }
     });
   }
+
+  private ChangeResource parseResource(PushOneCommit.Result r)
+      throws Exception {
+    List<ChangeControl> ctls = changeFinder.find(
+        r.getChangeId(), atrScope.get().getUser());
+    assertThat(ctls).hasSize(1);
+    return new ChangeResource(ctls.get(0));
+  }
 }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java

@@ -52,7 +52,6 @@ import com.google.gerrit.extensions.restapi.IdString;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Patch;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.server.ChangeFinder;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.GetRevisionActions;
 import com.google.gerrit.server.change.RevisionResource;
@@ -79,9 +78,6 @@ import java.util.Map;
 @NoHttpd
 public class RevisionIT extends AbstractDaemonTest {
 
-  @Inject
-  private ChangeFinder changeFinder;
-
   @Inject
   private GetRevisionActions getRevisionActions;
 

gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java

@@ -589,25 +589,22 @@ public class BatchUpdate implements AutoCloseable {
           for (Op op : e.getValue()) {
             dirty |= op.updateChange(ctx);
           }
-          ctx.getChange().setLastUpdatedOn(ctx.getWhen());
-          Iterable<Change> changes = Collections.singleton(ctx.getChange());
-          if (newChanges.containsKey(id)) {
-            db.changes().insert(changes);
-          } else if (ctx.saved) {
-            db.changes().update(changes);
-          } else if (ctx.deleted) {
-            db.changes().delete(changes);
-          }
-          if (dirty) {
-            db.commit();
-          }
-        } finally {
-          db.rollback();
-        }
-
           if (!dirty) {
             return;
           }
+          if (newChanges.containsKey(id)) {
+            db.changes().insert(bumpLastUpdatedOn(ctx));
+          } else if (ctx.saved) {
+            db.changes().update(bumpLastUpdatedOn(ctx));
+          } else if (ctx.deleted) {
+            db.changes().delete(bumpLastUpdatedOn(ctx));
+          } else {
+            db.changes().update(bumpRowVersionNotLastUpdatedOn(ctx));
+          }
+          db.commit();
+        } finally {
+          db.rollback();
+        }
 
         if (ctx.deleted) {
           if (notesMigration.writeChanges()) {
@@ -636,6 +633,17 @@ public class BatchUpdate implements AutoCloseable {
     }
   }
 
+  private static Iterable<Change> bumpLastUpdatedOn(ChangeContext ctx) {
+    Change c = ctx.getChange();
+    c.setLastUpdatedOn(ctx.getWhen());
+    return Collections.singleton(c);
+  }
+
+  private static Iterable<Change> bumpRowVersionNotLastUpdatedOn(
+      ChangeContext ctx) {
+    return Collections.singleton(ctx.getChange());
+  }
+
   private ChangeContext newChangeContext(Change.Id id) throws Exception {
     Change c = newChanges.get(id);
     if (c == null) {