Allow plugins to define custom data in SubmitRequirement

Replace the raw short/long reasons of SubmitRequirement with a dynamic
type and map, allowing for more powerful implementations later.
Also remove the label field, superseeded by the data Map.

Use AutoValues for the SubmitRequirement class, as it is marked
GwtIncompatible.

The idea behind this change is to provide a future proof API that
both plugins and end users will enjoy. A user interface is being
worked on, and will benefit of these changes.

Change-Id: I030609cd164d308f2231a2abba2eb16b09524b7f

Documentation/json.txt

@@ -185,14 +185,16 @@ in order for the change to be submittable.
 
 [[requirement]]
 == requirement
-Information about a requirement (not met) in order to submit a change.
+Information about a requirement in order to submit a change.
 
-shortReason:: A short description of the requirement (a hint).
+fallbackText:: A human readable description of the requirement.
 
-fullReason:: A longer and descriptive message explaining what needs to
+type:: Alphanumerical (plus hyphens or underscores) string to identify what the requirement is and
-be changed to meet the requirement.
+why it was triggered. Can be seen as a class: requirements sharing the same type were created for a
+similar reason, and the data structure will follow one set of rules.
 
-label:: (Optional) The name of the linked label, if set by a pre-submit rule.
+data:: (Optional) Additional key-value data linked to this requirement. This is used in templates to
+render rich status messages.
 
 [[label]]
 == label

java/com/google/gerrit/common/BUILD

@@ -22,6 +22,7 @@ gwt_module(
         "//lib:guava",
         "//lib:gwtorm_client",
         "//lib:servlet-api-3_1",
+        "//lib/auto:auto-value",
         "//lib/jgit/org.eclipse.jgit:jgit",
         "//lib/log:api",
     ],
@@ -46,6 +47,7 @@ java_library(
         "//lib:gwtjsonrpc",
         "//lib:gwtorm",
         "//lib:servlet-api-3_1",
+        "//lib/auto:auto-value",
         "//lib/jgit/org.eclipse.jgit:jgit",
         "//lib/log:api",
     ],

java/com/google/gerrit/common/data/SubmitRecord.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.common.data;
 
+import com.google.common.annotations.GwtIncompatible;
 import com.google.gerrit.reviewdb.client.Account;
 import java.util.Collection;
 import java.util.List;
