Migrate ApprovalTypes away from ApprovalCategory(Value)

Many callers of ApprovalTypes depend directly on ApprovalCategory and
ApprovalCategoryValue, which are the types stored in the database. We
want to move away from storing approval categories in the database in
favor of arbitrary labels that can be defined on a per-project level,
including implicitly in submit rules. Leaking the database types
around the code makes this harder.

Replace ApprovalCategoryValue everywhere except the database code with
a similar and more concise LabelValue type. Use Strings instead of the
database Id types when referring to labels in general. In the short
term this may create some confusion in the code (alleviated,
hopefully, by documentation and variable names), since some places use
legacy category IDs as unique keys and others use new style label
names as unique keys. In the long term we will stop using category IDs
except to map them to unique label names.

Unlike ApprovalCategory, the new and improved ApprovalType does not
distinguish between the label name and the human-readable name.
Generally these do not vary significantly other than the label name
having dashes instead of spaces, which is minor enough in the UI. The
other affected functionality here is searching by label; since label
names can no longer contain spaces "label:CodeReview" no longer works,
only "label:Code-Review".

Change-Id: I6854259d60af88b6d9df8d7553120a9af64c74ad

Documentation/user-search.txt

@@ -37,7 +37,7 @@ text and let Gerrit figure out the meaning:
 |Full or abbreviated Change-Id    | Ic0ff33
 |Full or abbreviated commit SHA-1 | d81b32ef
 |Email address                    | user@example.com
-|Approval requirement             | CodeReview>=+2, Verified=1
+|Approval requirement             | Code-Review>=+2, Verified=1
 |=============================================================
 
 
@@ -317,16 +317,12 @@ Labels
 ------
 Label operators can be used to match approval scores given during
 a code review.  The specific set of supported labels depends on
-the server configuration, however `CodeReview` and `Verified`
+the server configuration, however `Code-Review` and `Verified`
 are the default labels provided out of the box.
 
 A label name is any of the following:
 
-* The category name.  If the category name contains spaces,
+* The label name.
-  it must be wrapped in double quotes.  Example: `label:"Code Review"`.
-
-* The name, without spaces.  This avoids needing to use double quotes
-  for the common category Code Review.  Example: `label:CodeReview`.
 
 * The internal short name.  Example: `label:CRVW`, or `label:VRIF`.
 
@@ -336,38 +332,38 @@ A label name is any of the following:
 A label name must be followed by a score, or an operator and a score.
 The easiest way to explain this is by example.
 
-`label:CodeReview=2`::
+`label:Code-Review=2`::
-`label:CodeReview=+2`::
+`label:Code-Review=+2`::
-`label:CodeReview+2`::
+`label:Code-Review+2`::
 +
-Matches changes where there is at least one +2 score for Code Review.
+Matches changes where there is at least one +2 score for Code-Review.
 The + prefix is optional for positive score values.  If the + is used,
 the = operator is optional.
 
-`label:CodeReview=-2`::
+`label:Code-Review=-2`::
-`label:CodeReview-2`::
+`label:Code-Review-2`::
 +
-Matches changes where there is at least one -2 score for Code Review.
+Matches changes where there is at least one -2 score for Code-Review.
 Because the negative sign is required, the = operator is optional.
 
-`label:CodeReview=1`::
+`label:Code-Review=1`::
 +
-Matches changes where there is at least one +1 score for Code Review.
+Matches changes where there is at least one +1 score for Code-Review.
 Scores of +2 are not matched, even though they are higher.
 
-`label:CodeReview>=1`::
+`label:Code-Review>=1`::
 +
 Matches changes with either a +1, +2, or any higher score.
 
-`label:CodeReview<=-1`::
+`label:Code-Review<=-1`::
 +
 Matches changes with either a -1, -2, or any lower score.
 
-`is:open CodeReview+2 Verified+1 -Verified-1 -CodeReview-2`::
+`is:open Code-Review+2 Verified+1 -Verified-1 -Code-Review-2`::
 +
 Matches changes that are ready to be submitted.
 
-`is:open (Verified-1 OR CodeReview-2)`::
+`is:open (Verified-1 OR Code-Review-2)`::
 +
 Changes that are blocked from submission due to a blocking score.
 

gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalSummary.java

@@ -14,7 +14,6 @@
 
 package com.google.gerrit.common.data;
 
-import com.google.gerrit.reviewdb.client.ApprovalCategory;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 
 import java.util.Collections;
