opendev/gerrit
Code Issues Proposed changes

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

Browse Source
This commit is contained in:
David Pursehouse
2017-06-08 12:09:04 +00:00
committed by Gerrit Code Review
parent a85e8147b3 30ca4e0eee
commit ad6b3ccba5
9 changed files with 32 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
View File

@@ -132,9 +132,9 @@ public class AbstractMailIT extends AbstractDaemonTest {
+ "> \n"; + "> \n";
} }
protected static String textFooterForChange(String changeId, String timestamp) { protected static String textFooterForChange(int changeNumber, String timestamp) {
return "Gerrit-Change-Id: " return "Gerrit-Change-Number: "
+ changeId + changeNumber
+ "\n" + "\n"
+ "Gerrit-PatchSet: 1\n" + "Gerrit-PatchSet: 1\n"
+ "Gerrit-MessageType: comment\n" + "Gerrit-MessageType: comment\n"

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ListMailFilterIT.java
View File

@@ -113,7 +113,7 @@ public class ListMailFilterIT extends AbstractMailIT {
null, null,
null, null,
null); null);
b.textContent(txt + textFooterForChange(changeId, ts)); b.textContent(txt + textFooterForChange(changeInfo._number, ts));
mailProcessor.process(b.build()); mailProcessor.process(b.build());
return changeInfo; return changeInfo;

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailMetadataIT.java
View File

@@ -67,7 +67,8 @@ public class MailMetadataIT extends AbstractDaemonTest {
Map<String, Object> expectedHeaders = new HashMap<>(); Map<String, Object> expectedHeaders = new HashMap<>();
expectedHeaders.put("Gerrit-PatchSet", "1"); 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-MessageType", "newchange");
expectedHeaders.put("Gerrit-Commit", newChange.getCommit().getId().name()); expectedHeaders.put("Gerrit-Commit", newChange.getCommit().getId().name());
expectedHeaders.put("Gerrit-ChangeURL", changeURL); expectedHeaders.put("Gerrit-ChangeURL", changeURL);
@@ -102,7 +103,8 @@ public class MailMetadataIT extends AbstractDaemonTest {
String changeURL = "<" + canonicalWebUrl.get() + newChange.getChange().getId().get() + ">"; String changeURL = "<" + canonicalWebUrl.get() + newChange.getChange().getId().get() + ">";
Map<String, Object> expectedHeaders = new HashMap<>(); Map<String, Object> expectedHeaders = new HashMap<>();
expectedHeaders.put("Gerrit-PatchSet", "1"); 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-MessageType", "comment");
expectedHeaders.put("Gerrit-Commit", newChange.getCommit().getId().name()); expectedHeaders.put("Gerrit-Commit", newChange.getCommit().getId().name());
expectedHeaders.put("Gerrit-ChangeURL", changeURL); expectedHeaders.put("Gerrit-ChangeURL", changeURL);

12
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
View File

@@ -51,7 +51,7 @@ public class MailProcessorIT extends AbstractMailIT {
null, null,
null, null,
null); null);
b.textContent(txt + textFooterForChange(changeId, ts)); b.textContent(txt + textFooterForChange(changeInfo._number, ts));
mailProcessor.process(b.build()); mailProcessor.process(b.build());
@@ -79,7 +79,7 @@ public class MailProcessorIT extends AbstractMailIT {
"Some Inline Comment", "Some Inline Comment",
null, null,
null); null);
b.textContent(txt + textFooterForChange(changeId, ts)); b.textContent(txt + textFooterForChange(changeInfo._number, ts));
mailProcessor.process(b.build()); mailProcessor.process(b.build());
@@ -115,7 +115,7 @@ public class MailProcessorIT extends AbstractMailIT {
null, null,
"Some Comment on File 1", "Some Comment on File 1",
null); null);
b.textContent(txt + textFooterForChange(changeId, ts)); b.textContent(txt + textFooterForChange(changeInfo._number, ts));
mailProcessor.process(b.build()); mailProcessor.process(b.build());
@@ -152,7 +152,7 @@ public class MailProcessorIT extends AbstractMailIT {
"Some Inline Comment", "Some Inline Comment",
null, null,
null); null);
b.textContent(txt + textFooterForChange(changeId, ts)); b.textContent(txt + textFooterForChange(changeInfo._number, ts));
mailProcessor.process(b.build()); mailProcessor.process(b.build());
comments = gApi.changes().id(changeId).current().commentsAsList(); comments = gApi.changes().id(changeId).current().commentsAsList();
@@ -183,7 +183,7 @@ public class MailProcessorIT extends AbstractMailIT {
"Some Inline Comment", "Some Inline Comment",
null, null,
null); null);
b.textContent(txt + textFooterForChange(changeId, ts)); b.textContent(txt + textFooterForChange(changeInfo._number, ts));
// Set account state to inactive // Set account state to inactive
gApi.accounts().id("user").setActive(false); gApi.accounts().id("user").setActive(false);
@@ -219,7 +219,7 @@ public class MailProcessorIT extends AbstractMailIT {
MailMessage.Builder b = MailMessage.Builder b =
messageBuilderWithDefaultFields() messageBuilderWithDefaultFields()
.from(user.emailAddress) .from(user.emailAddress)
.textContent(txt + textFooterForChange(changeId, ts)); .textContent(txt + textFooterForChange(changeInfo._number, ts));
sender.clear(); sender.clear();
mailProcessor.process(b.build()); mailProcessor.process(b.build());

2
gerrit-server/src/main/java/com/google/gerrit/server/mail/MetadataName.java
View File

@@ -15,7 +15,7 @@
package com.google.gerrit.server.mail; package com.google.gerrit.server.mail;
public final class MetadataName { 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 PATCH_SET = "Gerrit-PatchSet";
public static final String MESSAGE_TYPE = "Gerrit-MessageType"; public static final String MESSAGE_TYPE = "Gerrit-MessageType";
public static final String TIMESTAMP = "Gerrit-Comment-Date"; public static final String TIMESTAMP = "Gerrit-Comment-Date";

6
gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailMetadata.java
View File

@@ -19,14 +19,14 @@ import java.sql.Timestamp;
/** MailMetadata represents metadata parsed from inbound email. */ /** MailMetadata represents metadata parsed from inbound email. */
public class MailMetadata { public class MailMetadata {
public String changeId; public Integer changeNumber;
public Integer patchSet; public Integer patchSet;
public String author; // Author of the email public String author; // Author of the email
public Timestamp timestamp; public Timestamp timestamp;
public String messageType; // we expect comment here public String messageType; // we expect comment here
public boolean hasRequiredFields() { public boolean hasRequiredFields() {
return changeId != null return changeNumber != null
&& patchSet != null && patchSet != null
&& author != null && author != null
&& timestamp != null && timestamp != null
@@ -36,7 +36,7 @@ public class MailMetadata {
@Override @Override
public String toString() { public String toString() {
return MoreObjects.toStringHelper(this) return MoreObjects.toStringHelper(this)
.add("Change-Id", changeId) .add("Change-Number", changeNumber)
.add("Patch-Set", patchSet) .add("Patch-Set", patchSet)
.add("Author", author) .add("Author", author)
.add("Timestamp", timestamp) .add("Timestamp", timestamp)

4
gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java
View File

@@ -160,12 +160,12 @@ public class MailProcessor {
try (ManualRequestContext ctx = oneOffRequestContext.openAs(account)) { try (ManualRequestContext ctx = oneOffRequestContext.openAs(account)) {
List<ChangeData> changeDataList = List<ChangeData> changeDataList =
queryProvider.get().byKey(Change.Key.parse(metadata.changeId)); queryProvider.get().byLegacyChangeId(new Change.Id(metadata.changeNumber));
if (changeDataList.size() != 1) { if (changeDataList.size() != 1) {
log.error( log.error(
String.format( String.format(
"Message %s references unique change %s, but there are %d matching changes in the index. Will delete message.", "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; return;
} }
ChangeData cd = changeDataList.get(0); ChangeData cd = changeDataList.get(0);

11
gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MetadataParser.java
View File

@@ -38,9 +38,9 @@ public class MetadataParser {
// Check email headers for X-Gerrit-<Name> // Check email headers for X-Gerrit-<Name>
for (String header : m.additionalHeaders()) { for (String header : m.additionalHeaders()) {
if (header.startsWith(toHeaderWithDelimiter(MetadataName.CHANGE_ID))) { if (header.startsWith(toHeaderWithDelimiter(MetadataName.CHANGE_NUMBER))) {
metadata.changeId = String num = header.substring(toHeaderWithDelimiter(MetadataName.CHANGE_NUMBER).length());
header.substring(toHeaderWithDelimiter(MetadataName.CHANGE_ID).length()); metadata.changeNumber = Ints.tryParse(num);
} else if (header.startsWith(toHeaderWithDelimiter(MetadataName.PATCH_SET))) { } else if (header.startsWith(toHeaderWithDelimiter(MetadataName.PATCH_SET))) {
String ps = header.substring(toHeaderWithDelimiter(MetadataName.PATCH_SET).length()); String ps = header.substring(toHeaderWithDelimiter(MetadataName.PATCH_SET).length());
metadata.patchSet = Ints.tryParse(ps); metadata.patchSet = Ints.tryParse(ps);
@@ -84,8 +84,9 @@ public class MetadataParser {
private static void extractFooters(String[] lines, MailMetadata metadata, MailMessage m) { private static void extractFooters(String[] lines, MailMetadata metadata, MailMessage m) {
for (String line : lines) { for (String line : lines) {
if (metadata.changeId == null && line.contains(MetadataName.CHANGE_ID)) { if (metadata.changeNumber == null && line.contains(MetadataName.CHANGE_NUMBER)) {
metadata.changeId = extractFooter(toFooterWithDelimiter(MetadataName.CHANGE_ID), line); metadata.changeNumber =
Ints.tryParse(extractFooter(toFooterWithDelimiter(MetadataName.CHANGE_NUMBER), line));
} else if (metadata.patchSet == null && line.contains(MetadataName.PATCH_SET)) { } else if (metadata.patchSet == null && line.contains(MetadataName.PATCH_SET)) {
metadata.patchSet = metadata.patchSet =
Ints.tryParse(extractFooter(toFooterWithDelimiter(MetadataName.PATCH_SET), line)); Ints.tryParse(extractFooter(toFooterWithDelimiter(MetadataName.PATCH_SET), line));

12
gerrit-server/src/test/java/com/google/gerrit/server/mail/receive/MetadataParserTest.java
View File

@@ -34,7 +34,7 @@ public class MetadataParserTest {
b.dateReceived(new DateTime()); b.dateReceived(new DateTime());
b.subject(""); 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.PATCH_SET) + "1");
b.addAdditionalHeader(toHeaderWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment"); b.addAdditionalHeader(toHeaderWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment");
b.addAdditionalHeader( b.addAdditionalHeader(
@@ -45,7 +45,7 @@ public class MetadataParserTest {
MailMetadata meta = MetadataParser.parse(b.build()); MailMetadata meta = MetadataParser.parse(b.build());
assertThat(meta.author).isEqualTo(author.getEmail()); assertThat(meta.author).isEqualTo(author.getEmail());
assertThat(meta.changeId).isEqualTo("cid"); assertThat(meta.changeNumber).isEqualTo(123);
assertThat(meta.patchSet).isEqualTo(1); assertThat(meta.patchSet).isEqualTo(1);
assertThat(meta.messageType).isEqualTo("comment"); assertThat(meta.messageType).isEqualTo("comment");
assertThat(meta.timestamp.getTime()) assertThat(meta.timestamp.getTime())
@@ -62,7 +62,7 @@ public class MetadataParserTest {
b.subject(""); b.subject("");
StringBuilder stringBuilder = new StringBuilder(); 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.PATCH_SET) + "1\n");
stringBuilder.append(toFooterWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment\n"); stringBuilder.append(toFooterWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment\n");
stringBuilder.append( stringBuilder.append(
@@ -74,7 +74,7 @@ public class MetadataParserTest {
MailMetadata meta = MetadataParser.parse(b.build()); MailMetadata meta = MetadataParser.parse(b.build());
assertThat(meta.author).isEqualTo(author.getEmail()); assertThat(meta.author).isEqualTo(author.getEmail());
assertThat(meta.changeId).isEqualTo("cid"); assertThat(meta.changeNumber).isEqualTo(123);
assertThat(meta.patchSet).isEqualTo(1); assertThat(meta.patchSet).isEqualTo(1);
assertThat(meta.messageType).isEqualTo("comment"); assertThat(meta.messageType).isEqualTo("comment");
assertThat(meta.timestamp.getTime()) assertThat(meta.timestamp.getTime())
@@ -92,7 +92,7 @@ public class MetadataParserTest {
StringBuilder stringBuilder = new StringBuilder(); StringBuilder stringBuilder = new StringBuilder();
stringBuilder.append( 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.PATCH_SET) + "1</div>");
stringBuilder.append( stringBuilder.append(
"<div>" + toFooterWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment</div>"); "<div>" + toFooterWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment</div>");
@@ -108,7 +108,7 @@ public class MetadataParserTest {
MailMetadata meta = MetadataParser.parse(b.build()); MailMetadata meta = MetadataParser.parse(b.build());
assertThat(meta.author).isEqualTo(author.getEmail()); assertThat(meta.author).isEqualTo(author.getEmail());
assertThat(meta.changeId).isEqualTo("cid"); assertThat(meta.changeNumber).isEqualTo(123);
assertThat(meta.patchSet).isEqualTo(1); assertThat(meta.patchSet).isEqualTo(1);
assertThat(meta.messageType).isEqualTo("comment"); assertThat(meta.messageType).isEqualTo("comment");
assertThat(meta.timestamp.getTime()) assertThat(meta.timestamp.getTime())