diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 8c10bc7bbc..9cd04b899b 100644 --- a/Documentation/rest-api-changes.txt +++ b/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. + 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 c05b56f124..d42a078d2f 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 @@ -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> actual = + gApi.changes().id(r1.getChangeId()).drafts(); + assertThat((Iterable) actual.keySet()).containsExactly(FILE_NAME); + List 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> actual = gApi.changes() + .id(r2.getChangeId()) + .comments(); + assertThat(actual.keySet()).containsExactly(FILE_NAME); + + List 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.> 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(); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java index 06f0a75e31..6781af275a 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java +++ b/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 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> 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> 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> comments() throws RestApiException { + throw new NotImplementedException(); + } + + @Override + public Map> drafts() throws RestApiException { + throw new NotImplementedException(); + } + @Override public ChangeInfo check() throws RestApiException { throw new NotImplementedException(); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java index e79df1c026..b9863d7652 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java +++ b/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. + *

+ * Only set in contexts where comments may come from multiple patch sets. + */ + public Integer patchSet; + public String id; public String path; public Side side; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java index 06c59eebfb..78b3f10c00 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java +++ b/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; 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, 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> comments() throws RestApiException { + try { + return listComments.apply(change); + } catch (OrmException e) { + throw new RestApiException("Cannot get comments", e); + } + } + + @Override + public Map> 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 { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java index 57ae5eaa2b..ec4b7c1c28 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java +++ b/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 list : out.values()) { - Collections.sort(list, new Comparator() { - @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 COMPARATOR = + new Comparator() { + private final Comparator> 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) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeComments.java new file mode 100644 index 0000000000..97befa097c --- /dev/null +++ b/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 { + private final Provider db; + private final ChangeData.Factory changeDataFactory; + private final Provider commentJson; + private final PatchLineCommentsUtil plcUtil; + + @Inject + ListChangeComments(Provider db, + ChangeData.Factory changeDataFactory, + Provider commentJson, + PatchLineCommentsUtil plcUtil) { + this.db = db; + this.changeDataFactory = changeDataFactory; + this.commentJson = commentJson; + this.plcUtil = plcUtil; + } + + @Override + public Map> 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())); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeDrafts.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeDrafts.java new file mode 100644 index 0000000000..2b5d7d908a --- /dev/null +++ b/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 { + private final Provider db; + private final ChangeData.Factory changeDataFactory; + private final Provider commentJson; + private final PatchLineCommentsUtil plcUtil; + + @Inject + ListChangeDrafts(Provider db, + ChangeData.Factory changeDataFactory, + Provider commentJson, + PatchLineCommentsUtil plcUtil) { + this.db = db; + this.changeDataFactory = changeDataFactory; + this.commentJson = commentJson; + this.plcUtil = plcUtil; + } + + @Override + public Map> 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 drafts = + plcUtil.draftByChangeAuthor(db.get(), cd.notes(), user.getAccountId()); + return commentJson.get() + .setFillAccounts(false) + .setFillPatchSet(true) + .format(drafts); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java index d0e4c99c94..b73af9318b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java +++ b/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);