Extract and rewrite the logic for comment threads
We need the logic for comment threads in more places (see I9d8091c3a
and I4af5946ce). Instead of duplicating it and possibly getting it
wrong, we now consolidate it in one place. This also includes the logic
when a comment thread is considered as unresolved/resolved.
A large benefit of this extraction is that we finally have proper test
coverage for various situations around comment threads. The previous
integration tests only verified a common case.
The new logic should also cover the issues previously solved by
I97c6aeb47 and I2788fdb22. In addition, the new logic also properly
covers longer child branches for which comments are not left one after
the other but with mixed dates. For just determining the unresolved
state of the whole thread, this didn't matter as only the last comment
(previously also determined by date for child branches) is relevant.
Now that we build the full comment threads and hand them out, we should
get the order of the contained comments right.
Unfortunately, the frontend orders comments on its own into threads.
That logic (currently contained in gr-comment-api.js) might differ from
the one implemented in the backend. Improving this situation or aligning
the logic is beyond the scope of this change, though.
Change-Id: Ie2713ebdbe05b69a705d2c9c03375b206a248e01
diff --git a/java/com/google/gerrit/server/change/CommentThread.java b/java/com/google/gerrit/server/change/CommentThread.java
new file mode 100644
index 0000000..7b729d2
--- /dev/null
+++ b/java/com/google/gerrit/server/change/CommentThread.java
@@ -0,0 +1,69 @@
+// Copyright (C) 2020 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.auto.value.AutoValue;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.gerrit.entities.Comment;
+import java.util.List;
+
+/**
+ * Representation of a comment thread.
+ *
+ * <p>A comment thread consists of at least one comment.
+ *
+ * @param <T> type of comments in the thread. Can also be {@link Comment} if the thread mixes
+ * comments of different types.
+ */
+@AutoValue
+public abstract class CommentThread<T extends Comment> {
+
+ /** Comments in the thread in exactly the order they appear in the thread. */
+ public abstract ImmutableList<T> comments();
+
+ /** Whether the whole thread is considered as unresolved. */
+ public boolean unresolved() {
+ return Iterables.getLast(comments()).unresolved;
+ }
+
+ public static <T extends Comment> Builder<T> builder() {
+ return new AutoValue_CommentThread.Builder<>();
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder<T extends Comment> {
+
+ public abstract Builder<T> comments(List<T> value);
+
+ public Builder<T> addComment(T comment) {
+ commentsBuilder().add(comment);
+ return this;
+ }
+
+ abstract ImmutableList.Builder<T> commentsBuilder();
+
+ abstract ImmutableList<T> comments();
+
+ abstract CommentThread<T> autoBuild();
+
+ public CommentThread<T> build() {
+ Preconditions.checkState(
+ !comments().isEmpty(), "A comment thread must contain at least one comment.");
+ return autoBuild();
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/change/CommentThreads.java b/java/com/google/gerrit/server/change/CommentThreads.java
new file mode 100644
index 0000000..ecf9b46
--- /dev/null
+++ b/java/com/google/gerrit/server/change/CommentThreads.java
@@ -0,0 +1,92 @@
+// Copyright (C) 2020 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 static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static java.util.stream.Collectors.groupingBy;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Streams;
+import com.google.gerrit.entities.Comment;
+import java.util.Comparator;
+import java.util.Map;
+import java.util.PriorityQueue;
+import java.util.Queue;
+
+/**
+ * Identifier of comment threads.
+ *
+ * <p>Comments are ordered into threads according to their parent relationship indicated via {@link
+ * Comment#parentUuid}. It's possible that two comments refer to the same parent, which especially
+ * happens when two persons reply in parallel. If such branches exist, we merge them into a flat
+ * list taking the comment creation date ({@link Comment#writtenOn} into account (but still
+ * preserving the general parent order). Remaining ties are resolved by using the natural order of
+ * the comment UUID, which is unique.
+ *
+ * @param <T> type of comments in the threads. Can also be {@link Comment} if the threads mix
+ * comments of different types.
+ */
+public class CommentThreads<T extends Comment> {
+
+ private final Iterable<T> comments;
+
+ private CommentThreads(Iterable<T> comments) {
+ this.comments = comments;
+ }
+
+ public static <T extends Comment> CommentThreads<T> forComments(Iterable<T> comments) {
+ return new CommentThreads<>(comments);
+ }
+
+ public ImmutableSet<CommentThread<T>> getThreads() {
+ ImmutableSet<String> commentUuids =
+ Streams.stream(comments).map(comment -> comment.key.uuid).collect(toImmutableSet());
+
+ ImmutableSet<T> roots =
+ Streams.stream(comments)
+ .filter(
+ comment -> comment.parentUuid == null || !commentUuids.contains(comment.parentUuid))
+ .collect(toImmutableSet());
+
+ Map<String, ImmutableSet<T>> childrenPerParent =
+ Streams.stream(comments)
+ .filter(comment -> comment.parentUuid != null)
+ .collect(groupingBy(comment -> comment.parentUuid, toImmutableSet()));
+
+ return roots.stream()
+ .map(root -> buildCommentThread(root, childrenPerParent))
+ .collect(toImmutableSet());
+ }
+
+ private static <T extends Comment> CommentThread<T> buildCommentThread(
+ T root, Map<String, ImmutableSet<T>> childrenPerParent) {
+ CommentThread.Builder<T> commentThread = CommentThread.builder();
+ // Expand comments gradually from the root. If there is more than one child per level, place the
+ // earlier-created child earlier in the thread. Break ties with the UUID to be deterministic.
+ Queue<T> unvisited =
+ new PriorityQueue<>(
+ Comparator.comparing((T comment) -> comment.writtenOn)
+ .thenComparing(comment -> comment.key.uuid));
+ unvisited.add(root);
+ while (!unvisited.isEmpty()) {
+ T nextComment = unvisited.remove();
+ commentThread.addComment(nextComment);
+ ImmutableSet<T> children =
+ childrenPerParent.getOrDefault(nextComment.key.uuid, ImmutableSet.of());
+ unvisited.addAll(children);
+ }
+ return commentThread.build();
+ }
+}
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 7f7df8c..8301576 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -60,6 +60,8 @@
import com.google.gerrit.server.ReviewerStatusUpdate;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.StarredChangesUtil.StarRef;
+import com.google.gerrit.server.change.CommentThread;
+import com.google.gerrit.server.change.CommentThreads;
import com.google.gerrit.server.change.MergeabilityCache;
import com.google.gerrit.server.change.PureRevert;
import com.google.gerrit.server.config.AllUsersName;
@@ -790,47 +792,15 @@
List<Comment> comments =
Stream.concat(publishedComments().stream(), robotComments().stream()).collect(toList());
- // Build a map of uuid to list of direct descendants.
- Map<String, List<Comment>> forest = new HashMap<>();
- for (Comment comment : comments) {
- List<Comment> siblings = forest.get(comment.parentUuid);
- if (siblings == null) {
- siblings = new ArrayList<>();
- forest.put(comment.parentUuid, siblings);
- }
- siblings.add(comment);
- }
-
- // Find latest comment in each thread and apply to unresolved counter.
- int unresolved = 0;
- if (forest.containsKey(null)) {
- for (Comment root : forest.get(null)) {
- if (getLatestComment(forest, root).unresolved) {
- unresolved++;
- }
- }
- }
- unresolvedCommentCount = unresolved;
+ ImmutableSet<CommentThread<Comment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+ unresolvedCommentCount =
+ (int) commentThreads.stream().filter(CommentThread::unresolved).count();
}
return unresolvedCommentCount;
}
- protected Comment getLatestComment(Map<String, List<Comment>> forest, Comment root) {
- List<Comment> children = forest.get(root.key.uuid);
- if (children == null) {
- return root;
- }
- Comment latest = null;
- for (Comment comment : children) {
- Comment branchLatest = getLatestComment(forest, comment);
- if (latest == null || branchLatest.writtenOn.after(latest.writtenOn)) {
- latest = branchLatest;
- }
- }
- return latest;
- }
-
public void setUnresolvedCommentCount(Integer count) {
this.unresolvedCommentCount = count;
}
diff --git a/javatests/com/google/gerrit/server/change/CommentThreadTest.java b/javatests/com/google/gerrit/server/change/CommentThreadTest.java
new file mode 100644
index 0000000..dc46e48
--- /dev/null
+++ b/javatests/com/google/gerrit/server/change/CommentThreadTest.java
@@ -0,0 +1,80 @@
+// Copyright (C) 2020 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 static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Comment;
+import com.google.gerrit.entities.Comment.Key;
+import com.google.gerrit.entities.HumanComment;
+import java.sql.Timestamp;
+import org.junit.Test;
+
+public class CommentThreadTest {
+
+ @Test
+ public void threadMustContainAtLeastOneComment() {
+ assertThrows(IllegalStateException.class, () -> CommentThread.builder().build());
+ }
+
+ @Test
+ public void threadCanBeUnresolved() {
+ HumanComment root = unresolved(createComment("root"));
+ CommentThread<Comment> commentThread = CommentThread.builder().addComment(root).build();
+
+ assertThat(commentThread.unresolved()).isTrue();
+ }
+
+ @Test
+ public void threadCanBeResolved() {
+ HumanComment root = resolved(createComment("root"));
+ CommentThread<Comment> commentThread = CommentThread.builder().addComment(root).build();
+
+ assertThat(commentThread.unresolved()).isFalse();
+ }
+
+ @Test
+ public void lastCommentInThreadDeterminesUnresolvedStatus() {
+ HumanComment root = resolved(createComment("root"));
+ HumanComment child = unresolved(createComment("child"));
+ CommentThread<Comment> commentThread =
+ CommentThread.builder().addComment(root).addComment(child).build();
+
+ assertThat(commentThread.unresolved()).isTrue();
+ }
+
+ private static HumanComment createComment(String commentUuid) {
+ return new HumanComment(
+ new Key(commentUuid, "myFile", 1),
+ Account.id(100),
+ new Timestamp(1234),
+ (short) 1,
+ "Comment text",
+ "serverId",
+ true);
+ }
+
+ private static HumanComment resolved(HumanComment comment) {
+ comment.unresolved = false;
+ return comment;
+ }
+
+ private static HumanComment unresolved(HumanComment comment) {
+ comment.unresolved = true;
+ return comment;
+ }
+}
diff --git a/javatests/com/google/gerrit/server/change/CommentThreadsTest.java b/javatests/com/google/gerrit/server/change/CommentThreadsTest.java
new file mode 100644
index 0000000..0d1df5d
--- /dev/null
+++ b/javatests/com/google/gerrit/server/change/CommentThreadsTest.java
@@ -0,0 +1,201 @@
+// Copyright (C) 2020 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 static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Comment.Key;
+import com.google.gerrit.entities.HumanComment;
+import java.sql.Timestamp;
+import org.junit.Test;
+
+public class CommentThreadsTest {
+
+ @Test
+ public void threadsAreEmptyWhenNoCommentsAreProvided() {
+ ImmutableList<HumanComment> comments = ImmutableList.of();
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads = ImmutableSet.of();
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void threadsCanBeCreatedFromSingleRoot() {
+ HumanComment root = createComment("root");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(root);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads = ImmutableSet.of(toThread(root));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void threadsCanBeCreatedFromUnorderedComments() {
+ HumanComment root = createComment("root");
+ HumanComment child1 = asReply(createComment("child1"), "root");
+ HumanComment child2 = asReply(createComment("child2"), "child1");
+ HumanComment child3 = asReply(createComment("child3"), "child2");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(child2, child1, root, child3);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, child1, child2, child3));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void childWithNotAvailableParentIsAssumedToBeRoot() {
+ HumanComment child1 = asReply(createComment("child1"), "root");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(child1);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads = ImmutableSet.of(toThread(child1));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void threadsIgnoreDuplicateRoots() {
+ HumanComment root = createComment("root");
+ HumanComment child1 = asReply(createComment("child1"), "root");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(root, root, child1);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, child1));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void threadsIgnoreDuplicateChildren() {
+ HumanComment root = createComment("root");
+ HumanComment child1 = asReply(createComment("child1"), "root");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(root, child1, child1);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, child1));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void commentsAreOrderedIntoCorrectThreads() {
+ HumanComment thread1Root = createComment("thread1Root");
+ HumanComment thread1Child1 = asReply(createComment("thread1Child1"), "thread1Root");
+ HumanComment thread1Child2 = asReply(createComment("thread1Child2"), "thread1Child1");
+ HumanComment thread2Root = createComment("thread2Root");
+ HumanComment thread2Child1 = asReply(createComment("thread2Child1"), "thread2Root");
+
+ ImmutableList<HumanComment> comments =
+ ImmutableList.of(thread2Root, thread1Child2, thread1Child1, thread1Root, thread2Child1);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(
+ toThread(thread1Root, thread1Child1, thread1Child2),
+ toThread(thread2Root, thread2Child1));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void branchedThreadsAreFlattenedAccordingToDate() {
+ HumanComment root = writtenOn(createComment("root"), new Timestamp(1));
+ HumanComment sibling1 = writtenOn(asReply(createComment("sibling1"), "root"), new Timestamp(2));
+ HumanComment sibling2 = writtenOn(asReply(createComment("sibling2"), "root"), new Timestamp(3));
+ HumanComment sibling1Child =
+ writtenOn(asReply(createComment("sibling1Child"), "sibling1"), new Timestamp(4));
+ HumanComment sibling2Child =
+ writtenOn(asReply(createComment("sibling2Child"), "sibling2"), new Timestamp(5));
+
+ ImmutableList<HumanComment> comments =
+ ImmutableList.of(sibling2, sibling2Child, sibling1, sibling1Child, root);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, sibling1, sibling2, sibling1Child, sibling2Child));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void threadsConsiderParentRelationshipStrongerThanDate() {
+ HumanComment root = writtenOn(createComment("root"), new Timestamp(3));
+ HumanComment child1 = writtenOn(asReply(createComment("child1"), "root"), new Timestamp(2));
+ HumanComment child2 = writtenOn(asReply(createComment("child2"), "child1"), new Timestamp(1));
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(child2, child1, root);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, child1, child2));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void threadsFallBackToUuidOrderIfParentAndDateAreTheSame() {
+ HumanComment root = writtenOn(createComment("root"), new Timestamp(1));
+ HumanComment sibling1 = writtenOn(asReply(createComment("sibling1"), "root"), new Timestamp(2));
+ HumanComment sibling2 = writtenOn(asReply(createComment("sibling2"), "root"), new Timestamp(2));
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(sibling2, sibling1, root);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, sibling1, sibling2));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ private static HumanComment createComment(String commentUuid) {
+ return new HumanComment(
+ new Key(commentUuid, "myFile", 1),
+ Account.id(100),
+ new Timestamp(1234),
+ (short) 1,
+ "Comment text",
+ "serverId",
+ true);
+ }
+
+ private static HumanComment asReply(HumanComment comment, String parentUuid) {
+ comment.parentUuid = parentUuid;
+ return comment;
+ }
+
+ private static HumanComment writtenOn(HumanComment comment, Timestamp writtenOn) {
+ comment.writtenOn = writtenOn;
+ return comment;
+ }
+
+ private static CommentThread<HumanComment> toThread(HumanComment... comments) {
+ return CommentThread.<HumanComment>builder().comments(ImmutableList.copyOf(comments)).build();
+ }
+}
