Change endpoints for all draft/published comments on a change

We would like to be able to publish all drafts on all patch sets at
once, which implies showing all of them in the reply box first.
Sending multiple requests from the client would be wasteful, so we
need a single endpoint on the change to list all drafts. Similarly, we
need to fetch all published comments at once to line them up in the
history table.

The output of these endpoints is keyed by file, then sorted by patch
set. The typical way a code review proceeds is one file at a time:
correct all comments on one file, regardless of patch set, then move
on to the next file. We avoid a further nested map as this JSON type
would just get a little messy.

Change-Id: I0448f74357a45689a5a125968d3a0219e3d98fa3

Documentation/rest-api-changes.txt

@@ -1146,6 +1146,109 @@ Adds or updates the change in the secondary index.
   HTTP/1.1 204 No Content
 ----
 
+[[list-change-comments]]
+=== List Change Comments
+--
+'GET /changes/link:#change-id[\{change-id\}]/comments'
+--
+
+Lists the published comments of all revisions of the change.
+
+Returns a map of file paths to lists of link:#comment-info[CommentInfo]
+entries. The entries in the map are sorted by file path, and the
+comments for each path are sorted by patch set number. Each comment has
+the `patch_set` and `author` fields set.
+
+.Request
+----
+  GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/comments HTTP/1.0
+----
+
+.Response
+----
+  HTTP/1.1 200 OK
+  Content-Disposition: attachment
+  Content-Type: application/json; charset=UTF-8
+
+  )]}'
+  {
+    "gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java": [
+      {
+        "patch_set": 1,
+        "id": "TvcXrmjM",
+        "line": 23,
+        "message": "[nit] trailing whitespace",
+        "updated": "2013-02-26 15:40:43.986000000"
+        "author": {
+          "_account_id": 1000096,
+          "name": "John Doe",
+          "email": "john.doe@example.com"
+        }
+      },
+      {
+        "patch_set": 2,
+        "id": "TveXwFiA",
+        "line": 49,
+        "in_reply_to": "TfYX-Iuo",
+        "message": "Done",
+        "updated": "2013-02-26 15:40:45.328000000"
+        "author": {
+          "_account_id": 1000097,
+          "name": "Jane Roe",
+          "email": "jane.roe@example.com"
+        }
+      }
+    ]
+  }
+----
+
+[[list-change-drafts]]
+=== List Change Drafts
+--
+'GET /changes/link:#change-id[\{change-id\}]/drafts'
+--
+
+Lists the draft comments of all revisions of the change that belong to
+the calling user.
+
+Returns a map of file paths to lists of link:#comment-info[CommentInfo]
+entries. The entries in the map are sorted by file path, and the
+comments for each path are sorted by patch set number. Each comment has
+the `patch_set` field set, and no `author`.
+
+.Request
+----
+  GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/drafts HTTP/1.0
+----
+
+.Response
+----
+  HTTP/1.1 200 OK
+  Content-Disposition: attachment
+  Content-Type: application/json; charset=UTF-8
+
+  )]}'
+  {
+    "gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java": [
+      {
+        "patch_set": 1,
+        "id": "TvcXrmjM",
+        "line": 23,
+        "message": "[nit] trailing whitespace",
+        "updated": "2013-02-26 15:40:43.986000000"
+      },
+      {
+        "patch_set": 2,
+        "id": "TveXwFiA",
+        "line": 49,
+        "in_reply_to": "TfYX-Iuo",
+        "message": "Done",
+        "updated": "2013-02-26 15:40:45.328000000"
+      }
+    ]
+  }
+----
+
 [[check-change]]
 === Check change
 --
@@ -2586,7 +2689,7 @@ all LFs are included in the Prolog code:
 ----
 
 [[list-drafts]]
-=== List Drafts
+=== List Revision Drafts
 --
 'GET /changes/link:#change-id[\{change-id\}]/revisions/link:#revision-id[\{revision-id\}]/drafts/'
 --
@@ -2594,9 +2697,8 @@ all LFs are included in the Prolog code:
 Lists the draft comments of a revision that belong to the calling
 user.
 
