From bd46f1ed51cb384316450e15a7b2b7ebff9f5087 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 13 Jan 2016 11:40:50 -0500 Subject: [PATCH] Notedb: Support applying labels for other users directly We already supported removing labels for other users; generalize this to support modifying arbitrary labels as well. However, this still won't be used by PostReview in the near future, since that also needs to update inline comments and drafts, which complicates things considerably. Change-Id: I1b47b691904bb9dec4d35fccd41907f1422b0691 --- .../server/notedb/ChangeNotesParser.java | 153 +++++++++--------- .../gerrit/server/notedb/ChangeUpdate.java | 48 +++--- .../gerrit/server/notedb/ChangeNotesTest.java | 30 ++++ 3 files changed, 139 insertions(+), 92 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index 3bca5d59f3..81e07b83e1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -27,7 +27,6 @@ import com.google.common.base.Optional; import com.google.common.base.Splitter; import com.google.common.base.Supplier; import com.google.common.collect.ArrayListMultimap; -import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableSet; @@ -87,9 +86,8 @@ class ChangeNotesParser implements AutoCloseable { private final ObjectId tip; private final RevWalk walk; private final Repository repo; - private final Map> - approvals; - private final Map> removedApprovals; + private final Map>> approvals; private final List allChangeMessages; private final Multimap changeMessagesByPatchSet; @@ -102,7 +100,6 @@ class ChangeNotesParser implements AutoCloseable { this.repo = repoManager.openMetadataRepository(ChangeNotes.getProjectName(change)); approvals = Maps.newHashMap(); - removedApprovals = Maps.newHashMap(); reviewers = Maps.newLinkedHashMap(); allPastReviewers = Lists.newArrayList(); submitRecords = Lists.newArrayListWithExpectedSize(1); @@ -130,9 +127,11 @@ class ChangeNotesParser implements AutoCloseable { buildApprovals() { Multimap result = ArrayListMultimap.create(approvals.keySet().size(), 3); - for (Table curr : approvals.values()) { - for (PatchSetApproval psa : curr.values()) { - result.put(psa.getPatchSetId(), psa); + for (Table> curr : approvals.values()) { + for (Optional psa : curr.values()) { + if (psa.isPresent()) { + result.put(psa.get().getPatchSetId(), psa.get()); + } } } for (Collection v : result.asMap().values()) { @@ -315,92 +314,98 @@ class ChangeNotesParser implements AutoCloseable { } } - private void parseAddApproval(PatchSet.Id psId, Account.Id accountId, + private void parseAddApproval(PatchSet.Id psId, Account.Id committerId, RevCommit commit, String line) throws ConfigInvalidException { + Account.Id accountId; + String labelVoteStr; + int s = line.indexOf(' '); + if (s > 0) { + labelVoteStr = line.substring(0, s); + PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1)); + checkFooter(ident != null, FOOTER_LABEL, line); + accountId = parseIdent(ident); + } else { + labelVoteStr = line; + accountId = committerId; + } + LabelVote l; try { - l = LabelVote.parseWithEquals(line); + l = LabelVote.parseWithEquals(labelVoteStr); } catch (IllegalArgumentException e) { ConfigInvalidException pe = parseException("invalid %s: %s", FOOTER_LABEL, line); pe.initCause(e); throw pe; } - if (isApprovalRemoved(psId, accountId, l.label())) { - return; - } - Table curr = approvals.get(psId); + Table> curr = + getApprovalsTableIfNoVotePresent(psId, accountId, l.label()); if (curr != null) { - if (curr.contains(accountId, l.label())) { - return; - } - } else { - curr = newApprovalsTable(); - approvals.put(psId, curr); + curr.put(accountId, l.label(), Optional.of(new PatchSetApproval( + new PatchSetApproval.Key( + psId, + accountId, + new LabelId(l.label())), + l.value(), + new Timestamp(commit.getCommitterIdent().getWhen().getTime())))); } - curr.put(accountId, l.label(), new PatchSetApproval( - new PatchSetApproval.Key( - psId, - accountId, - new LabelId(l.label())), - l.value(), - new Timestamp(commit.getCommitterIdent().getWhen().getTime()))); } - private static Table - newApprovalsTable() { - return Tables.newCustomTable( - Maps.> + private void parseRemoveApproval(PatchSet.Id psId, Account.Id committerId, + String line) throws ConfigInvalidException { + Account.Id accountId; + String label; + int s = line.indexOf(' '); + if (s > 0) { + label = line.substring(1, s); + PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1)); + checkFooter(ident != null, FOOTER_LABEL, line); + accountId = parseIdent(ident); + } else { + label = line.substring(1); + accountId = committerId; + } + + try { + LabelType.checkNameInternal(label); + } catch (IllegalArgumentException e) { + ConfigInvalidException pe = + parseException("invalid %s: %s", FOOTER_LABEL, line); + pe.initCause(e); + throw pe; + } + + Table> curr = + getApprovalsTableIfNoVotePresent(psId, accountId, label); + if (curr != null) { + curr.put(accountId, label, Optional. absent()); + } + } + + private Table> + getApprovalsTableIfNoVotePresent(PatchSet.Id psId, Account.Id accountId, + String label) { + + Table> curr = + approvals.get(psId); + if (curr != null) { + if (curr.contains(accountId, label)) { + return null; + } + } else { + curr = Tables.newCustomTable( + Maps.>> newHashMapWithExpectedSize(2), - new Supplier>() { + new Supplier>>() { @Override - public Map get() { + public Map> get() { return Maps.newLinkedHashMap(); } }); - } - - private void parseRemoveApproval(PatchSet.Id psId, Account.Id accountId, - String line) throws ConfigInvalidException { - Multimap curr = removedApprovals.get(psId); - if (curr == null) { - curr = HashMultimap.create(1, 1); - removedApprovals.put(psId, curr); + approvals.put(psId, curr); } - String label; - Account.Id removedAccountId; - line = line.substring(1); - int s = line.indexOf(' '); - if (s > 0) { - label = line.substring(0, s); - PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1)); - checkFooter(ident != null, FOOTER_LABEL, line); - removedAccountId = parseIdent(ident); - } else { - label = line; - removedAccountId = accountId; - } - - Table added = approvals.get(psId); - if (added != null && added.contains(accountId, label)) { - return; - } - - try { - curr.put(removedAccountId, LabelType.checkNameInternal(label)); - } catch (IllegalArgumentException e) { - ConfigInvalidException pe = - parseException("invalid %s: %s", FOOTER_LABEL, line); - pe.initCause(e); - throw pe; - } - } - - private boolean isApprovalRemoved(PatchSet.Id psId, Account.Id accountId, - String label) { - Multimap curr = removedApprovals.get(psId); - return curr != null && curr.containsEntry(accountId, label); + return curr; } private void parseSubmitRecords(List lines) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index f1843bd7ff..f896d89e89 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -24,13 +24,16 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC; import static com.google.gerrit.server.notedb.CommentsInNotesUtil.addCommentToMap; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Function; import com.google.common.base.Joiner; +import com.google.common.base.Optional; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.common.collect.Multimap; -import com.google.common.collect.MultimapBuilder; +import com.google.common.collect.Ordering; +import com.google.common.collect.Table; +import com.google.common.collect.TreeBasedTable; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; @@ -88,9 +91,8 @@ public class ChangeUpdate extends AbstractChangeUpdate { } private final AccountCache accountCache; - private final Map approvals; + private final Table> approvals; private final Map reviewers; - private final Multimap removedApprovals; private Change.Status status; private String subject; private List submitRecords; @@ -163,10 +165,15 @@ public class ChangeUpdate extends AbstractChangeUpdate { this.draftUpdateFactory = draftUpdateFactory; this.accountCache = accountCache; this.commentsUtil = commentsUtil; - this.approvals = Maps.newTreeMap(labelNameComparator); + this.approvals = TreeBasedTable.create( + labelNameComparator, + Ordering.natural().onResultOf(new Function() { + @Override + public Integer apply(Account.Id in) { + return in.get(); + } + })); this.reviewers = Maps.newLinkedHashMap(); - this.removedApprovals = - MultimapBuilder.linkedHashKeys(1).arrayListValues(1).build(); this.comments = Lists.newArrayList(); } @@ -181,7 +188,11 @@ public class ChangeUpdate extends AbstractChangeUpdate { } public void putApproval(String label, short value) { - approvals.put(label, value); + putApprovalFor(getUser().getAccountId(), label, value); + } + + public void putApprovalFor(Account.Id reviewer, String label, short value) { + approvals.put(label, reviewer, Optional.of(value)); } public void removeApproval(String label) { @@ -189,7 +200,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { } public void removeApprovalFor(Account.Id reviewer, String label) { - removedApprovals.put(reviewer, label); + approvals.put(label, reviewer, Optional. absent()); } public void merge(Iterable submitRecords) { @@ -446,14 +457,16 @@ public class ChangeUpdate extends AbstractChangeUpdate { addIdent(msg, e.getKey()).append('\n'); } - for (Map.Entry e : approvals.entrySet()) { - addFooter(msg, FOOTER_LABEL, LabelVote.create( - e.getKey(), e.getValue()).formatWithEquals()); - } - - for (Map.Entry e : removedApprovals.entries()) { - addFooter(msg, FOOTER_LABEL).append('-').append(e.getValue()); - Account.Id id = e.getKey(); + for (Table.Cell> c + : approvals.cellSet()) { + addFooter(msg, FOOTER_LABEL); + if (!c.getValue().isPresent()) { + msg.append('-').append(c.getRowKey()); + } else { + msg.append(LabelVote.create( + c.getRowKey(), c.getValue().get()).formatWithEquals()); + } + Account.Id id = c.getColumnKey(); if (!id.equals(ctl.getUser().getAccountId())) { addIdent(msg.append(' '), id); } @@ -496,7 +509,6 @@ public class ChangeUpdate extends AbstractChangeUpdate { private boolean isEmpty() { return approvals.isEmpty() - && removedApprovals.isEmpty() && changeMessage == null && comments.isEmpty() && reviewers.isEmpty() diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java index 611054d7c3..a57294dfc6 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -20,6 +20,7 @@ import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import static com.google.gerrit.testutil.TestChanges.incrementPatchSet; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.base.Function; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMultimap; @@ -27,6 +28,7 @@ import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; +import com.google.common.collect.Ordering; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Account; @@ -228,7 +230,35 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { assertThat(psa.getAccountId()).isEqualTo(otherUserId); assertThat(psa.getLabel()).isEqualTo("Not-For-Long"); assertThat(psa.getValue()).isEqualTo((short) 2); + } + @Test + public void putOtherUsersApprovals() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + update.putApproval("Code-Review", (short) 1); + update.putApprovalFor(otherUser.getAccountId(), "Code-Review", (short) -1); + update.commit(); + + ChangeNotes notes = newNotes(c); + List approvals = Ordering.natural().onResultOf( + new Function() { + @Override + public Integer apply(PatchSetApproval in) { + return in.getAccountId().get(); + } + }).sortedCopy(notes.getApprovals().get(c.currentPatchSetId())); + assertThat(approvals).hasSize(2); + + assertThat(approvals.get(0).getAccountId()) + .isEqualTo(changeOwner.getAccountId()); + assertThat(approvals.get(0).getLabel()).isEqualTo("Code-Review"); + assertThat(approvals.get(0).getValue()).isEqualTo((short) 1); + + assertThat(approvals.get(1).getAccountId()) + .isEqualTo(otherUser.getAccountId()); + assertThat(approvals.get(1).getLabel()).isEqualTo("Code-Review"); + assertThat(approvals.get(1).getValue()).isEqualTo((short) -1); } @Test