Don't validate LabelTypes in ChangeUpdate

We don't perform other types of validation on the approvals input,
e.g. for permissions, which are just handled by LabelNormalizer. Since
it's assumed callers will be passing their inputs through
LabelNormalizer if necessary anyway, they are likely already taking
care of validation.

Still pass in the Comparator for aesthetics when ordering (analogous
to how we use account names for aesthetics when referring to
identities).

Change-Id: I1e8005db9cc867a60cfb384bbec55e363f250dac

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java

@@ -21,8 +21,6 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.Maps;
-import com.google.gerrit.common.data.LabelType;
-import com.google.gerrit.common.data.LabelTypes;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -48,6 +46,7 @@ import org.eclipse.jgit.revwalk.FooterKey;
 import org.eclipse.jgit.revwalk.RevCommit;
 
 import java.io.IOException;
+import java.util.Comparator;
 import java.util.Date;
 import java.util.Map;
 
@@ -66,14 +65,14 @@ public class ChangeUpdate extends VersionedMetaData {
     ChangeUpdate create(ChangeControl ctl);
     ChangeUpdate create(ChangeControl ctl, Date when);
     @VisibleForTesting
-    ChangeUpdate create(ChangeControl ctl, Date when, LabelTypes labelTypes);
+    ChangeUpdate create(ChangeControl ctl, Date when,
+        Comparator<String> labelNameComparator);
   }
 
   private final NotesMigration migration;
   private final GitRepositoryManager repoManager;
   private final AccountCache accountCache;
   private final MetaDataUpdate.User updateFactory;
-  private final LabelTypes labelTypes;
   private final ChangeControl ctl;
   private final PersonIdent serverIdent;
   private final Date when;
