Serve label predicate from Lucene index

To build the Lucene query we need to know the maximum range a label can
have. Since the label ranges can be defined per project the maximum
range for a label is hard to compute. For this we would need to scan
over all projects and parse the project.config files. Instead of doing
this we hard code for now a maximum query range from -4 to +4. This
range should be wide enough to support most labels.

The query behaviour for searching labels with value 0 has changed. We
now return all changes that have no voting on that label from any
reviewer (this includes changes without any reviewer). Earlier we
returned changes that had a reviewer for which an explicit 0 value was
stored. This old behaviour was unexpected by most users.

Change-Id: Ic0e5ae86933d605e67c9da8ef289a0b099d82cbf
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>

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

@@ -45,7 +45,7 @@ import java.util.Set;
  */
 public class ChangeField {
   /** Increment whenever making schema changes. */
-  public static final int SCHEMA_VERSION = 9;
+  public static final int SCHEMA_VERSION = 10;
 
   /** Legacy change ID. */
   public static final FieldDef<ChangeData, Integer> LEGACY_ID =
@@ -203,6 +203,27 @@ public class ChangeField {
         }
       };
 
+  /** List of labels on the current patch set. */
+  public static final FieldDef<ChangeData, Iterable<String>> LABEL =
+      new FieldDef.Repeatable<ChangeData, String>(
+          ChangeQueryBuilder.FIELD_LABEL, FieldType.EXACT, false) {
+        @Override
+        public Iterable<String> get(ChangeData input, FillArgs args)
+            throws OrmException {
+          Set<String> distinctApprovals = Sets.newHashSet();
+          for (PatchSetApproval a : input.currentApprovals(args.db)) {
+            if (a.getValue() != 0) {
+              distinctApprovals.add(formatLabel(a.getLabel(), a.getValue()));
+            }
+          }
+          return distinctApprovals;
+        }
+      };
+
+  public static String formatLabel(String label, int value) {
+    return label.toLowerCase() + (value >= 0 ? "+" : "") + value;
+  }
+
   public static final ImmutableMap<String, FieldDef<ChangeData, ?>> ALL;
 
   static {

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

@@ -0,0 +1,139 @@
+// 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.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.Change;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.index.ChangeField;
+import com.google.gerrit.server.index.IndexPredicate;
+import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Provider;
+
+class EqualsLabelPredicate extends IndexPredicate<ChangeData> {
+  private final ProjectCache projectCache;
+  private final ChangeControl.GenericFactory ccFactory;
+  private final IdentifiedUser.GenericFactory userFactory;
+  private final Provider<ReviewDb> dbProvider;
+  private final String label;
+  private final int expVal;
+
+  EqualsLabelPredicate(ProjectCache projectCache,
+      ChangeControl.GenericFactory ccFactory,
+      IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider,
+      String label, int expVal) {
+    super(ChangeField.LABEL, ChangeField.formatLabel(label, expVal));
+    this.ccFactory = ccFactory;
+    this.projectCache = projectCache;
+    this.userFactory = userFactory;
+    this.dbProvider = dbProvider;
+    this.label = label;
+    this.expVal = expVal;
+  }
+
+  @Override
+  public boolean match(ChangeData object) throws OrmException {
+    Change c = object.change(dbProvider);
+    if (c == null) {
+      // The change has disappeared.
+      //
+      return false;
+    }
+    ProjectState project = projectCache.get(c.getDest().getParentKey());
+    if (project == null) {
+      // The project has disappeared.
+      //
+      return false;
+    }
+    LabelType labelType = type(project.getLabelTypes(), label);
+    boolean hasVote = false;
+    for (PatchSetApproval p : object.currentApprovals(dbProvider)) {
+      if (labelType.matches(p)) {
+        hasVote = true;
+        if (match(c, p.getValue(), p.getAccountId(), labelType)) {
+          return true;
+        }
+      }
+    }
+
+    if (!hasVote && expVal == 0) {
+      return true;
+    }
+
+    return false;
+  }
+
+  private static LabelType type(LabelTypes types, String toFind) {
+    if (types.byLabel(toFind) != null) {
+      return types.byLabel(toFind);
+    }
+
+    for (LabelType lt : types.getLabelTypes()) {
+      if (toFind.equalsIgnoreCase(lt.getName())) {
+        return lt;
+      }
+    }
+
+    for (LabelType lt : types.getLabelTypes()) {
+      if (toFind.equalsIgnoreCase(lt.getAbbreviation())) {
+        return lt;
+      }
+    }
+
+    return LabelType.withDefaultValues(toFind);
+  }
+
+  private boolean match(Change change, int value, Account.Id approver,
+      LabelType type) throws OrmException {
+    int psVal = value;
+    if (psVal == expVal) {
+      // Double check the value is still permitted for the user.
+      //
+      try {
+        ChangeControl cc = ccFactory.controlFor(change, //
+            userFactory.create(dbProvider, approver));
+        if (!cc.isVisible(dbProvider.get())) {
+          // The user can't see the change anymore.
+          //
+          return false;
+        }
+        psVal = cc.getRange(Permission.forLabel(type.getName())).squash(psVal);
+      } catch (NoSuchChangeException e) {
+        // The project has disappeared.
+        //
+        return false;
+      }
+
+      if (psVal == expVal) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  @Override
+  public int getCost() {
+    return 1;
+  }
+}

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

@@ -14,85 +14,107 @@
 
 package com.google.gerrit.server.query.change;
 
-import com.google.gerrit.common.data.LabelType;
+import com.google.common.collect.Lists;
-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.Change;
-import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.project.ProjectState;
+import com.google.gerrit.server.query.OrPredicate;
-import com.google.gerrit.server.query.OperatorPredicate;
+import com.google.gerrit.server.query.Predicate;
-import com.google.gwtorm.server.OrmException;
 import com.google.inject.Provider;
 
-import java.util.HashSet;
+import java.util.List;
-import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
-class LabelPredicate extends OperatorPredicate<ChangeData> {
+public class LabelPredicate extends OrPredicate<ChangeData> {
+  private static final int MAX_LABEL_VALUE = 4;
+
   private static enum Test {
-    EQ {
+    EQ, GT_EQ, LT_EQ;
-      @Override
-      public boolean match(int psValue, int expValue) {
-        return psValue == expValue;
-      }
-    },
-    GT_EQ {
-      @Override
-      public boolean match(int psValue, int expValue) {
-        return psValue >= expValue;
-      }
-    },
-    LT_EQ {
-      @Override
-      public boolean match(int psValue, int expValue) {
-        return psValue <= expValue;
-      }
-    };
 
-    abstract boolean match(int psValue, int expValue);
+    boolean isEq() {
+      return EQ.equals(this);
     }
 
-  private static LabelType type(LabelTypes types, String toFind) {
+    boolean isGtEq() {
-    if (types.byLabel(toFind) != null) {
+      return GT_EQ.equals(this);
-      return types.byLabel(toFind);
     }
 
-    for (LabelType lt : types.getLabelTypes()) {
+    static Test op(String op) {
-      if (toFind.equalsIgnoreCase(lt.getName())) {
-        return lt;
-      }
-    }
-
-    for (LabelType lt : types.getLabelTypes()) {
-      if (toFind.equalsIgnoreCase(lt.getAbbreviation())) {
-        return lt;
-      }
-    }
-
-    return LabelType.withDefaultValues(toFind);
-  }
-
-  private static Test op(String op) {
       if ("=".equals(op)) {
-      return Test.EQ;
+        return EQ;
 
       } else if (">=".equals(op)) {
-      return Test.GT_EQ;
+        return GT_EQ;
 
       } else if ("<=".equals(op)) {
-      return Test.LT_EQ;
+        return LT_EQ;
 
       } else {
         throw new IllegalArgumentException("Unsupported operation " + op);
       }
     }
+  }
+
+  private final String value;
+
+  LabelPredicate(ProjectCache projectCache,
+      ChangeControl.GenericFactory ccFactory,
+      IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider,
+      String value) {
+    super(predicates(projectCache, ccFactory, userFactory,
+          dbProvider, value));
+    this.value = value;
+  }
+
+  private static List<Predicate<ChangeData>> predicates(
+      ProjectCache projectCache, ChangeControl.GenericFactory ccFactory,
+      IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider,
+      String value) {
+    String label;
+    Test test;
+    int expVal;
+    Matcher m1 = Pattern.compile("(=|>=|<=)([+-]?\\d+)$").matcher(value);
+    Matcher m2 = Pattern.compile("([+-]\\d+)$").matcher(value);
+    if (m1.find()) {
+      label = value.substring(0, m1.start());
+      test = Test.op(m1.group(1));
+      expVal = value(m1.group(2));
+
+    } else if (m2.find()) {
+      label = value.substring(0, m2.start());
+      test = Test.EQ;
+      expVal = value(m2.group(1));
+
+    } else {
+      label = value;
+      test = Test.EQ;
+      expVal = 1;
+    }
+
+    List<Predicate<ChangeData>> r = Lists.newArrayListWithCapacity(2 * MAX_LABEL_VALUE);
+    if (test.isEq()) {
+      if (expVal != 0) {
+        r.add(equalsLabelPredicate(projectCache, ccFactory, userFactory,
+            dbProvider, label, expVal));
+      } else {
+        r.add(noLabelQuery(projectCache, ccFactory, userFactory,
+            dbProvider, label));
+      }
+    } 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)));
+        } else {
+          r.add(noLabelQuery(projectCache, ccFactory, userFactory,
+              dbProvider, label));
+        }
+      }
+    }
+    return r;
+  }
 
   private static int value(String value) {
     if (value.startsWith("+")) {
@@ -101,113 +123,31 @@ class LabelPredicate extends OperatorPredicate<ChangeData> {
     return Integer.parseInt(value);
   }
 
-  private final ProjectCache projectCache;
+  private static int neg(int value) {
-  private final ChangeControl.GenericFactory ccFactory;
+    return -1 * value;
-  private final IdentifiedUser.GenericFactory userFactory;
-  private final Provider<ReviewDb> dbProvider;
-  private final Test test;
-  private final String type;
-  private final int expVal;
-
-  LabelPredicate(ProjectCache projectCache,
-      ChangeControl.GenericFactory ccFactory,
-      IdentifiedUser.GenericFactory userFactory,
-      Provider<ReviewDb> dbProvider,
-      String value) {
-    super(ChangeQueryBuilder.FIELD_LABEL, value);
-    this.ccFactory = ccFactory;
-    this.projectCache = projectCache;
-    this.userFactory = userFactory;
-    this.dbProvider = dbProvider;
-
-    Matcher m1 = Pattern.compile("(=|>=|<=)([+-]?\\d+)$").matcher(value);
-    Matcher m2 = Pattern.compile("([+-]\\d+)$").matcher(value);
-    if (m1.find()) {
-      type = value.substring(0, m1.start());
-      test = op(m1.group(1));
-      expVal = value(m1.group(2));
-
-    } else if (m2.find()) {
-      type = value.substring(0, m2.start());
-      test = Test.EQ;
-      expVal = value(m2.group(1));
-
-    } else {
-      type = value;
-      test = Test.EQ;
-      expVal = 1;
   }
+
+  private static Predicate<ChangeData> noLabelQuery(ProjectCache projectCache, ChangeControl.GenericFactory ccFactory,
+      IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider, String label) {
+    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)));
+      r.add(not(equalsLabelPredicate(projectCache, ccFactory, userFactory,
+          dbProvider, label, neg(i))));
+    }
+    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);
   }
 
   @Override
