Store the number of unresolved comments in change index

When changes are queried, we want to return the number of unresolved
comments with each change. This number should be pre-counted rather than
loading all the comments and counting each time.

We also add new search operators, which allow user to query by
"has:unresolved" and "unresolved:<RELATION><NUMBER>".

The 'unresolvedCommentCount' field of 'ChangeData' will be null if
'lazyLoad' is false.

Change-Id: I97c6aeb47db48f56f94c5d184dfa36c8d7868cc8
This commit is contained in:
Changcheng Xiao 2017-02-08 13:04:07 +01:00
parent 051e68013b
commit 81c4809ac3
15 changed files with 236 additions and 6 deletions
Documentation
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance
api/revision
server/change
gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common
gerrit-gwtui/src/main/java/com/google/gerrit/client
gerrit-lucene/src/main/java/com/google/gerrit/lucene
gerrit-server/src
main/java/com/google/gerrit/server
test/java/com/google/gerrit/server/query/change

@ -5152,6 +5152,8 @@ Only set if link:#submittable[requested].
Number of inserted lines.
|`deletions` ||
Number of deleted lines.
|`unresolved_comment_count` |optional|
Number of unresolved comments. Not set if the current change index doesn't have the data.
|`_number` ||The legacy numeric ID of the change.
|`owner` ||
The owner of the change as an link:rest-api-accounts.html#account-info[

@ -278,6 +278,10 @@ has:edit::
+
True if the change has inline edit created by the current user.
has:unresolved::
+
True if the change has unresolved comments.
[[is]]
[[is-starred]]
is:starred::
@ -417,6 +421,16 @@ link:rest-api-changes.html#submit-record[SubmitRecord]. This operator
only applies to the top-level status; individual label statuses can be
searched link:#labels[by label].
[[unresolved]]
unresolved:'RELATION''NUMBER'::
+
True if the number of unresolved comments satisfies the given relation for the given number.
+
For example, unresolved:>0 will be true for any change which has at least one unresolved
comment while unresolved:0 will be true for any change which has all comments resolved.
+
Valid relations are >=, >, <=, <, or no relation, which will match if the number of unresolved
comments is exactly equal.
== Argument Quoting

