Merge "Use change number instead of change id for inbound email" into stable-2.14

9 files changed, 32 insertions, 29 deletions

diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
index 4c38d83030..d6ad26911d 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
@@ -132,9 +132,9 @@ public class AbstractMailIT extends AbstractDaemonTest {
         + "> \n";
   }
 
-  protected static String textFooterForChange(String changeId, String timestamp) {
-    return "Gerrit-Change-Id: "
-        + changeId
+  protected static String textFooterForChange(int changeNumber, String timestamp) {
+    return "Gerrit-Change-Number: "
+        + changeNumber
         + "\n"
         + "Gerrit-PatchSet: 1\n"
         + "Gerrit-MessageType: comment\n"
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ListMailFilterIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ListMailFilterIT.java
index 1dcdd97f1d..a96c6ece73 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ListMailFilterIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ListMailFilterIT.java
@@ -113,7 +113,7 @@ public class ListMailFilterIT extends AbstractMailIT {
             null,
             null,
             null);
-    b.textContent(txt + textFooterForChange(changeId, ts));
+    b.textContent(txt + textFooterForChange(changeInfo._number, ts));
 
     mailProcessor.process(b.build());
     return changeInfo;
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailMetadataIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailMetadataIT.java
index ada222aac3..7cef8e7637 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailMetadataIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailMetadataIT.java
@@ -67,7 +67,8 @@ public class MailMetadataIT extends AbstractDaemonTest {
 
     Map<String, Object> expectedHeaders = new HashMap<>();
     expectedHeaders.put("Gerrit-PatchSet", "1");
-    expectedHeaders.put("Gerrit-Change-Id", newChange.getChangeId());
+    expectedHeaders.put(
+        "Gerrit-Change-Number", String.valueOf(newChange.getChange().getId().get()));
     expectedHeaders.put("Gerrit-MessageType", "newchange");
     expectedHeaders.put("Gerrit-Commit", newChange.getCommit().getId().name());
     expectedHeaders.put("Gerrit-ChangeURL", changeURL);
@@ -102,7 +103,8 @@ public class MailMetadataIT extends AbstractDaemonTest {
     String changeURL = "<" + canonicalWebUrl.get() + newChange.getChange().getId().get() + ">";
     Map<String, Object> expectedHeaders = new HashMap<>();
     expectedHeaders.put("Gerrit-PatchSet", "1");
-    expectedHeaders.put("Gerrit-Change-Id", newChange.getChangeId());
+    expectedHeaders.put(
+        "Gerrit-Change-Number", String.valueOf(newChange.getChange().getId().get()));
     expectedHeaders.put("Gerrit-MessageType", "comment");
     expectedHeaders.put("Gerrit-Commit", newChange.getCommit().getId().name());
     expectedHeaders.put("Gerrit-ChangeURL", changeURL);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
index e7a0cda335..9de4797312 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
@@ -51,7 +51,7 @@ public class MailProcessorIT extends AbstractMailIT {
             null,
             null,
             null);
-    b.textContent(txt + textFooterForChange(changeId, ts));
+    b.textContent(txt + textFooterForChange(changeInfo._number, ts));
 
     mailProcessor.process(b.build());
 
@@ -79,7 +79,7 @@ public class MailProcessorIT extends AbstractMailIT {
             "Some Inline Comment",
             null,
             null);
-    b.textContent(txt + textFooterForChange(changeId, ts));
+    b.textContent(txt + textFooterForChange(changeInfo._number, ts));
 
     mailProcessor.process(b.build());
 
@@ -115,7 +115,7 @@ public class MailProcessorIT extends AbstractMailIT {
             null,
             "Some Comment on File 1",
             null);
-    b.textContent(txt + textFooterForChange(changeId, ts));
+    b.textContent(txt + textFooterForChange(changeInfo._number, ts));
 
     mailProcessor.process(b.build());
 
@@ -152,7 +152,7 @@ public class MailProcessorIT extends AbstractMailIT {
             "Some Inline Comment",
             null,
             null);
-    b.textContent(txt + textFooterForChange(changeId, ts));
+    b.textContent(txt + textFooterForChange(changeInfo._number, ts));
 
     mailProcessor.process(b.build());
     comments = gApi.changes().id(changeId).current().commentsAsList();
@@ -183,7 +183,7 @@ public class MailProcessorIT extends AbstractMailIT {
             "Some Inline Comment",
             null,
             null);
-    b.textContent(txt + textFooterForChange(changeId, ts));
+    b.textContent(txt + textFooterForChange(changeInfo._number, ts));
 
     // Set account state to inactive
     gApi.accounts().id("user").setActive(false);
@@ -219,7 +219,7 @@ public class MailProcessorIT extends AbstractMailIT {
     MailMessage.Builder b =
         messageBuilderWithDefaultFields()
             .from(user.emailAddress)
-            .textContent(txt + textFooterForChange(changeId, ts));
+            .textContent(txt + textFooterForChange(changeInfo._number, ts));
 
     sender.clear();
     mailProcessor.process(b.build());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MetadataName.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MetadataName.java
index 3db55c0007..3080e4fc80 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MetadataName.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MetadataName.java
@@ -15,7 +15,7 @@
 package com.google.gerrit.server.mail;
 
 public final class MetadataName {
-  public static final String CHANGE_ID = "Gerrit-Change-Id";
+  public static final String CHANGE_NUMBER = "Gerrit-Change-Number";
   public static final String PATCH_SET = "Gerrit-PatchSet";
   public static final String MESSAGE_TYPE = "Gerrit-MessageType";
   public static final String TIMESTAMP = "Gerrit-Comment-Date";
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailMetadata.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailMetadata.java
index f85291c10f..04c2addeed 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailMetadata.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailMetadata.java
@@ -19,14 +19,14 @@ import java.sql.Timestamp;
 
 /** MailMetadata represents metadata parsed from inbound email. */
 public class MailMetadata {
-  public String changeId;
+  public Integer changeNumber;
   public Integer patchSet;
   public String author; // Author of the email
   public Timestamp timestamp;
   public String messageType; // we expect comment here
 
   public boolean hasRequiredFields() {
-    return changeId != null
+    return changeNumber != null
         && patchSet != null
         && author != null
         && timestamp != null
@@ -36,7 +36,7 @@ public class MailMetadata {
   @Override
   public String toString() {
     return MoreObjects.toStringHelper(this)
-        .add("Change-Id", changeId)
+        .add("Change-Number", changeNumber)
         .add("Patch-Set", patchSet)
         .add("Author", author)
         .add("Timestamp", timestamp)
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index 862da9f7fb..accf7ba0e4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -160,12 +160,12 @@ public class MailProcessor {
 
     try (ManualRequestContext ctx = oneOffRequestContext.openAs(account)) {
       List<ChangeData> changeDataList =
-          queryProvider.get().byKey(Change.Key.parse(metadata.changeId));
+          queryProvider.get().byLegacyChangeId(new Change.Id(metadata.changeNumber));
       if (changeDataList.size() != 1) {
         log.error(
             String.format(
                 "Message %s references unique change %s, but there are %d matching changes in the index. Will delete message.",
-                message.id(), metadata.changeId, changeDataList.size()));
+                message.id(), metadata.changeNumber, changeDataList.size()));
         return;
       }
       ChangeData cd = changeDataList.get(0);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MetadataParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MetadataParser.java
index a18da689c4..7085051b86 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MetadataParser.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MetadataParser.java
@@ -38,9 +38,9 @@ public class MetadataParser {
 
     // Check email headers for X-Gerrit-<Name>
     for (String header : m.additionalHeaders()) {
-      if (header.startsWith(toHeaderWithDelimiter(MetadataName.CHANGE_ID))) {
-        metadata.changeId =
-            header.substring(toHeaderWithDelimiter(MetadataName.CHANGE_ID).length());
+      if (header.startsWith(toHeaderWithDelimiter(MetadataName.CHANGE_NUMBER))) {
+        String num = header.substring(toHeaderWithDelimiter(MetadataName.CHANGE_NUMBER).length());
+        metadata.changeNumber = Ints.tryParse(num);
       } else if (header.startsWith(toHeaderWithDelimiter(MetadataName.PATCH_SET))) {
         String ps = header.substring(toHeaderWithDelimiter(MetadataName.PATCH_SET).length());
         metadata.patchSet = Ints.tryParse(ps);
@@ -84,8 +84,9 @@ public class MetadataParser {
 
   private static void extractFooters(String[] lines, MailMetadata metadata, MailMessage m) {
     for (String line : lines) {
-      if (metadata.changeId == null && line.contains(MetadataName.CHANGE_ID)) {
-        metadata.changeId = extractFooter(toFooterWithDelimiter(MetadataName.CHANGE_ID), line);
+      if (metadata.changeNumber == null && line.contains(MetadataName.CHANGE_NUMBER)) {
+        metadata.changeNumber =
+            Ints.tryParse(extractFooter(toFooterWithDelimiter(MetadataName.CHANGE_NUMBER), line));
       } else if (metadata.patchSet == null && line.contains(MetadataName.PATCH_SET)) {
         metadata.patchSet =
             Ints.tryParse(extractFooter(toFooterWithDelimiter(MetadataName.PATCH_SET), line));
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/mail/receive/MetadataParserTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/mail/receive/MetadataParserTest.java
index 0c4507ef08..84bae96a8e 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/mail/receive/MetadataParserTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/mail/receive/MetadataParserTest.java
@@ -34,7 +34,7 @@ public class MetadataParserTest {
     b.dateReceived(new DateTime());
     b.subject("");
 
-    b.addAdditionalHeader(toHeaderWithDelimiter(MetadataName.CHANGE_ID) + "cid");
+    b.addAdditionalHeader(toHeaderWithDelimiter(MetadataName.CHANGE_NUMBER) + "123");
     b.addAdditionalHeader(toHeaderWithDelimiter(MetadataName.PATCH_SET) + "1");
     b.addAdditionalHeader(toHeaderWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment");
     b.addAdditionalHeader(
@@ -45,7 +45,7 @@ public class MetadataParserTest {
 
     MailMetadata meta = MetadataParser.parse(b.build());
     assertThat(meta.author).isEqualTo(author.getEmail());
-    assertThat(meta.changeId).isEqualTo("cid");
+    assertThat(meta.changeNumber).isEqualTo(123);
     assertThat(meta.patchSet).isEqualTo(1);
     assertThat(meta.messageType).isEqualTo("comment");
     assertThat(meta.timestamp.getTime())
@@ -62,7 +62,7 @@ public class MetadataParserTest {
     b.subject("");
 
     StringBuilder stringBuilder = new StringBuilder();
-    stringBuilder.append(toFooterWithDelimiter(MetadataName.CHANGE_ID) + "cid\r\n");
+    stringBuilder.append(toFooterWithDelimiter(MetadataName.CHANGE_NUMBER) + "123\r\n");
     stringBuilder.append("> " + toFooterWithDelimiter(MetadataName.PATCH_SET) + "1\n");
     stringBuilder.append(toFooterWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment\n");
     stringBuilder.append(
@@ -74,7 +74,7 @@ public class MetadataParserTest {
 
     MailMetadata meta = MetadataParser.parse(b.build());
     assertThat(meta.author).isEqualTo(author.getEmail());
-    assertThat(meta.changeId).isEqualTo("cid");
+    assertThat(meta.changeNumber).isEqualTo(123);
     assertThat(meta.patchSet).isEqualTo(1);
     assertThat(meta.messageType).isEqualTo("comment");
     assertThat(meta.timestamp.getTime())
@@ -92,7 +92,7 @@ public class MetadataParserTest {
 
     StringBuilder stringBuilder = new StringBuilder();
     stringBuilder.append(
-        "<div id\"someid\">" + toFooterWithDelimiter(MetadataName.CHANGE_ID) + "cid</div>");
+        "<div id\"someid\">" + toFooterWithDelimiter(MetadataName.CHANGE_NUMBER) + "123</div>");
     stringBuilder.append("<div>" + toFooterWithDelimiter(MetadataName.PATCH_SET) + "1</div>");
     stringBuilder.append(
         "<div>" + toFooterWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment</div>");
@@ -108,7 +108,7 @@ public class MetadataParserTest {
 
     MailMetadata meta = MetadataParser.parse(b.build());
     assertThat(meta.author).isEqualTo(author.getEmail());
-    assertThat(meta.changeId).isEqualTo("cid");
+    assertThat(meta.changeNumber).isEqualTo(123);
     assertThat(meta.patchSet).isEqualTo(1);
     assertThat(meta.messageType).isEqualTo("comment");
     assertThat(meta.timestamp.getTime())