[Rollup] Remove builders from MetricConfig (#32536)

Related to #29827

Author: tlrx

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/MetricConfig.java

@@ -12,9 +12,10 @@
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
-import org.elasticsearch.common.xcontent.ObjectParser;
-import org.elasticsearch.common.xcontent.ToXContentFragment;
+import org.elasticsearch.common.xcontent.ConstructingObjectParser;
+import org.elasticsearch.common.xcontent.ToXContentObject;
 import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.search.aggregations.metrics.avg.AvgAggregationBuilder;
 import org.elasticsearch.search.aggregations.metrics.max.MaxAggregationBuilder;
 import org.elasticsearch.search.aggregations.metrics.min.MinAggregationBuilder;
@@ -32,6 +33,8 @@
 import java.util.Objects;
 import java.util.stream.Collectors;
 
+import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
+
 /**
  * The configuration object for the metrics portion of a rollup job config
  *
@@ -48,14 +51,7 @@
  *     ]
  * }
  */
-public class MetricConfig implements Writeable, ToXContentFragment {
-    private static final String NAME = "metric_config";
-
-    private String field;
-    private List<String> metrics;
-
-    private static final ParseField FIELD = new ParseField("field");
-    private static final ParseField METRICS = new ParseField("metrics");
+public class MetricConfig implements Writeable, ToXContentObject {
 
     // TODO: replace these with an enum
     private static final ParseField MIN = new ParseField("min");
@@ -64,27 +60,54 @@ public class MetricConfig implements Writeable, ToXContentFragment {
     private static final ParseField AVG = new ParseField("avg");
     private static final ParseField VALUE_COUNT = new ParseField("value_count");
 
-    public static final ObjectParser<MetricConfig.Builder, Void> PARSER = new ObjectParser<>(NAME, MetricConfig.Builder::new);
-
+    private static final String NAME = "metrics";
+    private static final String FIELD = "field";
+    private static final String METRICS = "metrics";
+    private static final ConstructingObjectParser<MetricConfig, Void> PARSER;
     static {
-        PARSER.declareString(MetricConfig.Builder::setField, FIELD);
-        PARSER.declareStringArray(MetricConfig.Builder::setMetrics, METRICS);
+        PARSER = new ConstructingObjectParser<>(NAME, args -> {
+            @SuppressWarnings("unchecked") List<String> metrics = (List<String>) args[1];
+            return new MetricConfig((String) args[0], metrics);
+        });
+        PARSER.declareString(constructorArg(), new ParseField(FIELD));
+        PARSER.declareStringArray(constructorArg(), new ParseField(METRICS));
     }
 
-    MetricConfig(String name, List<String> metrics) {
-        this.field = name;
+    private final String field;
+    private final List<String> metrics;
+
+    public MetricConfig(final String field, final List<String> metrics) {
+        if (field == null || field.isEmpty()) {
+            throw new IllegalArgumentException("Field must be a non-null, non-empty string");
+        }
+        if (metrics == null || metrics.isEmpty()) {
+            throw new IllegalArgumentException("Metrics must be a non-null, non-empty array of strings");
+        }
+        metrics.forEach(m -> {
+            if (RollupField.SUPPORTED_METRICS.contains(m) == false) {
+                throw new IllegalArgumentException("Unsupported metric [" + m + "]. " +
+                    "Supported metrics include: " + RollupField.SUPPORTED_METRICS);
+            }
+        });
+        this.field = field;
         this.metrics = metrics;
     }
 
-    MetricConfig(StreamInput in) throws IOException {
+    MetricConfig(final StreamInput in) throws IOException {
         field = in.readString();
         metrics = in.readList(StreamInput::readString);
     }
 
+    /**
+     * @return the name of the field used in the metric configuration. Never {@code null}.
+     */
     public String getField() {
         return field;
     }
 
+    /**
+     * @return the names of the metrics used in the metric configuration. Never {@code null}.
+     */
     public List<String> getMetrics() {
         return metrics;
     }
@@ -159,10 +182,13 @@ public void validateMappings(Map<String, Map<String, FieldCapabilities>> fieldCa
     }
 
     @Override
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-        builder.field(FIELD.getPreferredName(), field);
-        builder.field(METRICS.getPreferredName(), metrics);
-        return builder;
+    public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException {
+        builder.startObject();
+        {
+            builder.field(FIELD, field);
+            builder.field(METRICS, metrics);
+        }
+        return builder.endObject();
     }
 
     @Override
@@ -172,19 +198,16 @@ public void writeTo(StreamOutput out) throws IOException {
     }
 
     @Override
-    public boolean equals(Object other) {
+    public boolean equals(final Object other) {
         if (this == other) {
             return true;
         }
-
         if (other == null || getClass() != other.getClass()) {
             return false;
         }
 
-        MetricConfig that = (MetricConfig) other;
-
-        return Objects.equals(this.field, that.field)
-                && Objects.equals(this.metrics, that.metrics);
+        final MetricConfig that = (MetricConfig) other;
+        return Objects.equals(field, that.field) && Objects.equals(metrics, that.metrics);
     }
 
     @Override
@@ -197,52 +220,7 @@ public String toString() {
         return Strings.toString(this, true, true);
     }
 
-
-    public static class Builder {
-        private String field;
-        private List<String> metrics;
-
-        public Builder() {
-        }
-
-        public Builder(MetricConfig config) {
-            this.field = config.getField();
-            this.metrics = config.getMetrics();
-        }
-
-        public String getField() {
-            return field;
-        }
-
-        public MetricConfig.Builder setField(String field) {
-            this.field = field;
-            return this;
-        }
-
-        public List<String> getMetrics() {
-            return metrics;
-        }
-
-        public MetricConfig.Builder setMetrics(List<String> metrics) {
-            this.metrics = metrics;
-            return this;
-        }
-
-        public MetricConfig build() {
-            if (Strings.isNullOrEmpty(field) == true) {
-                throw new IllegalArgumentException("Parameter [" + FIELD.getPreferredName() + "] must be a non-null, non-empty string.");
-            }
-            if (metrics == null || metrics.isEmpty()) {
-                throw new IllegalArgumentException("Parameter [" + METRICS.getPreferredName()
-                        + "] must be a non-null, non-empty array of strings.");
-            }
-            metrics.forEach(m -> {
-                if (RollupField.SUPPORTED_METRICS.contains(m) == false) {
-                    throw new IllegalArgumentException("Unsupported metric [" + m + "].  " +
-                            "Supported metrics include: " + RollupField.SUPPORTED_METRICS);
-                }
-            });
-            return new MetricConfig(field, metrics);
-        }
+    public static MetricConfig fromXContent(final XContentParser parser) throws IOException {
+        return PARSER.parse(parser, null);
     }
 }

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/RollupJobConfig.java

@@ -63,7 +63,7 @@ public class RollupJobConfig implements NamedWriteable, ToXContentObject {
     static {
         PARSER.declareString(RollupJobConfig.Builder::setId, RollupField.ID);
         PARSER.declareObject(RollupJobConfig.Builder::setGroupConfig, (p, c) -> GroupConfig.PARSER.apply(p,c).build(), GROUPS);
-        PARSER.declareObjectArray(RollupJobConfig.Builder::setMetricsConfig, (p, c) -> MetricConfig.PARSER.apply(p, c).build(), METRICS);
+        PARSER.declareObjectArray(RollupJobConfig.Builder::setMetricsConfig, (p, c) -> MetricConfig.fromXContent(p), METRICS);
         PARSER.declareString((params, val) ->
                 params.setTimeout(TimeValue.parseTimeValue(val, TIMEOUT.getPreferredName())), TIMEOUT);
         PARSER.declareString(RollupJobConfig.Builder::setIndexPattern, INDEX_PATTERN);
@@ -160,10 +160,8 @@ public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params par
         }
         if (metricsConfig != null) {
             builder.startArray(METRICS.getPreferredName());
-            for (MetricConfig config : metricsConfig) {
-                builder.startObject();
-                config.toXContent(builder, params);
-                builder.endObject();
+            for (MetricConfig metric : metricsConfig) {
+                metric.toXContent(builder, params);
             }
             builder.endArray();
         }

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/ConfigTestHelpers.java

@@ -17,6 +17,7 @@
 import org.elasticsearch.xpack.core.rollup.job.TermsGroupConfig;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Random;
 import java.util.stream.Collectors;
@@ -38,11 +39,7 @@ public static RollupJobConfig.Builder getRollupJob(String jobId) {
         builder.setGroupConfig(ConfigTestHelpers.getGroupConfig().build());
         builder.setPageSize(ESTestCase.randomIntBetween(1,10));
         if (ESTestCase.randomBoolean()) {
-            List<MetricConfig> metrics = IntStream.range(1, ESTestCase.randomIntBetween(1,10))
-                    .mapToObj(n -> ConfigTestHelpers.getMetricConfig().build())
-                    .collect(Collectors.toList());
-
-            builder.setMetricsConfig(metrics);
+            builder.setMetricsConfig(randomMetricsConfigs(ESTestCase.random()));
         }
         return builder;
     }
@@ -59,32 +56,6 @@ public static GroupConfig.Builder getGroupConfig() {
         return groupBuilder;
     }
 
-    public static MetricConfig.Builder getMetricConfig() {
-        MetricConfig.Builder builder = new MetricConfig.Builder();
-        builder.setField(ESTestCase.randomAlphaOfLength(15));  // large names so we don't accidentally collide
-        List<String> metrics = new ArrayList<>();
-        if (ESTestCase.randomBoolean()) {
-            metrics.add("min");
-        }
-        if (ESTestCase.randomBoolean()) {
-            metrics.add("max");
-        }
-        if (ESTestCase.randomBoolean()) {
-            metrics.add("sum");
-        }
-        if (ESTestCase.randomBoolean()) {
-            metrics.add("avg");
-        }
-        if (ESTestCase.randomBoolean()) {
-            metrics.add("value_count");
-        }
-        if (metrics.size() == 0) {
-            metrics.add("min");
-        }
-        builder.setMetrics(metrics);
-        return builder;
-    }
-
     private static final String[] TIME_SUFFIXES = new String[]{"d", "h", "ms", "s", "m"};
     public static String randomPositiveTimeValue() {
         return ESTestCase.randomIntBetween(1, 1000) + ESTestCase.randomFrom(TIME_SUFFIXES);
@@ -123,6 +94,39 @@ public static HistogramGroupConfig randomHistogramGroupConfig(final Random rando
         return new HistogramGroupConfig(randomInterval(random), randomFields(random));
     }
 
+    public static List<MetricConfig> randomMetricsConfigs(final Random random) {
+        final int numMetrics = randomIntBetween(random, 1, 10);
+        final List<MetricConfig> metrics = new ArrayList<>(numMetrics);
+        for (int i = 0; i < numMetrics; i++) {
+            metrics.add(randomMetricConfig(random));
+        }
+        return Collections.unmodifiableList(metrics);
+    }
+
+    public static MetricConfig randomMetricConfig(final Random random) {
+        final String field = randomAsciiAlphanumOfLengthBetween(random, 15, 25);  // large names so we don't accidentally collide
+        final List<String> metrics = new ArrayList<>();
+        if (random.nextBoolean()) {
+            metrics.add("min");
+        }
+        if (random.nextBoolean()) {
+            metrics.add("max");
+        }
+        if (random.nextBoolean()) {
+            metrics.add("sum");
+        }
+        if (random.nextBoolean()) {
+            metrics.add("avg");
+        }
+        if (random.nextBoolean()) {
+            metrics.add("value_count");
+        }
+        if (metrics.size() == 0) {
+            metrics.add("min");
+        }
+        return new MetricConfig(field, Collections.unmodifiableList(metrics));
+    }
+
     public static TermsGroupConfig randomTermsGroupConfig(final Random random) {
         return new TermsGroupConfig(randomFields(random));
     }