@ -17,14 +17,17 @@ package com.google.gerrit.acceptance.api.revision;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME;
import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT;
import static com.google.gerrit.extensions.common.RobotCommentInfoSubject.assertThatList;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput;
import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.FixReplacementInfo;
import com.google.gerrit.extensions.common.FixSuggestionInfo;
import com.google.gerrit.extensions.common.RobotCommentInfo;
@ -364,6 +367,31 @@ public class RobotCommentsIT extends AbstractDaemonTest {
gApi.changes().id(changeId).current().review(reviewInput);
}
@Test
public void queryChangesWithUnresolvedCommentCount() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
PushOneCommit.Result r1 = createChange();
PushOneCommit.Result r2 =
pushFactory
.create(
db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "new content", r1.getChangeId())
.to("refs/for/master");
addRobotComment(r2.getChangeId(), createRobotCommentInputWithMandatoryFields());
AcceptanceTestRequestScope.Context ctx = disableDb();
try {
ChangeInfo result = Iterables.getOnlyElement(query(r2.getChangeId()));
// currently, we create all robot comments as 'resolved' by default.
// if we allow users to resolve a robot comment, then this test should
// be modified.
assertThat(result.unresolvedCommentCount).isEqualTo(0);
} finally {
enableDb(ctx);
}
}
private RobotCommentInput createRobotCommentInputWithMandatoryFields() {
RobotCommentInput in = new RobotCommentInput();
in.robotId = "happyRobot";

@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.extensions.api.changes.DraftInput;
@ -31,6 +32,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.IdString;
@ -401,7 +403,7 @@ public class CommentsIT extends AbstractDaemonTest {
addComment(r1, "nit: trailing whitespace");
Map<String, List<CommentInfo>> result = getPublishedComments(changeId, revId);
assertThat(result.get(FILE_NAME)).hasSize(2);
addComment(r1, "nit: trailing whitespace", true);
addComment(r1, "nit: trailing whitespace", true, false);
result = getPublishedComments(changeId, revId);
assertThat(result.get(FILE_NAME)).hasSize(2);
@ -411,7 +413,7 @@ public class CommentsIT extends AbstractDaemonTest {
.to("refs/for/master");
changeId = r2.getChangeId();
revId = r2.getCommit().getName();
addComment(r2, "nit: trailing whitespace", true);
addComment(r2, "nit: trailing whitespace", true, false);
result = getPublishedComments(changeId, revId);
assertThat(result.get(FILE_NAME)).hasSize(1);
}
@ -694,6 +696,30 @@ public class CommentsIT extends AbstractDaemonTest {
assertThat(drafts.get(0).tag).isEqualTo("tag2");
}
@Test
public void queryChangesWithUnresolvedCommentCount() throws Exception {
PushOneCommit.Result r1 = createChange();
addComment(r1, "comment 1", false, true);
addComment(r1, "nit: trailing whitespace", false, null);
PushOneCommit.Result r2 =
pushFactory
.create(
db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "new cntent", r1.getChangeId())
.to("refs/for/master");
addComment(r2, "typo: content", false, false);
AcceptanceTestRequestScope.Context ctx = disableDb();
try {
ChangeInfo result = Iterables.getOnlyElement(query(r2.getChangeId()));
assertThat(result.unresolvedCommentCount).isEqualTo(1);
} finally {
enableDb(ctx);
}
}
private static String extractComments(String msg) {
// Extract lines between start "....." and end "-- ".
Pattern p = Pattern.compile(".*[.]{5}\n+(.*)\\n+-- \n.*", Pattern.DOTALL);
@ -709,15 +735,17 @@ public class CommentsIT extends AbstractDaemonTest {
}
private void addComment(PushOneCommit.Result r, String message) throws Exception {
addComment(r, message, false);
addComment(r, message, false, false);
}
private void addComment(PushOneCommit.Result r, String message, boolean omitDuplicateComments)
private void addComment(
PushOneCommit.Result r, String message, boolean omitDuplicateComments, Boolean unresolved)
throws Exception {
CommentInput c = new CommentInput();
c.line = 1;
c.message = message;
c.path = FILE_NAME;
c.unresolved = unresolved;
ReviewInput in = newInput(c);
in.omitDuplicateComments = omitDuplicateComments;
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in);

@ -353,6 +353,7 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
ChangeField.STORED_SUBMIT_RECORD_LENIENT.getName(),
ChangeField.SUBMIT_RULE_OPTIONS_LENIENT,
cd);
decodeUnresolvedCommentCount(source, ChangeField.UNRESOLVED_COMMENT_COUNT.getName(), cd);
if (source.get(ChangeField.REF_STATE.getName()) != null) {
JsonArray refStates = source.get(ChangeField.REF_STATE.getName()).getAsJsonArray();
@ -381,5 +382,13 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
opts,
out);
}
private void decodeUnresolvedCommentCount(JsonObject doc, String fieldName, ChangeData out) {
JsonElement count = doc.get(fieldName);
if (count == null) {
return;
}
out.setUnresolvedCommentCount(count.getAsInt());
}
}
}

