From 30ca4e0eee8e9852a0a2f175397b1c2d00a569fa Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Thu, 8 Jun 2017 09:57:34 +0200 Subject: [PATCH] Use change number instead of change id for inbound email Previously, we used the change id to identify a change from an inbound email. This is error prone as the change id as such isn't unique without a project and a branch (triplet). This change replaces this use with the change number instead, which is unique by itself. Bug: Issue 6449 Change-Id: I4f9696f48c2136d47ae94f59bf96a148ae598ebb --- .../acceptance/server/mail/AbstractMailIT.java | 6 +++--- .../acceptance/server/mail/ListMailFilterIT.java | 2 +- .../acceptance/server/mail/MailMetadataIT.java | 6 ++++-- .../acceptance/server/mail/MailProcessorIT.java | 12 ++++++------ .../com/google/gerrit/server/mail/MetadataName.java | 2 +- .../gerrit/server/mail/receive/MailMetadata.java | 6 +++--- .../gerrit/server/mail/receive/MailProcessor.java | 4 ++-- .../gerrit/server/mail/receive/MetadataParser.java | 11 ++++++----- .../server/mail/receive/MetadataParserTest.java | 12 ++++++------ 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 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 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 606da440b3..f6c1a1aee4 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 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- 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( - "
" + toFooterWithDelimiter(MetadataName.CHANGE_ID) + "cid
"); + "
" + toFooterWithDelimiter(MetadataName.CHANGE_NUMBER) + "123
"); stringBuilder.append("
" + toFooterWithDelimiter(MetadataName.PATCH_SET) + "1
"); stringBuilder.append( "
" + toFooterWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment
"); @@ -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())