@@ -26,19 +25,21 @@ import java.util.Map;
  * category, with all of the approvals coming from a single patch set.
  */
 public class ApprovalSummary {
-  protected Map<ApprovalCategory.Id, PatchSetApproval> approvals;
+  protected Map<String, PatchSetApproval> approvals;
 
   protected ApprovalSummary() {
   }
 
   public ApprovalSummary(final Iterable<PatchSetApproval> list) {
-    approvals = new HashMap<ApprovalCategory.Id, PatchSetApproval>();
+    approvals = new HashMap<String, PatchSetApproval>();
     for (final PatchSetApproval a : list) {
-      approvals.put(a.getCategoryId(), a);
+      approvals.put(a.getCategoryId().get(), a);
     }
   }
 
-  public Map<ApprovalCategory.Id, PatchSetApproval> getApprovalMap() {
+  // TODO: Convert keys to label names.
+  /** @return a map of approvals keyed by ID string. */
+  public Map<String, PatchSetApproval> getApprovalMap() {
     return Collections.unmodifiableMap(approvals);
   }
 }

gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalType.java

@@ -26,27 +26,97 @@ import java.util.List;
 import java.util.Map;
 
 public class ApprovalType {
-  protected ApprovalCategory category;
+  public static ApprovalType fromApprovalCategory(ApprovalCategory ac,
-  protected List<ApprovalCategoryValue> values;
+      List<ApprovalCategoryValue> acvs) {
+    List<LabelValue> values = new ArrayList<LabelValue>(acvs.size());
+    for (ApprovalCategoryValue acv : acvs) {
+      values.add(
+          new LabelValue(acv.getValue(), acv.getName()));
+    }
+    ApprovalType at =
+        new ApprovalType(ac.getId().get(), ac.getLabelName(), values);
+    at.setAbbreviatedName(ac.getAbbreviatedName());
+    at.setFunctionName(ac.getFunctionName());
+    at.setCopyMinScore(ac.isCopyMinScore());
+    at.setPosition(ac.getPosition());
+    return at;
+  }
+
+  public static ApprovalType withDefaultValues(String id, String name) {
+    checkName(name);
+    List<LabelValue> values = new ArrayList<LabelValue>(2);
+    values.add(new LabelValue((short) 0, "Rejected"));
+    values.add(new LabelValue((short) 1, "Approved"));
+    return new ApprovalType(id, name, values);
+  }
+
+  private static String checkId(String id) {
+    if (id == null || id.length() > 4) {
+      throw new IllegalArgumentException(
+          "Illegal label ID \"" + id + "\"");
+    }
+    return id;
+  }
+
+  private static String checkName(String name) {
+    for (int i = 0; i < name.length(); i++) {
+      char c = name.charAt(i);
+      if (!((c >= 'a' && c <= 'z') ||
+            (c >= 'A' && c <= 'Z') ||
+            (c >= '0' && c <= '9') ||
+            c == '-')) {
+        throw new IllegalArgumentException(
+            "Illegal label name \"" + name + "\"");
+      }
+    }
+    return name;
+  }
+
+  private static String defaultAbbreviation(String name) {
+    StringBuilder abbr = new StringBuilder();
+    for (int i = 0; i < name.length(); i++) {
+      char c = name.charAt(i);
+      if (c >= 'A' && c <= 'Z') {
+        abbr.append(c);
+      }
+    }
+    if (abbr.length() == 0) {
+      abbr.append(Character.toUpperCase(name.charAt(0)));
+    }
+    return abbr.toString();
+  }
+
+  protected String name;
+
+  protected String id;
+  protected String abbreviatedName;
+  protected String functionName;
+  protected boolean copyMinScore;
+  protected short position;
+
+  protected List<LabelValue> values;
   protected short maxNegative;
   protected short maxPositive;
 
   private transient List<Integer> intList;
-  private transient Map<Short, ApprovalCategoryValue> byValue;
+  private transient Map<Short, LabelValue> byValue;
 
   protected ApprovalType() {
   }
 
-  public ApprovalType(final ApprovalCategory ac,
+  public ApprovalType(String id, String name, List<LabelValue> valueList) {
-      final List<ApprovalCategoryValue> valueList) {
+    this.id = checkId(id);
-    category = ac;
+    this.name = checkName(name);
-    values = new ArrayList<ApprovalCategoryValue>(valueList);
+    values = new ArrayList<LabelValue>(valueList);
-    Collections.sort(values, new Comparator<ApprovalCategoryValue>() {
+    Collections.sort(values, new Comparator<LabelValue>() {
-      public int compare(ApprovalCategoryValue o1, ApprovalCategoryValue o2) {
+      public int compare(LabelValue o1, LabelValue o2) {
         return o1.getValue() - o2.getValue();
       }
     });
 
+    abbreviatedName = defaultAbbreviation(name);
+    functionName = "MaxWithBlock";
+
     maxNegative = Short.MIN_VALUE;
     maxPositive = Short.MAX_VALUE;
     if (values.size() > 0) {
@@ -57,57 +127,90 @@ public class ApprovalType {
         maxPositive = values.get(values.size() - 1).getValue();
       }
     }
-
-    // Force the label name to pre-compute so we don't have data race conditions.
-    getCategory().getLabelName();
   }
 
-  public ApprovalCategory getCategory() {
+  public String getName() {
-    return category;
+    return name;
   }
 
-  public List<ApprovalCategoryValue> getValues() {
+  public String getId() {
+    return id;
+  }
+
+  public String getAbbreviatedName() {
+    return abbreviatedName;
+  }
+
+  public void setAbbreviatedName(String abbreviatedName) {
+    this.abbreviatedName = abbreviatedName;
+  }
+
+  public String getFunctionName() {
+    return functionName;
+  }
+
+  public void setFunctionName(String functionName) {
+    this.functionName = functionName;
+  }
+
+  public short getPosition() {
+    return position;
+  }
+
+  public void setPosition(short position) {
+    this.position = position;
+  }
+
+  public List<LabelValue> getValues() {
     return values;
   }
 
-  public ApprovalCategoryValue getMin() {
+  public LabelValue getMin() {
     if (values.isEmpty()) {
       return null;
     }
     return values.get(0);
   }
 
-  public ApprovalCategoryValue getMax() {
+  public LabelValue getMax() {
     if (values.isEmpty()) {
       return null;
     }
-    final ApprovalCategoryValue v = values.get(values.size() - 1);
+    final LabelValue v = values.get(values.size() - 1);
     return v.getValue() > 0 ? v : null;
   }
 
-  public boolean isMaxNegative(final PatchSetApproval ca) {
+  public boolean isCopyMinScore() {
+    return copyMinScore;
+  }
+
+  public void setCopyMinScore(boolean copyMinScore) {
+    this.copyMinScore = copyMinScore;
+  }
+
+  public boolean isMaxNegative(PatchSetApproval ca) {
     return maxNegative == ca.getValue();
   }
 
-  public boolean isMaxPositive(final PatchSetApproval ca) {
+  public boolean isMaxPositive(PatchSetApproval ca) {
     return maxPositive == ca.getValue();
   }
 
-  public ApprovalCategoryValue getValue(final short value) {
+  public LabelValue getValue(short value) {
     initByValue();
     return byValue.get(value);
   }
 
-  public ApprovalCategoryValue getValue(final PatchSetApproval ca) {
+  public LabelValue getValue(final PatchSetApproval ca) {
     initByValue();
     return byValue.get(ca.getValue());
   }
 
   private void initByValue() {
     if (byValue == null) {
-      byValue = new HashMap<Short, ApprovalCategoryValue>();
+      byValue = new HashMap<Short, LabelValue>();
-      for (final ApprovalCategoryValue acv : values) {
+      for (final LabelValue v : values) {
-        byValue.put(acv.getValue(), acv);
+        byValue.put(v.getValue(), v);
       }
     }
   }
@@ -115,12 +218,22 @@ public class ApprovalType {
   public List<Integer> getValuesAsList() {
     if (intList == null) {
       intList = new ArrayList<Integer>(values.size());
-      for (ApprovalCategoryValue acv : values) {
+      for (LabelValue v : values) {
-        intList.add(Integer.valueOf(acv.getValue()));
+        intList.add(Integer.valueOf(v.getValue()));
       }
       Collections.sort(intList);
       Collections.reverse(intList);
     }
     return intList;
   }
+
+  @Deprecated
+  public ApprovalCategoryValue.Id getApprovalCategoryValueId(short value) {
+    return new ApprovalCategoryValue.Id(getApprovalCategoryId(), value);
+  }
+
+  @Deprecated
+  public ApprovalCategory.Id getApprovalCategoryId() {
+    return new ApprovalCategory.Id(getId());
+  }
 }

gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalTypes.java

@@ -14,15 +14,13 @@
 
 package com.google.gerrit.common.data;
 
-import com.google.gerrit.reviewdb.client.ApprovalCategory;
-
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
 public class ApprovalTypes {
   protected List<ApprovalType> approvalTypes;
-  private transient Map<ApprovalCategory.Id, ApprovalType> byId;
+  private transient Map<String, ApprovalType> byId;
   private transient Map<String, ApprovalType> byLabel;
 
   protected ApprovalTypes() {
@@ -30,23 +28,23 @@ public class ApprovalTypes {
 
   public ApprovalTypes(final List<ApprovalType> approvals) {
     approvalTypes = approvals;
-    byCategory();
+    byId();
   }
 
   public List<ApprovalType> getApprovalTypes() {
     return approvalTypes;
   }
 
-  public ApprovalType byId(final ApprovalCategory.Id id) {
+  public ApprovalType byId(String id) {
-    return byCategory().get(id);
+    return byId().get(id);
   }
 
-  private Map<ApprovalCategory.Id, ApprovalType> byCategory() {
+  private Map<String, ApprovalType> byId() {
     if (byId == null) {
-      byId = new HashMap<ApprovalCategory.Id, ApprovalType>();
+      byId = new HashMap<String, ApprovalType>();
       if (approvalTypes != null) {
         for (final ApprovalType t : approvalTypes) {
-          byId.put(t.getCategory().getId(), t);
+          byId.put(t.getId(), t);
         }
       }
     }
@@ -62,7 +60,7 @@ public class ApprovalTypes {
       byLabel = new HashMap<String, ApprovalType>();
       if (approvalTypes != null) {
         for (ApprovalType t : approvalTypes) {
-          byLabel.put(t.getCategory().getLabelName().toLowerCase(), t);
+          byLabel.put(t.getName().toLowerCase(), t);
         }
       }
     }

gerrit-common/src/main/java/com/google/gerrit/common/data/LabelValue.java

@@ -0,0 +1,62 @@
+// 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.common.data;
+
+import com.google.gerrit.reviewdb.client.ApprovalCategoryValue;
+
+public class LabelValue {
+  @Deprecated
+  public static LabelValue fromApprovalCategoryValue(ApprovalCategoryValue acv) {
+    return new LabelValue(acv.getValue(), acv.getName());
+  }
+
+  public static String formatValue(short value) {
+    if (value < 0) {
+      return Short.toString(value);
+    } else if (value == 0) {
+      return " 0";
+    } else {
+      return "+" + Short.toString(value);
+    }
+  }
+
+  protected short value;
+  protected String text;
+
+  public LabelValue(short value, String text) {
+    this.value = value;
+    this.text = text;
+  }
+
+  protected LabelValue() {
+  }
+
+  public short getValue() {
+    return value;
+  }
+
+  public String getText() {
+    return text;
+  }
+
+  public String formatValue() {
+    return formatValue(value);
+  }
+
+  public String format() {
+    return new StringBuilder().append(formatValue())
+        .append(' ').append(text).toString();
+  }
+}

gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AccessSectionEditor.java

@@ -228,8 +228,7 @@ public class AccessSectionEditor extends Composite implements
     } else if (RefConfigSection.isValid(value.getName())) {
       for (ApprovalType t : Gerrit.getConfig().getApprovalTypes()
           .getApprovalTypes()) {
-        String varName = Permission.LABEL + t.getCategory().getLabelName();
+        addPermission(Permission.LABEL + t.getName(), perms);
-        addPermission(varName, perms);
       }
       for (String varName : Util.C.permissionNames().keySet()) {
         addPermission(varName, perms);

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java

@@ -14,7 +14,7 @@
 
 package com.google.gerrit.client.changes;
 
-import static com.google.gerrit.reviewdb.client.ApprovalCategoryValue.formatValue;
+import static com.google.gerrit.common.data.LabelValue.formatValue;
 
 import com.google.gerrit.client.ConfirmationCallback;
 import com.google.gerrit.client.ConfirmationDialog;

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeTable.java

@@ -33,10 +33,9 @@ import com.google.gerrit.common.data.ApprovalSummary;
 import com.google.gerrit.common.data.ApprovalSummarySet;
 import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ChangeInfo;
+import com.google.gerrit.common.data.LabelValue;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGeneralPreferences;
-import com.google.gerrit.reviewdb.client.ApprovalCategory;
-import com.google.gerrit.reviewdb.client.ApprovalCategoryValue;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gwt.dom.client.Element;
@@ -98,14 +97,13 @@ public class ChangeTable extends NavigationTable<ChangeInfo> {
     table.setText(0, C_LAST_UPDATE, Util.C.changeTableColumnLastUpdate());
     for (int i = BASE_COLUMNS; i < columns; i++) {
       final ApprovalType type = approvalTypes.get(i - BASE_COLUMNS);
-      final ApprovalCategory cat = type.getCategory();
+      String text = type.getAbbreviatedName();
-      String text = cat.getAbbreviatedName();
       if (text == null) {
-        text = cat.getName();
+        text = type.getName();
       }
       table.setText(0, i, text);
       if (text != null) {
-        table.getCellFormatter().getElement(0, i).setTitle(cat.getName());
+        table.getCellFormatter().getElement(0, i).setTitle(type.getName());
       }
     }
 
@@ -282,8 +280,7 @@ public class ChangeTable extends NavigationTable<ChangeInfo> {
   private void displayApprovals(final int row, final ApprovalSummary summary,
       final AccountInfoCache aic, final boolean highlightUnreviewed) {
     final CellFormatter fmt = table.getCellFormatter();
-    final Map<ApprovalCategory.Id, PatchSetApproval> approvals =
+    final Map<String, PatchSetApproval> approvals = summary.getApprovalMap();
-        summary.getApprovalMap();
     int col = BASE_COLUMNS;
     boolean haveReview = false;
 
@@ -298,7 +295,7 @@ public class ChangeTable extends NavigationTable<ChangeInfo> {
     }
 
     for (final ApprovalType type : approvalTypes) {
-      final PatchSetApproval ca = approvals.get(type.getCategory().getId());
+      final PatchSetApproval ca = approvals.get(type.getId());
 
       fmt.removeStyleName(row, col, Gerrit.RESOURCES.css().negscore());
       fmt.removeStyleName(row, col, Gerrit.RESOURCES.css().posscore());
@@ -310,7 +307,7 @@ public class ChangeTable extends NavigationTable<ChangeInfo> {
       } else {
         haveReview = true;
 
-        final ApprovalCategoryValue acv = type.getValue(ca);
+        final LabelValue v = type.getValue(ca);
         final AccountInfo ai = aic.get(ca.getAccountId());
 
         if (type.isMaxNegative(ca)) {
@@ -355,7 +352,7 @@ public class ChangeTable extends NavigationTable<ChangeInfo> {
         // so we include a space before the newline to accommodate both.
         //
         fmt.getElement(row, col).setTitle(
-            acv.getName() + " \nby " + FormatUtil.nameEmail(ai));
+            v.getText() + " \nby " + FormatUtil.nameEmail(ai));
       }
 
       col++;

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java

@@ -189,7 +189,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
                 continue;
               }
               if (change.getStatus().isOpen()) {
-                fs.normalize(approvalTypes.byId(category), ca);
+                fs.normalize(approvalTypes.byId(category.get()), ca);
               }
               if (ca.getValue() == 0) {
                 continue;
@@ -235,7 +235,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
                 continue;
               }
               if (change.getStatus().isOpen()) {
-                fs.normalize(approvalTypes.byId(category), ca);
+                fs.normalize(approvalTypes.byId(category.get()), ca);
               }
               if (ca.getValue() == 0) {
                 continue;

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/ApprovalCategoryValue.java

@@ -87,27 +87,4 @@ public final class ApprovalCategoryValue {
   public void setName(final String n) {
     name = n;
   }
-
-  public static String formatValue(short value) {
-    if (value < 0) {
-      return Short.toString(value);
-    } else if (value == 0) {
-      return " 0";
-    } else {
-      return "+" + Short.toString(value);
-    }
-  }
-
-  public String formatValue() {
-    return formatValue(getValue());
-  }
-
-  public static String format(String name, short value) {
-    return new StringBuilder().append(formatValue(value))
-        .append(' ').append(name).toString();
-  }
-
-  public String format() {
-    return format(getName(), getValue());
-  }
 }

gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java

@@ -616,9 +616,9 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener {
             Entry<ApprovalCategory.Id, ApprovalCategoryValue.Id> approval) {
         ApprovalAttribute a = new ApprovalAttribute();
         a.type = approval.getKey().get();
-        ApprovalType at = approvalTypes.byId(approval.getKey());
+        ApprovalType at = approvalTypes.byId(approval.getKey().get());
         if (at != null) {
-          a.description = at.getCategory().getName();
+          a.description = at.getName();
         }
         a.value = Short.toString(approval.getValue().get());
         return a;

gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server;
 
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import com.google.gerrit.common.data.ApprovalType;
@@ -90,9 +91,9 @@ public class ApprovalsUtil {
     for (PatchSetApproval a : patchSetApprovals) {
       // ApprovalCategory.SUBMIT is still in db but not relevant in git-store
       if (!ApprovalCategory.SUBMIT.equals(a.getCategoryId())) {
-        final ApprovalType type = approvalTypes.byId(a.getCategoryId());
+        final ApprovalType type = approvalTypes.byId(a.getCategoryId().get());
         if (a.getPatchSetId().equals(source) &&
-            type.getCategory().isCopyMinScore() &&
+            type.isCopyMinScore() &&
             type.isMaxNegative(a)) {
           db.patchSetApprovals().insert(
               Collections.singleton(new PatchSetApproval(dest, a)));
@@ -128,7 +129,8 @@ public class ApprovalsUtil {
     need.removeAll(existingReviewers);
 
     List<PatchSetApproval> cells = Lists.newArrayListWithCapacity(need.size());
-    ApprovalCategory.Id catId = allTypes.get(allTypes.size() - 1).getCategory().getId();
+    ApprovalCategory.Id catId =
+        Iterables.getLast(allTypes).getApprovalCategoryId();
     for (Account.Id account : need) {
       PatchSetApproval psa = new PatchSetApproval(
           new PatchSetApproval.Key(ps.getId(), account, catId),

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -37,12 +37,12 @@ import com.google.common.collect.Sets;
 import com.google.gerrit.common.changes.ListChangesOption;
 import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
+import com.google.gerrit.common.data.LabelValue;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.common.data.PermissionRange;
 import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.extensions.restapi.Url;
 import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.ApprovalCategoryValue;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Change.Status;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -408,7 +408,7 @@ public class ChangeJson {
     for (PatchSetApproval psa : cd.currentApprovals(db)) {
       short val = psa.getValue();
       if (val != 0 && min < val && val < max
-          && psa.getCategoryId().equals(type.getCategory().getId())) {
+          && psa.getCategoryId().get().equals(type.getId())) {
         if (0 < val) {
           label.recommended = accountLoader.get(psa.getAccountId());
           label.value = val != 1 ? val : null;
@@ -429,25 +429,24 @@ public class ChangeJson {
     FunctionState fs =
         functionState.create(ctl, cd.change(db).currentPatchSetId(), approvals);
     for (ApprovalType at : approvalTypes.getApprovalTypes()) {
-      CategoryFunction.forCategory(at.getCategory()).run(at, fs);
+      CategoryFunction.forType(at).run(at, fs);
     }
 
     Multimap<Account.Id, String> existing =
         HashMultimap.create(approvals.size(), labels.size());
     for (PatchSetApproval psa : approvals) {
-      ApprovalType at = approvalTypes.byId(psa.getCategoryId());
+      ApprovalType at = approvalTypes.byId(psa.getCategoryId().get());
       if (at == null) {
         continue;
       }
-      String name = at.getCategory().getLabelName();
+      LabelInfo p = labels.get(at.getName());
-      LabelInfo p = labels.get(name);
       if (p == null) {
         continue; // TODO: support arbitrary labels.
       }
-      if (!getRange(ctl, psa.getAccountId(), name).isEmpty()) {
+      if (!getRange(ctl, psa.getAccountId(), at.getName()).isEmpty()) {
         p.addApproval(approvalInfo(psa.getAccountId(), psa.getValue()));
       }
-      existing.put(psa.getAccountId(), at.getCategory().getLabelName());
+      existing.put(psa.getAccountId(), at.getName());
     }
 
     // Add dummy approvals for all permitted labels for each user even if they
@@ -478,11 +477,11 @@ public class ChangeJson {
     Map<String, LabelInfo> labels =
         new TreeMap<String, LabelInfo>(LabelOrdering.create(approvalTypes));
     for (PatchSetApproval psa : cd.currentApprovals(db)) {
-      ApprovalType type = approvalTypes.byId(psa.getCategoryId());
+      ApprovalType type = approvalTypes.byId(psa.getCategoryId().get());
       if (type == null) {
         continue;
       }
-      String label = type.getCategory().getLabelName();
+      String label = type.getName();
       LabelInfo li = labels.get(label);
       if (li == null) {
         li = new LabelInfo();
@@ -540,8 +539,8 @@ public class ChangeJson {
 
   private void setLabelValues(ApprovalType type, LabelInfo label) {
     label.values = Maps.newLinkedHashMap();
-    for (ApprovalCategoryValue acv : type.getValues()) {
+    for (LabelValue v : type.getValues()) {
-      label.values.put(acv.formatValue(), acv.getName());
+      label.values.put(v.formatValue(), v.getText());
     }
     if (isOnlyZero(label.values.keySet())) {
       label.values = null;
@@ -562,9 +561,9 @@ public class ChangeJson {
           continue; // TODO: Support arbitrary labels.
         }
         PermissionRange range = ctl.getRange(Permission.forLabel(r.label));
-        for (ApprovalCategoryValue acv : type.getValues()) {
+        for (LabelValue v : type.getValues()) {
-          if (range.contains(acv.getValue())) {
+          if (range.contains(v.getValue())) {
-            permitted.put(r.label, acv.formatValue());
+            permitted.put(r.label, v.formatValue());
           }
         }
       }

gerrit-server/src/main/java/com/google/gerrit/server/change/LabelOrdering.java

@@ -26,7 +26,7 @@ class LabelOrdering {
           @Override
           public Short apply(String n) {
             ApprovalType at = approvalTypes.byLabel(n);
-            return at != null ? at.getCategory().getPosition() : null;
+            return at != null ? at.getPosition() : null;
           }
         }).compound(Ordering.natural());
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java

@@ -209,7 +209,7 @@ public class PostReview implements RestModifyView<RevisionResource, Input> {
         }
       }
 
-      String name = at.getCategory().getLabelName();
+      String name = at.getName();
       PermissionRange range = ctl.getRange(Permission.forLabel(name));
       if (range == null || !range.contains(ent.getValue())) {
         if (strict) {
@@ -346,7 +346,7 @@ public class PostReview implements RestModifyView<RevisionResource, Input> {
     for (Map.Entry<String, Short> ent : labels.entrySet()) {
       // TODO Support arbitrary label names.
       ApprovalType at = approvalTypes.byLabel(ent.getKey());
-      String name = at.getCategory().getLabelName();
+      String name = at.getName();
       if (change.getStatus().isClosed()) {
         // TODO Allow updating some labels even when closed.
         continue;
@@ -368,23 +368,23 @@ public class PostReview implements RestModifyView<RevisionResource, Input> {
         upd.add(c);
         labelDelta.add(format(name, c.getValue()));
         categories.put(
-            at.getCategory().getId(),
+            at.getApprovalCategoryId(),
-            at.getValue(c.getValue()).getId());
+            at.getApprovalCategoryValueId(c.getValue()));
       } else if (c != null && c.getValue() == ent.getValue()) {
         current.put(name, c);
       } else if (c == null) {
         c = new PatchSetApproval(new PatchSetApproval.Key(
                 rsrc.getPatchSet().getId(),
                 rsrc.getAccountId(),
-                at.getCategory().getId()),
+                at.getApprovalCategoryId()),
             ent.getValue());
         c.setGranted(timestamp);
         c.cache(change);
         ins.add(c);
         labelDelta.add(format(name, c.getValue()));
         categories.put(
-            at.getCategory().getId(),
+            at.getApprovalCategoryId(),
-            at.getValue(c.getValue()).getId());
+            at.getApprovalCategoryValueId(c.getValue()));
       }
     }
 
@@ -406,7 +406,7 @@ public class PostReview implements RestModifyView<RevisionResource, Input> {
         PatchSetApproval c = new PatchSetApproval(new PatchSetApproval.Key(
             rsrc.getPatchSet().getId(),
             rsrc.getAccountId(),
-            approvalTypes.getApprovalTypes().get(0).getCategory().getId()),
+            approvalTypes.getApprovalTypes().get(0).getApprovalCategoryId()),
             (short) 0);
         c.setGranted(timestamp);
         c.cache(change);
@@ -433,9 +433,9 @@ public class PostReview implements RestModifyView<RevisionResource, Input> {
         continue;
       }
 
-      ApprovalType at = approvalTypes.byId(a.getCategoryId());
+      ApprovalType at = approvalTypes.byId(a.getCategoryId().get());
       if (at != null) {
-        current.put(at.getCategory().getLabelName(), a);
+        current.put(at.getName(), a);
       } else {
         del.add(a);
       }

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java

@@ -117,8 +117,8 @@ public class PostReviewers implements RestModifyView<ChangeResource, Input> {
     this.accountCache = accountCache;
     this.json = json;
 
-    this.addReviewerCategoryId = Iterables.getLast(
+    this.addReviewerCategoryId = Iterables.getLast(approvalTypes.getApprovalTypes())
-        approvalTypes.getApprovalTypes()).getCategory().getId();
+        .getApprovalCategoryId();
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java

@@ -14,7 +14,7 @@
 
 package com.google.gerrit.server.change;
 
-import static com.google.gerrit.reviewdb.client.ApprovalCategoryValue.formatValue;
+import static com.google.gerrit.common.data.LabelValue.formatValue;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
@@ -85,7 +85,7 @@ public class ReviewerJson {
 
     FunctionState fs = functionState.create(ctl, psId, approvals);
     for (ApprovalType at : approvalTypes.getApprovalTypes()) {
-      CategoryFunction.forCategory(at.getCategory()).run(at, fs);
+      CategoryFunction.forType(at).run(at, fs);
     }
 
     // Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167.
@@ -95,10 +95,9 @@ public class ReviewerJson {
       for (PermissionRange pr : ctl.getLabelRanges()) {
         if (!pr.isEmpty()) {
           // TODO: Support arbitrary labels.
-          ApprovalType at = approvalTypes.byId(ca.getCategoryId());
+          ApprovalType at = approvalTypes.byId(ca.getCategoryId().get());
           if (at != null) {
-            out.approvals.put(at.getCategory().getLabelName(),
+            out.approvals.put(at.getName(), formatValue(ca.getValue())); }
-                formatValue(ca.getValue())); }
         }
       }
     }

gerrit-server/src/main/java/com/google/gerrit/server/config/ApprovalTypesProvider.java

@@ -47,7 +47,7 @@ public class ApprovalTypesProvider implements Provider<ApprovalTypes> {
         for (final ApprovalCategory c : db.approvalCategories().all()) {
           final List<ApprovalCategoryValue> values =
               db.approvalCategoryValues().byCategory(c.getId()).toList();
-          types.add(new ApprovalType(c, values));
+          types.add(ApprovalType.fromApprovalCategory(c, values));
         }
       } finally {
         db.close();

gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java

@@ -460,9 +460,9 @@ public class EventFactory {
     a.by = asAccountAttribute(approval.getAccountId());
     a.grantedOn = approval.getGranted().getTime() / 1000L;
 
-    ApprovalType at = approvalTypes.byId(approval.getCategoryId());
+    ApprovalType at = approvalTypes.byId(approval.getCategoryId().get());
     if (at != null) {
-      a.description = at.getCategory().getName();
+      a.description = at.getName();
     }
     return a;
   }

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java

@@ -937,7 +937,7 @@ public class MergeOp {
               identifiedUserFactory.create(c.getOwner())),
               merged, approvals);
       for (ApprovalType at : approvalTypes.getApprovalTypes()) {
-        CategoryFunction.forCategory(at.getCategory()).run(at, fs);
+        CategoryFunction.forType(at).run(at, fs);
       }
       for (PatchSetApproval a : approvals) {
         if (a.getValue() > 0

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java

@@ -254,12 +254,12 @@ public class MergeUtil {
       } else if (VRIF.equals(a.getCategoryId())) {
         tag = "Tested-by";
       } else {
-        final ApprovalType at = approvalTypes.byId(a.getCategoryId());
+        final ApprovalType at = approvalTypes.byId(a.getCategoryId().get());
         if (at == null) {
-          // A deprecated/deleted approval type, ignore it.
+          // TODO: Support arbitrary labels.
           continue;
         }
-        tag = at.getCategory().getName().replace(' ', '-');
+        tag = at.getName();
       }
 
       if (!contains(footers, new FooterKey(tag), identbuf.toString())) {

gerrit-server/src/main/java/com/google/gerrit/server/git/ReviewNoteHeaderFormatter.java

@@ -14,8 +14,8 @@
 
 package com.google.gerrit.server.git;
 
+import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.ApprovalCategory;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 
@@ -51,9 +51,8 @@ class ReviewNoteHeaderFormatter {
     sb.append("Change-Id: ").append(changeKey.get()).append("\n");
   }
 
-  void appendApproval(ApprovalCategory category,
+  void appendApproval(ApprovalType type, short value, Account user) {
-      short value, Account user) {
+    sb.append(type.getName());
-    sb.append(category.getLabelName());
     sb.append(value < 0 ? "-" : "+").append(Math.abs(value)).append(": ");
     appendUserData(user);
     sb.append("\n");

gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java

@@ -14,22 +14,20 @@
 
 package com.google.gerrit.server.mail;
 
+import com.google.common.collect.HashBasedTable;
+import com.google.common.collect.Table;
 import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
+import com.google.gerrit.common.data.LabelValue;
 import com.google.gerrit.common.errors.EmailException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
-import com.google.gerrit.reviewdb.client.ApprovalCategory;
-import com.google.gerrit.reviewdb.client.ApprovalCategoryValue;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
 
-import java.util.HashMap;
-import java.util.Map;
-
 /** Send notice about a change successfully merged. */
 public class MergedSender extends ReplyToChangeSender {
   public static interface Factory {
@@ -61,18 +59,18 @@ public class MergedSender extends ReplyToChangeSender {
 
   public String getApprovals() {
     try {
-      final Map<Account.Id, Map<ApprovalCategory.Id, PatchSetApproval>> pos =
+      Table<Account.Id, String, PatchSetApproval> pos = HashBasedTable.create();
-          new HashMap<Account.Id, Map<ApprovalCategory.Id, PatchSetApproval>>();
+      Table<Account.Id, String, PatchSetApproval> neg = HashBasedTable.create();
-
-      final Map<Account.Id, Map<ApprovalCategory.Id, PatchSetApproval>> neg =
-          new HashMap<Account.Id, Map<ApprovalCategory.Id, PatchSetApproval>>();
-
       for (PatchSetApproval ca : args.db.get().patchSetApprovals()
           .byPatchSet(patchSet.getId())) {
+        ApprovalType lt = approvalTypes.byId(ca.getCategoryId().get());
+        if (lt == null) {
+          continue;
+        }
         if (ca.getValue() > 0) {
-          insert(pos, ca);
+          pos.put(ca.getAccountId(), lt.getName(), ca);
         } else if (ca.getValue() < 0) {
-          insert(neg, ca);
+          neg.put(ca.getAccountId(), lt.getName(), ca);
         }
       }
 
@@ -83,22 +81,20 @@ public class MergedSender extends ReplyToChangeSender {
     return "";
   }
 
-  private String format(final String type,
+  private String format(String type,
-      final Map<Account.Id, Map<ApprovalCategory.Id, PatchSetApproval>> list) {
+      Table<Account.Id, String, PatchSetApproval> approvals) {
     StringBuilder txt = new StringBuilder();
-    if (list.isEmpty()) {
+    if (approvals.isEmpty()) {
       return "";
     }
-    txt.append(type + ":\n");
+    txt.append(type).append(":\n");
-    for (final Map.Entry<Account.Id, Map<ApprovalCategory.Id, PatchSetApproval>> ent : list
+    for (Account.Id id : approvals.rowKeySet()) {
-        .entrySet()) {
-      final Map<ApprovalCategory.Id, PatchSetApproval> l = ent.getValue();
       txt.append("  ");
-      txt.append(getNameFor(ent.getKey()));
+      txt.append(getNameFor(id));
       txt.append(": ");
       boolean first = true;
-      for (ApprovalType at : approvalTypes.getApprovalTypes()) {
+      for (ApprovalType lt : approvalTypes.getApprovalTypes()) {
-        final PatchSetApproval ca = l.get(at.getCategory().getId());
+        PatchSetApproval ca = approvals.get(id, lt.getName());
         if (ca == null) {
           continue;
         }
@@ -109,32 +105,18 @@ public class MergedSender extends ReplyToChangeSender {
           txt.append("; ");
         }
 
-        final ApprovalCategoryValue v = at.getValue(ca);
+        LabelValue v = lt.getValue(ca);
         if (v != null) {
-          txt.append(v.getName());
+          txt.append(v.getText());
         } else {
-          txt.append(at.getCategory().getName());
+          txt.append(lt.getName());
-          txt.append("=");
+          txt.append('=');
-          if (ca.getValue() > 0) {
+          txt.append(LabelValue.formatValue(ca.getValue()));
-            txt.append("+");
-          }
-          txt.append("" + ca.getValue());
         }
       }
-      txt.append("\n");
+      txt.append('\n');
     }
-    txt.append("\n");
+    txt.append('\n');
     return txt.toString();
   }
-
-  private void insert(
-      final Map<Account.Id, Map<ApprovalCategory.Id, PatchSetApproval>> list,
-      final PatchSetApproval ca) {
-    Map<ApprovalCategory.Id, PatchSetApproval> m = list.get(ca.getAccountId());
-    if (m == null) {
-      m = new HashMap<ApprovalCategory.Id, PatchSetApproval>();
-      list.put(ca.getAccountId(), m);
-    }
-    m.put(ca.getCategoryId(), ca);
-  }
 }

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

@@ -18,7 +18,6 @@ import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.ApprovalCategory;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -58,34 +57,28 @@ class LabelPredicate extends OperatorPredicate<ChangeData> {
     abstract boolean match(int psValue, int expValue);
   }
 
-  private static ApprovalCategory category(ApprovalTypes types, String toFind) {
+  private static ApprovalType type(ApprovalTypes types, String toFind) {
     if (types.byLabel(toFind) != null) {
-      return types.byLabel(toFind).getCategory();
+      return types.byLabel(toFind);
     }
 
-    if (types.byId(new ApprovalCategory.Id(toFind)) != null) {
+    if (types.byId(toFind) != null) {
-      return types.byId(new ApprovalCategory.Id(toFind)).getCategory();
+      return types.byId(toFind);
     }
 
     for (ApprovalType at : types.getApprovalTypes()) {
-      ApprovalCategory category = at.getCategory();
+      if (toFind.equalsIgnoreCase(at.getName())) {
-
+        return at;
-      if (toFind.equalsIgnoreCase(category.getName())) {
-        return category;
-
-      } else if (toFind.equalsIgnoreCase(category.getName().replace(" ", ""))) {
-        return category;
       }
     }
 
     for (ApprovalType at : types.getApprovalTypes()) {
-      ApprovalCategory category = at.getCategory();
+      if (toFind.equalsIgnoreCase(at.getAbbreviatedName())) {
-      if (toFind.equalsIgnoreCase(category.getAbbreviatedName())) {
+        return at;
-        return category;
       }
     }
 
-    return new ApprovalCategory(new ApprovalCategory.Id(toFind), toFind);
+    return ApprovalType.withDefaultValues(toFind.substring(0, 4), toFind);
   }
 
   private static Test op(String op) {
@@ -114,7 +107,7 @@ class LabelPredicate extends OperatorPredicate<ChangeData> {
   private final IdentifiedUser.GenericFactory userFactory;
   private final Provider<ReviewDb> dbProvider;
   private final Test test;
-  private final ApprovalCategory category;
+  private final ApprovalType type;
   private final String permissionName;
   private final int expVal;
 
@@ -129,22 +122,22 @@ class LabelPredicate extends OperatorPredicate<ChangeData> {
     Matcher m1 = Pattern.compile("(=|>=|<=)([+-]?\\d+)$").matcher(value);
     Matcher m2 = Pattern.compile("([+-]\\d+)$").matcher(value);
     if (m1.find()) {
-      category = category(types, value.substring(0, m1.start()));
+      type = type(types, value.substring(0, m1.start()));
       test = op(m1.group(1));
       expVal = value(m1.group(2));
 
     } else if (m2.find()) {
-      category = category(types, value.substring(0, m2.start()));
+      type = type(types, value.substring(0, m2.start()));
       test = Test.EQ;
       expVal = value(m2.group(1));
 
     } else {
-      category = category(types, value);
+      type = type(types, value);
       test = Test.EQ;
       expVal = 1;
     }
 
-    this.permissionName = Permission.forLabel(category.getLabelName());
+    this.permissionName = Permission.forLabel(type.getName());
   }
 
   @Override
@@ -153,7 +146,7 @@ class LabelPredicate extends OperatorPredicate<ChangeData> {
     final Set<Account.Id> approversThatVotedInCategory = new HashSet<Account.Id>();
     for (PatchSetApproval p : object.currentApprovals(dbProvider)) {
       allApprovers.add(p.getAccountId());
-      if (p.getCategoryId().equals(category.getId())) {
+      if (p.getCategoryId().get().equals(type.getId())) {
         approversThatVotedInCategory.add(p.getAccountId());
         if (match(object.change(dbProvider), p.getValue(), p.getAccountId())) {
           return true;

gerrit-server/src/main/java/com/google/gerrit/server/workflow/CategoryFunction.java

@@ -32,6 +32,22 @@ public abstract class CategoryFunction {
     all.put(NoBlock.NAME, new NoBlock());
   }
 
+  public static CategoryFunction forName(String name) {
+    return all.get(name);
+  }
+
+  /**
+   * Locate a function by type.
+   *
+   * @param type the type the function is for.
+   * @return the function implementation; {@link NoOpFunction} if the function
+   *         is not known to Gerrit and thus cannot be executed.
+   */
+  public static CategoryFunction forType(ApprovalType type) {
+    CategoryFunction r = all.get(type.getFunctionName());
+    return r != null ? r : new NoOpFunction();
+  }
+
   /**
    * Locate a function by category.
    *

gerrit-server/src/main/java/com/google/gerrit/server/workflow/FunctionState.java

@@ -16,14 +16,12 @@ package com.google.gerrit.server.workflow;
 
 import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
+import com.google.gerrit.common.data.LabelValue;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.common.data.PermissionRange;
-import com.google.gerrit.reviewdb.client.ApprovalCategory;
-import com.google.gerrit.reviewdb.client.ApprovalCategoryValue;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.reviewdb.client.ApprovalCategory.Id;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.project.ChangeControl;
@@ -47,10 +45,9 @@ public class FunctionState {
   private final ApprovalTypes approvalTypes;
   private final IdentifiedUser.GenericFactory userFactory;
 
-  private final Map<ApprovalCategory.Id, Collection<PatchSetApproval>> approvals =
+  private final Map<String, Collection<PatchSetApproval>> approvals =
-      new HashMap<ApprovalCategory.Id, Collection<PatchSetApproval>>();
+      new HashMap<String, Collection<PatchSetApproval>>();
-  private final Map<ApprovalCategory.Id, Boolean> valid =
+  private final Map<String, Boolean> valid = new HashMap<String, Boolean>();
-      new HashMap<ApprovalCategory.Id, Boolean>();
   private final ChangeControl callerChangeControl;
   private final Change change;
 
@@ -67,10 +64,15 @@ public class FunctionState {
 
     for (final PatchSetApproval ca : all) {
       if (psId.equals(ca.getPatchSetId())) {
-        Collection<PatchSetApproval> l = approvals.get(ca.getCategoryId());
+        Collection<PatchSetApproval> l =
+            approvals.get(ca.getCategoryId().get());
         if (l == null) {
           l = new ArrayList<PatchSetApproval>();
-          approvals.put(ca.getCategoryId(), l);
+          ApprovalType at = approvalTypes.byId(ca.getCategoryId().get());
+          if (at != null) {
+            // TODO: Support arbitrary labels
+            approvals.put(at.getName(), l);
+          }
         }
         l.add(ca);
       }
@@ -90,20 +92,20 @@ public class FunctionState {
   }
 
   public boolean isValid(final ApprovalType at) {
-    return isValid(id(at));
+    return isValid(at.getName());
   }
 
-  public boolean isValid(final ApprovalCategory.Id id) {
+  public boolean isValid(final String labelName) {
-    final Boolean b = valid.get(id);
+    final Boolean b = valid.get(labelName);
     return b != null && b;
   }
 
   public Collection<PatchSetApproval> getApprovals(final ApprovalType at) {
-    return getApprovals(id(at));
+    return getApprovals(at.getName());
   }
 
-  public Collection<PatchSetApproval> getApprovals(final ApprovalCategory.Id id) {
+  public Collection<PatchSetApproval> getApprovals(final String labelName) {
-    final Collection<PatchSetApproval> l = approvals.get(id);
+    final Collection<PatchSetApproval> l = approvals.get(labelName);
     return l != null ? l : Collections.<PatchSetApproval> emptySet();
   }
 
@@ -113,13 +115,13 @@ public class FunctionState {
    * <p>
    */
   private void applyTypeFloor(final ApprovalType at, final PatchSetApproval a) {
-    final ApprovalCategoryValue atMin = at.getMin();
+    final LabelValue atMin = at.getMin();
 
     if (atMin != null && a.getValue() < atMin.getValue()) {
       a.setValue(atMin.getValue());
     }
 
-    final ApprovalCategoryValue atMax = at.getMax();
+    final LabelValue atMax = at.getMax();
     if (atMax != null && a.getValue() > atMax.getValue()) {
       a.setValue(atMax.getValue());
     }
@@ -135,8 +137,7 @@ public class FunctionState {
    * <p>
    */
   private void applyRightFloor(final ApprovalType at, final PatchSetApproval a) {
-    final ApprovalCategory category = at.getCategory();
+    final String permission = Permission.forLabel(at.getName());
-    final String permission = Permission.forLabel(category.getLabelName());
     final IdentifiedUser user = userFactory.create(a.getAccountId());
     final PermissionRange range = controlFor(user).getRange(permission);
     a.setValue((short) range.squash(a.getValue()));
@@ -152,7 +153,7 @@ public class FunctionState {
     applyRightFloor(at, ca);
   }
 
-  private static Id id(final ApprovalType at) {
+  private static String id(final ApprovalType at) {
-    return at.getCategory().getId();
+    return at.getId();
   }
 }

gerrit-server/src/main/java/gerrit/PRED__load_commit_labels_1.java

@@ -60,14 +60,14 @@ class PRED__load_commit_labels_1 extends Predicate.P1 {
           continue;
         }
 
-        ApprovalType t = types.byId(a.getCategoryId());
+        ApprovalType t = types.byId(a.getCategoryId().get());
         if (t == null) {
           continue;
         }
 
         StructureTerm labelTerm = new StructureTerm(
             sym_label,
-            SymbolTerm.intern(t.getCategory().getLabelName()),
+            SymbolTerm.intern(t.getName()),
             new IntegerTerm(a.getValue()));
 
         StructureTerm userTerm = new StructureTerm(

gerrit-server/src/main/java/gerrit/PRED_get_legacy_approval_types_1.java

@@ -73,9 +73,9 @@ class PRED_get_legacy_approval_types_1 extends Predicate.P1 {
 
   static Term export(ApprovalType type) {
     return new StructureTerm(symApprovalType,
-        SymbolTerm.intern(type.getCategory().getLabelName()),
+        SymbolTerm.intern(type.getName()),
-        SymbolTerm.intern(type.getCategory().getId().get()),
+        SymbolTerm.intern(type.getId()),
-        SymbolTerm.intern(type.getCategory().getFunctionName()),
+        SymbolTerm.intern(type.getFunctionName()),
         new IntegerTerm(type.getMin().getValue()),
         new IntegerTerm(type.getMax().getValue()));
   }

gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java

@@ -16,13 +16,10 @@ package com.google.gerrit.rules;
 
 import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
-import com.google.gerrit.reviewdb.client.ApprovalCategory;
+import com.google.gerrit.common.data.LabelValue;
-import com.google.gerrit.reviewdb.client.ApprovalCategoryValue;
 import com.google.inject.AbstractModule;
 
-import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.List;
 
 public class GerritCommonTest extends PrologTestCase {
   @Override
@@ -30,8 +27,16 @@ public class GerritCommonTest extends PrologTestCase {
     super.setUp();
 
     final ApprovalTypes types = new ApprovalTypes(Arrays.asList(
-        codeReviewCategory(),
+        category(0, "CRVW", "Code-Review",
-        verifiedCategory()
+            value(2, "Looks good to me, approved"),
+            value(1, "Looks good to me, but someone else must approve"),
+            value(0, "No score"),
+            value(-1, "I would prefer that you didn't submit this"),
+            value(-2, "Do not submit")),
+        category(1, "VRIF", "Verified",
+            value(1, "Verified"),
+            value(0, "No score"),
+            value(-1, "Fails"))
     ));
 
     load("gerrit", "gerrit_common_test.pl", new AbstractModule() {
@@ -42,40 +47,14 @@ public class GerritCommonTest extends PrologTestCase {
     });
   }
 
-  private static ApprovalType codeReviewCategory() {
+  private static LabelValue value(int value, String text) {
-    ApprovalCategory cat = category(0, "CRVW", "Code Review");
+    return new LabelValue((short) value, text);
-    List<ApprovalCategoryValue> vals = newList();
-    vals.add(value(cat, 2, "Looks good to me, approved"));
-    vals.add(value(cat, 1, "Looks good to me, but someone else must approve"));
-    vals.add(value(cat, 0, "No score"));
-    vals.add(value(cat, -1, "I would prefer that you didn't submit this"));
-    vals.add(value(cat, -2, "Do not submit"));
-    return new ApprovalType(cat, vals);
   }
 
-  private static ApprovalType verifiedCategory() {
+  private static ApprovalType category(int pos, String id, String name,
-    ApprovalCategory cat = category(1, "VRIF", "Verified");
+      LabelValue... values) {
-    List<ApprovalCategoryValue> vals = newList();
+    ApprovalType type = new ApprovalType(id, name, Arrays.asList(values));
-    vals.add(value(cat, 1, "Verified"));
+    type.setPosition((short) pos);
-    vals.add(value(cat, 0, "No score"));
+    return type;
-    vals.add(value(cat, -1, "Fails"));
-    return new ApprovalType(cat, vals);
-  }
-
-  private static ApprovalCategory category(int pos, String id, String name) {
-    ApprovalCategory cat;
-    cat = new ApprovalCategory(new ApprovalCategory.Id(id), name);
-    cat.setPosition((short) pos);
-    return cat;
-  }
-
-  private static ArrayList<ApprovalCategoryValue> newList() {
-    return new ArrayList<ApprovalCategoryValue>();
-  }
-
-  private static ApprovalCategoryValue value(ApprovalCategory c, int v, String n) {
-    return new ApprovalCategoryValue(
-        new ApprovalCategoryValue.Id(c.getId(), (short) v),
-        n);
   }
 }

gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ApproveOption.java

@@ -15,7 +15,7 @@
 package com.google.gerrit.sshd.commands;
 
 import com.google.gerrit.common.data.ApprovalType;
-import com.google.gerrit.reviewdb.client.ApprovalCategoryValue;
+import com.google.gerrit.common.data.LabelValue;
 
 import org.kohsuke.args4j.CmdLineException;
 import org.kohsuke.args4j.CmdLineParser;
@@ -100,7 +100,7 @@ final class ApproveOption implements Option, Setter<Short> {
   }
 
   String getLabelName() {
-    return type.getCategory().getLabelName();
+    return type.getName();
   }
 
   public static class Handler extends OneArgumentOptionHandler<Short> {
@@ -121,8 +121,8 @@ final class ApproveOption implements Option, Setter<Short> {
       }
 
       final short value = Short.parseShort(argument);
-      final ApprovalCategoryValue min = cmdOption.type.getMin();
+      final LabelValue min = cmdOption.type.getMin();
-      final ApprovalCategoryValue max = cmdOption.type.getMax();
+      final LabelValue max = cmdOption.type.getMax();
 
       if (value < min.getValue() || value > max.getValue()) {
         final String name = cmdOption.name();

gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java

@@ -18,13 +18,12 @@ import com.google.common.base.Strings;
 import com.google.common.collect.Maps;
 import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
+import com.google.gerrit.common.data.LabelValue;
 import com.google.gerrit.common.data.ReviewResult;
 import com.google.gerrit.common.data.ReviewResult.Error.Type;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.reviewdb.client.ApprovalCategory;
-import com.google.gerrit.reviewdb.client.ApprovalCategoryValue;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.RevId;
@@ -32,11 +31,11 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.change.Abandon;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.PostReview;
-import com.google.gerrit.server.change.RevisionResource;
 import com.google.gerrit.server.change.Restore;
+import com.google.gerrit.server.change.RevisionResource;
+import com.google.gerrit.server.change.Submit;
 import com.google.gerrit.server.changedetail.DeleteDraftPatchSet;
 import com.google.gerrit.server.changedetail.PublishDraft;
-import com.google.gerrit.server.change.Submit;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
@@ -403,15 +402,13 @@ public class ReviewCommand extends SshCommand {
 
     for (ApprovalType type : approvalTypes.getApprovalTypes()) {
       String usage = "";
-      final ApprovalCategory category = type.getCategory();
+      usage = "score for " + type.getName() + "\n";
-      usage = "score for " + category.getName() + "\n";
 
-      for (ApprovalCategoryValue v : type.getValues()) {
+      for (LabelValue v : type.getValues()) {
         usage += v.format() + "\n";
       }
 
-      final String name =
+      final String name = "--" + type.getName().toLowerCase();
-          "--" + category.getName().toLowerCase().replace(' ', '-');
       optionList.add(new ApproveOption(name, usage, type));
     }