From dd6748a79f363d3d25401823386a537d5c38ece2 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 10 Apr 2014 10:41:45 +0200 Subject: [PATCH] Reject inline comments on files that do not exist in the patch set Bug: issue 2583 Change-Id: I8df23393bc2d311c46a29fb82bca2fa7bf20488a Signed-off-by: Edwin Kempin --- .../gerrit/server/change/PostReview.java | 22 +++++++++-- .../gerrit/server/mail/CommentSender.java | 3 +- .../server/query/change/ChangeData.java | 39 ++++++++++++------- .../change/AbstractQueryChangesTest.java | 3 +- .../query/change/RegexPathPredicateTest.java | 2 +- 5 files changed, 49 insertions(+), 20 deletions(-) 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 0a2ccef1bc..0079068044 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 @@ -20,6 +20,7 @@ import com.google.common.base.Objects; import com.google.common.base.Strings; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import com.google.common.util.concurrent.CheckedFuture; import com.google.common.util.concurrent.Futures; import com.google.gerrit.common.ChangeHooks; @@ -51,6 +52,7 @@ import com.google.gerrit.server.account.AccountsCollection; import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.util.LabelVote; import com.google.gerrit.server.util.TimeUtil; import com.google.gwtorm.server.OrmException; @@ -66,6 +68,7 @@ import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; public class PostReview implements RestModifyView { private static final Logger log = LoggerFactory.getLogger(PostReview.class); @@ -76,6 +79,7 @@ public class PostReview implements RestModifyView private final Provider db; private final ChangesCollection changes; + private final ChangeData.Factory changeDataFactory; private final ChangeUpdate.Factory updateFactory; private final ApprovalsUtil approvalsUtil; private final ChangeIndexer indexer; @@ -93,6 +97,7 @@ public class PostReview implements RestModifyView @Inject PostReview(Provider db, ChangesCollection changes, + ChangeData.Factory changeDataFactory, ChangeUpdate.Factory updateFactory, ApprovalsUtil approvalsUtil, ChangeIndexer indexer, @@ -101,6 +106,7 @@ public class PostReview implements RestModifyView ChangeHooks hooks) { this.db = db; this.changes = changes; + this.changeDataFactory = changeDataFactory; this.updateFactory = updateFactory; this.approvalsUtil = approvalsUtil; this.indexer = indexer; @@ -120,7 +126,7 @@ public class PostReview implements RestModifyView checkLabels(revision, input.strictLabels, input.labels); } if (input.comments != null) { - checkComments(input.comments); + checkComments(revision, input.comments); } if (input.notify == null) { log.warn("notify = null; assuming notify = NONE"); @@ -266,13 +272,23 @@ public class PostReview implements RestModifyView } } - private void checkComments(Map> in) - throws BadRequestException { + private void checkComments(RevisionResource revision, Map> in) + throws BadRequestException, OrmException { Iterator>> mapItr = in.entrySet().iterator(); + Set filePaths = + Sets.newHashSet(changeDataFactory.create( + db.get(), revision.getChange()).filePaths( + revision.getPatchSet())); while (mapItr.hasNext()) { Map.Entry> ent = mapItr.next(); String path = ent.getKey(); + if (!filePaths.contains(path) && !Patch.COMMIT_MSG.equals(path)) { + throw new BadRequestException(String.format( + "file %s not found in revision %s", + path, revision.getChange().currentPatchSetId())); + } + List list = ent.getValue(); if (list == null) { mapItr.remove(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java index 32a95ddeac..2beb49fbd7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java @@ -62,7 +62,8 @@ public class CommentSender extends ReplyToChangeSender { this.notify = notify; } - public void setPatchLineComments(final List plc) { + public void setPatchLineComments(final List plc) + throws OrmException { inlineComments = plc; Set paths = new HashSet<>(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index 3d7c7793d1..e5248f2ab7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -62,6 +62,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -150,8 +151,10 @@ public class ChangeData { * @param id change ID * @return instance for testing. */ - static ChangeData createForTest(Change.Id id) { - return new ChangeData(null, null, null, null, null, null, null, null, id); + static ChangeData createForTest(Change.Id id, int currentPatchSetId) { + ChangeData cd = new ChangeData(null, null, null, null, null, null, null, null, id); + cd.currentPatchSet = new PatchSet(new PatchSet.Id(id, currentPatchSetId)); + return cd; } private final ReviewDb db; @@ -172,7 +175,7 @@ public class ChangeData { private Collection patches; private ListMultimap allApprovals; private List currentApprovals; - private List currentFiles; + private Map> files = new HashMap<>(); private Collection comments; private CurrentUser visibleTo; private ChangeControl changeControl; @@ -258,27 +261,35 @@ public class ChangeData { returnedBySource = s; } - public void setCurrentFilePaths(List filePaths) { - currentFiles = ImmutableList.copyOf(filePaths); + public void setCurrentFilePaths(List filePaths) throws OrmException { + PatchSet ps = currentPatchSet(); + if (ps != null) { + files.put(ps.getPatchSetId(), ImmutableList.copyOf(filePaths)); + } } public List currentFilePaths() throws OrmException { - if (currentFiles == null) { + PatchSet ps = currentPatchSet(); + if (ps == null) { + return null; + } + return filePaths(currentPatchSet); + } + + public List filePaths(PatchSet ps) throws OrmException { + if (!files.containsKey(ps.getPatchSetId())) { Change c = change(); if (c == null) { return null; } - PatchSet ps = currentPatchSet(); - if (ps == null) { - return null; - } PatchList p; try { p = patchListCache.get(c, ps); } catch (PatchListNotAvailableException e) { - currentFiles = Collections.emptyList(); - return currentFiles; + List emptyFileList = Collections.emptyList(); + files.put(ps.getPatchSetId(), emptyFileList); + return emptyFileList; } List r = new ArrayList<>(p.getPatches().size()); @@ -302,9 +313,9 @@ public class ChangeData { } } Collections.sort(r); - currentFiles = Collections.unmodifiableList(r); + files.put(ps.getPatchSetId(), Collections.unmodifiableList(r)); } - return currentFiles; + return files.get(ps.getPatchSetId()); } public ChangedLines changedLines() throws OrmException { diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index ef92cbf250..0bea141897 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -34,6 +34,7 @@ import com.google.gerrit.lifecycle.LifecycleManager; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; @@ -721,7 +722,7 @@ public abstract class AbstractQueryChangesTest { comment.line = 1; comment.message = "inline"; input.comments = ImmutableMap.> of( - "Foo.java", ImmutableList. of(comment)); + Patch.COMMIT_MSG, ImmutableList. of(comment)); postReview.apply(new RevisionResource( changes.parse(change.getId()), ins.getPatchSet()), input); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexPathPredicateTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexPathPredicateTest.java index a79922857f..8f1fa868a5 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexPathPredicateTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexPathPredicateTest.java @@ -84,7 +84,7 @@ public class RegexPathPredicateTest { private static ChangeData change(String... files) throws OrmException { Arrays.sort(files); - ChangeData cd = ChangeData.createForTest(new Change.Id(1)); + ChangeData cd = ChangeData.createForTest(new Change.Id(1), 1); cd.setCurrentFilePaths(Arrays.asList(files)); return cd; }