@ -43,6 +43,7 @@ public class ChangeInfo {
public Boolean submittable;
public Integer insertions;
public Integer deletions;
public Integer unresolvedCommentCount;
public int _number;

@ -118,6 +118,7 @@ public class SearchSuggestOracle extends HighlightSuggestOracle {
suggestions.add("has:edit");
suggestions.add("has:star");
suggestions.add("has:stars");
suggestions.add("has:unresolved");
suggestions.add("star:");
suggestions.add("is:");
@ -148,6 +149,8 @@ public class SearchSuggestOracle extends HighlightSuggestOracle {
suggestions.add("delta:");
suggestions.add("size:");
suggestions.add("unresolved:");
if (Gerrit.isNoteDbEnabled()) {
suggestions.add("hashtag:");
}

@ -127,6 +127,8 @@ public class LuceneChangeIndex implements ChangeIndex {
ChangeField.STORED_SUBMIT_RECORD_LENIENT.getName();
private static final String SUBMIT_RECORD_STRICT_FIELD =
ChangeField.STORED_SUBMIT_RECORD_STRICT.getName();
private static final String UNRESOLVED_COMMENT_COUNT_FIELD =
ChangeField.UNRESOLVED_COMMENT_COUNT.getName();
static Term idTerm(ChangeData cd) {
return QueryBuilder.intTerm(LEGACY_ID.getName(), cd.getId().get());
@ -467,6 +469,8 @@ public class LuceneChangeIndex implements ChangeIndex {
if (fields.contains(REF_STATE_PATTERN_FIELD)) {
decodeRefStatePatterns(doc, cd);
}
decodeUnresolvedCommentCount(doc, cd);
return cd;
}
@ -568,6 +572,14 @@ public class LuceneChangeIndex implements ChangeIndex {
cd.setRefStatePatterns(copyAsBytes(doc.get(REF_STATE_PATTERN_FIELD)));
}
private void decodeUnresolvedCommentCount(
ListMultimap<String, IndexableField> doc, ChangeData cd) {
IndexableField f = Iterables.getFirst(doc.get(UNRESOLVED_COMMENT_COUNT_FIELD), null);
if (f != null && f.numericValue() != null) {
cd.setUnresolvedCommentCount(f.numericValue().intValue());
}
}
private static <T> List<T> decodeProtos(
ListMultimap<String, IndexableField> doc, String fieldName, ProtobufCodec<T> codec) {
Collection<IndexableField> fields = doc.get(fieldName);

@ -477,6 +477,7 @@ public class ChangeJson {
out.created = in.getCreatedOn();
out.updated = in.getLastUpdatedOn();
out._number = in.getId().get();
out.unresolvedCommentCount = cd.unresolvedCommentCount();
if (user.isIdentifiedUser()) {
Collection<String> stars = cd.stars(user.getAccountId());

@ -547,6 +547,16 @@ public class ChangeField {
}
};
/** Number of unresolved comments of the change. */
public static final FieldDef<ChangeData, Integer> UNRESOLVED_COMMENT_COUNT =
new FieldDef.Single<ChangeData, Integer>(
ChangeQueryBuilder.FIELD_UNRESOLVED_COMMENT_COUNT, FieldType.INTEGER_RANGE, true) {
@Override
public Integer get(ChangeData input, FillArgs args) throws OrmException {
return input.unresolvedCommentCount();
}
};
/** Whether the change is mergeable. */
public static final FieldDef<ChangeData, String> MERGEABLE =
new FieldDef.Single<ChangeData, String>(

@ -85,7 +85,9 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions<ChangeData> {
static final Schema<ChangeData> V36 =
schema(V35, ChangeField.REF_STATE, ChangeField.REF_STATE_PATTERN);
static final Schema<ChangeData> V37 = schema(V36);
@Deprecated static final Schema<ChangeData> V37 = schema(V36);
static final Schema<ChangeData> V38 = schema(V37, ChangeField.UNRESOLVED_COMMENT_COUNT);
public static final String NAME = "changes";
public static final ChangeSchemaDefinitions INSTANCE = new ChangeSchemaDefinitions();

@ -41,6 +41,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RobotComment;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.ApprovalsUtil;
@ -83,6 +84,7 @@ import java.util.Optional;
import java.util.Set;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
@ -334,6 +336,7 @@ public class ChangeData {
private Map<Integer, List<String>> files;
private Map<Integer, Optional<DiffSummary>> diffSummaries;
private Collection<Comment> publishedComments;
private Collection<RobotComment> robotComments;
private CurrentUser visibleTo;
private ChangeControl changeControl;
private List<ChangeMessage> messages;
@ -351,6 +354,7 @@ public class ChangeData {
private List<ReviewerStatusUpdate> reviewerUpdates;
private PersonIdent author;
private PersonIdent committer;
private Integer unresolvedCommentCount;
private ImmutableList<byte[]> refStates;
private ImmutableList<byte[]> refStatePatterns;
@ -977,6 +981,34 @@ public class ChangeData {
return publishedComments;
}
public Collection<RobotComment> robotComments() throws OrmException {
if (robotComments == null) {
if (!lazyLoad) {
return Collections.emptyList();
}
robotComments = commentsUtil.robotCommentsByChange(notes());
}
return robotComments;
}
public Integer unresolvedCommentCount() throws OrmException {
if (unresolvedCommentCount == null) {
if (!lazyLoad) {
return null;
}
Long count =
Stream.concat(publishedComments().stream(), robotComments().stream())
.filter(c -> (c.unresolved == Boolean.TRUE))
.count();
unresolvedCommentCount = count.intValue();
}
return unresolvedCommentCount;
}
public void setUnresolvedCommentCount(Integer count) {
this.unresolvedCommentCount = count;
}
public List<ChangeMessage> messages() throws OrmException {
if (messages == null) {
if (!lazyLoad) {

@ -159,6 +159,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
public static final String FIELD_STATUS = "status";
public static final String FIELD_SUBMISSIONID = "submissionid";
public static final String FIELD_TR = "tr";
public static final String FIELD_UNRESOLVED_COMMENT_COUNT = "unresolved";
public static final String FIELD_VISIBLETO = "visibleto";
public static final String FIELD_WATCHEDBY = "watchedby";
@ -513,6 +514,10 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
return new EditByPredicate(self());
}
if ("unresolved".equalsIgnoreCase(value)) {
return new IsUnresolvedPredicate();
}
// for plugins the value will be operandName_pluginName
String[] names = value.split("_");
if (names.length == 2) {
@ -677,7 +682,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
// label:CodeReview=1,jsmith or
// label:CodeReview=1,group=android_approvers or
// label:CodeReview=1,android_approvers
// user/groups without a label will first attempt to match user
// user/groups without a label will first attempt to match user
// Special case: votes by owners can be tracked with ",owner":
// label:Code-Review+2,owner
// label:Code-Review+2,user=owner
@ -1056,6 +1061,11 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
return new SubmittablePredicate(status);
}
@Operator
public Predicate<ChangeData> unresolved(String value) throws QueryParseException {
return new IsUnresolvedPredicate(value);
}
@Override
protected Predicate<ChangeData> defaultField(String query) throws QueryParseException {
if (query.startsWith("refs/")) {

@ -0,0 +1,34 @@
// Copyright (C) 2017 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.query.change;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.query.QueryParseException;
import com.google.gwtorm.server.OrmException;
public class IsUnresolvedPredicate extends IntegerRangeChangePredicate {
IsUnresolvedPredicate() throws QueryParseException {
this(">0");
}
IsUnresolvedPredicate(String value) throws QueryParseException {
super(ChangeField.UNRESOLVED_COMMENT_COUNT, value);
}
@Override
protected Integer getValueInt(ChangeData changeData) throws OrmException {
return ChangeField.UNRESOLVED_COMMENT_COUNT.get(changeData, null);
}
}

@ -1548,6 +1548,37 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
assertQuery("has:edit", change2);
}
@Test
public void byUnresolved() throws Exception {
TestRepository<Repo> repo = createProject("repo");
Change change1 = insert(repo, newChange(repo));
Change change2 = insert(repo, newChange(repo));
Change change3 = insert(repo, newChange(repo));
// Change1 has one resolved comment (unresolvedcount = 0)
// Change2 has one unresolved comment (unresolvedcount = 1)
// Change3 has one resolved comment and one unresolved comment (unresolvedcount = 1)
addComment(change1.getChangeId(), "comment 1", false);
addComment(change2.getChangeId(), "comment 2", true);
addComment(change3.getChangeId(), "comment 3", false);
addComment(change3.getChangeId(), "comment 4", true);
assertQuery("has:unresolved", change3, change2);
assertQuery("unresolved:0", change1);
List<ChangeInfo> changeInfos = assertQuery("unresolved:>=0", change3, change2, change1);
assertThat(changeInfos.get(0).unresolvedCommentCount).isEqualTo(1); // Change3
assertThat(changeInfos.get(1).unresolvedCommentCount).isEqualTo(1); // Change2
assertThat(changeInfos.get(2).unresolvedCommentCount).isEqualTo(0); // Change1
assertQuery("unresolved:>0", change3, change2);
assertQuery("unresolved:<1", change1);
assertQuery("unresolved:<=1", change3, change2, change1);
assertQuery("unresolved:1", change3, change2);
assertQuery("unresolved:>1");
assertQuery("unresolved:>=1", change3, change2);
}
@Test
public void byCommitsOnBranchNotMerged() throws Exception {
TestRepository<Repo> repo = createProject("repo");
@ -1595,6 +1626,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
cd.changedLines();
cd.reviewedBy();
cd.reviewers();
cd.unresolvedCommentCount();
// TODO(dborowitz): Swap out GitRepositoryManager somehow? Will probably be
// necessary for NoteDb anyway.
@ -1932,4 +1964,16 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
protected static long lastUpdatedMs(Change c) {
return c.getLastUpdatedOn().getTime();
}
private void addComment(int changeId, String message, Boolean unresolved) throws Exception {
ReviewInput input = new ReviewInput();
ReviewInput.CommentInput comment = new ReviewInput.CommentInput();
comment.line = 1;
comment.message = message;
comment.unresolved = unresolved;
input.comments =
ImmutableMap.<String, List<ReviewInput.CommentInput>>of(
Patch.COMMIT_MSG, ImmutableList.<ReviewInput.CommentInput>of(comment));
gApi.changes().id(changeId).current().review(input);
}
}