Browse Source

Teach ObjectParser a happy pattern (#50691)

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.
Nik Everett 5 năm trước cách đây
mục cha
commit
792b5e150b

+ 28 - 17
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<Value, Context> extends AbstractObjectParser<Val
 
     private final Map<String, FieldParser> fieldParserMap = new HashMap<>();
     private final String name;
-    private final Supplier<Value> valueSupplier;
+    private final Function<Context, Value> valueBuilder;
     private final UnknownFieldParser<Value, Context> unknownFieldParser;
 
     /**
@@ -155,16 +157,27 @@ public final class ObjectParser<Value, Context> extends AbstractObjectParser<Val
      * @param name the parsers name, used to reference the parser in exceptions and messages.
      */
     public ObjectParser(String name) {
-        this(name, null);
+        this(name, errorOnUnknown(), null);
     }
 
     /**
      * Creates a new ObjectParser.
      * @param name the parsers name, used to reference the parser in exceptions and messages.
-     * @param valueSupplier a supplier that creates a new Value instance used when the parser is used as an inner object parser.
+     * @param valueSupplier A supplier that creates a new Value instance. Used when the parser is used as an inner object parser.
      */
     public ObjectParser(String name, @Nullable Supplier<Value> 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 <Value, Context> ObjectParser<Value, Context> fromBuilder(String name, Function<Context, Value> valueBuilder) {
+        requireNonNull(valueBuilder, "Use the single argument ctor instead");
+        return new ObjectParser<Value, Context>(name, errorOnUnknown(), valueBuilder);
     }
 
     /**
@@ -175,7 +188,7 @@ public final class ObjectParser<Value, Context> extends AbstractObjectParser<Val
      * @param valueSupplier a supplier that creates a new Value instance used when the parser is used as an inner object parser.
      */
     public ObjectParser(String name, boolean ignoreUnknownFields, @Nullable Supplier<Value> valueSupplier) {
-        this(name, ignoreUnknownFields ? ignoreUnknown() : errorOnUnknown(), valueSupplier);
+        this(name, ignoreUnknownFields ? ignoreUnknown() : errorOnUnknown(), c -> valueSupplier.get());
     }
 
     /**
@@ -185,7 +198,7 @@ public final class ObjectParser<Value, Context> extends AbstractObjectParser<Val
      * @param valueSupplier a supplier that creates a new Value instance used when the parser is used as an inner object parser.
      */
     public ObjectParser(String name, UnknownFieldConsumer<Value> unknownFieldConsumer, @Nullable Supplier<Value> valueSupplier) {
-        this(name, consumeUnknownField(unknownFieldConsumer), valueSupplier);
+        this(name, consumeUnknownField(unknownFieldConsumer), c -> valueSupplier.get());
     }
 
     /**
@@ -202,19 +215,20 @@ public final class ObjectParser<Value, Context> extends AbstractObjectParser<Val
         BiConsumer<Value, C> unknownFieldConsumer,
         @Nullable Supplier<Value> 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<Value, Context> unknownFieldParser, @Nullable Supplier<Value> valueSupplier) {
+    private ObjectParser(String name, UnknownFieldParser<Value, Context> unknownFieldParser,
+                @Nullable Function<Context, Value> valueBuilder) {
         this.name = name;
-        this.valueSupplier = valueSupplier;
         this.unknownFieldParser = unknownFieldParser;
+        this.valueBuilder = valueBuilder;
     }
 
     /**
@@ -226,10 +240,10 @@ public final class ObjectParser<Value, Context> extends AbstractObjectParser<Val
      */
     @Override
     public Value parse(XContentParser parser, Context context) throws IOException {
-        if (valueSupplier == null) {
-            throw new NullPointerException("valueSupplier is not set");
+        if (valueBuilder == null) {
+            throw new NullPointerException("valueBuilder is not set");
         }
-        return parse(parser, valueSupplier.get(), context);
+        return parse(parser, valueBuilder.apply(context), context);
     }
 
     /**
@@ -277,11 +291,8 @@ public final class ObjectParser<Value, Context> extends AbstractObjectParser<Val
 
     @Override
     public Value apply(XContentParser parser, Context context) {
-        if (valueSupplier == null) {
-            throw new NullPointerException("valueSupplier is not set");
-        }
         try {
-            return parse(parser, valueSupplier.get(), context);
+            return parse(parser, context);
         } catch (IOException e) {
             throw new XContentParseException(parser.getTokenLocation(), "[" + name  + "] failed to parse object", e);
         }

+ 8 - 0
libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java

@@ -39,6 +39,7 @@ import java.util.Map;
 import java.util.concurrent.atomic.AtomicReference;
 
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 
 public class ObjectParserTests extends ESTestCase {
@@ -805,4 +806,11 @@ public class ObjectParserTests extends ESTestCase {
             assertEquals("[1:2] [test] unable to parse Object with name [not_supported_field]: parser not found", ex.getMessage());
         }
     }
+
+    public void testContextBuilder() throws IOException {
+        ObjectParser<AtomicReference<String>, String> parser = ObjectParser.fromBuilder("test", AtomicReference::new);
+        String context = randomAlphaOfLength(5);
+        AtomicReference<String> parsed = parser.parse(createParser(JsonXContent.jsonXContent, "{}"), context);
+        assertThat(parsed.get(), equalTo(context));
+    }
 }

+ 1 - 1
server/src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeAction.java

@@ -264,7 +264,7 @@ public class AnalyzeAction extends ActionType<AnalyzeAction.Response> {
             return request;
         }
 
-        private static final ObjectParser<Request, Void> PARSER = new ObjectParser<>("analyze_request", null);
+        private static final ObjectParser<Request, Void> PARSER = new ObjectParser<>("analyze_request");
         static {
             PARSER.declareStringArray(Request::text, new ParseField("text"));
             PARSER.declareString(Request::analyzer, new ParseField("analyzer"));

+ 1 - 1
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<ResizeRequest> implements IndicesRequest, ToXContentObject {
 
-    public static final ObjectParser<ResizeRequest, Void> PARSER = new ObjectParser<>("resize_request", null);
+    public static final ObjectParser<ResizeRequest, Void> PARSER = new ObjectParser<>("resize_request");
     static {
         PARSER.declareField((parser, request, context) -> request.getTargetIndexRequest().settings(parser.map()),
             new ParseField("settings"), ObjectParser.ValueType.OBJECT);

+ 2 - 3
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<ValuesSource.Numeric, AvgAggregationBuilder> {
     public static final String NAME = "avg";
 
-    private static final ObjectParser<AvgAggregationBuilder, Void> PARSER;
+    private static final ObjectParser<AvgAggregationBuilder, String> 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) {

+ 1 - 1
server/src/main/java/org/elasticsearch/search/rescore/QueryRescorerBuilder.java

@@ -45,7 +45,7 @@ public class QueryRescorerBuilder extends RescorerBuilder<QueryRescorerBuilder>
     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<InnerBuilder, Void> QUERY_RESCORE_PARSER = new ObjectParser<>(NAME, null);
+    private static final ObjectParser<InnerBuilder, Void> QUERY_RESCORE_PARSER = new ObjectParser<>(NAME);
     static {
         QUERY_RESCORE_PARSER.declareObject(InnerBuilder::setQueryBuilder, (p, c) -> {
             try {

+ 1 - 1
server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java

@@ -74,7 +74,7 @@ public class CompletionSuggestionBuilder extends SuggestionBuilder<CompletionSug
      *     "payload" : STRING_ARRAY
      * }
      */
-    private static final ObjectParser<CompletionSuggestionBuilder.InnerBuilder, Void> PARSER = new ObjectParser<>(SUGGESTION_NAME, null);
+    private static final ObjectParser<CompletionSuggestionBuilder.InnerBuilder, Void> PARSER = new ObjectParser<>(SUGGESTION_NAME);
     static {
         PARSER.declareField((parser, completionSuggestionContext, context) -> {
                 if (parser.currentToken() == XContentParser.Token.VALUE_BOOLEAN) {

+ 1 - 1
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<Builder, Void> CATEGORY_PARSER = new ObjectParser<>(NAME, null);
+    private static final ObjectParser<Builder, Void> CATEGORY_PARSER = new ObjectParser<>(NAME);
     static {
         CATEGORY_PARSER.declareField(Builder::setCategory, XContentParser::text, new ParseField(CONTEXT_VALUE),
                 ObjectParser.ValueType.VALUE);

+ 1 - 1
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<GeoQueryContext.Builder, Void> GEO_CONTEXT_PARSER = new ObjectParser<>(NAME, null);
+    private static final ObjectParser<GeoQueryContext.Builder, Void> GEO_CONTEXT_PARSER = new ObjectParser<>(NAME);
     static {
         GEO_CONTEXT_PARSER.declareField((parser, geoQueryContext,
                 geoContextMapping) -> geoQueryContext.setGeoPoint(GeoUtils.parseGeoPoint(parser)),