From dc86ab526e6c58a94cad8c8c33effb16f8b5bfd9 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 15 Mar 2016 13:13:57 +0100 Subject: [PATCH] Fix PUBLISH_ALL_REVISIONS in NoteDb There was a bug in the PUBLISH_ALL_REVISIONS case where we passed the wrong PatchSet to PatchLineCommentsUtil.setCommentRevId, resulting in saving comments on the wrong side. Add a checkArgument to setCommentRevId that exposes this error, and fix it by passing the right PatchSet object. This means we may have to look up additional PatchSets for the change when writing to NoteDb. There is no way around this: if we publish a comment for PS1 when posting a review for PS2, we have not previously loaded PS1 to get the RevId, so we need to do that now. Test in CommentsIT for publishing comments on both sides simultaneously. Change-Id: Idbbc8d905a9e37ca6be9d45f8bddd0ca1fe0b248 --- .../acceptance/server/change/CommentsIT.java | 33 +++++++++++----- .../reviewdb/client/PatchLineComment.java | 4 ++ .../gerrit/server/PatchLineCommentsUtil.java | 16 +++++--- .../google/gerrit/server/PatchSetUtil.java | 15 ++++++++ .../gerrit/server/change/PostReview.java | 38 +++++++++++++++---- .../gerrit/server/notedb/ChangeUpdate.java | 2 +- 6 files changed, 86 insertions(+), 22 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java index d1f2bfe497..b7a6b93817 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -52,6 +52,8 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.regex.Matcher; +import java.util.regex.Pattern; @NoHttpd public class CommentsIT extends AbstractDaemonTest { @@ -375,6 +377,8 @@ public class CommentsIT extends AbstractDaemonTest { addDraft(r1.getChangeId(), r1.getCommit().getName(), newDraft(FILE_NAME, Side.REVISION, 1, "nit: trailing whitespace")); + addDraft(r1.getChangeId(), r1.getCommit().getName(), + newDraft(FILE_NAME, Side.PARENT, 2, "what happened to this?")); addDraft(r2.getChangeId(), r2.getCommit().getName(), newDraft(FILE_NAME, Side.REVISION, 1, "join lines")); addDraft(r2.getChangeId(), r2.getCommit().getName(), @@ -410,8 +414,11 @@ public class CommentsIT extends AbstractDaemonTest { .comments(); assertThat(ps1Map.keySet()).containsExactly(FILE_NAME); List ps1List = ps1Map.get(FILE_NAME); - assertThat(ps1List).hasSize(1); - assertThat(ps1List.get(0).message).isEqualTo("nit: trailing whitespace"); + assertThat(ps1List).hasSize(2); + assertThat(ps1List.get(0).message).isEqualTo("what happened to this?"); + assertThat(ps1List.get(0).side).isEqualTo(Side.PARENT); + assertThat(ps1List.get(1).message).isEqualTo("nit: trailing whitespace"); + assertThat(ps1List.get(1).side).isNull(); assertThat(gApi.changes() .id(r2.getChangeId()) @@ -433,17 +440,20 @@ public class CommentsIT extends AbstractDaemonTest { assertThat(messages).hasSize(1); String url = canonicalWebUrl.get(); int c = r1.getChange().getId().get(); - assertThat(messages.get(0).body()).contains( - "\n" - + "Patch Set 2:\n" + assertThat(extractComments(messages.get(0).body())).isEqualTo( + "Patch Set 2:\n" + "\n" - + "(3 comments)\n" + + "(4 comments)\n" + "\n" + "comments\n" + "\n" + url + "#/c/" + c + "/1/a.txt\n" + "File a.txt:\n" + "\n" + + "PS1, Line 2: \n" + + "what happened to this?\n" + + "\n" + + "\n" + "PS1, Line 1: ew\n" + "nit: trailing whitespace\n" + "\n" @@ -457,9 +467,14 @@ public class CommentsIT extends AbstractDaemonTest { + "\n" + "PS2, Line 2: nten\n" + "typo: content\n" - + "\n" - + "\n" - + "-- \n"); + + "\n"); + } + + private static String extractComments(String msg) { + // Extract lines between start "....." and end "-- ". + Pattern p = Pattern.compile(".*[.]{5}\n+(.*)\\n+-- \n.*", Pattern.DOTALL); + Matcher m = p.matcher(msg); + return m.matches() ? m.group(1) : msg; } private void addComment(PushOneCommit.Result r, String message) diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java index bd7b7c1345..c04b729461 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java @@ -148,6 +148,10 @@ public final class PatchLineComment { return key; } + public PatchSet.Id getPatchSetId() { + return key.getParentKey().getParentKey(); + } + public int getLine() { return lineNbr; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java index e741594c93..ecd9027bfb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java @@ -15,6 +15,7 @@ package com.google.gerrit.server; import static com.google.common.base.MoreObjects.firstNonNull; +import static com.google.common.base.Preconditions.checkArgument; import com.google.common.base.Optional; import com.google.common.base.Predicate; @@ -353,13 +354,18 @@ public class PatchLineCommentsUtil { public static RevId setCommentRevId(PatchLineComment c, PatchListCache cache, Change change, PatchSet ps) throws OrmException { + checkArgument(c.getPatchSetId().equals(ps.getId()), + "cannot set RevId for patch set %s on comment %s", ps.getId(), c); if (c.getRevId() == null) { try { - // TODO(dborowitz): Bypass cache if side is REVISION. - PatchList patchList = cache.get(change, ps); - c.setRevId((c.getSide() == (short) 0) - ? new RevId(ObjectId.toString(patchList.getOldId())) - : new RevId(ObjectId.toString(patchList.getNewId()))); + if (Side.fromShort(c.getSide()) == Side.REVISION) { + c.setRevId(ps.getRevision()); + } else { + PatchList patchList = cache.get(change, ps); + c.setRevId((c.getSide() == (short) 0) + ? new RevId(ObjectId.toString(patchList.getOldId())) + : new RevId(ObjectId.toString(patchList.getNewId()))); + } } catch (PatchListNotAvailableException e) { throw new OrmException(e); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java index 134189ebd4..1bbd735f0d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java @@ -20,6 +20,7 @@ import static com.google.gerrit.server.notedb.PatchSetState.DRAFT; import static com.google.gerrit.server.notedb.PatchSetState.PUBLISHED; import com.google.common.collect.ImmutableCollection; +import com.google.common.collect.ImmutableMap; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.RevId; @@ -72,6 +73,20 @@ public class PatchSetUtil { return notes.load().getPatchSets().values(); } + public ImmutableMap byChangeAsMap(ReviewDb db, + ChangeNotes notes) throws OrmException { + if (!migration.readChanges()) { + ImmutableMap.Builder result = + ImmutableMap.builder(); + for (PatchSet ps : ChangeUtil.PS_ID_ORDER.sortedCopy( + db.patchSets().byChange(notes.getChangeId()))) { + result.put(ps.getId(), ps); + } + return result.build(); + } + return notes.load().getPatchSets(); + } + public PatchSet insert(ReviewDb db, RevWalk rw, ChangeUpdate update, PatchSet.Id psId, ObjectId commit, boolean draft, List groups, String pushCertificate) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index 0cfe445059..74929698c2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -23,6 +23,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import com.google.auto.value.AutoValue; import com.google.common.base.MoreObjects; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Ordering; @@ -469,18 +470,15 @@ public class PostReview implements RestModifyView del.addAll(drafts.values()); break; case PUBLISH: - case PUBLISH_ALL_REVISIONS: for (PatchLineComment e : drafts.values()) { - e.setStatus(PatchLineComment.Status.PUBLISHED); - e.setWrittenOn(ctx.getWhen()); - setCommentRevId(e, patchListCache, ctx.getChange(), ps); - ups.add(e); + ups.add(publishComment(ctx, e, ps)); } break; + case PUBLISH_ALL_REVISIONS: + publishAllRevisions(ctx, drafts, ups); + break; } ChangeUpdate u = ctx.getUpdate(psId); - // TODO(dborowitz): Currently doesn't work for PUBLISH_ALL_REVISIONS with - // NoteDb. plcUtil.deleteComments(ctx.getDb(), u, del); plcUtil.putComments(ctx.getDb(), u, ups); comments.addAll(ups); @@ -526,6 +524,32 @@ public class PostReview implements RestModifyView return labels; } + private PatchLineComment publishComment(ChangeContext ctx, + PatchLineComment c, PatchSet ps) throws OrmException { + c.setStatus(PatchLineComment.Status.PUBLISHED); + c.setWrittenOn(ctx.getWhen()); + setCommentRevId(c, patchListCache, ctx.getChange(), checkNotNull(ps)); + return c; + } + + private void publishAllRevisions(ChangeContext ctx, + Map drafts, List ups) + throws OrmException { + boolean needOtherPatchSets = false; + for (PatchLineComment c : drafts.values()) { + if (!c.getPatchSetId().equals(psId)) { + needOtherPatchSets = true; + break; + } + } + Map patchSets = needOtherPatchSets + ? psUtil.byChangeAsMap(ctx.getDb(), ctx.getNotes()) + : ImmutableMap.of(psId, ps); + for (PatchLineComment e : drafts.values()) { + ups.add(publishComment(ctx, e, patchSets.get(e.getPatchSetId()))); + } + } + private boolean updateLabels(ChangeContext ctx) throws OrmException, ResourceConflictException { Map inLabels = MoreObjects.firstNonNull(in.labels, 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 d3084263bc..8741c3781c 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 @@ -306,7 +306,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { } private void verifyComment(PatchLineComment c) { - checkArgument(c.getRevId() != null); + checkArgument(c.getRevId() != null, "RevId required for comment: %s", c); checkArgument(c.getAuthor().equals(getUser().getAccountId()), "The author for the following comment does not match the author of" + " this ChangeDraftUpdate (%s): %s", getUser().getAccountId(), c);