Extract a type for MergeOp's submissionId

This stronger type allows easier reuse from other classes,
particularly ReceiveCommits, without duplicating the logic for
computing a unique string; it also cuts down on the use of Strings in
long method signatures.

Change-Id: Ida22c44b926f02f3e002a8736783a72a7e0118e6

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

@@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.client.SubmoduleSubscription;
 import com.google.gerrit.server.config.CanonicalWebUrl;
 import com.google.gerrit.server.git.MergeOpRepoManager.OpenRepo;
 import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.util.RequestId;
 import com.google.gerrit.server.util.SubmoduleSectionParser;
 import com.google.inject.assistedinject.Assisted;
 import com.google.inject.assistedinject.AssistedInject;
@@ -53,7 +54,7 @@ public class GitModules {
 
   private static final String GIT_MODULES = ".gitmodules";
 
-  private final String submissionId;
+  private final RequestId submissionId;
   Set<SubmoduleSubscription> subscriptions;
 
   @AssistedInject
@@ -109,7 +110,7 @@ public class GitModules {
 
   private void logDebug(String msg, Object... args) {
     if (log.isDebugEnabled()) {
-      log.debug("[" + submissionId + "]" + msg, args);
+      log.debug(submissionId + msg, args);
     }
   }
 }

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

@@ -33,8 +33,6 @@ import com.google.common.collect.Multimap;
 import com.google.common.collect.MultimapBuilder;
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
-import com.google.common.hash.Hasher;
-import com.google.common.hash.Hashing;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.common.data.SubmitRecord;
@@ -67,6 +65,7 @@ import com.google.gerrit.server.project.NoSuchProjectException;
 import com.google.gerrit.server.project.SubmitRuleEvaluator;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.InternalChangeQuery;
+import com.google.gerrit.server.util.RequestId;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 
@@ -79,8 +78,6 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
-import java.net.InetAddress;
-import java.net.UnknownHostException;
 import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -226,18 +223,8 @@ public class MergeOp implements AutoCloseable {
   private final SubmoduleOp.Factory subOpFactory;
   private final MergeOpRepoManager orm;
 
-  private static final String MACHINE_ID;
-  static {
-    String id;
-    try {
-      id = InetAddress.getLocalHost().getHostAddress();
-    } catch (UnknownHostException e) {
-      id = "unknown";
-    }
-    MACHINE_ID = id;
-  }
   private Timestamp ts;
-  private String submissionId;
+  private RequestId submissionId;
   private IdentifiedUser caller;
 
   private CommitStatus commits;
@@ -413,21 +400,13 @@ public class MergeOp implements AutoCloseable {
     }
   }
 
-  private void updateSubmissionId(Change change) {
-    Hasher h = Hashing.sha1().newHasher();
-    h.putLong(Thread.currentThread().getId())
-        .putUnencodedChars(MACHINE_ID);
-    ts = TimeUtil.nowTs();
-    submissionId = change.getId().get() + "-" + ts.getTime() +
-        "-" + h.hash().toString().substring(0, 8);
-  }
-
   public void merge(ReviewDb db, Change change, IdentifiedUser caller,
       boolean checkSubmitRules, SubmitInput submitInput)
       throws OrmException, RestApiException {
     this.submitInput = submitInput;
     this.caller = caller;
-    updateSubmissionId(change);
+    this.ts = TimeUtil.nowTs();
+    submissionId = RequestId.forChange(change);
     this.db = db;
     orm.setContext(db, ts, caller, submissionId);
 
@@ -800,28 +779,28 @@ public class MergeOp implements AutoCloseable {
 
   private void logDebug(String msg, Object... args) {
     if (log.isDebugEnabled()) {
-      log.debug("[" + submissionId + "]" + msg, args);
+      log.debug(submissionId + msg, args);
     }
   }
 
   private void logWarn(String msg, Throwable t) {
     if (log.isWarnEnabled()) {
-      log.warn("[" + submissionId + "]" + msg, t);
+      log.warn(submissionId + msg, t);
     }
   }
 
   private void logWarn(String msg) {
     if (log.isWarnEnabled()) {
-      log.warn("[" + submissionId + "]" + msg);
+      log.warn(submissionId + msg);
     }
   }
 
   private void logError(String msg, Throwable t) {
     if (log.isErrorEnabled()) {
       if (t != null) {
-        log.error("[" + submissionId + "]" + msg, t);
+        log.error(submissionId + msg, t);
       } else {
-        log.error("[" + submissionId + "]" + msg);
+        log.error(submissionId + msg);
       }
     }
   }

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

@@ -25,6 +25,7 @@ import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
 import com.google.gerrit.server.project.NoSuchProjectException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
+import com.google.gerrit.server.util.RequestId;
 import com.google.inject.Inject;
 
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
@@ -153,7 +154,7 @@ public class MergeOpRepoManager implements AutoCloseable {
   private ReviewDb db;
   private Timestamp ts;
   private IdentifiedUser caller;
-  private String submissionId;
+  private RequestId submissionId;
 
   @Inject
   MergeOpRepoManager(
@@ -168,14 +169,14 @@ public class MergeOpRepoManager implements AutoCloseable {
   }
 
   void setContext(ReviewDb db, Timestamp ts, IdentifiedUser caller,
-      String submissionId) {
+      RequestId submissionId) {
     this.db = db;
     this.ts = ts;
     this.caller = caller;
     this.submissionId = submissionId;
   }
 
-  public String getSubmissionId() {
+  public RequestId getSubmissionId() {
     return submissionId;
   }
 

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

@@ -115,6 +115,7 @@ import com.google.gerrit.server.query.change.InternalChangeQuery;
 import com.google.gerrit.server.ssh.SshInfo;
 import com.google.gerrit.server.util.LabelVote;
 import com.google.gerrit.server.util.MagicBranch;
+import com.google.gerrit.server.util.RequestId;
 import com.google.gerrit.server.util.RequestScopePropagator;
 import com.google.gerrit.util.cli.CmdLineParser;
 import com.google.gwtorm.server.OrmException;
@@ -307,6 +308,7 @@ public class ReceiveCommits {
   private final Repository repo;
   private final ReceivePack rp;
   private final NoteMap rejectCommits;
+  private final RequestId receiveId;
   private MagicBranchInput magicBranch;
   private boolean newChangeForAllNotInTarget;
 
@@ -406,6 +408,7 @@ public class ReceiveCommits {
     this.repo = repo;
     this.rp = new ReceivePack(repo);
     this.rejectCommits = BanCommit.loadRejectCommitsMap(repo, rp.getRevWalk());
+    this.receiveId = RequestId.forProject(project.getNameKey());
 
     this.subOpFactory = subOpFactory;
     this.mergeOpProvider = mergeOpProvider;
@@ -645,7 +648,7 @@ public class ReceiveCommits {
 
     // Update superproject gitlinks if required.
     try (MergeOpRepoManager orm = ormProvider.get()) {
-      orm.setContext(db, TimeUtil.nowTs(), user, "receiveID");
+      orm.setContext(db, TimeUtil.nowTs(), user, receiveId);
       SubmoduleOp op = subOpFactory.create(branches, orm);
       op.updateSuperProjects();
     } catch (SubmoduleException e) {

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

@@ -582,7 +582,7 @@ public class SubmoduleOp {
 
   private void logDebug(String msg, Object... args) {
     if (log.isDebugEnabled()) {
-      log.debug("[" + orm.getSubmissionId() + "]" + msg, args);
+      log.debug(orm.getSubmissionId() + msg, args);
     }
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java

@@ -47,6 +47,7 @@ import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
+import com.google.gerrit.server.util.RequestId;
 import com.google.inject.Module;
 import com.google.inject.assistedinject.Assisted;
 import com.google.inject.assistedinject.AssistedInject;
@@ -93,7 +94,7 @@ public abstract class SubmitStrategy {
           RevFlag canMergeFlag,
           ReviewDb db,
           Set<RevCommit> alreadyAccepted,
-          String submissionId,
+          RequestId submissionId,
           NotifyHandling notifyHandling,
           SubmoduleOp submoduleOp);
     }
@@ -124,7 +125,7 @@ public abstract class SubmitStrategy {
     final RevFlag canMergeFlag;
     final ReviewDb db;
     final Set<RevCommit> alreadyAccepted;
-    final String submissionId;
+    final RequestId submissionId;
     final SubmitType submitType;
     final NotifyHandling notifyHandling;
     final SubmoduleOp submoduleOp;
@@ -161,7 +162,7 @@ public abstract class SubmitStrategy {
         @Assisted RevFlag canMergeFlag,
         @Assisted ReviewDb db,
         @Assisted Set<RevCommit> alreadyAccepted,
-        @Assisted String submissionId,
+        @Assisted RequestId submissionId,
         @Assisted SubmitType submitType,
         @Assisted NotifyHandling notifyHandling,
         @Assisted SubmoduleOp submoduleOp) {

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java

@@ -24,6 +24,7 @@ import com.google.gerrit.server.git.IntegrationException;
 import com.google.gerrit.server.git.MergeOp.CommitStatus;
 import com.google.gerrit.server.git.MergeTip;
 import com.google.gerrit.server.git.SubmoduleOp;
+import com.google.gerrit.server.util.RequestId;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
@@ -53,7 +54,7 @@ public class SubmitStrategyFactory {
       Repository repo, CodeReviewRevWalk rw, ObjectInserter inserter,
       RevFlag canMergeFlag, Set<RevCommit> alreadyAccepted,
       Branch.NameKey destBranch, IdentifiedUser caller, MergeTip mergeTip,
-      CommitStatus commits, String submissionId, NotifyHandling notifyHandling,
+      CommitStatus commits, RequestId submissionId, NotifyHandling notifyHandling,
       SubmoduleOp submoduleOp)
       throws IntegrationException {
     SubmitStrategy.Arguments args = argsFactory.create(submitType, destBranch,

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java

@@ -484,7 +484,7 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op {
     ReviewDb db = ctx.getDb();
     logDebug("Setting change {} merged", c.getId());
     c.setStatus(Change.Status.MERGED);
-    c.setSubmissionId(args.submissionId);
+    c.setSubmissionId(args.submissionId.toStringForStorage());
 
     // TODO(dborowitz): We need to be able to change the author of the message,
     // which is not the user from the update context. addMergedMessage was able
@@ -588,22 +588,22 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op {
 
   protected final void logDebug(String msg, Object... args) {
     if (log.isDebugEnabled()) {
-      log.debug("[" + this.args.submissionId + "]" + msg, args);
+      log.debug(this.args.submissionId + msg, args);
     }
   }
 
   protected final void logWarn(String msg, Throwable t) {
     if (log.isWarnEnabled()) {
-      log.warn("[" + args.submissionId + "]" + msg, t);
+      log.warn(args.submissionId + msg, t);
     }
   }
 
   protected void logError(String msg, Throwable t) {
     if (log.isErrorEnabled()) {
       if (t != null) {
-        log.error("[" + args.submissionId + "]" + msg, t);
+        log.error(args.submissionId + msg, t);
       } else {
-        log.error("[" + args.submissionId + "]" + msg);
+        log.error(args.submissionId + msg);
       }
     }
   }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java

@@ -55,6 +55,7 @@ import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.util.LabelVote;
+import com.google.gerrit.server.util.RequestId;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.assistedinject.Assisted;
 import com.google.inject.assistedinject.AssistedInject;
@@ -265,9 +266,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     approvals.put(label, reviewer, Optional.<Short> absent());
   }
 
-  public void merge(String submissionId, Iterable<SubmitRecord> submitRecords) {
+  public void merge(RequestId submissionId,
+      Iterable<SubmitRecord> submitRecords) {
     this.status = Change.Status.MERGED;
-    this.submissionId = submissionId;
+    this.submissionId = submissionId.toStringForStorage();
     this.submitRecords = ImmutableList.copyOf(submitRecords);
     checkArgument(!this.submitRecords.isEmpty(),
         "no submit records specified at submit time");

gerrit-server/src/main/java/com/google/gerrit/server/util/RequestId.java

@@ -0,0 +1,65 @@
+// Copyright (C) 2016 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.server.util;
+
+import com.google.common.hash.Hasher;
+import com.google.common.hash.Hashing;
+import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Project;
+
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+
+/** Unique identifier for an end-user request, used in logs and similar. */
+public class RequestId {
+  private static final String MACHINE_ID;
+  static {
+    String id;
+    try {
+      id = InetAddress.getLocalHost().getHostAddress();
+    } catch (UnknownHostException e) {
+      id = "unknown";
+    }
+    MACHINE_ID = id;
+  }
+
+  public static RequestId forChange(Change c) {
+    return new RequestId(c.getId().toString());
+  }
+
+  public static RequestId forProject(Project.NameKey p) {
+    return new RequestId(p.toString());
+  }
+
+  private final String str;
+
+  private RequestId(String resourceId) {
+    Hasher h = Hashing.sha1().newHasher();
+    h.putLong(Thread.currentThread().getId())
+        .putUnencodedChars(MACHINE_ID);
+    str = "[" + resourceId + "-" + TimeUtil.nowTs().getTime() +
+        "-" + h.hash().toString().substring(0, 8) + "]";
+  }
+
+  @Override
+  public String toString() {
+    return str;
+  }
+
+  public String toStringForStorage() {
+    return str.substring(1, str.length() - 1);
+  }
+}

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -50,6 +50,7 @@ import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
+import com.google.gerrit.server.util.RequestId;
 import com.google.gerrit.testutil.TestChanges;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -481,10 +482,11 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
   @Test
   public void submitRecords() throws Exception {
     Change c = newChange();
+    RequestId submissionId = RequestId.forChange(c);
     ChangeUpdate update = newUpdate(c, changeOwner);
     update.setSubjectForCommit("Submit patch set 1");
 
-    update.merge("1-1453387607626-96fabc25", ImmutableList.of(
+    update.merge(submissionId, ImmutableList.of(
         submitRecord("NOT_READY", null,
           submitLabel("Verified", "OK", changeOwner.getAccountId()),
           submitLabel("Code-Review", "NEED", null)),
@@ -505,15 +507,16 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
           submitLabel("Verified", "OK", changeOwner.getAccountId()),
           submitLabel("Alternative-Code-Review", "NEED", null)));
     assertThat(notes.getChange().getSubmissionId())
-        .isEqualTo("1-1453387607626-96fabc25");
+        .isEqualTo(submissionId.toStringForStorage());
   }
 
   @Test
   public void latestSubmitRecordsOnly() throws Exception {
     Change c = newChange();
+    RequestId submissionId = RequestId.forChange(c);
     ChangeUpdate update = newUpdate(c, changeOwner);
     update.setSubjectForCommit("Submit patch set 1");
-    update.merge("1-1453387607626-96fabc25", ImmutableList.of(
+    update.merge(submissionId, ImmutableList.of(
         submitRecord("OK", null,
           submitLabel("Code-Review", "OK", otherUser.getAccountId()))));
     update.commit();
@@ -521,7 +524,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     incrementPatchSet(c);
     update = newUpdate(c, changeOwner);
     update.setSubjectForCommit("Submit patch set 2");
-    update.merge("1-1453387901516-5d1e2450", ImmutableList.of(
+    update.merge(submissionId, ImmutableList.of(
         submitRecord("OK", null,
           submitLabel("Code-Review", "OK", changeOwner.getAccountId()))));
     update.commit();
@@ -531,7 +534,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         submitRecord("OK", null,
           submitLabel("Code-Review", "OK", changeOwner.getAccountId())));
     assertThat(notes.getChange().getSubmissionId())
-        .isEqualTo("1-1453387901516-5d1e2450");
+        .isEqualTo(submissionId.toStringForStorage());
   }
 
   @Test
@@ -754,7 +757,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
 
     // Finish off by merging the change.
     update = newUpdate(c, changeOwner);
-    update.merge("1-1453387607626-96fabc25", ImmutableList.of(
+    update.merge(RequestId.forChange(c), ImmutableList.of(
         submitRecord("NOT_READY", null,
           submitLabel("Verified", "OK", changeOwner.getAccountId()),
           submitLabel("Alternative-Code-Review", "NEED", null))));

gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java

@@ -22,6 +22,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.server.util.RequestId;
 import com.google.gerrit.testutil.TestChanges;
 
 import org.eclipse.jgit.lib.ObjectId;
@@ -138,7 +139,8 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
     ChangeUpdate update = newUpdate(c, changeOwner);
     update.setSubjectForCommit("Submit patch set 1");
 
-    update.merge("1-1453387607626-96fabc25", ImmutableList.of(
+    RequestId submissionId = RequestId.forChange(c);
+    update.merge(submissionId, ImmutableList.of(
         submitRecord("NOT_READY", null,
           submitLabel("Verified", "OK", changeOwner.getAccountId()),
           submitLabel("Code-Review", "NEED", null)),
@@ -152,7 +154,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         + "\n"
         + "Patch-set: 1\n"
         + "Status: merged\n"
-        + "Submission-id: 1-1453387607626-96fabc25\n"
+        + "Submission-id: " + submissionId.toStringForStorage() + "\n"
         + "Submitted-with: NOT_READY\n"
         + "Submitted-with: OK: Verified: Change Owner <1@gerrit>\n"
         + "Submitted-with: NEED: Code-Review\n"
@@ -204,7 +206,8 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
     ChangeUpdate update = newUpdate(c, changeOwner);
     update.setSubjectForCommit("Submit patch set 1");
 
-    update.merge("1-1453387607626-96fabc25", ImmutableList.of(
+    RequestId submissionId = RequestId.forChange(c);
+    update.merge(submissionId, ImmutableList.of(
         submitRecord("RULE_ERROR", "Problem with patch set:\n1")));
     update.commit();
 
@@ -212,7 +215,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         + "\n"
         + "Patch-set: 1\n"
         + "Status: merged\n"
-        + "Submission-id: 1-1453387607626-96fabc25\n"
+        + "Submission-id: " + submissionId.toStringForStorage() + "\n"
         + "Submitted-with: RULE_ERROR Problem with patch set: 1\n",
         update.getResult());
   }