@@ -60,7 +61,7 @@ public class SubmitRecord {
 
   public Status status;
   public List<Label> labels;
-  public List<SubmitRequirement> requirements;
+  @GwtIncompatible public List<SubmitRequirement> requirements;
   public String errorMessage;
 
   public static class Label {
@@ -131,6 +132,7 @@ public class SubmitRecord {
     }
   }
 
+  @GwtIncompatible
   @Override
   public String toString() {
     StringBuilder sb = new StringBuilder();
@@ -158,6 +160,7 @@ public class SubmitRecord {
     return sb.toString();
   }
 
+  @GwtIncompatible
   @Override
   public boolean equals(Object o) {
     if (o instanceof SubmitRecord) {
@@ -170,6 +173,7 @@ public class SubmitRecord {
     return false;
   }
 
+  @GwtIncompatible
   @Override
   public int hashCode() {
     return Objects.hash(status, labels, errorMessage, requirements);

java/com/google/gerrit/common/data/SubmitRequirement.java

@@ -14,67 +14,65 @@
 
 package com.google.gerrit.common.data;
 
-import static java.util.Objects.requireNonNull;
+import com.google.auto.value.AutoValue;
-
+import com.google.common.annotations.GwtIncompatible;
-import com.google.gerrit.common.Nullable;
+import com.google.common.base.CharMatcher;
-import java.util.Objects;
+import com.google.common.base.Preconditions;
-import java.util.Optional;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
 
 /** Describes a requirement to submit a change. */
-public final class SubmitRequirement {
+@GwtIncompatible
-  private final String shortReason;
+@AutoValue
-  private final String fullReason;
+@AutoValue.CopyAnnotations
-  @Nullable private final String label;
+public abstract class SubmitRequirement {
+  private static final CharMatcher TYPE_MATCHER =
+      CharMatcher.inRange('a', 'z')
+          .or(CharMatcher.inRange('A', 'Z'))
+          .or(CharMatcher.inRange('0', '9'))
+          .or(CharMatcher.anyOf("-_"));
 
-  public SubmitRequirement(String shortReason, String fullReason, @Nullable String label) {
+  @AutoValue.Builder
-    this.shortReason = requireNonNull(shortReason);
+  public abstract static class Builder {
-    this.fullReason = requireNonNull(fullReason);
+    public abstract Builder setType(String value);
-    this.label = label;
+
+    public abstract Builder setFallbackText(String value);
+
+    public Builder setData(Map<String, String> value) {
+      return setData(ImmutableMap.copyOf(value));
     }
 
-  public String shortReason() {
+    public Builder addCustomValue(String key, String value) {
-    return shortReason;
+      dataBuilder().put(key, value);
+      return this;
     }
 
-  public String fullReason() {
+    public SubmitRequirement build() {
-    return fullReason;
+      SubmitRequirement requirement = autoBuild();
+      Preconditions.checkState(
+          validateType(requirement.type()),
+          "SubmitRequirement's type contains non alphanumerical symbols.");
+      return requirement;
     }
 
-  public Optional<String> label() {
+    abstract Builder setData(ImmutableMap<String, String> value);
-    return Optional.ofNullable(label);
+
+    abstract ImmutableMap.Builder<String, String> dataBuilder();
+
+    abstract SubmitRequirement autoBuild();
   }
 
-  @Override
+  public abstract String fallbackText();
-  public boolean equals(Object o) {
+
-    if (this == o) {
+  public abstract String type();
-      return true;
+
-    }
+  public abstract ImmutableMap<String, String> data();
-    if (o instanceof SubmitRequirement) {
+
-      SubmitRequirement that = (SubmitRequirement) o;
+  public static Builder builder() {
-      return Objects.equals(shortReason, that.shortReason)
+    return new AutoValue_SubmitRequirement.Builder();
-          && Objects.equals(fullReason, that.fullReason)
-          && Objects.equals(label, that.label);
-    }
-    return false;
   }
 
-  @Override
+  private static boolean validateType(String type) {
-  public int hashCode() {
+    return TYPE_MATCHER.matchesAllOf(type);
-    return Objects.hash(shortReason, fullReason, label);
-  }
-
-  @Override
-  public String toString() {
-    return "SubmitRequirement{"
-        + "shortReason='"
-        + shortReason
-        + '\''
-        + ", fullReason='"
-        + fullReason
-        + '\''
-        + ", label='"
-        + label
-        + '\''
-        + '}';
   }
 }

java/com/google/gerrit/server/data/SubmitRequirementAttribute.java

@@ -14,12 +14,14 @@
 
 package com.google.gerrit.server.data;
 
+import java.util.Map;
+
 /**
  * Represents a {@link com.google.gerrit.common.data.SubmitRequirement} that does not depend on
  * Gerrit internal classes, to be serialized
  */
 public class SubmitRequirementAttribute {
-  public String shortReason;
+  public Map<String, String> data;
-  public String fullReason;
+  public String type;
-  public String label;
+  public String fallbackText;
 }

java/com/google/gerrit/server/events/EventFactory.java

@@ -261,9 +261,9 @@ public class EventFactory {
       sa.requirements = new ArrayList<>();
       for (SubmitRequirement req : submitRecord.requirements) {
         SubmitRequirementAttribute re = new SubmitRequirementAttribute();
-        re.shortReason = req.shortReason();
+        re.fallbackText = req.fallbackText();
-        re.fullReason = req.fullReason();
+        re.type = req.type();
-        re.label = req.label().orElse(null);
+        re.data = req.data();
         sa.requirements.add(re);
       }
     }

java/com/google/gerrit/server/index/change/ChangeField.java

@@ -36,7 +36,6 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Table;
 import com.google.common.primitives.Longs;
-import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.common.data.SubmitRequirement;
 import com.google.gerrit.index.FieldDef;
@@ -77,6 +76,7 @@ import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
@@ -657,9 +657,9 @@ public class ChangeField {
     }
 
     static class StoredRequirement {
-      String shortReason;
+      String fallbackText;
-      String fullReason;
+      String type;
-      @Nullable String label;
+      Map<String, String> data;
     }
 
     SubmitRecord.Status status;
@@ -684,9 +684,9 @@ public class ChangeField {
         this.requirements = new ArrayList<>(rec.requirements.size());
         for (SubmitRequirement requirement : rec.requirements) {
           StoredRequirement sr = new StoredRequirement();
-          sr.shortReason = requirement.shortReason();
+          sr.type = requirement.type();
-          sr.fullReason = requirement.fullReason();
+          sr.fallbackText = requirement.fallbackText();
-          sr.label = requirement.label().orElse(null);
+          sr.data = requirement.data();
           this.requirements.add(sr);
         }
       }
@@ -708,10 +708,13 @@ public class ChangeField {
       }
       if (requirements != null) {
         rec.requirements = new ArrayList<>(requirements.size());
-        for (StoredRequirement requirement : requirements) {
+        for (StoredRequirement req : requirements) {
           SubmitRequirement sr =
-              new SubmitRequirement(
+              SubmitRequirement.builder()
-                  requirement.shortReason, requirement.fullReason, requirement.label);
+                  .setType(req.type)
+                  .setFallbackText(req.fallbackText)
+                  .setData(req.data)
+                  .build();
           rec.requirements.add(sr);
         }
       }

javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java

@@ -93,7 +93,7 @@ public class ChangeFieldTest extends GerritBaseTests {
   }
 
   @Test
-  public void storedSubmitRecordsWithRequirements() {
+  public void storedSubmitRecordsWithRequirement() {
     SubmitRecord r =
         record(
             SubmitRecord.Status.OK,
@@ -101,10 +101,27 @@ public class ChangeFieldTest extends GerritBaseTests {
             label(SubmitRecord.Label.Status.OK, "Label-2", 1));
 
     SubmitRequirement sr =
-        new SubmitRequirement(
+        SubmitRequirement.builder()
-            "short reason",
+            .setType("short_type")
-            "Full reason can be a long string with special symbols like < > \\ / ; :",
+            .setFallbackText("Fallback text may contain special symbols like < > \\ / ; :")
-            null);
+            .addCustomValue("custom_data", "my value")
+            .build();
+    r.requirements = Collections.singletonList(sr);
+
+    assertStoredRecordRoundTrip(r);
+  }
+
+  @Test
+  public void storedSubmitRequirementWithoutCustomData() {
+    SubmitRecord r =
+        record(
+            SubmitRecord.Status.OK,
+            label(SubmitRecord.Label.Status.MAY, "Label-1", null),
+            label(SubmitRecord.Label.Status.OK, "Label-2", 1));
+
+    // Doesn't have any custom data value
+    SubmitRequirement sr =
+        SubmitRequirement.builder().setFallbackText("short_type").setType("ci_status").build();
     r.requirements = Collections.singletonList(sr);
 
     assertStoredRecordRoundTrip(r);