From e6e14b970b579336a168028b5b4da264b1a1a155 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 9 Mar 2015 12:36:23 -0700 Subject: [PATCH 1/2] Add "commentby" search operator to search authors of comments Like the "comment" field, this includes both top-level and inline comments, although in practice the REST API doesn't allow adding inline comments without a corresponding top-level comment. Change-Id: Icc1ca78d8de46b1dbba0cff378fa53b94f9af1a2 --- Documentation/user-search.txt | 7 +++ .../gerrit/server/index/ChangeField.java | 21 +++++++ .../gerrit/server/index/ChangeSchemas.java | 28 +++++++++ .../query/change/ChangeQueryBuilder.java | 12 ++++ .../query/change/CommentByPredicate.java | 57 +++++++++++++++++++ .../change/AbstractQueryChangesTest.java | 36 ++++++++++++ 6 files changed, 161 insertions(+) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/query/change/CommentByPredicate.java diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt index 83accb815e..be4f8907af 100644 --- a/Documentation/user-search.txt +++ b/Documentation/user-search.txt @@ -326,6 +326,13 @@ lines. Valid relations are >=, >, <=, <, or no relation, which will match if the number of lines is exactly equal. +[[commentby]] +commentby:'USER':: ++ +Changes containing a top-level or inline comment by 'USER'. The special +case of `commentby:self` will find changes where the caller has +commented. + == Argument Quoting diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java index 2993739f9f..821343f457 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java @@ -44,6 +44,7 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.sql.Timestamp; import java.util.Collection; +import java.util.HashSet; import java.util.List; import java.util.Set; @@ -498,6 +499,26 @@ public class ChangeField { } }; + /** Users who have commented on this change. */ + public static final FieldDef> COMMENTBY = + new FieldDef.Repeatable( + ChangeQueryBuilder.FIELD_COMMENTBY, FieldType.INTEGER, false) { + @Override + public Iterable get(ChangeData input, FillArgs args) + throws OrmException { + Set r = new HashSet<>(); + for (ChangeMessage m : input.messages()) { + if (m.getAuthor() != null) { + r.add(m.getAuthor().get()); + } + } + for (PatchLineComment c : input.publishedComments()) { + r.add(c.getAuthor().get()); + } + return r; + } + }; + private static List toProtos(ProtobufCodec codec, Collection objs) throws OrmException { List result = Lists.newArrayListWithCapacity(objs.size()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java index 05bf9bde6c..ef6d6261b9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java @@ -115,6 +115,34 @@ public class ChangeSchemas { ChangeField.DELTA, ChangeField.HASHTAG); + static final Schema V15 = schema( + ChangeField.LEGACY_ID, + ChangeField.ID, + ChangeField.STATUS, + ChangeField.PROJECT, + ChangeField.PROJECTS, + ChangeField.REF, + ChangeField.TOPIC, + ChangeField.UPDATED, + ChangeField.FILE_PART, + ChangeField.PATH, + ChangeField.OWNER, + ChangeField.REVIEWER, + ChangeField.COMMIT, + ChangeField.TR, + ChangeField.LABEL, + ChangeField.REVIEWED, + ChangeField.COMMIT_MESSAGE, + ChangeField.COMMENT, + ChangeField.CHANGE, + ChangeField.APPROVAL, + ChangeField.MERGEABLE, + ChangeField.ADDED, + ChangeField.DELETED, + ChangeField.DELTA, + ChangeField.HASHTAG, + ChangeField.COMMENTBY); + private static Schema schema(Collection> fields) { return new Schema<>(ImmutableList.copyOf(fields)); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index c9d7e6cd24..5460296e15 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java @@ -88,6 +88,7 @@ public class ChangeQueryBuilder extends QueryBuilder { public static final String FIELD_BRANCH = "branch"; public static final String FIELD_CHANGE = "change"; public static final String FIELD_COMMENT = "comment"; + public static final String FIELD_COMMENTBY = "commentby"; public static final String FIELD_COMMIT = "commit"; public static final String FIELD_CONFLICTS = "conflicts"; public static final String FIELD_DELETED = "deleted"; @@ -743,6 +744,17 @@ public class ChangeQueryBuilder extends QueryBuilder { return new DeltaPredicate(value); } + @Operator + public Predicate commentby(String who) + throws QueryParseException, OrmException { + Set m = parseAccount(who); + List p = Lists.newArrayListWithCapacity(m.size()); + for (Account.Id id : m) { + p.add(new OwnerPredicate(id)); + } + return Predicate.or(p); + } + @Override protected Predicate defaultField(String query) throws QueryParseException { if (query.startsWith("refs/")) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/CommentByPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/CommentByPredicate.java new file mode 100644 index 0000000000..dee7086b21 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/CommentByPredicate.java @@ -0,0 +1,57 @@ +// 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.query.change; + +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.ChangeMessage; +import com.google.gerrit.reviewdb.client.PatchLineComment; +import com.google.gerrit.server.index.ChangeField; +import com.google.gerrit.server.index.IndexPredicate; +import com.google.gwtorm.server.OrmException; + +import java.util.Objects; + +class CommentByPredicate extends IndexPredicate { + private final Account.Id id; + + CommentByPredicate(Account.Id id) { + super(ChangeField.COMMENTBY, id.toString()); + this.id = id; + } + + Account.Id getAccountId() { + return id; + } + + @Override + public boolean match(ChangeData cd) throws OrmException { + for (ChangeMessage m : cd.messages()) { + if (Objects.equals(m.getAuthor(), id)) { + return true; + } + } + for (PatchLineComment c : cd.publishedComments()) { + if (Objects.equals(c.getAuthor(), id)) { + return true; + } + } + return false; + } + + @Override + public int getCost() { + return 1; + } +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 34588fab99..d03c5e50ed 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -43,6 +43,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Patch; +import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; @@ -1091,6 +1092,41 @@ public abstract class AbstractQueryChangesTest { assertResultEquals(change1, queryOne(q + " visibleto:" + user2.get())); } + @Test + public void byCommentBy() throws Exception { + TestRepository repo = createProject("repo"); + ChangeInserter ins1 = newChange(repo, null, null, null, null); + Change change1 = ins1.insert(); + PatchSet ps1 = ins1.getPatchSet(); + ChangeInserter ins2 = newChange(repo, null, null, null, null); + Change change2 = ins2.insert(); + PatchSet ps2 = ins2.getPatchSet(); + + int user2 = accountManager.authenticate(AuthRequest.forUser("anotheruser")) + .getAccountId().get(); + + ReviewInput input = new ReviewInput(); + input.message = "toplevel"; + ReviewInput.CommentInput comment = new ReviewInput.CommentInput(); + comment.line = 1; + comment.message = "inline"; + input.comments = ImmutableMap.> of( + Patch.COMMIT_MSG, ImmutableList. of(comment)); + postReview.apply(new RevisionResource(changes.parse(change1.getId()), ps1), + input); + + input = new ReviewInput(); + input.message = "toplevel"; + postReview.apply(new RevisionResource(changes.parse(change2.getId()), ps2), + input); + + List results = query("commentby:" + userId.get()); + assertThat(results).hasSize(2); + assertResultEquals(change2, results.get(0)); + assertResultEquals(change1, results.get(1)); + assertThat(query("commentby:" + user2)).isEmpty(); + } + protected ChangeInserter newChange( TestRepository repo, @Nullable RevCommit commit, @Nullable String key, @Nullable Integer owner, From 657aa4d57e50e417a335ca641dd42e145cde939d Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 9 Mar 2015 15:00:14 -0700 Subject: [PATCH 2/2] Add "from" search operator to match by owner or comments In the current implementation, searching by owner is somewhat redundant, since creating a new change inserts a new change message, which is picked up by commentby; but we consider this an implementation detail. Change-Id: I2a8bd6c43b2498b4d9bf82de09e09538dc047e10 --- Documentation/user-search.txt | 6 ++++ .../query/change/ChangeQueryBuilder.java | 27 +++++++++++++----- .../change/AbstractQueryChangesTest.java | 28 +++++++++++++++++++ 3 files changed, 54 insertions(+), 7 deletions(-) diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt index be4f8907af..1f95d9560d 100644 --- a/Documentation/user-search.txt +++ b/Documentation/user-search.txt @@ -333,6 +333,12 @@ Changes containing a top-level or inline comment by 'USER'. The special case of `commentby:self` will find changes where the caller has commented. +[[from]] +from:'USER':: ++ +Changes containing a top-level or inline comment by 'USER', or owned by +'USER'. Equivalent to `(owner:USER OR commentby:USER)`. + == Argument Quoting diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index 5460296e15..4e0cc3ec0a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java @@ -660,9 +660,12 @@ public class ChangeQueryBuilder extends QueryBuilder { @Operator public Predicate owner(String who) throws QueryParseException, OrmException { - Set m = parseAccount(who); - List p = Lists.newArrayListWithCapacity(m.size()); - for (Account.Id id : m) { + return owner(parseAccount(who)); + } + + private Predicate owner(Set who) { + List p = Lists.newArrayListWithCapacity(who.size()); + for (Account.Id id : who) { p.add(new OwnerPredicate(id)); } return Predicate.or(p); @@ -747,14 +750,24 @@ public class ChangeQueryBuilder extends QueryBuilder { @Operator public Predicate commentby(String who) throws QueryParseException, OrmException { - Set m = parseAccount(who); - List p = Lists.newArrayListWithCapacity(m.size()); - for (Account.Id id : m) { - p.add(new OwnerPredicate(id)); + return commentby(parseAccount(who)); + } + + private Predicate commentby(Set who) { + List p = Lists.newArrayListWithCapacity(who.size()); + for (Account.Id id : who) { + p.add(new CommentByPredicate(id)); } return Predicate.or(p); } + @Operator + public Predicate from(String who) + throws QueryParseException, OrmException { + Set ownerIds = parseAccount(who); + return Predicate.or(owner(ownerIds), commentby(ownerIds)); + } + @Override protected Predicate defaultField(String query) throws QueryParseException { if (query.startsWith("refs/")) { diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index d03c5e50ed..d64f327139 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -1127,6 +1127,34 @@ public abstract class AbstractQueryChangesTest { assertThat(query("commentby:" + user2)).isEmpty(); } + @Test + public void byFrom() throws Exception { + TestRepository repo = createProject("repo"); + Change change1 = newChange(repo, null, null, null, null).insert(); + + int user2 = accountManager.authenticate(AuthRequest.forUser("anotheruser")) + .getAccountId().get(); + ChangeInserter ins2 = newChange(repo, null, null, user2, null); + Change change2 = ins2.insert(); + PatchSet ps2 = ins2.getPatchSet(); + + ReviewInput input = new ReviewInput(); + input.message = "toplevel"; + ReviewInput.CommentInput comment = new ReviewInput.CommentInput(); + comment.line = 1; + comment.message = "inline"; + input.comments = ImmutableMap.> of( + Patch.COMMIT_MSG, ImmutableList. of(comment)); + postReview.apply(new RevisionResource(changes.parse(change2.getId()), ps2), + input); + + List results = query("from:" + userId.get()); + assertThat(results).hasSize(2); + assertResultEquals(change2, results.get(0)); + assertResultEquals(change1, results.get(1)); + assertResultEquals(change2, queryOne("from:" + user2)); + } + protected ChangeInserter newChange( TestRepository repo, @Nullable RevCommit commit, @Nullable String key, @Nullable Integer owner,