Merge pull request #11778 from polyfractal/bugfix/11487

Aggregations: moving_avg model parser should accept any numeric

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgParser.java

@@ -33,6 +33,7 @@ import org.elasticsearch.search.aggregations.support.format.ValueFormatter;
 import org.elasticsearch.search.internal.SearchContext;
 
 import java.io.IOException;
+import java.text.ParseException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -144,7 +145,14 @@ public class MovAvgParser implements PipelineAggregator.Parser {
             throw new SearchParseException(context, "Unknown model [" + model + "] specified.  Valid options are:"
                     + movAvgModelParserMapper.getAllNames().toString(), parser.getTokenLocation());
         }
-        MovAvgModel movAvgModel = modelParser.parse(settings, pipelineAggregatorName, context, window);
+
+        MovAvgModel movAvgModel;
+        try {
+            movAvgModel = modelParser.parse(settings, pipelineAggregatorName, window);
+        } catch (ParseException exception) {
+            throw new SearchParseException(context, "Could not parse settings for model [" + model + "].", null, exception);
+        }
+
 
         return new MovAvgPipelineAggregator.Factory(pipelineAggregatorName, bucketsPaths, formatter, gapPolicy, window, predict,
                 movAvgModel);

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/EwmaModel.java

@@ -28,6 +28,7 @@ import org.elasticsearch.search.aggregations.pipeline.movavg.MovAvgParser;
 import org.elasticsearch.search.internal.SearchContext;
 
 import java.io.IOException;
+import java.text.ParseException;
 import java.util.Collection;
 import java.util.Map;
 
@@ -92,9 +93,9 @@ public class EwmaModel extends MovAvgModel {
         }
 
         @Override
-        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName,  SearchContext context, int windowSize) {
+        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize) throws ParseException {
 
-            double alpha = parseDoubleParam(context, settings, "alpha", 0.5);
+            double alpha = parseDoubleParam(settings, "alpha", 0.5);
 
             return new EwmaModel(alpha);
         }

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/HoltLinearModel.java

@@ -28,6 +28,7 @@ import org.elasticsearch.search.aggregations.pipeline.movavg.MovAvgParser;
 import org.elasticsearch.search.internal.SearchContext;
 
 import java.io.IOException;
+import java.text.ParseException;
 import java.util.*;
 
 /**
@@ -151,10 +152,10 @@ public class HoltLinearModel extends MovAvgModel {
         }
 
         @Override
-        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, SearchContext context, int windowSize) {
+        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize) throws ParseException {
 
-            double alpha = parseDoubleParam(context, settings, "alpha", 0.5);
+            double alpha = parseDoubleParam(settings, "alpha", 0.5);
-            double beta = parseDoubleParam(context, settings, "beta", 0.5);
+            double beta = parseDoubleParam(settings, "beta", 0.5);
             return new HoltLinearModel(alpha, beta);
         }
     }

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/HoltWintersModel.java

@@ -32,6 +32,7 @@ import org.elasticsearch.search.aggregations.pipeline.movavg.MovAvgParser;
 import org.elasticsearch.search.internal.SearchContext;
 
 import java.io.IOException;
+import java.text.ParseException;
 import java.util.*;
 
 /**
@@ -316,17 +317,17 @@ public class HoltWintersModel extends MovAvgModel {
         }
 
         @Override
-        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, SearchContext context, int windowSize) {
+        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize) throws ParseException {
 
-            double alpha = parseDoubleParam(context, settings, "alpha", 0.5);
+            double alpha = parseDoubleParam(settings, "alpha", 0.5);
-            double beta = parseDoubleParam(context, settings, "beta", 0.5);
+            double beta = parseDoubleParam(settings, "beta", 0.5);
-            double gamma = parseDoubleParam(context, settings, "gamma", 0.5);
+            double gamma = parseDoubleParam(settings, "gamma", 0.5);
-            int period = parseIntegerParam(context, settings, "period", 1);
+            int period = parseIntegerParam(settings, "period", 1);
 
             if (windowSize < 2 * period) {
-                throw new SearchParseException(context, "Field [window] must be at least twice as large as the period when " +
+                throw new ParseException("Field [window] must be at least twice as large as the period when " +
                         "using Holt-Winters.  Value provided was [" + windowSize + "], which is less than (2*period) == "
-                        + (2 * period), null);
+                        + (2 * period), 0);
             }
 
             SeasonalityType seasonalityType = SeasonalityType.ADDITIVE;
@@ -337,13 +338,13 @@ public class HoltWintersModel extends MovAvgModel {
                     if (value instanceof String) {
                         seasonalityType = SeasonalityType.parse((String)value);
                     } else {
-                        throw new SearchParseException(context, "Parameter [type] must be a String, type `"
+                        throw new ParseException("Parameter [type] must be a String, type `"
-                                + value.getClass().getSimpleName() + "` provided instead", null);
+                                + value.getClass().getSimpleName() + "` provided instead", 0);
                     }
                 }
             }
 
-            boolean pad = parseBoolParam(context, settings, "pad", seasonalityType.equals(SeasonalityType.MULTIPLICATIVE));
+            boolean pad = parseBoolParam(settings, "pad", seasonalityType.equals(SeasonalityType.MULTIPLICATIVE));
 
             return new HoltWintersModel(alpha, beta, gamma, period, seasonalityType, pad);
         }

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/LinearModel.java

@@ -29,6 +29,7 @@ import org.elasticsearch.search.aggregations.pipeline.movavg.MovAvgParser;
 import org.elasticsearch.search.internal.SearchContext;
 
 import java.io.IOException;
+import java.text.ParseException;
 import java.util.Collection;
 import java.util.Map;
 
@@ -79,7 +80,7 @@ public class LinearModel extends MovAvgModel {
         }
 
         @Override
-        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName,  SearchContext context, int windowSize) {
+        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize) throws ParseException {
             return new LinearModel();
         }
     }

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/MovAvgModel.java

@@ -27,6 +27,7 @@ import org.elasticsearch.search.SearchParseException;
 import org.elasticsearch.search.internal.SearchContext;
 
 import java.io.IOException;
+import java.text.ParseException;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Map;
@@ -125,26 +126,24 @@ public abstract class MovAvgModel {
          *
          * @param settings      Map of settings, extracted from the request
          * @param pipelineName   Name of the parent pipeline agg
-         * @param context       The parser context that we are in
          * @param windowSize    Size of the window for this moving avg
          * @return              A fully built moving average model
          */
