Add reviewer/group to label query

Adding ability to specify a user or group by:

* Adding a ',' after the label name followed by a reviewer id
  or 'user=' and the user id.
  e.g., label:CodeReview=1,jsmith
        label.CodeReview=1,user=jsmith

* Adding a ',' after the category name followed by a group id
  or 'group=' and the group id.
  e.g., label:CodeReview=1,android_bait
        label:CodeReview=1,group=android_bait

The user specific label query is backed by the label field in Lucene.
The filtering on the group is done only after results have been
retrieved from Lucene and thus it is more expensive.

Change-Id: I39a1000712743a6ed238e70d45c0ccc1eec50031

Documentation/user-search.txt

@@ -327,6 +327,12 @@ A label name is any of the following:
 * The one or two character abbreviation shown in the column header
   of change list pages.  Example: `label:R` or `label:V`.
 
+* The label name followed by a ',' followed by a reviewer id or a
+  group id.  To make it clear whether a user or group is being looked
+  for, precede the value by a user or group argument identifier
+  ('user=' or 'group=').  If an LDAP group is being referenced make
+  sure to use 'ldap/<groupname>'.
+
 A label name must be followed by a score, or an operator and a score.
 The easiest way to explain this is by example.
 
@@ -353,7 +359,20 @@ Scores of +2 are not matched, even though they are higher.
 +
 Matches changes with either a +1, +2, or any higher score.
 
-`label:Code-Review<=-1`::
+`label:CodeReview=+2,aname`::
++
+Matches changes with a +2 code review where the reviewer or group is aname.
+
+`label:CodeReview=2,user=jsmith`::
++
+Matches changes with a +2 code review where the reviewer is jsmith.
+
+`label:CodeReview=+1,group=ldap/linux.workflow`::
++
+Matches changes with a +1 code review where the reviewer is in the
+ldap/linux.workflow group.
+
+`label:CodeReview<=-1`::
 +
 Matches changes with either a -1, -2, or any lower score.
 

gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java

@@ -17,8 +17,9 @@ package com.google.gerrit.server.index;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
-import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.TrackingId;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -45,7 +46,7 @@ import java.util.Set;
  */
 public class ChangeField {
   /** Increment whenever making schema changes. */
-  public static final int SCHEMA_VERSION = 10;
+  public static final int SCHEMA_VERSION = 11;
 
   /** Legacy change ID. */
   public static final FieldDef<ChangeData, Integer> LEGACY_ID =
@@ -210,18 +211,27 @@ public class ChangeField {
         @Override
         public Iterable<String> get(ChangeData input, FillArgs args)
             throws OrmException {
+          Set<String> allApprovals = Sets.newHashSet();
           Set<String> distinctApprovals = Sets.newHashSet();
           for (PatchSetApproval a : input.currentApprovals(args.db)) {
             if (a.getValue() != 0) {
+              allApprovals.add(formatLabel(a.getLabel(), a.getValue(),
+                  a.getAccountId()));
               distinctApprovals.add(formatLabel(a.getLabel(), a.getValue()));
             }
           }
-          return distinctApprovals;
+          allApprovals.addAll(distinctApprovals);
+          return allApprovals;
         }
       };
 
   public static String formatLabel(String label, int value) {
-    return label.toLowerCase() + (value >= 0 ? "+" : "") + value;
+    return formatLabel(label, value, null);
+  }
+
+  public static String formatLabel(String label, int value, Account.Id accountId) {
+    return label.toLowerCase() + (value >= 0 ? "+" : "") + value
+        + (accountId != null ? "," + accountId.get() : "");
   }
 
   public static final ImmutableMap<String, FieldDef<ChangeData, ?>> ALL;

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java

@@ -52,6 +52,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.regex.Pattern;
 
@@ -100,6 +101,10 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
   public static final String FIELD_VISIBLETO = "visibleto";
   public static final String FIELD_WATCHEDBY = "watchedby";
 
+  public static final String ARG_ID_USER = "user";
+  public static final String ARG_ID_GROUP = "group";
+
+
   private static final QueryBuilder.Definition<ChangeData, ChangeQueryBuilder> mydef =
       new QueryBuilder.Definition<ChangeData, ChangeQueryBuilder>(
           ChangeQueryBuilder.class);
@@ -320,10 +325,57 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
   }
 
   @Operator
