Reject inline comments on files that do not exist in the patch set

Bug: issue 2583
Change-Id: I8df23393bc2d311c46a29fb82bca2fa7bf20488a
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>

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<RevisionResource, ReviewInput> {
   private static final Logger log = LoggerFactory.getLogger(PostReview.class);
@@ -76,6 +79,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
 
   private final Provider<ReviewDb> 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<RevisionResource, ReviewInput>
   @Inject
   PostReview(Provider<ReviewDb> db,
       ChangesCollection changes,
+      ChangeData.Factory changeDataFactory,
       ChangeUpdate.Factory updateFactory,
       ApprovalsUtil approvalsUtil,
       ChangeIndexer indexer,
@@ -101,6 +106,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       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<RevisionResource, ReviewInput>
       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<RevisionResource, ReviewInput>
     }
   }
 
-  private void checkComments(Map<String, List<Comment>> in)
+  private void checkComments(RevisionResource revision, Map<String, List<Comment>> in)
-      throws BadRequestException {
+      throws BadRequestException, OrmException {
     Iterator<Map.Entry<String, List<Comment>>> mapItr =
         in.entrySet().iterator();
+    Set<String> filePaths =
+        Sets.newHashSet(changeDataFactory.create(
+            db.get(), revision.getChange()).filePaths(
+                revision.getPatchSet()));
     while (mapItr.hasNext()) {
       Map.Entry<String, List<Comment>> 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<Comment> list = ent.getValue();
       if (list == null) {
         mapItr.remove();

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<PatchLineComment> plc) {
+  public void setPatchLineComments(final List<PatchLineComment> plc)
+      throws OrmException {
     inlineComments = plc;
 
     Set<String> paths = new HashSet<>();

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) {
+  static ChangeData createForTest(Change.Id id, int currentPatchSetId) {
-    return new ChangeData(null, null, null, null, null, null, null, null, id);
+    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<PatchSet> patches;
   private ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals;
   private List<PatchSetApproval> currentApprovals;
-  private List<String> currentFiles;
+  private Map<Integer, List<String>> files = new HashMap<>();
   private Collection<PatchLineComment> comments;
   private CurrentUser visibleTo;
   private ChangeControl changeControl;
@@ -258,27 +261,35 @@ public class ChangeData {
     returnedBySource = s;
   }
 
-  public void setCurrentFilePaths(List<String> filePaths) {
+  public void setCurrentFilePaths(List<String> filePaths) throws OrmException {
-    currentFiles = ImmutableList.copyOf(filePaths);
+    PatchSet ps = currentPatchSet();
+    if (ps != null) {
+      files.put(ps.getPatchSetId(), ImmutableList.copyOf(filePaths));
+    }
   }
 
   public List<String> currentFilePaths() throws OrmException {
-    if (currentFiles == null) {
-      Change c = change();
-      if (c == null) {
-        return null;
-      }
     PatchSet ps = currentPatchSet();
     if (ps == null) {
       return null;
     }
+    return filePaths(currentPatchSet);
+  }
+
+  public List<String> filePaths(PatchSet ps) throws OrmException {
+    if (!files.containsKey(ps.getPatchSetId())) {
+      Change c = change();
+      if (c == null) {
+        return null;
+      }
 
       PatchList p;
       try {
         p = patchListCache.get(c, ps);
       } catch (PatchListNotAvailableException e) {
-        currentFiles = Collections.emptyList();
+        List<String> emptyFileList = Collections.emptyList();
-        return currentFiles;
+        files.put(ps.getPatchSetId(), emptyFileList);
+        return emptyFileList;
       }
 
       List<String> 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 {

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.<String, List<ReviewInput.Comment>> of(
-        "Foo.java", ImmutableList.<ReviewInput.Comment> of(comment));
+        Patch.COMMIT_MSG, ImmutableList.<ReviewInput.Comment> of(comment));
     postReview.apply(new RevisionResource(
         changes.parse(change.getId()), ins.getPatchSet()), input);
 

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;
   }