From deb0991667815769d30cd27eaba0c8cb40c6e271 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 7 Jan 2020 11:57:41 -0500 Subject: [PATCH] Teach ObjectParser a happy pattern (#50691) (#50710) We *very* commonly have object with ctors like: ``` public Foo(String name) ``` And then declare a bunch of setters on the object. Every aggregation works like this, for example. This change teaches `ObjectParser` how to build these aggregations all on its own, without any help. This'll make it much cleaner to parse aggs, and, probably, a bunch of other things. It'll let us remove lots of wrapping. I've used this new power for the `avg` aggregation just to prove that it works outside of a unit test. --- .../common/xcontent/ObjectParser.java | 45 ++++++++++++------- .../common/xcontent/ObjectParserTests.java | 8 ++++ .../admin/indices/analyze/AnalyzeAction.java | 2 +- .../admin/indices/shrink/ResizeRequest.java | 2 +- .../metrics/AvgAggregationBuilder.java | 5 +-- .../search/rescore/QueryRescorerBuilder.java | 2 +- .../CompletionSuggestionBuilder.java | 2 +- .../context/CategoryQueryContext.java | 2 +- .../completion/context/GeoQueryContext.java | 2 +- 9 files changed, 44 insertions(+), 26 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java index 09a997e7a15..69a4a4bd31e 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java @@ -32,8 +32,10 @@ import java.util.Map; import java.util.function.BiConsumer; import java.util.function.BiFunction; import java.util.function.Consumer; +import java.util.function.Function; import java.util.function.Supplier; +import static java.util.Objects.requireNonNull; import static org.elasticsearch.common.xcontent.XContentParser.Token.START_ARRAY; import static org.elasticsearch.common.xcontent.XContentParser.Token.START_OBJECT; import static org.elasticsearch.common.xcontent.XContentParser.Token.VALUE_BOOLEAN; @@ -147,7 +149,7 @@ public final class ObjectParser extends AbstractObjectParser fieldParserMap = new HashMap<>(); private final String name; - private final Supplier valueSupplier; + private final Function valueBuilder; private final UnknownFieldParser unknownFieldParser; /** @@ -155,16 +157,27 @@ public final class ObjectParser extends AbstractObjectParser valueSupplier) { - this(name, errorOnUnknown(), valueSupplier); + this(name, errorOnUnknown(), c -> valueSupplier.get()); + } + + /** + * Creates a new ObjectParser. + * @param name the parsers name, used to reference the parser in exceptions and messages. + * @param valueBuilder A function that creates a new Value from the parse Context. Used + * when the parser is used as an inner object parser. + */ + public static ObjectParser fromBuilder(String name, Function valueBuilder) { + requireNonNull(valueBuilder, "Use the single argument ctor instead"); + return new ObjectParser(name, errorOnUnknown(), valueBuilder); } /** @@ -175,7 +188,7 @@ public final class ObjectParser extends AbstractObjectParser valueSupplier) { - this(name, ignoreUnknownFields ? ignoreUnknown() : errorOnUnknown(), valueSupplier); + this(name, ignoreUnknownFields ? ignoreUnknown() : errorOnUnknown(), c -> valueSupplier.get()); } /** @@ -185,7 +198,7 @@ public final class ObjectParser extends AbstractObjectParser unknownFieldConsumer, @Nullable Supplier valueSupplier) { - this(name, consumeUnknownField(unknownFieldConsumer), valueSupplier); + this(name, consumeUnknownField(unknownFieldConsumer), c -> valueSupplier.get()); } /** @@ -202,19 +215,20 @@ public final class ObjectParser extends AbstractObjectParser unknownFieldConsumer, @Nullable Supplier valueSupplier ) { - this(name, unknownIsNamedXContent(categoryClass, unknownFieldConsumer), valueSupplier); + this(name, unknownIsNamedXContent(categoryClass, unknownFieldConsumer), c -> valueSupplier.get()); } /** * Creates a new ObjectParser instance with a name. * @param name the parsers name, used to reference the parser in exceptions and messages. * @param unknownFieldParser how to parse unknown fields - * @param valueSupplier a supplier that creates a new Value instance used when the parser is used as an inner object parser. + * @param valueBuilder builds the value from the context. Used when the ObjectParser is not passed a value. */ - private ObjectParser(String name, UnknownFieldParser unknownFieldParser, @Nullable Supplier valueSupplier) { + private ObjectParser(String name, UnknownFieldParser unknownFieldParser, + @Nullable Function valueBuilder) { this.name = name; - this.valueSupplier = valueSupplier; this.unknownFieldParser = unknownFieldParser; + this.valueBuilder = valueBuilder; } /** @@ -226,10 +240,10 @@ public final class ObjectParser extends AbstractObjectParser extends AbstractObjectParser, String> parser = ObjectParser.fromBuilder("test", AtomicReference::new); + String context = randomAlphaOfLength(5); + AtomicReference parsed = parser.parse(createParser(JsonXContent.jsonXContent, "{}"), context); + assertThat(parsed.get(), equalTo(context)); + } } diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeAction.java index 51ac4bee892..41de8ce531b 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeAction.java @@ -264,7 +264,7 @@ public class AnalyzeAction extends ActionType { return request; } - private static final ObjectParser PARSER = new ObjectParser<>("analyze_request", null); + private static final ObjectParser PARSER = new ObjectParser<>("analyze_request"); static { PARSER.declareStringArray(Request::text, new ParseField("text")); PARSER.declareString(Request::analyzer, new ParseField("analyzer")); diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java b/server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java index 363986fb87b..f5a0810e516 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java @@ -45,7 +45,7 @@ import static org.elasticsearch.action.ValidateActions.addValidationError; */ public class ResizeRequest extends AcknowledgedRequest implements IndicesRequest, ToXContentObject { - public static final ObjectParser PARSER = new ObjectParser<>("resize_request", null); + public static final ObjectParser PARSER = new ObjectParser<>("resize_request"); static { PARSER.declareField((parser, request, context) -> request.getTargetIndexRequest().settings(parser.map()), new ParseField("settings"), ObjectParser.ValueType.OBJECT); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregationBuilder.java index 7bf6034473a..32e9aaf5e2b 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregationBuilder.java @@ -42,14 +42,13 @@ import java.util.Map; public class AvgAggregationBuilder extends ValuesSourceAggregationBuilder.LeafOnly { public static final String NAME = "avg"; - private static final ObjectParser PARSER; + private static final ObjectParser PARSER = ObjectParser.fromBuilder(NAME, AvgAggregationBuilder::new); static { - PARSER = new ObjectParser<>(AvgAggregationBuilder.NAME); ValuesSourceParserHelper.declareNumericFields(PARSER, true, true, false); } public static AggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { - return PARSER.parse(parser, new AvgAggregationBuilder(aggregationName), null); + return PARSER.parse(parser, aggregationName); } public AvgAggregationBuilder(String name) { diff --git a/server/src/main/java/org/elasticsearch/search/rescore/QueryRescorerBuilder.java b/server/src/main/java/org/elasticsearch/search/rescore/QueryRescorerBuilder.java index 02e748dfbc9..684b44a5ed7 100644 --- a/server/src/main/java/org/elasticsearch/search/rescore/QueryRescorerBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/rescore/QueryRescorerBuilder.java @@ -45,7 +45,7 @@ public class QueryRescorerBuilder extends RescorerBuilder private static final ParseField RESCORE_QUERY_WEIGHT_FIELD = new ParseField("rescore_query_weight"); private static final ParseField SCORE_MODE_FIELD = new ParseField("score_mode"); - private static final ObjectParser QUERY_RESCORE_PARSER = new ObjectParser<>(NAME, null); + private static final ObjectParser QUERY_RESCORE_PARSER = new ObjectParser<>(NAME); static { QUERY_RESCORE_PARSER.declareObject(InnerBuilder::setQueryBuilder, (p, c) -> { try { diff --git a/server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java b/server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java index 6c84379c3df..cfee70a3304 100644 --- a/server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java @@ -75,7 +75,7 @@ public class CompletionSuggestionBuilder extends SuggestionBuilder PARSER = new ObjectParser<>(SUGGESTION_NAME, null); + private static final ObjectParser PARSER = new ObjectParser<>(SUGGESTION_NAME); static { PARSER.declareField((parser, completionSuggestionContext, context) -> { if (parser.currentToken() == XContentParser.Token.VALUE_BOOLEAN) { diff --git a/server/src/main/java/org/elasticsearch/search/suggest/completion/context/CategoryQueryContext.java b/server/src/main/java/org/elasticsearch/search/suggest/completion/context/CategoryQueryContext.java index 9326e0ff666..e830377c4ce 100644 --- a/server/src/main/java/org/elasticsearch/search/suggest/completion/context/CategoryQueryContext.java +++ b/server/src/main/java/org/elasticsearch/search/suggest/completion/context/CategoryQueryContext.java @@ -95,7 +95,7 @@ public final class CategoryQueryContext implements ToXContentObject { return result; } - private static final ObjectParser CATEGORY_PARSER = new ObjectParser<>(NAME, null); + private static final ObjectParser CATEGORY_PARSER = new ObjectParser<>(NAME); static { CATEGORY_PARSER.declareField(Builder::setCategory, XContentParser::text, new ParseField(CONTEXT_VALUE), ObjectParser.ValueType.VALUE); diff --git a/server/src/main/java/org/elasticsearch/search/suggest/completion/context/GeoQueryContext.java b/server/src/main/java/org/elasticsearch/search/suggest/completion/context/GeoQueryContext.java index 439fa161d38..b2882e0e832 100644 --- a/server/src/main/java/org/elasticsearch/search/suggest/completion/context/GeoQueryContext.java +++ b/server/src/main/java/org/elasticsearch/search/suggest/completion/context/GeoQueryContext.java @@ -112,7 +112,7 @@ public final class GeoQueryContext implements ToXContentObject { return new Builder(); } - private static final ObjectParser GEO_CONTEXT_PARSER = new ObjectParser<>(NAME, null); + private static final ObjectParser GEO_CONTEXT_PARSER = new ObjectParser<>(NAME); static { GEO_CONTEXT_PARSER.declareField((parser, geoQueryContext, geoContextMapping) -> geoQueryContext.setGeoPoint(GeoUtils.parseGeoPoint(parser)),