-  public Predicate<ChangeData> label(String name) {
+  public Predicate<ChangeData> label(String name) throws QueryParseException,
+      OrmException {
+    Set<Account.Id> accounts = null;
+    AccountGroup.UUID group = null;
+
+    // Parse for:
+    // label:CodeReview=1,user=jsmith or
+    // 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
+    String[] splitReviewer = name.split(",", 2);
+    name = splitReviewer[0];        // remove all but the vote piece, e.g.'CodeReview=1'
+
+    if (splitReviewer.length == 2) {
+      // process the user/group piece
+      PredicateArgs lblArgs = new PredicateArgs(splitReviewer[1]);
+
+      for (Map.Entry<String, String> pair : lblArgs.keyValue.entrySet()) {
+        if (pair.getKey().equalsIgnoreCase(ARG_ID_USER)) {
+          accounts = parseAccount(pair.getValue());
+        } else if (pair.getKey().equalsIgnoreCase(ARG_ID_GROUP)) {
+          group = parseGroup(pair.getValue()).getUUID();
+        } else {
+          throw new QueryParseException(
+              "Invalid argument identifier '"   + pair.getKey() + "'");
+        }
+      }
+
+      for (String value : lblArgs.positional) {
+       if (accounts != null || group != null) {
+          throw new QueryParseException("more than one user/group specified (" +
+              value + ")");
+        }
+        try {
+          accounts = parseAccount(value);
+        } catch (QueryParseException qpex) {
+          // If it doesn't match an account, see if it matches a group
+          // (accounts get precedence)
+          try {
+            group = parseGroup(value).getUUID();
+          } catch (QueryParseException e) {
+            throw error("Neither user nor group " + value + " found");
+          }
+        }
+      }
+    }
+
     return new LabelPredicate(args.projectCache,
         args.changeControlGenericFactory, args.userFactory, args.dbProvider,
-        name);
+        name, accounts, group);
   }
 
   @Operator
@@ -535,7 +587,11 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
       }
 
     } else if (PAT_LABEL.matcher(query).find()) {
-      return label(query);
+      try {
+        return label(query);
+      } catch (OrmException err) {
+        throw error("Cannot lookup user", err);
+      }
 
     } else {
       // Try to match a project name by substring query.
@@ -572,6 +628,15 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     return matches;
   }
 
