Merge pull request #12280 from polyfractal/bugfix/movavg_validation

polyfractal · polyfractal · commit 380648ab8685 · 2015-07-16T11:44:32.000-04:00
Aggregations: Add better validation of moving_avg model settings
diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgBuilder.java
@@ -25,6 +25,7 @@
 import org.elasticsearch.search.aggregations.pipeline.movavg.models.MovAvgModelBuilder;
 
 import java.io.IOException;
+import java.util.Map;
 
 /**
  * A builder to create MovingAvg pipeline aggregations
@@ -37,6 +38,7 @@ public class MovAvgBuilder extends PipelineAggregatorBuilder<MovAvgBuilder> {
     private Integer window;
     private Integer predict;
     private Boolean minimize;
+    private Map<String, Object> settings;
 
     public MovAvgBuilder(String name) {
         super(name, MovAvgPipelineAggregator.TYPE.name());
@@ -107,6 +109,18 @@ public MovAvgBuilder minimize(boolean minimize) {
         return this;
     }
 
+    /**
+     * The hash of settings that should be provided to the model when it is
+     * instantiated
+     *
+     * @param settings
+     * @return
+     */
+    public MovAvgBuilder settings(Map<String, Object> settings) {
+        this.settings = settings;
+        return this;
+    }
+
 
     @Override
     protected XContentBuilder internalXContent(XContentBuilder builder, Params params) throws IOException {
@@ -128,6 +142,9 @@ protected XContentBuilder internalXContent(XContentBuilder builder, Params param
         if (minimize != null) {
             builder.field(MovAvgParser.MINIMIZE.getPreferredName(), minimize);
         }
+        if (settings != null) {
+            builder.field(MovAvgParser.SETTINGS.getPreferredName(), settings);
+        }
         return builder;
     }
 
diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/EwmaModel.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/EwmaModel.java
@@ -120,9 +120,11 @@ public String getName() {
         }
 
         @Override
-        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize, ParseFieldMatcher parseFieldMatcher) throws ParseException {
+        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize,
+                                 ParseFieldMatcher parseFieldMatcher) throws ParseException {
 
             double alpha = parseDoubleParam(settings, "alpha", 0.3);
+            checkUnrecognizedParams(settings);
             return new EwmaModel(alpha);
         }
 
diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/HoltLinearModel.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/HoltLinearModel.java
@@ -180,10 +180,12 @@ public String getName() {
         }
 
         @Override
-        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize, ParseFieldMatcher parseFieldMatcher) throws ParseException {
+        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize,
+                                 ParseFieldMatcher parseFieldMatcher) throws ParseException {
 
             double alpha = parseDoubleParam(settings, "alpha", 0.3);
             double beta = parseDoubleParam(settings, "beta", 0.1);
+            checkUnrecognizedParams(settings);
             return new HoltLinearModel(alpha, beta);
         }
     }
diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/HoltWintersModel.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/HoltWintersModel.java
@@ -356,7 +356,8 @@ public String getName() {
         }
 
         @Override
-        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize, ParseFieldMatcher parseFieldMatcher) throws ParseException {
+        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize,
+                                 ParseFieldMatcher parseFieldMatcher) throws ParseException {
 
             double alpha = parseDoubleParam(settings, "alpha", 0.3);
             double beta = parseDoubleParam(settings, "beta", 0.1);
@@ -376,6 +377,7 @@ public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipeline
                 if (value != null) {
                     if (value instanceof String) {
                         seasonalityType = SeasonalityType.parse((String)value, parseFieldMatcher);
+                        settings.remove("type");
                     } else {
                         throw new ParseException("Parameter [type] must be a String, type `"
                                 + value.getClass().getSimpleName() + "` provided instead", 0);
@@ -385,6 +387,7 @@ public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipeline
 
             boolean pad = parseBoolParam(settings, "pad", seasonalityType.equals(SeasonalityType.MULTIPLICATIVE));
 
+            checkUnrecognizedParams(settings);
             return new HoltWintersModel(alpha, beta, gamma, period, seasonalityType, pad);
         }
     }
diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/LinearModel.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/LinearModel.java
@@ -107,7 +107,9 @@ public String getName() {
         }
 
         @Override
-        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize, ParseFieldMatcher parseFieldMatcher) throws ParseException {
+        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize,
+                                 ParseFieldMatcher parseFieldMatcher) throws ParseException {
+            checkUnrecognizedParams(settings);
             return new LinearModel();
         }
     }
diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/MovAvgModel.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/MovAvgModel.java
@@ -155,7 +155,8 @@ public abstract static class AbstractModelParser {
          * @param parseFieldMatcher  Matcher for field names
          * @return                   A fully built moving average model
          */
-        public abstract MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize, ParseFieldMatcher parseFieldMatcher) throws ParseException;
+        public abstract MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName,
+                                          int windowSize, ParseFieldMatcher parseFieldMatcher) throws ParseException;
 
 
         /**
@@ -180,6 +181,7 @@ protected double parseDoubleParam(@Nullable Map<String, Object> settings, String
             } else if (value instanceof Number) {
                 double v = ((Number) value).doubleValue();
                 if (v >= 0 && v <= 1) {
+                    settings.remove(name);
                     return v;
                 }
 
@@ -211,6 +213,7 @@ protected int parseIntegerParam(@Nullable Map<String, Object> settings, String n
             if (value == null) {
                 return defaultValue;
             } else if (value instanceof Number) {
+                settings.remove(name);
                 return ((Number) value).intValue();
             }
 
@@ -238,12 +241,19 @@ protected boolean parseBoolParam(@Nullable Map<String, Object> settings, String
             if (value == null) {
                 return defaultValue;
             } else if (value instanceof Boolean) {
+                settings.remove(name);
                 return (Boolean)value;
             }
 
             throw new ParseException("Parameter [" + name + "] must be a boolean, type `"
                     + value.getClass().getSimpleName() + "` provided instead", 0);
         }
+
+        protected void checkUnrecognizedParams(@Nullable Map<String, Object> settings) throws ParseException {
+            if (settings != null && settings.size() > 0) {
+                throw new ParseException("Unrecognized parameter(s): [" + String.join(", ", settings.keySet()) + "]", 0);
+            }
+        }
     }
 
 }
diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/SimpleModel.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/SimpleModel.java
@@ -60,7 +60,7 @@ public MovAvgModel clone() {
     protected <T extends Number> double[] doPredict(Collection<T> values, int numPredictions) {
         double[] predictions = new double[numPredictions];
 
-        // EWMA just emits the same final prediction repeatedly.
+        // Simple just emits the same final prediction repeatedly.
         Arrays.fill(predictions, next(values));
 
         return predictions;
@@ -100,7 +100,9 @@ public String getName() {
         }
 
         @Override
-        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize, ParseFieldMatcher parseFieldMatcher) throws ParseException {
+        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize,
+                                 ParseFieldMatcher parseFieldMatcher) throws ParseException {
+            checkUnrecognizedParams(settings);
             return new SimpleModel();
         }
     }
diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/moving/avg/MovAvgTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/moving/avg/MovAvgTests.java
@@ -25,7 +25,6 @@
 import org.elasticsearch.action.index.IndexRequestBuilder;
 import org.elasticsearch.action.search.SearchPhaseExecutionException;
 import org.elasticsearch.action.search.SearchResponse;
-import org.elasticsearch.search.SearchParseException;
 import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
 import org.elasticsearch.search.aggregations.bucket.histogram.InternalHistogram;
 import org.elasticsearch.search.aggregations.bucket.histogram.InternalHistogram.Bucket;
@@ -1265,6 +1264,42 @@ public void checkIfTunableCanBeMinimized() {
                 fail("Model [" + builder.toString() + "] can be minimized, but an exception was thrown");
             }
         }
+    }
+
+    @Test
+    public void testUnrecognizedParams() {
+
+        MovAvgModelBuilder[] builders = new MovAvgModelBuilder[]{
+                new SimpleModel.SimpleModelBuilder(),
+                new LinearModel.LinearModelBuilder(),
+                new EwmaModel.EWMAModelBuilder(),
+                new HoltLinearModel.HoltLinearModelBuilder(),
+                new HoltWintersModel.HoltWintersModelBuilder()
+        };
+        Map<String, Object> badSettings = new HashMap<>(1);
+        badSettings.put("abc", 1.2);
+
+        for (MovAvgModelBuilder builder : builders) {
+            try {
+                SearchResponse response = client()
+                        .prepareSearch("idx").setTypes("type")
+                        .addAggregation(
+                                histogram("histo").field(INTERVAL_FIELD).interval(interval)
+                                        .extendedBounds(0L, (long) (interval * (numBuckets - 1)))
+                                        .subAggregation(metric)
+                                        .subAggregation(movingAvg("movavg_counts")
+                                                .window(10)
+                                                .modelBuilder(builder)
+                                                .gapPolicy(gapPolicy)
+                                                .settings(badSettings)
+                                                .setBucketsPaths("_count"))
+                        ).execute().actionGet();
+            } catch (SearchPhaseExecutionException e) {
+                // All good
+            }
+        }
+
+
 
 
     }
diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/moving/avg/MovAvgUnitTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/moving/avg/MovAvgUnitTests.java
@@ -611,8 +611,6 @@ public void testNumericValidation() {
         for (MovAvgModel.AbstractModelParser parser : parsers) {
             for (Object v : values) {
                 settings.put("alpha", v);
-                settings.put("beta", v);
-                settings.put("gamma", v);
 
                 try {
                     parser.parse(settings, "pipeline", 10, ParseFieldMatcher.STRICT);

Original file line number	Diff line number	Diff line change
`@@ -120,9 +120,11 @@ public String getName() {`
`120`	`120`	`}`
`121`	`121`
`122`	`122`	`@Override`
`123`		`- public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize, ParseFieldMatcher parseFieldMatcher) throws ParseException {`
	`123`	`+ public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize,`
	`124`	`+ ParseFieldMatcher parseFieldMatcher) throws ParseException {`
`124`	`125`
`125`	`126`	`double alpha = parseDoubleParam(settings, "alpha", 0.3);`
	`127`	`+ checkUnrecognizedParams(settings);`
`126`	`128`	`return new EwmaModel(alpha);`
`127`	`129`	`}`
`128`	`130`
Original file line number	Diff line number	Diff line change
`@@ -180,10 +180,12 @@ public String getName() {`
`180`	`180`	`}`
`181`	`181`
`182`	`182`	`@Override`
`183`		`- public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize, ParseFieldMatcher parseFieldMatcher) throws ParseException {`
	`183`	`+ public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize,`
	`184`	`+ ParseFieldMatcher parseFieldMatcher) throws ParseException {`
`184`	`185`
`185`	`186`	`double alpha = parseDoubleParam(settings, "alpha", 0.3);`
`186`	`187`	`double beta = parseDoubleParam(settings, "beta", 0.1);`
	`188`	`+ checkUnrecognizedParams(settings);`
`187`	`189`	`return new HoltLinearModel(alpha, beta);`
`188`	`190`	`}`
`189`	`191`	`}`
Original file line number	Diff line number	Diff line change
`@@ -107,7 +107,9 @@ public String getName() {`
`107`	`107`	`}`
`108`	`108`
`109`	`109`	`@Override`
`110`		`- public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize, ParseFieldMatcher parseFieldMatcher) throws ParseException {`
	`110`	`+ public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize,`
	`111`	`+ ParseFieldMatcher parseFieldMatcher) throws ParseException {`
	`112`	`+ checkUnrecognizedParams(settings);`
`111`	`113`	`return new LinearModel();`
`112`	`114`	`}`
`113`	`115`	`}`
Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,7 @@ public MovAvgModel clone() {`
`60`	`60`	`protected <T extends Number> double[] doPredict(Collection<T> values, int numPredictions) {`
`61`	`61`	`double[] predictions = new double[numPredictions];`
`62`	`62`
`63`		`- // EWMA just emits the same final prediction repeatedly.`
	`63`	`+ // Simple just emits the same final prediction repeatedly.`
`64`	`64`	`Arrays.fill(predictions, next(values));`
`65`	`65`
`66`	`66`	`return predictions;`
`@@ -100,7 +100,9 @@ public String getName() {`
`100`	`100`	`}`
`101`	`101`
`102`	`102`	`@Override`
`103`		`- public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize, ParseFieldMatcher parseFieldMatcher) throws ParseException {`
	`103`	`+ public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize,`
	`104`	`+ ParseFieldMatcher parseFieldMatcher) throws ParseException {`
	`105`	`+ checkUnrecognizedParams(settings);`
`104`	`106`	`return new SimpleModel();`
`105`	`107`	`}`
`106`	`108`	`}`