From 289a69bf68966f8e5f9450a9211e5f8c03051243 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 13 Oct 2016 14:45:51 +0200 Subject: [PATCH] Use ObjectParser in ScriptSortBuilder --- .../java/org/elasticsearch/script/Script.java | 10 +++ .../search/sort/FieldSortBuilder.java | 23 ++---- .../search/sort/GeoDistanceSortBuilder.java | 4 +- .../search/sort/ScoreSortBuilder.java | 5 +- .../search/sort/ScriptSortBuilder.java | 76 +++++-------------- .../search/sort/SortBuilder.java | 15 ++++ .../search/sort/AbstractSortTestCase.java | 6 +- .../search/sort/ScriptSortBuilderTests.java | 37 ++++----- 8 files changed, 66 insertions(+), 110 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/script/Script.java b/core/src/main/java/org/elasticsearch/script/Script.java index 05da7138d19..47ecfd60ca1 100644 --- a/core/src/main/java/org/elasticsearch/script/Script.java +++ b/core/src/main/java/org/elasticsearch/script/Script.java @@ -23,6 +23,7 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParseFieldMatcher; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -32,6 +33,7 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.index.query.QueryParseContext; import org.elasticsearch.script.ScriptService.ScriptType; import java.io.IOException; @@ -189,6 +191,14 @@ public final class Script implements ToXContent, Writeable { return parse(parser, parseFieldMatcher, null); } + public static Script parse(XContentParser parser, QueryParseContext context) { + try { + return parse(parser, context.getParseFieldMatcher(), context.getDefaultScriptLanguage()); + } catch (IOException e) { + throw new ParsingException(parser.getTokenLocation(), "Error parsing [" + ScriptField.SCRIPT.getPreferredName() + "] field", e); + } + } + public static Script parse(XContentParser parser, ParseFieldMatcher parseFieldMatcher, @Nullable String lang) throws IOException { XContentParser.Token token = parser.currentToken(); // If the parser hasn't yet been pushed to the first token, do it now diff --git a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java index 46326c5611e..e1585d708cd 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -21,7 +21,6 @@ package org.elasticsearch.search.sort; import org.apache.lucene.search.SortField; import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ObjectParser; @@ -46,10 +45,7 @@ import java.util.Objects; */ public class FieldSortBuilder extends SortBuilder { public static final String NAME = "field_sort"; - public static final ParseField NESTED_PATH = new ParseField("nested_path"); - public static final ParseField NESTED_FILTER = new ParseField("nested_filter"); public static final ParseField MISSING = new ParseField("missing"); - public static final ParseField ORDER = new ParseField("order"); public static final ParseField SORT_MODE = new ParseField("mode"); public static final ParseField UNMAPPED_TYPE = new ParseField("unmapped_type"); @@ -239,10 +235,10 @@ public class FieldSortBuilder extends SortBuilder { builder.field(SORT_MODE.getPreferredName(), sortMode); } if (nestedFilter != null) { - builder.field(NESTED_FILTER.getPreferredName(), nestedFilter, params); + builder.field(NESTED_FILTER_FIELD.getPreferredName(), nestedFilter, params); } if (nestedPath != null) { - builder.field(NESTED_PATH.getPreferredName(), nestedPath); + builder.field(NESTED_PATH_FIELD.getPreferredName(), nestedPath); } builder.endObject(); builder.endObject(); @@ -334,17 +330,10 @@ public class FieldSortBuilder extends SortBuilder { static { PARSER.declareField(FieldSortBuilder::missing, p -> p.objectText(), MISSING, ValueType.VALUE); - PARSER.declareString(FieldSortBuilder::setNestedPath , NESTED_PATH); + PARSER.declareString(FieldSortBuilder::setNestedPath , NESTED_PATH_FIELD); PARSER.declareString(FieldSortBuilder::unmappedType , UNMAPPED_TYPE); - PARSER.declareField(FieldSortBuilder::order, p -> SortOrder.fromString(p.text()), ORDER_FIELD, ValueType.STRING); - PARSER.declareField(FieldSortBuilder::sortMode, p -> SortMode.fromString(p.text()), SORT_MODE, ValueType.STRING); - PARSER.declareObject(FieldSortBuilder::setNestedFilter, (p, c) -> { - try { - QueryBuilder builder = c.parseInnerQueryBuilder().orElseThrow( - () -> new ParsingException(p.getTokenLocation(), "Expected " + NESTED_FILTER.getPreferredName() + " element.")); - return builder; - } catch (Exception e) { - throw new ParsingException(p.getTokenLocation(), "Expected " + NESTED_FILTER.getPreferredName() + " element.", e); - }}, NESTED_FILTER); + PARSER.declareString((b, v) -> b.order(SortOrder.fromString(v)) , ORDER_FIELD); + PARSER.declareString((b, v) -> b.sortMode(SortMode.fromString(v)), SORT_MODE); + PARSER.declareObject(FieldSortBuilder::setNestedFilter, SortBuilder::parseNestedFilter, NESTED_FILTER_FIELD); } } diff --git a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java index c3511ef0ae1..d7b369ac257 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -80,8 +80,6 @@ public class GeoDistanceSortBuilder extends SortBuilder private static final ParseField COERCE_FIELD = new ParseField("coerce", "normalize") .withAllDeprecated("use validation_method instead"); private static final ParseField SORTMODE_FIELD = new ParseField("mode", "sort_mode"); - private static final ParseField NESTED_PATH_FIELD = new ParseField("nested_path"); - private static final ParseField NESTED_FILTER_FIELD = new ParseField("nested_filter"); private final String fieldName; private final List points = new ArrayList<>(); @@ -511,7 +509,7 @@ public class GeoDistanceSortBuilder extends SortBuilder public SortFieldAndFormat build(QueryShardContext context) throws IOException { final boolean indexCreatedBeforeV2_0 = context.indexVersionCreated().before(Version.V_2_0_0); // validation was not available prior to 2.x, so to support bwc percolation queries we only ignore_malformed on 2.x created indexes - List localPoints = new ArrayList(); + List localPoints = new ArrayList<>(); for (GeoPoint geoPoint : this.points) { localPoints.add(new GeoPoint(geoPoint)); } diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java index 39a0576d7d1..52f25301783 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java @@ -20,11 +20,9 @@ package org.elasticsearch.search.sort; import org.apache.lucene.search.SortField; -import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ObjectParser; -import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.query.QueryParseContext; import org.elasticsearch.index.query.QueryShardContext; @@ -39,7 +37,6 @@ import java.util.Objects; public class ScoreSortBuilder extends SortBuilder { public static final String NAME = "_score"; - public static final ParseField ORDER_FIELD = new ParseField("order"); private static final SortFieldAndFormat SORT_SCORE = new SortFieldAndFormat( new SortField(null, SortField.Type.SCORE), DocValueFormat.RAW); private static final SortFieldAndFormat SORT_SCORE_REVERSE = new SortFieldAndFormat( @@ -91,7 +88,7 @@ public class ScoreSortBuilder extends SortBuilder { private static ObjectParser PARSER = new ObjectParser<>(NAME, ScoreSortBuilder::new); static { - PARSER.declareField(ScoreSortBuilder::order, p -> SortOrder.fromString(p.text()), ORDER_FIELD, ValueType.STRING); + PARSER.declareString((builder, order) -> builder.order(SortOrder.fromString(order)), ORDER_FIELD); } @Override diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java index 2a1ca78f76c..bce30e844b5 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java @@ -26,13 +26,12 @@ import org.apache.lucene.search.SortField; import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.BytesRefBuilder; import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.ParseFieldMatcher; -import org.elasticsearch.common.ParsingException; 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.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.fielddata.FieldData; import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource.Nested; @@ -57,7 +56,8 @@ import java.io.IOException; import java.util.Collections; import java.util.Locale; import java.util.Objects; -import java.util.Optional; + +import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; /** * Script sort builder allows to sort based on a custom script expression. @@ -68,8 +68,6 @@ public class ScriptSortBuilder extends SortBuilder { public static final ParseField TYPE_FIELD = new ParseField("type"); public static final ParseField SCRIPT_FIELD = new ParseField("script"); public static final ParseField SORTMODE_FIELD = new ParseField("mode"); - public static final ParseField NESTED_PATH_FIELD = new ParseField("nested_path"); - public static final ParseField NESTED_FILTER_FIELD = new ParseField("nested_filter"); private final Script script; @@ -216,6 +214,18 @@ public class ScriptSortBuilder extends SortBuilder { return builder; } + private static ConstructingObjectParser PARSER = new ConstructingObjectParser<>(NAME, + a -> new ScriptSortBuilder((Script) a[0], (ScriptSortType) a[1])); + + static { + PARSER.declareField(constructorArg(), Script::parse, ScriptField.SCRIPT, ValueType.OBJECT_OR_STRING); + PARSER.declareField(constructorArg(), p -> ScriptSortType.fromString(p.text()), TYPE_FIELD, ValueType.STRING); + PARSER.declareString((b, v) -> b.order(SortOrder.fromString(v)), ORDER_FIELD); + PARSER.declareString((b, v) -> b.sortMode(SortMode.fromString(v)), SORTMODE_FIELD); + PARSER.declareString(ScriptSortBuilder::setNestedPath , NESTED_PATH_FIELD); + PARSER.declareObject(ScriptSortBuilder::setNestedFilter, SortBuilder::parseNestedFilter, NESTED_FILTER_FIELD); + } + /** * Creates a new {@link ScriptSortBuilder} from the query held by the {@link QueryParseContext} in * {@link org.elasticsearch.common.xcontent.XContent} format. @@ -226,59 +236,7 @@ public class ScriptSortBuilder extends SortBuilder { * in '{ "foo": { "order" : "asc"} }'. When parsing the inner object, the field name can be passed in via this argument */ public static ScriptSortBuilder fromXContent(QueryParseContext context, String elementName) throws IOException { - XContentParser parser = context.parser(); - ParseFieldMatcher parseField = context.getParseFieldMatcher(); - Script script = null; - ScriptSortType type = null; - SortMode sortMode = null; - SortOrder order = null; - Optional nestedFilter = Optional.empty(); - String nestedPath = null; - - XContentParser.Token token; - String currentName = parser.currentName(); - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentName = parser.currentName(); - } else if (token == XContentParser.Token.START_OBJECT) { - if (parseField.match(currentName, ScriptField.SCRIPT)) { - script = Script.parse(parser, parseField, context.getDefaultScriptLanguage()); - } else if (parseField.match(currentName, NESTED_FILTER_FIELD)) { - nestedFilter = context.parseInnerQueryBuilder(); - } else { - throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] failed to parse field [" + currentName + "]"); - } - } else if (token.isValue()) { - if (parseField.match(currentName, ORDER_FIELD)) { - order = SortOrder.fromString(parser.text()); - } else if (parseField.match(currentName, TYPE_FIELD)) { - type = ScriptSortType.fromString(parser.text()); - } else if (parseField.match(currentName, SORTMODE_FIELD)) { - sortMode = SortMode.fromString(parser.text()); - } else if (parseField.match(currentName, NESTED_PATH_FIELD)) { - nestedPath = parser.text(); - } else if (parseField.match(currentName, ScriptField.SCRIPT)) { - script = Script.parse(parser, parseField, context.getDefaultScriptLanguage()); - } else { - throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] failed to parse field [" + currentName + "]"); - } - } else { - throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] unexpected token [" + token + "]"); - } - } - - ScriptSortBuilder result = new ScriptSortBuilder(script, type); - if (order != null) { - result.order(order); - } - if (sortMode != null) { - result.sortMode(sortMode); - } - nestedFilter.ifPresent(result::setNestedFilter); - if (nestedPath != null) { - result.setNestedPath(nestedPath); - } - return result; + return PARSER.apply(context.parser(), context); } diff --git a/core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java index 6f302626280..365c1f9fe4c 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java @@ -25,6 +25,7 @@ import org.apache.lucene.search.SortField; import org.apache.lucene.search.join.BitSetProducer; import org.elasticsearch.action.support.ToXContentToBytes; import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.io.stream.NamedWriteable; import org.elasticsearch.common.lucene.search.Queries; import org.elasticsearch.common.xcontent.XContentParser; @@ -49,7 +50,11 @@ import static java.util.Collections.unmodifiableMap; public abstract class SortBuilder> extends ToXContentToBytes implements NamedWriteable { protected SortOrder order = SortOrder.ASC; + + // parse fields common to more than one SortBuilder public static final ParseField ORDER_FIELD = new ParseField("order"); + public static final ParseField NESTED_FILTER_FIELD = new ParseField("nested_filter"); + public static final ParseField NESTED_PATH_FIELD = new ParseField("nested_path"); private static final Map> PARSERS; static { @@ -196,6 +201,16 @@ public abstract class SortBuilder> extends ToXContentTo return nested; } + protected static QueryBuilder parseNestedFilter(XContentParser parser, QueryParseContext context) { + try { + QueryBuilder builder = context.parseInnerQueryBuilder().orElseThrow(() -> new ParsingException(parser.getTokenLocation(), + "Expected " + NESTED_FILTER_FIELD.getPreferredName() + " element.")); + return builder; + } catch (Exception e) { + throw new ParsingException(parser.getTokenLocation(), "Expected " + NESTED_FILTER_FIELD.getPreferredName() + " element.", e); + } + } + @FunctionalInterface private interface Parser> { T fromXContent(QueryParseContext context, String elementName) throws IOException; diff --git a/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java index d9b3aa376fa..ce5fcc470a3 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java @@ -41,10 +41,10 @@ import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.cache.bitset.BitsetFilterCache; import org.elasticsearch.index.fielddata.IndexFieldDataService; import org.elasticsearch.index.mapper.ContentPath; -import org.elasticsearch.index.mapper.MappedFieldType; -import org.elasticsearch.index.mapper.ObjectMapper; import org.elasticsearch.index.mapper.LegacyDoubleFieldMapper.DoubleFieldType; +import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper.BuilderContext; +import org.elasticsearch.index.mapper.ObjectMapper; import org.elasticsearch.index.mapper.ObjectMapper.Nested; import org.elasticsearch.index.query.IdsQueryBuilder; import org.elasticsearch.index.query.MatchAllQueryBuilder; @@ -245,7 +245,7 @@ public abstract class AbstractSortTestCase> extends EST @Override public ObjectMapper getObjectMapper(String name) { BuilderContext context = new BuilderContext(this.getIndexSettings().getSettings(), new ContentPath()); - return (ObjectMapper) new ObjectMapper.Builder<>(name).nested(Nested.newNested(false, false)).build(context); + return new ObjectMapper.Builder<>(name).nested(Nested.newNested(false, false)).build(context); } }; } diff --git a/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java index e96b02c0e2c..82d0de87801 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java @@ -22,7 +22,6 @@ package org.elasticsearch.search.sort; import org.apache.lucene.search.SortField; import org.elasticsearch.common.ParseFieldMatcher; -import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryParseContext; @@ -30,8 +29,6 @@ import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService.ScriptType; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.sort.ScriptSortBuilder.ScriptSortType; -import org.junit.Rule; -import org.junit.rules.ExpectedException; import java.io.IOException; import java.util.HashSet; @@ -146,19 +143,14 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase ScriptSortType.fromString(null)); + assertEquals("input string is null", e.getMessage()); } public void testScriptSortTypeIllegalArgument() { - exceptionRule.expect(IllegalArgumentException.class); - exceptionRule.expectMessage("Unknown ScriptSortType [xyz]"); - ScriptSortType.fromString("xyz"); + Exception e = expectThrows(IllegalArgumentException.class, () -> ScriptSortType.fromString("xyz")); + assertEquals("Unknown ScriptSortType [xyz]", e.getMessage()); } public void testParseJson() throws IOException { @@ -226,9 +218,8 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase ScriptSortBuilder.fromXContent(context, null)); + assertEquals("[_script] unknown field [bad_field], parser not found", e.getMessage()); } public void testParseBadFieldNameExceptionsOnStartObject() throws IOException { @@ -240,9 +231,8 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase ScriptSortBuilder.fromXContent(context, null)); + assertEquals("[_script] unknown field [bad_field], parser not found", e.getMessage()); } public void testParseUnexpectedToken() throws IOException { @@ -253,9 +243,8 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase ScriptSortBuilder.fromXContent(context, null)); + assertEquals("[_script] script doesn't support values of type: START_ARRAY", e.getMessage()); } /** @@ -263,9 +252,9 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase builder.sortMode(SortMode.fromString(sortMode))); + assertEquals("script sort of type [string] doesn't support mode [" + sortMode + "]", e.getMessage()); } @Override