-        public abstract MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, SearchContext context, int windowSize);
+        public abstract MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize) throws ParseException;
 
 
         /**
          * Extracts a 0-1 inclusive double from the settings map, otherwise throws an exception
          *
-         * @param context       Search query context
          * @param settings      Map of settings provided to this model
          * @param name          Name of parameter we are attempting to extract
          * @param defaultValue  Default value to be used if value does not exist in map
          *
-         * @throws SearchParseException
+         * @throws ParseException
          *
          * @return Double value extracted from settings map
          */
-        protected double parseDoubleParam(SearchContext context, @Nullable Map<String, Object> settings, String name, double defaultValue) {
+        protected double parseDoubleParam(@Nullable Map<String, Object> settings, String name, double defaultValue) throws ParseException {
             if (settings == null) {
                 return defaultValue;
             }
@@ -152,33 +151,32 @@ public abstract class MovAvgModel {
             Object value = settings.get(name);
             if (value == null) {
                 return defaultValue;
-            } else if (value instanceof Double) {
+            } else if (value instanceof Number) {
-                double v = (Double)value;
+                double v = ((Number) value).doubleValue();
                 if (v >= 0 && v <= 1) {
                     return v;
                 }
 
-                throw new SearchParseException(context, "Parameter [" + name + "] must be between 0-1 inclusive.  Provided"
+                throw new ParseException("Parameter [" + name + "] must be between 0-1 inclusive.  Provided"
-                        + "value was [" + v + "]", null);
+                        + "value was [" + v + "]", 0);
             }
 
-            throw new SearchParseException(context, "Parameter [" + name + "] must be a double, type `"
+            throw new ParseException("Parameter [" + name + "] must be a double, type `"
-                    + value.getClass().getSimpleName() + "` provided instead", null);
+                    + value.getClass().getSimpleName() + "` provided instead", 0);
         }
 
         /**
          * Extracts an integer from the settings map, otherwise throws an exception
          *
-         * @param context       Search query context
          * @param settings      Map of settings provided to this model
          * @param name          Name of parameter we are attempting to extract
          * @param defaultValue  Default value to be used if value does not exist in map
          *
-         * @throws SearchParseException
+         * @throws ParseException
          *
          * @return Integer value extracted from settings map
          */
-        protected int parseIntegerParam(SearchContext context, @Nullable Map<String, Object> settings, String name, int defaultValue) {
+        protected int parseIntegerParam(@Nullable Map<String, Object> settings, String name, int defaultValue) throws ParseException {
             if (settings == null) {
                 return defaultValue;
             }
@@ -186,18 +184,17 @@ public abstract class MovAvgModel {
             Object value = settings.get(name);
             if (value == null) {
                 return defaultValue;
-            } else if (value instanceof Integer) {
+            } else if (value instanceof Number) {
-                return (Integer)value;
+                return ((Number) value).intValue();
             }
 
-            throw new SearchParseException(context, "Parameter [" + name + "] must be an integer, type `"
+            throw new ParseException("Parameter [" + name + "] must be an integer, type `"
-                    + value.getClass().getSimpleName() + "` provided instead", null);
+                    + value.getClass().getSimpleName() + "` provided instead", 0);
         }
 
         /**
          * Extracts a boolean from the settings map, otherwise throws an exception
          *
-         * @param context       Search query context
          * @param settings      Map of settings provided to this model
          * @param name          Name of parameter we are attempting to extract
          * @param defaultValue  Default value to be used if value does not exist in map
@@ -206,7 +203,7 @@ public abstract class MovAvgModel {
          *
          * @return Boolean value extracted from settings map
          */
-        protected boolean parseBoolParam(SearchContext context, @Nullable Map<String, Object> settings, String name, boolean defaultValue) {
+        protected boolean parseBoolParam(@Nullable Map<String, Object> settings, String name, boolean defaultValue) throws ParseException {
             if (settings == null) {
                 return defaultValue;
             }
@@ -218,8 +215,8 @@ public abstract class MovAvgModel {
                 return (Boolean)value;
             }
 
-            throw new SearchParseException(context, "Parameter [" + name + "] must be a boolean, type `"
+            throw new ParseException("Parameter [" + name + "] must be a boolean, type `"
-                    + value.getClass().getSimpleName() + "` provided instead", null);
+                    + value.getClass().getSimpleName() + "` provided instead", 0);
         }
     }
 

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/models/SimpleModel.java

@@ -28,6 +28,7 @@ import org.elasticsearch.search.aggregations.pipeline.movavg.MovAvgParser;
 import org.elasticsearch.search.internal.SearchContext;
 
 import java.io.IOException;
+import java.text.ParseException;
 import java.util.Collection;
 import java.util.Map;
 
@@ -72,7 +73,7 @@ public class SimpleModel extends MovAvgModel {
         }
 
         @Override
-        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName,  SearchContext context, int windowSize) {
+        public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize) throws ParseException {
             return new SimpleModel();
         }
     }

core/src/test/java/org/elasticsearch/search/aggregations/pipeline/moving/avg/MovAvgUnitTests.java

@@ -21,6 +21,7 @@ package org.elasticsearch.search.aggregations.pipeline.moving.avg;
 
 import com.google.common.collect.EvictingQueue;
 
+import org.elasticsearch.search.SearchParseException;
 import org.elasticsearch.search.aggregations.pipeline.movavg.models.*;
 import org.elasticsearch.test.ElasticsearchTestCase;
 
@@ -28,7 +29,8 @@ import static org.hamcrest.Matchers.equalTo;
 
 import org.junit.Test;
 
-import java.util.Arrays;
+import java.text.ParseException;
+import java.util.*;
 
 public class MovAvgUnitTests extends ElasticsearchTestCase {
 
@@ -583,4 +585,50 @@ public class MovAvgUnitTests extends ElasticsearchTestCase {
         }
 
     }
+
+    @Test
+    public void testNumericValidation() {
+
+        List<MovAvgModel.AbstractModelParser> parsers = new ArrayList<>(5);
+
+        // Simple and Linear don't have any settings to test
+        parsers.add(new EwmaModel.SingleExpModelParser());
+        parsers.add(new HoltWintersModel.HoltWintersModelParser());
+        parsers.add(new HoltLinearModel.DoubleExpModelParser());
+
+
+        Object[] values = {(byte)1, 1, 1L, (short)1, (double)1};
+        Map<String, Object> settings = new HashMap<>(2);
+
+        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);
+                } catch (ParseException e) {
+                    fail(parser.getName() + " parser should not have thrown SearchParseException while parsing [" +
+                            v.getClass().getSimpleName() +"]");
+                }
+
+            }
+        }
+
+        for (MovAvgModel.AbstractModelParser parser : parsers) {
+            settings.put("alpha", "abc");
+            settings.put("beta", "abc");
+            settings.put("gamma", "abc");
+
+            try {
+                parser.parse(settings, "pipeline", 10);
+            } catch (ParseException e) {
+                //all good
+                continue;
+            }
+
+            fail(parser.getName() + " parser should have thrown SearchParseException while parsing [String]");
+        }
+    }
 }