@@ -106,7 +105,8 @@ public class ChangeUpdate extends VersionedMetaData {
       @Assisted ChangeControl ctl,
       @Assisted Date when) {
     this(serverIdent, repoManager, migration, accountCache, updateFactory,
-        ctl, when, projectCache.get(getProjectName(ctl)).getLabelTypes());
+        ctl, when,
+        projectCache.get(getProjectName(ctl)).getLabelTypes().nameComparator());
   }
 
   private static Project.NameKey getProjectName(ChangeControl ctl) {
@@ -122,16 +122,15 @@ public class ChangeUpdate extends VersionedMetaData {
       MetaDataUpdate.User updateFactory,
       @Assisted ChangeControl ctl,
       @Assisted Date when,
-      @Assisted LabelTypes labelTypes) {
+      @Assisted Comparator<String> labelNameComparator) {
     this.repoManager = repoManager;
     this.migration = migration;
     this.accountCache = accountCache;
     this.updateFactory = updateFactory;
-    this.labelTypes = labelTypes;
     this.ctl = ctl;
     this.when = when;
     this.serverIdent = serverIdent;
-    this.approvals = Maps.newTreeMap(labelTypes.nameComparator());
+    this.approvals = Maps.newTreeMap(labelNameComparator);
     this.reviewers = Maps.newLinkedHashMap();
   }
 
@@ -281,11 +280,8 @@ public class ChangeUpdate extends VersionedMetaData {
           .append(" <").append(ident.getEmailAddress()).append(">\n");
     }
     for (Map.Entry<String, Short> e : approvals.entrySet()) {
-      LabelType lt = labelTypes.byLabel(e.getKey());
+      addFooter(msg, FOOTER_LABEL,
-      if (lt != null) {
+          new LabelVote(e.getKey(), e.getValue()).formatWithEquals());
-        addFooter(msg, FOOTER_LABEL,
-            new LabelVote(lt.getName(), e.getValue()).formatWithEquals());
-      }
     }
     commit.setMessage(msg.toString());
     return true;

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -16,19 +16,16 @@ package com.google.gerrit.server.notedb;
 
 import static com.google.gerrit.server.notedb.ReviewerState.CC;
 import static com.google.gerrit.server.notedb.ReviewerState.REVIEWER;
-import static com.google.gerrit.server.project.Util.category;
-import static com.google.gerrit.server.project.Util.value;
 import static com.google.inject.Scopes.SINGLETON;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.easymock.EasyMock.expect;
 import static org.junit.Assert.assertEquals;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
-import com.google.gerrit.common.data.LabelTypes;
+import com.google.common.collect.Ordering;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
@@ -87,16 +84,6 @@ public class ChangeNotesTest {
   private static final TimeZone TZ =
       TimeZone.getTimeZone("America/Los_Angeles");
 
-  private static final LabelTypes LABEL_TYPES = new LabelTypes(ImmutableList.of(
-      category("Verified",
-        value(1, "Verified"),
-        value(0, "No score"),
-        value(-1, "Fails")),
-      category("Code-Review",
-        value(1, "Looks Good To Me"),
-        value(0, "No score"),
-        value(-1, "Do Not Submit"))));
-
   private PersonIdent serverIdent;
   private Project.NameKey project;
   private InMemoryRepositoryManager repoManager;
@@ -182,8 +169,8 @@ public class ChangeNotesTest {
   public void approvalsCommitFormat() throws Exception {
     Change c = newChange();
     ChangeUpdate update = newUpdate(c, changeOwner);
-    update.putApproval("Code-Review", (short) -1);
     update.putApproval("Verified", (short) 1);
+    update.putApproval("Code-Review", (short) -1);
     update.putReviewer(changeOwner.getAccount().getId(), REVIEWER);
     update.putReviewer(otherUser.getAccount().getId(), CC);
     update.commit();
@@ -198,8 +185,8 @@ public class ChangeNotesTest {
           + "Patch-set: 1\n"
           + "Reviewer: Change Owner <1@gerrit>\n"
           + "CC: Other Account <2@gerrit>\n"
-          + "Label: Verified=+1\n"
+          + "Label: Code-Review=-1\n"
-          + "Label: Code-Review=-1\n",
+          + "Label: Verified=+1\n",
           commit.getFullMessage());
 
       PersonIdent author = commit.getAuthorIdent();
@@ -223,8 +210,8 @@ public class ChangeNotesTest {
   public void approvalsOnePatchSet() throws Exception {
     Change c = newChange();
     ChangeUpdate update = newUpdate(c, changeOwner);
-    update.putApproval("Code-Review", (short) -1);
     update.putApproval("Verified", (short) 1);
+    update.putApproval("Code-Review", (short) -1);
     update.commit();
 
     ChangeNotes notes = newNotes(c);
@@ -235,14 +222,14 @@ public class ChangeNotesTest {
 
     assertEquals(c.currentPatchSetId(), psas.get(0).getPatchSetId());
     assertEquals(1, psas.get(0).getAccountId().get());
-    assertEquals("Verified", psas.get(0).getLabel());
+    assertEquals("Code-Review", psas.get(0).getLabel());
-    assertEquals((short) 1, psas.get(0).getValue());
+    assertEquals((short) -1, psas.get(0).getValue());
     assertEquals(truncate(after(c, 1000)), psas.get(0).getGranted());
 
     assertEquals(c.currentPatchSetId(), psas.get(1).getPatchSetId());
     assertEquals(1, psas.get(1).getAccountId().get());
-    assertEquals("Code-Review", psas.get(1).getLabel());
+    assertEquals("Verified", psas.get(1).getLabel());
-    assertEquals((short) -1, psas.get(1).getValue());
+    assertEquals((short) 1, psas.get(1).getValue());
     assertEquals(psas.get(0).getGranted(), psas.get(1).getGranted());
   }
 
@@ -470,7 +457,8 @@ public class ChangeNotesTest {
         bind(IdentifiedUser.class).toInstance(user);
       }
     }).getInstance(ChangeUpdate.Factory.class).create(
-        stubChangeControl(c, user), TimeUtil.nowTs(), LABEL_TYPES);
+        stubChangeControl(c, user), TimeUtil.nowTs(),
+        Ordering.<String> natural());
   }
 
   private ChangeNotes newNotes(Change c) throws OrmException {