From ae742c4a038b5a1f7cbfdf1697845cd8ea4d71ac Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Fri, 19 Jun 2015 10:36:15 -0400 Subject: [PATCH] Aggregations: moving_avg model parser should accept any numeric, not just doubles Also changes the models to throw ParseExceptions instead of SearchParseExceptions, so that the validation can be unit-tested. Fixes #11487 --- .../pipeline/movavg/MovAvgParser.java | 10 +++- .../pipeline/movavg/models/EwmaModel.java | 5 +- .../movavg/models/HoltLinearModel.java | 7 +-- .../movavg/models/HoltWintersModel.java | 21 ++++---- .../pipeline/movavg/models/LinearModel.java | 3 +- .../pipeline/movavg/models/MovAvgModel.java | 41 +++++++-------- .../pipeline/movavg/models/SimpleModel.java | 3 +- .../pipeline/moving/avg/MovAvgUnitTests.java | 50 ++++++++++++++++++- 8 files changed, 99 insertions(+), 41 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgParser.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgParser.java index 261f811a751..951bf589dc3 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgParser.java +++ b/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); 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 index 2f33855d50e..6cc75b21ec7 100644 --- 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 @@ -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 settings, String pipelineName, SearchContext context, int windowSize) { + public MovAvgModel parse(@Nullable Map 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); } 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 index 3a7fd963c43..dd4b679bccc 100644 --- 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 @@ -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 settings, String pipelineName, SearchContext context, int windowSize) { + public MovAvgModel parse(@Nullable Map settings, String pipelineName, int windowSize) throws ParseException { - double alpha = parseDoubleParam(context, settings, "alpha", 0.5); - double beta = parseDoubleParam(context, settings, "beta", 0.5); + double alpha = parseDoubleParam(settings, "alpha", 0.5); + double beta = parseDoubleParam(settings, "beta", 0.5); 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 index ef3c7354500..f43ea730254 100644 --- 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 @@ -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 settings, String pipelineName, SearchContext context, int windowSize) { + public MovAvgModel parse(@Nullable Map settings, String pipelineName, int windowSize) throws ParseException { - double alpha = parseDoubleParam(context, settings, "alpha", 0.5); - double beta = parseDoubleParam(context, settings, "beta", 0.5); - double gamma = parseDoubleParam(context, settings, "gamma", 0.5); - int period = parseIntegerParam(context, settings, "period", 1); + double alpha = parseDoubleParam(settings, "alpha", 0.5); + double beta = parseDoubleParam(settings, "beta", 0.5); + double gamma = parseDoubleParam(settings, "gamma", 0.5); + 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 `" - + value.getClass().getSimpleName() + "` provided instead", null); + throw new ParseException("Parameter [type] must be a String, type `" + + 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); } 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 index c894f776ed4..4036a446f94 100644 --- 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 @@ -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 settings, String pipelineName, SearchContext context, int windowSize) { + public MovAvgModel parse(@Nullable Map settings, String pipelineName, int windowSize) throws ParseException { 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 index 5f41b24531b..c987aec76d2 100644 --- 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 @@ -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 settings, String pipelineName, SearchContext context, int windowSize); + public abstract MovAvgModel parse(@Nullable Map 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 settings, String name, double defaultValue) { + protected double parseDoubleParam(@Nullable Map 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) { - double v = (Double)value; + } else if (value instanceof Number) { + 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" - + "value was [" + v + "]", null); + throw new ParseException("Parameter [" + name + "] must be between 0-1 inclusive. Provided" + + "value was [" + v + "]", 0); } - throw new SearchParseException(context, "Parameter [" + name + "] must be a double, type `" - + value.getClass().getSimpleName() + "` provided instead", null); + throw new ParseException("Parameter [" + name + "] must be a double, type `" + + 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 settings, String name, int defaultValue) { + protected int parseIntegerParam(@Nullable Map 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) { - return (Integer)value; + } else if (value instanceof Number) { + return ((Number) value).intValue(); } - throw new SearchParseException(context, "Parameter [" + name + "] must be an integer, type `" - + value.getClass().getSimpleName() + "` provided instead", null); + throw new ParseException("Parameter [" + name + "] must be an integer, type `" + + 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 settings, String name, boolean defaultValue) { + protected boolean parseBoolParam(@Nullable Map 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 `" - + value.getClass().getSimpleName() + "` provided instead", null); + throw new ParseException("Parameter [" + name + "] must be a boolean, type `" + + value.getClass().getSimpleName() + "` provided instead", 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 index 78055b063eb..2e770a9acef 100644 --- 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 @@ -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 settings, String pipelineName, SearchContext context, int windowSize) { + public MovAvgModel parse(@Nullable Map settings, String pipelineName, int windowSize) throws ParseException { return new SimpleModel(); } } 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 index 0bd9711c7ef..8a94a04ad41 100644 --- 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 @@ -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 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 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]"); + } + } }