Support "cc:" query For NoteDb, where REVIEWER/CC are truly distinct, change "reviewer:" to only query for REVIEWER. Keep the old behavior of "reviewer:" for non-NoteDb and return results regardless of what state it guessed. For "cc:", actually search for CC, although in practice usually this will return no results (matching the fact that CCs don't show up in the UI). Change-Id: I0b620888f93d64cea1d2e34cefb8d47ee026f0fd
diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt index 2b5702e..9775c40 100644 --- a/Documentation/user-search.txt +++ b/Documentation/user-search.txt
@@ -118,6 +118,12 @@ special case of `reviewer:self` will find changes where the caller has been added as a reviewer. +[[cc]] +cc:'USER':: ++ +Changes that have the given user CC'ed on them. The special case of `cc:self` +will find changes where the caller has been CC'ed. + [[reviewerin]] reviewerin:'GROUP':: +
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/SearchSuggestOracle.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/SearchSuggestOracle.java index 165d0ca..5c0508e 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/SearchSuggestOracle.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/SearchSuggestOracle.java
@@ -40,7 +40,8 @@ "author:", "committer:", "from:", - "assignee:"), + "assignee:", + "cc:"), new AccountSuggestOracle() { @Override public void onRequestSuggestions(final Request request, final Callback done) { @@ -152,6 +153,7 @@ suggestions.add("unresolved:"); if (Gerrit.isNoteDbEnabled()) { + suggestions.add("cc:"); suggestions.add("hashtag:"); }
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 aa220e1..39968f1 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
@@ -16,6 +16,7 @@ import static com.google.gerrit.reviewdb.client.Change.CHANGE_ID_PATTERN; import static com.google.gerrit.server.query.change.ChangeData.asChanges; +import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toSet; import com.google.common.annotations.VisibleForTesting; @@ -61,6 +62,7 @@ import com.google.gerrit.server.index.change.ChangeIndexCollection; import com.google.gerrit.server.index.change.ChangeIndexRewriter; import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ListChildProjects; @@ -173,34 +175,35 @@ @VisibleForTesting public static class Arguments { - final Provider<ReviewDb> db; - final Provider<InternalChangeQuery> queryProvider; - final ChangeIndexRewriter rewriter; - final DynamicMap<ChangeOperatorFactory> opFactories; - final DynamicMap<ChangeHasOperandFactory> hasOperands; - final IdentifiedUser.GenericFactory userFactory; - final CapabilityControl.Factory capabilityControlFactory; - final ChangeControl.GenericFactory changeControlGenericFactory; - final ChangeNotes.Factory notesFactory; - final ChangeData.Factory changeDataFactory; - final FieldDef.FillArgs fillArgs; - final CommentsUtil commentsUtil; + final AccountCache accountCache; final AccountResolver accountResolver; - final GroupBackend groupBackend; final AllProjectsName allProjectsName; final AllUsersName allUsersName; - final PatchListCache patchListCache; - final GitRepositoryManager repoManager; - final ProjectCache projectCache; - final Provider<ListChildProjects> listChildProjects; - final SubmitDryRun submitDryRun; - final ConflictsCache conflictsCache; - final TrackingFooters trackingFooters; + final CapabilityControl.Factory capabilityControlFactory; + final ChangeControl.GenericFactory changeControlGenericFactory; + final ChangeData.Factory changeDataFactory; final ChangeIndex index; + final ChangeIndexRewriter rewriter; + final ChangeNotes.Factory notesFactory; + final CommentsUtil commentsUtil; + final ConflictsCache conflictsCache; + final DynamicMap<ChangeHasOperandFactory> hasOperands; + final DynamicMap<ChangeOperatorFactory> opFactories; + final FieldDef.FillArgs fillArgs; + final GitRepositoryManager repoManager; + final GroupBackend groupBackend; + final IdentifiedUser.GenericFactory userFactory; final IndexConfig indexConfig; + final NotesMigration notesMigration; + final PatchListCache patchListCache; + final ProjectCache projectCache; + final Provider<InternalChangeQuery> queryProvider; + final Provider<ListChildProjects> listChildProjects; final Provider<ListMembers> listMembers; + final Provider<ReviewDb> db; final StarredChangesUtil starredChangesUtil; - final AccountCache accountCache; + final SubmitDryRun submitDryRun; + final TrackingFooters trackingFooters; final boolean allowsDrafts; private final Provider<CurrentUser> self; @@ -237,7 +240,8 @@ Provider<ListMembers> listMembers, StarredChangesUtil starredChangesUtil, AccountCache accountCache, - @GerritServerConfig Config cfg) { + @GerritServerConfig Config cfg, + NotesMigration notesMigration) { this( db, queryProvider, @@ -268,7 +272,8 @@ listMembers, starredChangesUtil, accountCache, - cfg == null ? true : cfg.getBoolean("change", "allowDrafts", true)); + cfg == null ? true : cfg.getBoolean("change", "allowDrafts", true), + notesMigration); } private Arguments( @@ -301,7 +306,8 @@ Provider<ListMembers> listMembers, StarredChangesUtil starredChangesUtil, AccountCache accountCache, - boolean allowsDrafts) { + boolean allowsDrafts, + NotesMigration notesMigration) { this.db = db; this.queryProvider = queryProvider; this.rewriter = rewriter; @@ -332,6 +338,7 @@ this.accountCache = accountCache; this.allowsDrafts = allowsDrafts; this.hasOperands = hasOperands; + this.notesMigration = notesMigration; } Arguments asUser(CurrentUser otherUser) { @@ -365,7 +372,8 @@ listMembers, starredChangesUtil, accountCache, - allowsDrafts); + allowsDrafts, + notesMigration); } Arguments asUser(Account.Id otherId) { @@ -553,7 +561,11 @@ } if ("reviewer".equalsIgnoreCase(value)) { - return ReviewerPredicate.create(args, self()); + return ReviewerPredicate.reviewer(args, self()); + } + + if ("cc".equalsIgnoreCase(value)) { + return ReviewerPredicate.cc(args, self()); } if ("mergeable".equalsIgnoreCase(value)) { @@ -925,12 +937,17 @@ @Operator public Predicate<ChangeData> reviewer(String who) throws QueryParseException, OrmException { - Set<Account.Id> m = parseAccount(who); - List<Predicate<ChangeData>> p = Lists.newArrayListWithCapacity(m.size()); - for (Account.Id id : m) { - p.add(ReviewerPredicate.create(args, id)); - } - return Predicate.or(p); + return Predicate.or( + parseAccount(who) + .stream() + .map(id -> ReviewerPredicate.reviewer(args, id)) + .collect(toList())); + } + + @Operator + public Predicate<ChangeData> cc(String who) throws QueryParseException, OrmException { + return Predicate.or( + parseAccount(who).stream().map(id -> ReviewerPredicate.cc(args, id)).collect(toList())); } @Operator
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java index 4a11d28..6ce02fb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java
@@ -14,6 +14,8 @@ package com.google.gerrit.server.query.change; +import static java.util.stream.Collectors.toList; + import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.server.index.change.ChangeField; @@ -21,32 +23,50 @@ import com.google.gerrit.server.query.Predicate; import com.google.gerrit.server.query.change.ChangeQueryBuilder.Arguments; import com.google.gwtorm.server.OrmException; -import java.util.ArrayList; -import java.util.List; +import java.util.stream.Stream; class ReviewerPredicate extends ChangeIndexPredicate { - static Predicate<ChangeData> create(Arguments args, Account.Id id) { - List<Predicate<ChangeData>> and = new ArrayList<>(2); - ReviewerStateInternal[] states = ReviewerStateInternal.values(); - List<Predicate<ChangeData>> or = new ArrayList<>(states.length - 1); - for (ReviewerStateInternal state : states) { - if (state != ReviewerStateInternal.REMOVED) { - or.add(new ReviewerPredicate(state, id)); - } + static Predicate<ChangeData> reviewer(Arguments args, Account.Id id) { + Predicate<ChangeData> p; + if (args.notesMigration.readChanges()) { + // With NoteDb, Reviewer/CC are clearly distinct states, so only choose reviewer. + p = new ReviewerPredicate(ReviewerStateInternal.REVIEWER, id); + } else { + // Without NoteDb, Reviewer/CC are a bit unpredictable; maintain the old behavior of matching + // any reviewer state. + p = anyReviewerState(id); } - and.add(Predicate.or(or)); + return create(args, p); + } - // TODO(dborowitz): This really belongs much higher up e.g. QueryProcessor. + static Predicate<ChangeData> cc(Arguments args, Account.Id id) { + // As noted above, CC is nebulous without NoteDb, but it certainly doesn't make sense to return + // Reviewers for cc:foo. Most likely this will just not match anything, but let the index sort + // it out. + return create(args, new ReviewerPredicate(ReviewerStateInternal.CC, id)); + } + + private static Predicate<ChangeData> anyReviewerState(Account.Id id) { + return Predicate.or( + Stream.of(ReviewerStateInternal.values()) + .filter(s -> s != ReviewerStateInternal.REMOVED) + .map(s -> new ReviewerPredicate(s, id)) + .collect(toList())); + } + + private static Predicate<ChangeData> create(Arguments args, Predicate<ChangeData> p) { if (!args.allowsDrafts) { - and.add(Predicate.not(new ChangeStatusPredicate(Change.Status.DRAFT))); + // TODO(dborowitz): This really belongs much higher up e.g. QueryProcessor. Also, why are we + // even doing this? + return Predicate.and(p, Predicate.not(new ChangeStatusPredicate(Change.Status.DRAFT))); } - return Predicate.and(and); + return p; } private final ReviewerStateInternal state; private final Account.Id id; - ReviewerPredicate(ReviewerStateInternal state, Account.Id id) { + private ReviewerPredicate(ReviewerStateInternal state, Account.Id id) { super(ChangeField.REVIEWER, ChangeField.getReviewerFieldValue(state, id)); this.state = state; this.id = id;
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeQueryBuilder.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeQueryBuilder.java index 3652ec7..6fda100 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeQueryBuilder.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeQueryBuilder.java
@@ -28,7 +28,7 @@ new ChangeQueryBuilder.Arguments( null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, indexes, null, null, null, null, null, null, - null, null)); + null, null, null)); } @Operator
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 1b9fc61..6bde106 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
@@ -34,6 +34,7 @@ import com.google.gerrit.common.Nullable; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.api.GerritApi; +import com.google.gerrit.extensions.api.changes.AddReviewerInput; import com.google.gerrit.extensions.api.changes.Changes.QueryRequest; import com.google.gerrit.extensions.api.changes.DraftInput; import com.google.gerrit.extensions.api.changes.HashtagsInput; @@ -43,6 +44,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput; import com.google.gerrit.extensions.api.changes.StarsInput; import com.google.gerrit.extensions.api.groups.GroupInput; +import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ChangeMessageInfo; import com.google.gerrit.extensions.common.CommentInfo; @@ -1480,17 +1482,30 @@ } @Test - public void reviewer() throws Exception { + public void reviewerAndCc() throws Exception { + Account.Id user1 = createAccount("user1"); TestRepository<Repo> repo = createProject("repo"); Change change1 = insert(repo, newChange(repo)); Change change2 = insert(repo, newChange(repo)); insert(repo, newChange(repo)); - gApi.changes().id(change1.getId().get()).current().review(ReviewInput.approve()); - gApi.changes().id(change2.getId().get()).current().review(ReviewInput.approve()); + AddReviewerInput rin = new AddReviewerInput(); + rin.reviewer = user1.toString(); + rin.state = ReviewerState.REVIEWER; + gApi.changes().id(change1.getId().get()).addReviewer(rin); - Account.Id id = user.getAccountId(); - assertQuery("reviewer:" + id, change2, change1); + rin = new AddReviewerInput(); + rin.reviewer = user1.toString(); + rin.state = ReviewerState.CC; + gApi.changes().id(change2.getId().get()).addReviewer(rin); + + if (notesMigration.readChanges()) { + assertQuery("reviewer:" + user1, change1); + assertQuery("cc:" + user1, change2); + } else { + assertQuery("reviewer:" + user1, change2, change1); + assertQuery("cc:" + user1); + } } @Test
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js index e9fdbd1..a225b12 100644 --- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js +++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
@@ -22,6 +22,8 @@ 'author', 'branch', 'bug', + 'cc', + 'cc:self', 'change', 'comment', 'commentby', @@ -237,6 +239,7 @@ return this._fetchProjects(predicate, expression); case 'author': + case 'cc': case 'commentby': case 'committer': case 'from':