+  private GroupReference parseGroup(String group) throws QueryParseException {
+    GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend,
+        group);
+    if (g == null) {
+      throw error("Group " + group + " not found");
+    }
+    return g;
+  }
+
   private Account.Id self() {
     if (currentUser instanceof IdentifiedUser) {
       return ((IdentifiedUser) currentUser).getAccountId();

gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java

@@ -18,6 +18,7 @@ import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.LabelTypes;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -38,18 +39,23 @@ class EqualsLabelPredicate extends IndexPredicate<ChangeData> {
   private final Provider<ReviewDb> dbProvider;
   private final String label;
   private final int expVal;
+  private final Account.Id account;
+  private final AccountGroup.UUID group;
 
   EqualsLabelPredicate(ProjectCache projectCache,
       ChangeControl.GenericFactory ccFactory,
       IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider,
-      String label, int expVal) {
-    super(ChangeField.LABEL, ChangeField.formatLabel(label, expVal));
+      String label, int expVal, Account.Id account,
+      AccountGroup.UUID group) {
+    super(ChangeField.LABEL, ChangeField.formatLabel(label, expVal, account));
     this.ccFactory = ccFactory;
     this.projectCache = projectCache;
     this.userFactory = userFactory;
     this.dbProvider = dbProvider;
     this.label = label;
     this.expVal = expVal;
+    this.account = account;
+    this.group = group;
   }
 
   @Override
@@ -110,9 +116,9 @@ class EqualsLabelPredicate extends IndexPredicate<ChangeData> {
     if (psVal == expVal) {
       // Double check the value is still permitted for the user.
       //
+      IdentifiedUser reviewer = userFactory.create(dbProvider, approver);
       try {
-        ChangeControl cc = ccFactory.controlFor(change, //
-            userFactory.create(dbProvider, approver));
+        ChangeControl cc = ccFactory.controlFor(change, reviewer);
         if (!cc.isVisible(dbProvider.get())) {
           // The user can't see the change anymore.
           //
@@ -125,6 +131,14 @@ class EqualsLabelPredicate extends IndexPredicate<ChangeData> {
         return false;
       }
 
+      if (account != null && !account.equals(approver)) {
+        return false;
+      }
+
+      if (group != null && !reviewer.getEffectiveGroups().contains(group)) {
+        return false;
+      }
+
       if (psVal == expVal) {
         return true;
       }
@@ -134,6 +148,6 @@ class EqualsLabelPredicate extends IndexPredicate<ChangeData> {
 
   @Override
   public int getCost() {
-    return 1;
+    return 1 + (group == null ? 0 : 1);
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java

@@ -15,6 +15,8 @@
 package com.google.gerrit.server.query.change;
 
 import com.google.common.collect.Lists;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.project.ChangeControl;
@@ -24,6 +26,7 @@ import com.google.gerrit.server.query.Predicate;
 import com.google.inject.Provider;
 
 import java.util.List;
+import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -62,16 +65,16 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
   LabelPredicate(ProjectCache projectCache,
       ChangeControl.GenericFactory ccFactory,
       IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider,
-      String value) {
-    super(predicates(projectCache, ccFactory, userFactory,
-          dbProvider, value));
+      String value, Set<Account.Id> accounts, AccountGroup.UUID group) {
+    super(predicates(projectCache, ccFactory, userFactory, dbProvider, value,
+        accounts, group));
     this.value = value;
   }
 
   private static List<Predicate<ChangeData>> predicates(
       ProjectCache projectCache, ChangeControl.GenericFactory ccFactory,
       IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider,
-      String value) {
+      String value, Set<Account.Id> accounts, AccountGroup.UUID group) {
     String label;
     Test test;
     int expVal;
@@ -97,19 +100,19 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
     if (test.isEq()) {
       if (expVal != 0) {
         r.add(equalsLabelPredicate(projectCache, ccFactory, userFactory,
-            dbProvider, label, expVal));
+            dbProvider, label, expVal, accounts, group));
       } else {
         r.add(noLabelQuery(projectCache, ccFactory, userFactory,
-            dbProvider, label));
+            dbProvider, label, accounts, group));
       }
     } else {
       for (int i = test.isGtEq() ? expVal : neg(expVal); i <= MAX_LABEL_VALUE; i++) {
         if (i != 0) {
           r.add(equalsLabelPredicate(projectCache, ccFactory, userFactory,
-              dbProvider, label, test.isGtEq() ? i : neg(i)));
+              dbProvider, label, test.isGtEq() ? i : neg(i), accounts, group));
         } else {
           r.add(noLabelQuery(projectCache, ccFactory, userFactory,
-              dbProvider, label));
+              dbProvider, label, accounts, group));
         }
       }
     }
@@ -127,23 +130,37 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
     return -1 * value;
   }
 
-  private static Predicate<ChangeData> noLabelQuery(ProjectCache projectCache, ChangeControl.GenericFactory ccFactory,
-      IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider, String label) {
+  private static Predicate<ChangeData> noLabelQuery(ProjectCache projectCache,
+      ChangeControl.GenericFactory ccFactory,
+      IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider,
+      String label, Set<Account.Id> accounts, AccountGroup.UUID group) {
     List<Predicate<ChangeData>> r =
         Lists.newArrayListWithCapacity(2 * MAX_LABEL_VALUE);
     for (int i = 1; i <= MAX_LABEL_VALUE; i++) {
       r.add(not(equalsLabelPredicate(projectCache, ccFactory, userFactory,
-          dbProvider, label, i)));
+          dbProvider, label, i, accounts, group)));
       r.add(not(equalsLabelPredicate(projectCache, ccFactory, userFactory,
-          dbProvider, label, neg(i))));
+          dbProvider, label, neg(i), accounts, group)));
     }
     return and(r);
   }
 
