From 36c9725f3fd1f6c6c8e47ef6cebdcdfca64c6e71 Mon Sep 17 00:00:00 2001
From: Maxime Guerreiro <maximeg@google.com>
Date: Tue, 3 Apr 2018 17:26:28 +0000
Subject: [PATCH] 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                        |  12 ++-
 java/com/google/gerrit/common/BUILD           |   2 +
 .../gerrit/common/data/SubmitRecord.java      |   6 +-
 .../gerrit/common/data/SubmitRequirement.java | 100 +++++++++---------
 .../data/SubmitRequirementAttribute.java      |   8 +-
 .../gerrit/server/events/EventFactory.java    |   6 +-
 .../server/index/change/ChangeField.java      |  23 ++--
 .../server/index/change/ChangeFieldTest.java  |  27 ++++-
 8 files changed, 106 insertions(+), 78 deletions(-)

diff --git a/Documentation/json.txt b/Documentation/json.txt
index 7360bd4573..3b8a8cba28 100644
--- a/Documentation/json.txt
+++ b/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
-be changed to meet the requirement.
+type:: Alphanumerical (plus hyphens or underscores) string to identify what the requirement is and
+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
diff --git a/java/com/google/gerrit/common/BUILD b/java/com/google/gerrit/common/BUILD
index 919f53211b..a09ef49cc9 100644
--- a/java/com/google/gerrit/common/BUILD
+++ b/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",
     ],
diff --git a/java/com/google/gerrit/common/data/SubmitRecord.java b/java/com/google/gerrit/common/data/SubmitRecord.java
index 3cd9c43557..ae8b2bbdec 100644
--- a/java/com/google/gerrit/common/data/SubmitRecord.java
+++ b/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);
diff --git a/java/com/google/gerrit/common/data/SubmitRequirement.java b/java/com/google/gerrit/common/data/SubmitRequirement.java
index a75f3c0163..0a8d5ac0d9 100644
--- a/java/com/google/gerrit/common/data/SubmitRequirement.java
+++ b/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.gerrit.common.Nullable;
-import java.util.Objects;
-import java.util.Optional;
+import com.google.auto.value.AutoValue;
+import com.google.common.annotations.GwtIncompatible;
+import com.google.common.base.CharMatcher;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
 
 /** Describes a requirement to submit a change. */
-public final class SubmitRequirement {
-  private final String shortReason;
-  private final String fullReason;
-  @Nullable private final String label;
+@GwtIncompatible
+@AutoValue
+@AutoValue.CopyAnnotations
+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) {
-    this.shortReason = requireNonNull(shortReason);
-    this.fullReason = requireNonNull(fullReason);
-    this.label = label;
-  }
+  @AutoValue.Builder
+  public abstract static class Builder {
+    public abstract Builder setType(String value);
 
-  public String shortReason() {
-    return shortReason;
-  }
+    public abstract Builder setFallbackText(String value);
 
-  public String fullReason() {
-    return fullReason;
-  }
-
-  public Optional<String> label() {
-    return Optional.ofNullable(label);
-  }
-
-  @Override
-  public boolean equals(Object o) {
-    if (this == o) {
-      return true;
+    public Builder setData(Map<String, String> value) {
+      return setData(ImmutableMap.copyOf(value));
     }
-    if (o instanceof SubmitRequirement) {
-      SubmitRequirement that = (SubmitRequirement) o;
-      return Objects.equals(shortReason, that.shortReason)
-          && Objects.equals(fullReason, that.fullReason)
-          && Objects.equals(label, that.label);
+
+    public Builder addCustomValue(String key, String value) {
+      dataBuilder().put(key, value);
+      return this;
     }
-    return false;
+
+    public SubmitRequirement build() {
+      SubmitRequirement requirement = autoBuild();
+      Preconditions.checkState(
+          validateType(requirement.type()),
+          "SubmitRequirement's type contains non alphanumerical symbols.");
+      return requirement;
+    }
+
+    abstract Builder setData(ImmutableMap<String, String> value);
+
+    abstract ImmutableMap.Builder<String, String> dataBuilder();
+
+    abstract SubmitRequirement autoBuild();
   }
 
-  @Override
-  public int hashCode() {
-    return Objects.hash(shortReason, fullReason, label);
+  public abstract String fallbackText();
+
+  public abstract String type();
+
+  public abstract ImmutableMap<String, String> data();
+
+  public static Builder builder() {
+    return new AutoValue_SubmitRequirement.Builder();
   }
 
-  @Override
-  public String toString() {
-    return "SubmitRequirement{"
-        + "shortReason='"
-        + shortReason
-        + '\''
-        + ", fullReason='"
-        + fullReason
-        + '\''
-        + ", label='"
-        + label
-        + '\''
-        + '}';
+  private static boolean validateType(String type) {
+    return TYPE_MATCHER.matchesAllOf(type);
   }
 }
diff --git a/java/com/google/gerrit/server/data/SubmitRequirementAttribute.java b/java/com/google/gerrit/server/data/SubmitRequirementAttribute.java
index 8a0ea51525..32030248f2 100644
--- a/java/com/google/gerrit/server/data/SubmitRequirementAttribute.java
+++ b/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 String fullReason;
-  public String label;
+  public Map<String, String> data;
+  public String type;
+  public String fallbackText;
 }
diff --git a/java/com/google/gerrit/server/events/EventFactory.java b/java/com/google/gerrit/server/events/EventFactory.java
index d3d52e4ed4..e557250562 100644
--- a/java/com/google/gerrit/server/events/EventFactory.java
+++ b/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.fullReason = req.fullReason();
-        re.label = req.label().orElse(null);
+        re.fallbackText = req.fallbackText();
+        re.type = req.type();
+        re.data = req.data();
         sa.requirements.add(re);
       }
     }
diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index 6dbad6a948..2f8c78501f 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/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 fullReason;
-      @Nullable String label;
+      String fallbackText;
+      String type;
+      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.fullReason = requirement.fullReason();
-          sr.label = requirement.label().orElse(null);
+          sr.type = requirement.type();
+          sr.fallbackText = requirement.fallbackText();
+          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(
-                  requirement.shortReason, requirement.fullReason, requirement.label);
+              SubmitRequirement.builder()
+                  .setType(req.type)
+                  .setFallbackText(req.fallbackText)
+                  .setData(req.data)
+                  .build();
           rec.requirements.add(sr);
         }
       }
diff --git a/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java b/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
index 95ac3e3604..8e8a0ea38c 100644
--- a/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
+++ b/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(
-            "short reason",
-            "Full reason can be a long string with special symbols like < > \\ / ; :",
-            null);
+        SubmitRequirement.builder()
+            .setType("short_type")
+            .setFallbackText("Fallback text may contain special symbols like < > \\ / ; :")
+            .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);