-  public boolean match(final ChangeData object) throws OrmException {
+  public String toString() {
-    final Change c = object.change(dbProvider);
+    return ChangeQueryBuilder.FIELD_LABEL + ":" + value;
-    if (c == null) {
-      // The change has disappeared.
-      //
-      return false;
-    }
-    final ProjectState project = projectCache.get(c.getDest().getParentKey());
-    if (project == null) {
-      // The project has disappeared.
-      //
-      return false;
-    }
-    final LabelType labelType = type(project.getLabelTypes(), type);
-    final Set<Account.Id> allApprovers = new HashSet<Account.Id>();
-    final Set<Account.Id> approversThatVotedInCategory = new HashSet<Account.Id>();
-    for (PatchSetApproval p : object.currentApprovals(dbProvider)) {
-      allApprovers.add(p.getAccountId());
-      if (labelType.matches(p)) {
-        approversThatVotedInCategory.add(p.getAccountId());
-        if (match(c, p.getValue(), p.getAccountId(), labelType)) {
-          return true;
-        }
-      }
-    }
-
-    final Set<Account.Id> approversThatDidNotVoteInCategory = new HashSet<Account.Id>(allApprovers);
-    approversThatDidNotVoteInCategory.removeAll(approversThatVotedInCategory);
-    for (Account.Id a : approversThatDidNotVoteInCategory) {
-      if (match(c, 0, a, labelType)) {
-        return true;
-      }
-    }
-
-    return false;
-  }
-
-  private boolean match(final Change change, final int value,
-      final Account.Id approver, final LabelType type)
-      throws OrmException {
-    int psVal = value;
-    if (test.match(psVal, expVal)) {
-      // Double check the value is still permitted for the user.
-      //
-      try {
-        ChangeControl cc = ccFactory.controlFor(change, //
-            userFactory.create(dbProvider, approver));
-        if (!cc.isVisible(dbProvider.get())) {
-          // The user can't see the change anymore.
-          //
-          return false;
-        }
-        psVal = cc.getRange(Permission.forLabel(type.getName())).squash(psVal);
-      } catch (NoSuchChangeException e) {
-        // The project has disappeared.
-        //
-        return false;
-      }
-
-      if (test.match(psVal, expVal)) {
-        return true;
-      }
-    }
-    return false;
-  }
-
-  @Override
-  public int getCost() {
-    return 2;
   }
 }