-  private static Predicate<ChangeData> equalsLabelPredicate(ProjectCache projectCache, ChangeControl.GenericFactory ccFactory,
-      IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider, String label, int expVal) {
-    return new EqualsLabelPredicate(projectCache, ccFactory, userFactory,
-        dbProvider, label, expVal);
+  private static Predicate<ChangeData> equalsLabelPredicate(
+      ProjectCache projectCache, ChangeControl.GenericFactory ccFactory,
+      IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider,
+      String label, int expVal, Set<Account.Id> accounts,
+      AccountGroup.UUID group) {
+    if (accounts == null || accounts.isEmpty()) {
+      return new EqualsLabelPredicate(projectCache, ccFactory, userFactory,
+          dbProvider, label, expVal, null, group);
+    } else {
+      List<Predicate<ChangeData>> r = Lists.newArrayList();
+      for (Account.Id a : accounts) {
+        r.add(new EqualsLabelPredicate(projectCache, ccFactory, userFactory,
+            dbProvider, label, expVal, a, group));
+      }
+      return or(r);
+    }
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/query/change/PredicateArgs.java

@@ -0,0 +1,70 @@
+// Copyright (C) 2013 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.common.collect.Lists;
+import com.google.common.collect.Maps;
+import com.google.gerrit.server.query.QueryParseException;
+
+import java.util.List;
+import java.util.Map;
+
+/**
+ * This class is used to extract comma separated values in a predicate
+ *
+ * If tags for the values are present (e.g. "branch=jb_2.3,vote=approved") then
+ * the args are placed in a map that maps tag to value (e.g., "branch" to "jb_2.3").
+ * If no tag is present (e.g. "jb_2.3,approved") then the args are placed into a
+ * positional list.  Args may be mixed so some may appear in the map and others
+ * in the positional list (e.g. "vote=approved,jb_2.3).
+ */
+public class PredicateArgs {
+  public List<String> positional;
+  public Map<String, String> keyValue;
+
+  /**
+   * Parses query arguments into keyValue and/or positional values
+   * labels for these arguments should be kept in ChangeQueryBuilder
+   * as ARG_ID_{argument name}.
+   *
+   * @param args - arguments to be parsed
+   *
+   * @return - the static values keyValue and positional will contain
+   *           the parsed values.
+   * @throws QueryParseException
+   */
+  PredicateArgs(String args) throws QueryParseException {
+    positional = Lists.newArrayList();
+    keyValue = Maps.newHashMap();
+
+    String[] splitArgs = args.split(",");
+
+    for (String arg : splitArgs) {
+      String[] splitKeyValue = arg.split("=");
+
+      if (splitKeyValue.length == 1) {
+        positional.add(splitKeyValue[0]);
+      } else if (splitKeyValue.length == 2) {
+        if (!keyValue.containsKey(splitKeyValue[0])) {
+          keyValue.put(splitKeyValue[0], splitKeyValue[1]);
+        } else {
+          throw new QueryParseException("Duplicate key " + splitKeyValue[0]);
+        }
+      } else {
+        throw new QueryParseException("invalid arg " + arg);
+      }
+    }
+  }
+}