-As result a map is returned that maps the file path to a list of
-link:#comment-info[CommentInfo] entries. The entries in the map are
-sorted by file path.
+Returns a map of file paths to lists of link:#comment-info[CommentInfo]
+entries. The entries in the map are sorted by file path.
 
 .Request
 ----
@@ -2765,7 +2867,7 @@ Deletes a draft comment from a revision.
 ----
 
 [[list-comments]]
-=== List Comments
+=== List Revision Comments
 --
 'GET /changes/link:#change-id[\{change-id\}]/revisions/link:#revision-id[\{revision-id\}]/comments/'
 --
@@ -3494,6 +3596,9 @@ The `CommentInfo` entity contains information about an inline comment.
 [options="header",cols="1,^1,5"]
 |===========================
 |Field Name    ||Description
+|`patch_set`   |optional|
+The patch set number for the comment; only set in contexts where +
+comments may be returned for multiple patch sets.
 |`id`          ||The URL encoded UUID of the comment.
 |`path`        |optional|
 The path of the file for which the inline comment was done. +

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java

@@ -15,7 +15,11 @@
 package com.google.gerrit.acceptance.server.change;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME;
+import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT;
 
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -186,6 +190,98 @@ public class CommentsIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void listChangeDrafts() throws Exception {
+    PushOneCommit.Result r1 = createChange();
+
+    PushOneCommit.Result r2 = pushFactory.create(
+          db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "new cntent",
+          r1.getChangeId())
+        .to("refs/for/master");
+
+
+    setApiUser(admin);
+    addDraft(r1.getChangeId(), r1.getCommit().getName(),
+        newDraft(FILE_NAME, Side.REVISION, 1, "nit: trailing whitespace"));
+    addDraft(r2.getChangeId(), r2.getCommit().getName(),
+        newDraft(FILE_NAME, Side.REVISION, 1, "typo: content"));
+
+    setApiUser(user);
+    addDraft(r2.getChangeId(), r2.getCommit().getName(),
+        newDraft(FILE_NAME, Side.REVISION, 1, "+1, please fix"));
+
+    setApiUser(admin);
+    Map<String, List<CommentInfo>> actual =
+        gApi.changes().id(r1.getChangeId()).drafts();
+    assertThat((Iterable<?>) actual.keySet()).containsExactly(FILE_NAME);
+    List<CommentInfo> comments = actual.get(FILE_NAME);
+    assertThat(comments).hasSize(2);
+
+    CommentInfo c1 = comments.get(0);
+    assertThat(c1.author).isNull();
+    assertThat(c1.patchSet).isEqualTo(1);
+    assertThat(c1.message).isEqualTo("nit: trailing whitespace");
+    assertThat(c1.side).isNull();
+    assertThat(c1.line).isEqualTo(1);
+
+    CommentInfo c2 = comments.get(1);
+    assertThat(c2.author).isNull();
+    assertThat(c2.patchSet).isEqualTo(2);
+    assertThat(c2.message).isEqualTo("typo: content");
+    assertThat(c2.side).isNull();
+    assertThat(c2.line).isEqualTo(1);
+  }
+
+  @Test
+  public void listChangeComments() throws Exception {
+    PushOneCommit.Result r1 = createChange();
+
+    PushOneCommit.Result r2 = pushFactory.create(
+          db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "new cntent",
+          r1.getChangeId())
+        .to("refs/for/master");
+
+    addComment(r1, "nit: trailing whitespace");
+    addComment(r2, "typo: content");
+
+    Map<String, List<CommentInfo>> actual = gApi.changes()
+        .id(r2.getChangeId())
+        .comments();
+    assertThat(actual.keySet()).containsExactly(FILE_NAME);
+
+    List<CommentInfo> comments = actual.get(FILE_NAME);
+    assertThat(comments).hasSize(2);
+
+    CommentInfo c1 = comments.get(0);
+    assertThat(c1.author._accountId).isEqualTo(user.getId().get());
+    assertThat(c1.patchSet).isEqualTo(1);
+    assertThat(c1.message).isEqualTo("nit: trailing whitespace");
+    assertThat(c1.side).isNull();
+    assertThat(c1.line).isEqualTo(1);
+
+    CommentInfo c2 = comments.get(1);
+    assertThat(c2.author._accountId).isEqualTo(user.getId().get());
+    assertThat(c2.patchSet).isEqualTo(2);
+    assertThat(c2.message).isEqualTo("typo: content");
+    assertThat(c2.side).isNull();
+    assertThat(c2.line).isEqualTo(1);
+  }
+
+  private void addComment(PushOneCommit.Result r, String message)
+      throws Exception {
+    CommentInput c = new CommentInput();
+    c.line = 1;
+    c.message = message;
+    c.path = FILE_NAME;
+    ReviewInput in = new ReviewInput();
+    in.comments = ImmutableMap.<String, List<CommentInput>> of(
+        FILE_NAME, ImmutableList.of(c));
+    gApi.changes()
+        .id(r.getChangeId())
+        .revision(r.getCommit().name())
+        .review(in);
+  }
+
   private CommentInfo addDraft(String changeId, String revId, DraftInput in)
       throws Exception {
     return gApi.changes().id(changeId).revision(revId).createDraft(in).get();

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java

@@ -16,6 +16,7 @@ package com.google.gerrit.extensions.api.changes;
 
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.CommentInfo;
 import com.google.gerrit.extensions.common.EditInfo;
 import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
 import com.google.gerrit.extensions.restapi.NotImplementedException;
@@ -23,6 +24,7 @@ import com.google.gerrit.extensions.restapi.RestApiException;
 
 import java.util.EnumSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 public interface ChangeApi {
@@ -106,6 +108,24 @@ public interface ChangeApi {
    */
   Set<String> getHashtags() throws RestApiException;
 
+  /**
+   * Get all published comments on a change.
+   *
+   * @return comments in a map keyed by path; comments have the {@code revision}
+   *     field set to indicate their patch set.
+   * @throws RestApiException
+   */
+  Map<String, List<CommentInfo>> comments() throws RestApiException;
+
+  /**
+   * Get all draft comments for the current user on a change.
+   *
+   * @return drafts in a map keyed by path; comments have the {@code revision}
+   *     field set to indicate their patch set.
+   * @throws RestApiException
+   */
+  Map<String, List<CommentInfo>> drafts() throws RestApiException;
+
   ChangeInfo check() throws RestApiException;
   ChangeInfo check(FixInput fix) throws RestApiException;
 
@@ -249,6 +269,16 @@ public interface ChangeApi {
       throw new NotImplementedException();
     }
 
+    @Override
+    public Map<String, List<CommentInfo>> comments() throws RestApiException {
+      throw new NotImplementedException();
+    }
+
+    @Override
+    public Map<String, List<CommentInfo>> drafts() throws RestApiException {
+      throw new NotImplementedException();
+    }
+
     @Override
     public ChangeInfo check() throws RestApiException {
       throw new NotImplementedException();

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java

@@ -17,6 +17,13 @@ package com.google.gerrit.extensions.client;
 import java.sql.Timestamp;
 
 public abstract class Comment {
+  /**
+   * Patch set number containing this commit.
+   * <p>
+   * Only set in contexts where comments may come from multiple patch sets.
+   */
+  public Integer patchSet;
+
   public String id;
   public String path;
   public Side side;

gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java

@@ -26,6 +26,7 @@ import com.google.gerrit.extensions.api.changes.RevertInput;
 import com.google.gerrit.extensions.api.changes.RevisionApi;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.CommentInfo;
 import com.google.gerrit.extensions.common.EditInfo;
 import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
 import com.google.gerrit.extensions.restapi.IdString;
@@ -40,6 +41,8 @@ import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.Check;
 import com.google.gerrit.server.change.GetHashtags;
 import com.google.gerrit.server.change.GetTopic;
+import com.google.gerrit.server.change.ListChangeComments;
+import com.google.gerrit.server.change.ListChangeDrafts;
 import com.google.gerrit.server.change.PostHashtags;
 import com.google.gerrit.server.change.PostReviewers;
 import com.google.gerrit.server.change.PutTopic;
@@ -56,6 +59,7 @@ import com.google.inject.assistedinject.Assisted;
 import java.io.IOException;
 import java.util.EnumSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 class ChangeApiImpl implements ChangeApi {
@@ -78,6 +82,8 @@ class ChangeApiImpl implements ChangeApi {
   private final Provider<ChangeJson> changeJson;
   private final PostHashtags postHashtags;
   private final GetHashtags getHashtags;
+  private final ListChangeComments listComments;
+  private final ListChangeDrafts listDrafts;
   private final Check check;
   private final ChangeEdits.Detail editDetail;
 
@@ -96,6 +102,8 @@ class ChangeApiImpl implements ChangeApi {
       Provider<ChangeJson> changeJson,
       PostHashtags postHashtags,
       GetHashtags getHashtags,
+      ListChangeComments listComments,
+      ListChangeDrafts listDrafts,
       Check check,
       ChangeEdits.Detail editDetail,
       @Assisted ChangeResource change) {
@@ -113,6 +121,8 @@ class ChangeApiImpl implements ChangeApi {
     this.changeJson = changeJson;
     this.postHashtags = postHashtags;
     this.getHashtags = getHashtags;
+    this.listComments = listComments;
+    this.listDrafts = listDrafts;
     this.check = check;
     this.editDetail = editDetail;
     this.change = change;
@@ -297,6 +307,24 @@ class ChangeApiImpl implements ChangeApi {
     }
   }
 
+  @Override
+  public Map<String, List<CommentInfo>> comments() throws RestApiException {
+    try {
+      return listComments.apply(change);
+    } catch (OrmException e) {
+      throw new RestApiException("Cannot get comments", e);
+    }
+  }
+
+  @Override
+  public Map<String, List<CommentInfo>> drafts() throws RestApiException {
+    try {
+      return listDrafts.apply(change);
+    } catch (OrmException e) {
+      throw new RestApiException("Cannot get drafts", e);
+    }
+  }
+
   @Override
   public ChangeInfo check() throws RestApiException {
     try {

gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java

@@ -17,6 +17,8 @@ package com.google.gerrit.server.change;
 import static com.google.common.base.MoreObjects.firstNonNull;
 
 import com.google.common.base.Strings;
+import com.google.common.collect.ComparisonChain;
+import com.google.common.collect.Ordering;
 import com.google.gerrit.extensions.client.Comment.Range;
 import com.google.gerrit.extensions.client.Side;
 import com.google.gerrit.extensions.common.CommentInfo;
@@ -39,6 +41,7 @@ class CommentJson {
   private final AccountLoader.Factory accountLoaderFactory;
 
   private boolean fillAccounts = true;
+  private boolean fillPatchSet;
 
   @Inject
   CommentJson(AccountLoader.Factory accountLoaderFactory) {
@@ -50,6 +53,11 @@ class CommentJson {
     return this;
   }
 
+  CommentJson setFillPatchSet(boolean fillPatchSet) {
+    this.fillPatchSet = fillPatchSet;
+    return this;
+  }
+
   CommentInfo format(PatchLineComment c) throws OrmException {
     AccountLoader loader = null;
     if (fillAccounts) {
@@ -81,20 +89,7 @@ class CommentJson {
     }
 
     for (List<CommentInfo> list : out.values()) {
-      Collections.sort(list, new Comparator<CommentInfo>() {
-        @Override
-        public int compare(CommentInfo a, CommentInfo b) {
-          int c = firstNonNull(a.side, Side.REVISION).ordinal()
-                - firstNonNull(b.side, Side.REVISION).ordinal();
-          if (c == 0) {
-            c = firstNonNull(a.line, 0) - firstNonNull(b.line, 0);
-          }
-          if (c == 0) {
-            c = a.id.compareTo(b.id);
-          }
-          return c;
-        }
-      });
+      Collections.sort(list, COMPARATOR);
     }
 
     if (accountLoader != null) {
@@ -104,8 +99,31 @@ class CommentJson {
     return out;
   }
 
+  private static final Comparator<CommentInfo> COMPARATOR =
+      new Comparator<CommentInfo>() {
+        private final Comparator<Comparable<?>> ORDER =
+            Ordering.natural().nullsFirst();
+
+        @Override
+        public int compare(CommentInfo a, CommentInfo b) {
+          return ComparisonChain.start()
+              .compare(a.patchSet, b.patchSet, ORDER)
+              .compare(side(a), side(b))
+              .compare(a.line, b.line, ORDER)
+              .compare(a.id, b.id)
+              .result();
+        }
+
+        private int side(CommentInfo c) {
+          return firstNonNull(c.side, Side.REVISION).ordinal();
+        }
+      };
+
   private CommentInfo toCommentInfo(PatchLineComment c, AccountLoader loader) {
     CommentInfo r = new CommentInfo();
+    if (fillPatchSet) {
+      r.patchSet = c.getKey().getParentKey().getParentKey().get();
+    }
     r.id = Url.encode(c.getKey().get());
     r.path = c.getKey().getParentKey().getFileName();
     if (c.getSide() == 0) {

gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeComments.java

@@ -0,0 +1,58 @@
+// Copyright (C) 2015 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.RestReadView;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.PatchLineCommentsUtil;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+
+import java.util.List;
+import java.util.Map;
+
+@Singleton
+public class ListChangeComments implements RestReadView<ChangeResource> {
+  private final Provider<ReviewDb> db;
+  private final ChangeData.Factory changeDataFactory;
+  private final Provider<CommentJson> commentJson;
+  private final PatchLineCommentsUtil plcUtil;
+
+  @Inject
+  ListChangeComments(Provider<ReviewDb> db,
+      ChangeData.Factory changeDataFactory,
+      Provider<CommentJson> commentJson,
+      PatchLineCommentsUtil plcUtil) {
+    this.db = db;
+    this.changeDataFactory = changeDataFactory;
+    this.commentJson = commentJson;
+    this.plcUtil = plcUtil;
+  }
+
+  @Override
+  public Map<String, List<CommentInfo>> apply(
+      ChangeResource rsrc) throws AuthException, OrmException {
+    ChangeData cd = changeDataFactory.create(db.get(), rsrc.getControl());
+    return commentJson.get()
+        .setFillAccounts(true)
+        .setFillPatchSet(true)
+        .format(plcUtil.publishedByChange(db.get(), cd.notes()));
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeDrafts.java

@@ -0,0 +1,66 @@
+// Copyright (C) 2015 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.RestReadView;
+import com.google.gerrit.reviewdb.client.PatchLineComment;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.PatchLineCommentsUtil;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+
+import java.util.List;
+import java.util.Map;
+
+@Singleton
+public class ListChangeDrafts implements RestReadView<ChangeResource> {
+  private final Provider<ReviewDb> db;
+  private final ChangeData.Factory changeDataFactory;
+  private final Provider<CommentJson> commentJson;
+  private final PatchLineCommentsUtil plcUtil;
+
+  @Inject
+  ListChangeDrafts(Provider<ReviewDb> db,
+      ChangeData.Factory changeDataFactory,
+      Provider<CommentJson> commentJson,
+      PatchLineCommentsUtil plcUtil) {
+    this.db = db;
+    this.changeDataFactory = changeDataFactory;
+    this.commentJson = commentJson;
+    this.plcUtil = plcUtil;
+  }
+
+  @Override
+  public Map<String, List<CommentInfo>> apply(
+      ChangeResource rsrc) throws AuthException, OrmException {
+    if (!rsrc.getControl().getCurrentUser().isIdentifiedUser()) {
+      throw new AuthException("Authentication required");
+    }
+    IdentifiedUser user = (IdentifiedUser) rsrc.getControl().getCurrentUser();
+    ChangeData cd = changeDataFactory.create(db.get(), rsrc.getControl());
+    List<PatchLineComment> drafts =
+        plcUtil.draftByChangeAuthor(db.get(), cd.notes(), user.getAccountId());
+    return commentJson.get()
+        .setFillAccounts(false)
+        .setFillPatchSet(true)
+        .format(drafts);
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java

@@ -52,6 +52,8 @@ public class Module extends RestApiModule {
     get(CHANGE_KIND, "topic").to(GetTopic.class);
     get(CHANGE_KIND, "in").to(IncludedIn.class);
     get(CHANGE_KIND, "hashtags").to(GetHashtags.class);
+    get(CHANGE_KIND, "comments").to(ListChangeComments.class);
+    get(CHANGE_KIND, "drafts").to(ListChangeDrafts.class);
     get(CHANGE_KIND, "check").to(Check.class);
     post(CHANGE_KIND, "check").to(Check.class);
     put(CHANGE_KIND, "topic").to(PutTopic.class);