From c001f3229819758cfeb68d01181fece311173236 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 31 Oct 2018 14:31:08 -0700 Subject: [PATCH] Add indexed field for total inline comment count Like unresolved comment count, may be useful for downstream tooling. Not currently exposed as a searchable predicate, since the utility would be lower than `has:unresolved`. Change-Id: I9bb597b6f5c21424efa69510d41985175699977e --- Documentation/rest-api-changes.txt | 6 +++++- .../gerrit/extensions/common/ChangeInfo.java | 1 + .../gerrit/lucene/LuceneChangeIndex.java | 16 ++++++++++++++-- .../gerrit/server/change/ChangeJson.java | 1 + .../server/index/change/ChangeField.java | 6 +++++- .../index/change/ChangeSchemaDefinitions.java | 4 +++- .../server/query/change/ChangeData.java | 19 +++++++++++++++++++ .../api/revision/RobotCommentsIT.java | 3 ++- .../acceptance/server/change/CommentsIT.java | 5 ++++- .../change/AbstractQueryChangesTest.java | 6 ++++++ 10 files changed, 60 insertions(+), 7 deletions(-) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 1829c6b82a..b1c9a887c7 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -5769,8 +5769,12 @@ Only set if link:#submittable[requested]. Number of inserted lines. |`deletions` || Number of deleted lines. +|`total_comment_count` |optional| +Total number of inline comments across all patch sets. Not set if the current +change index doesn't have the data. |`unresolved_comment_count` |optional| -Number of unresolved comments. Not set if the current change index doesn't have the data. +Number of unresolved inline comment threads across all patch sets. 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[ diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java index 945c239508..9a739efafd 100644 --- a/java/com/google/gerrit/extensions/common/ChangeInfo.java +++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java @@ -46,6 +46,7 @@ public class ChangeInfo { public Boolean submittable; public Integer insertions; public Integer deletions; + public Integer totalCommentCount; public Integer unresolvedCommentCount; public Boolean isPrivate; public Boolean workInProgress; diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java index 66db468c98..b208a31a5a 100644 --- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -76,6 +76,7 @@ import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; +import java.util.function.Consumer; import org.apache.lucene.document.Document; import org.apache.lucene.index.IndexWriter; import org.apache.lucene.index.IndexableField; @@ -129,6 +130,7 @@ 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 TOTAL_COMMENT_COUNT_FIELD = ChangeField.TOTAL_COMMENT_COUNT.getName(); private static final String UNRESOLVED_COMMENT_COUNT_FIELD = ChangeField.UNRESOLVED_COMMENT_COUNT.getName(); @@ -504,6 +506,7 @@ public class LuceneChangeIndex implements ChangeIndex { } decodeUnresolvedCommentCount(doc, cd); + decodeTotalCommentCount(doc, cd); return cd; } @@ -633,9 +636,18 @@ public class LuceneChangeIndex implements ChangeIndex { private void decodeUnresolvedCommentCount( ListMultimap doc, ChangeData cd) { - IndexableField f = Iterables.getFirst(doc.get(UNRESOLVED_COMMENT_COUNT_FIELD), null); + decodeIntField(doc, UNRESOLVED_COMMENT_COUNT_FIELD, cd::setUnresolvedCommentCount); + } + + private void decodeTotalCommentCount(ListMultimap doc, ChangeData cd) { + decodeIntField(doc, TOTAL_COMMENT_COUNT_FIELD, cd::setTotalCommentCount); + } + + private static void decodeIntField( + ListMultimap doc, String fieldName, Consumer consumer) { + IndexableField f = Iterables.getFirst(doc.get(fieldName), null); if (f != null && f.numericValue() != null) { - cd.setUnresolvedCommentCount(f.numericValue().intValue()); + consumer.accept(f.numericValue().intValue()); } } diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java index c64cd129fe..b7049a7131 100644 --- a/java/com/google/gerrit/server/change/ChangeJson.java +++ b/java/com/google/gerrit/server/change/ChangeJson.java @@ -535,6 +535,7 @@ public class ChangeJson { out.created = in.getCreatedOn(); out.updated = in.getLastUpdatedOn(); out._number = in.getId().get(); + out.totalCommentCount = cd.totalCommentCount(); out.unresolvedCommentCount = cd.unresolvedCommentCount(); if (user.isIdentifiedUser()) { diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java index 5d12e79cfe..b79a1c2a08 100644 --- a/java/com/google/gerrit/server/index/change/ChangeField.java +++ b/java/com/google/gerrit/server/index/change/ChangeField.java @@ -508,11 +508,15 @@ public class ChangeField { cd.messages().stream().map(ChangeMessage::getMessage)) .collect(toSet())); - /** Number of unresolved comments of the change. */ + /** Number of unresolved comment threads of the change, including robot comments. */ public static final FieldDef UNRESOLVED_COMMENT_COUNT = intRange(ChangeQueryBuilder.FIELD_UNRESOLVED_COMMENT_COUNT) .build(ChangeData::unresolvedCommentCount); + /** Total number of published inline comments of the change, including robot comments. */ + public static final FieldDef TOTAL_COMMENT_COUNT = + intRange("total_comments").build(ChangeData::totalCommentCount); + /** Whether the change is mergeable. */ public static final FieldDef MERGEABLE = exact(ChangeQueryBuilder.FIELD_MERGEABLE) diff --git a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java index 2000cd176d..9016fd1b70 100644 --- a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java +++ b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java @@ -99,7 +99,9 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions { @Deprecated static final Schema V49 = schema(V48); // Bump Lucene version requires reindexing - static final Schema V50 = schema(V49); + @Deprecated static final Schema V50 = schema(V49); + + static final Schema V51 = schema(V50, ChangeField.TOTAL_COMMENT_COUNT); public static final String NAME = "changes"; public static final ChangeSchemaDefinitions INSTANCE = new ChangeSchemaDefinitions(); diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java index 83d68dbede..0f5d938b7b 100644 --- a/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/java/com/google/gerrit/server/query/change/ChangeData.java @@ -30,6 +30,7 @@ import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.primitives.Ints; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.SubmitRecord; @@ -391,6 +392,7 @@ public class ChangeData { private PersonIdent committer; private int parentCount; private Integer unresolvedCommentCount; + private Integer totalCommentCount; private LabelTypes labelTypes; private ImmutableList refStates; @@ -925,6 +927,23 @@ public class ChangeData { this.unresolvedCommentCount = count; } + public Integer totalCommentCount() throws OrmException { + if (totalCommentCount == null) { + if (!lazyLoad) { + return null; + } + + // Fail on overflow. + totalCommentCount = + Ints.checkedCast((long) publishedComments().size() + robotComments().size()); + } + return totalCommentCount; + } + + public void setTotalCommentCount(Integer count) { + this.totalCommentCount = count; + } + public List messages() throws OrmException { if (messages == null) { if (!lazyLoad) { diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java index cd20765940..d713db60db 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java @@ -1024,7 +1024,7 @@ public class RobotCommentsIT extends AbstractDaemonTest { } @Test - public void queryChangesWithUnresolvedCommentCount() throws Exception { + public void queryChangesWithCommentCounts() throws Exception { assume().that(notesMigration.readChanges()).isTrue(); PushOneCommit.Result r1 = createChange(); @@ -1043,6 +1043,7 @@ public class RobotCommentsIT extends AbstractDaemonTest { // if we allow users to resolve a robot comment, then this test should // be modified. assertThat(result.unresolvedCommentCount).isEqualTo(0); + assertThat(result.totalCommentCount).isEqualTo(1); } finally { enableDb(ctx); } diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java index 8b1f4bc314..0d40a1cdc3 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -716,7 +716,7 @@ public class CommentsIT extends AbstractDaemonTest { } @Test - public void queryChangesWithUnresolvedCommentCount() throws Exception { + public void queryChangesWithCommentCount() throws Exception { // PS1 has three comments in three different threads, PS2 has one comment in one thread. PushOneCommit.Result result = createChange("change 1", FILE_NAME, "content 1"); String changeId1 = result.getChangeId(); @@ -751,8 +751,11 @@ public class CommentsIT extends AbstractDaemonTest { ChangeInfo changeInfo2 = Iterables.getOnlyElement(query(changeId2)); ChangeInfo changeInfo3 = Iterables.getOnlyElement(query(changeId3)); assertThat(changeInfo1.unresolvedCommentCount).isEqualTo(2); + assertThat(changeInfo1.totalCommentCount).isEqualTo(4); assertThat(changeInfo2.unresolvedCommentCount).isEqualTo(0); + assertThat(changeInfo2.totalCommentCount).isEqualTo(2); assertThat(changeInfo3.unresolvedCommentCount).isEqualTo(1); + assertThat(changeInfo3.totalCommentCount).isEqualTo(2); } finally { enableDb(ctx); } diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index b9973e906c..f362e5a73b 100644 --- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -2378,6 +2378,12 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { cd.reviewers(); cd.unresolvedCommentCount(); + if (getSchemaVersion() < 51) { + assertMissingField(ChangeField.TOTAL_COMMENT_COUNT); + } else { + cd.totalCommentCount(); + } + // TODO(dborowitz): Swap out GitRepositoryManager somehow? Will probably be // necessary for NoteDb anyway. cd.isMergeable();