Skip to content

Commit deb0991

Browse files
authored
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.
1 parent 8009b07 commit deb0991

File tree

9 files changed

+44
-26
lines changed

9 files changed

+44
-26
lines changed

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

Lines changed: 28 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,10 @@
3232
import java.util.function.BiConsumer;
3333
import java.util.function.BiFunction;
3434
import java.util.function.Consumer;
35+
import java.util.function.Function;
3536
import java.util.function.Supplier;
3637

38+
import static java.util.Objects.requireNonNull;
3739
import static org.elasticsearch.common.xcontent.XContentParser.Token.START_ARRAY;
3840
import static org.elasticsearch.common.xcontent.XContentParser.Token.START_OBJECT;
3941
import static org.elasticsearch.common.xcontent.XContentParser.Token.VALUE_BOOLEAN;
@@ -147,24 +149,35 @@ private static <Value, Category, Context> UnknownFieldParser<Value, Context> unk
147149

148150
private final Map<String, FieldParser> fieldParserMap = new HashMap<>();
149151
private final String name;
150-
private final Supplier<Value> valueSupplier;
152+
private final Function<Context, Value> valueBuilder;
151153
private final UnknownFieldParser<Value, Context> unknownFieldParser;
152154

153155
/**
154156
* Creates a new ObjectParser.
155157
* @param name the parsers name, used to reference the parser in exceptions and messages.
156158
*/
157159
public ObjectParser(String name) {
158-
this(name, null);
160+
this(name, errorOnUnknown(), null);
159161
}
160162

161163
/**
162164
* Creates a new ObjectParser.
163165
* @param name the parsers name, used to reference the parser in exceptions and messages.
164-
* @param valueSupplier a supplier that creates a new Value instance used when the parser is used as an inner object parser.
166+
* @param valueSupplier A supplier that creates a new Value instance. Used when the parser is used as an inner object parser.
165167
*/
166168
public ObjectParser(String name, @Nullable Supplier<Value> valueSupplier) {
167-
this(name, errorOnUnknown(), valueSupplier);
169+
this(name, errorOnUnknown(), c -> valueSupplier.get());
170+
}
171+
172+
/**
173+
* Creates a new ObjectParser.
174+
* @param name the parsers name, used to reference the parser in exceptions and messages.
175+
* @param valueBuilder A function that creates a new Value from the parse Context. Used
176+
* when the parser is used as an inner object parser.
177+
*/
178+
public static <Value, Context> ObjectParser<Value, Context> fromBuilder(String name, Function<Context, Value> valueBuilder) {
179+
requireNonNull(valueBuilder, "Use the single argument ctor instead");
180+
return new ObjectParser<Value, Context>(name, errorOnUnknown(), valueBuilder);
168181
}
169182

170183
/**
@@ -175,7 +188,7 @@ public ObjectParser(String name, @Nullable Supplier<Value> valueSupplier) {
175188
* @param valueSupplier a supplier that creates a new Value instance used when the parser is used as an inner object parser.
176189
*/
177190
public ObjectParser(String name, boolean ignoreUnknownFields, @Nullable Supplier<Value> valueSupplier) {
178-
this(name, ignoreUnknownFields ? ignoreUnknown() : errorOnUnknown(), valueSupplier);
191+
this(name, ignoreUnknownFields ? ignoreUnknown() : errorOnUnknown(), c -> valueSupplier.get());
179192
}
180193

181194
/**
@@ -185,7 +198,7 @@ public ObjectParser(String name, boolean ignoreUnknownFields, @Nullable Supplier
185198
* @param valueSupplier a supplier that creates a new Value instance used when the parser is used as an inner object parser.
186199
*/
187200
public ObjectParser(String name, UnknownFieldConsumer<Value> unknownFieldConsumer, @Nullable Supplier<Value> valueSupplier) {
188-
this(name, consumeUnknownField(unknownFieldConsumer), valueSupplier);
201+
this(name, consumeUnknownField(unknownFieldConsumer), c -> valueSupplier.get());
189202
}
190203

191204
/**
@@ -202,19 +215,20 @@ public <C> ObjectParser(
202215
BiConsumer<Value, C> unknownFieldConsumer,
203216
@Nullable Supplier<Value> valueSupplier
204217
) {
205-
this(name, unknownIsNamedXContent(categoryClass, unknownFieldConsumer), valueSupplier);
218+
this(name, unknownIsNamedXContent(categoryClass, unknownFieldConsumer), c -> valueSupplier.get());
206219
}
207220

208221
/**
209222
* Creates a new ObjectParser instance with a name.
210223
* @param name the parsers name, used to reference the parser in exceptions and messages.
211224
* @param unknownFieldParser how to parse unknown fields
212-
* @param valueSupplier a supplier that creates a new Value instance used when the parser is used as an inner object parser.
225+
* @param valueBuilder builds the value from the context. Used when the ObjectParser is not passed a value.
213226
*/
214-
private ObjectParser(String name, UnknownFieldParser<Value, Context> unknownFieldParser, @Nullable Supplier<Value> valueSupplier) {
227+
private ObjectParser(String name, UnknownFieldParser<Value, Context> unknownFieldParser,
228+
@Nullable Function<Context, Value> valueBuilder) {
215229
this.name = name;
216-
this.valueSupplier = valueSupplier;
217230
this.unknownFieldParser = unknownFieldParser;
231+
this.valueBuilder = valueBuilder;
218232
}
219233

220234
/**
@@ -226,10 +240,10 @@ private ObjectParser(String name, UnknownFieldParser<Value, Context> unknownFiel
226240
*/
227241
@Override
228242
public Value parse(XContentParser parser, Context context) throws IOException {
229-
if (valueSupplier == null) {
230-
throw new NullPointerException("valueSupplier is not set");
243+
if (valueBuilder == null) {
244+
throw new NullPointerException("valueBuilder is not set");
231245
}
232-
return parse(parser, valueSupplier.get(), context);
246+
return parse(parser, valueBuilder.apply(context), context);
233247
}
234248

235249
/**
@@ -277,11 +291,8 @@ public Value parse(XContentParser parser, Value value, Context context) throws I
277291

278292
@Override
279293
public Value apply(XContentParser parser, Context context) {
280-
if (valueSupplier == null) {
281-
throw new NullPointerException("valueSupplier is not set");
282-
}
283294
try {
284-
return parse(parser, valueSupplier.get(), context);
295+
return parse(parser, context);
285296
} catch (IOException e) {
286297
throw new XContentParseException(parser.getTokenLocation(), "[" + name + "] failed to parse object", e);
287298
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import java.util.concurrent.atomic.AtomicReference;
4040

4141
import static org.hamcrest.Matchers.containsString;
42+
import static org.hamcrest.Matchers.equalTo;
4243
import static org.hamcrest.Matchers.hasSize;
4344
import static org.hamcrest.Matchers.instanceOf;
4445

@@ -806,4 +807,11 @@ public void testTopLevelNamedXContent() throws IOException {
806807
assertEquals("[1:2] [test] unable to parse Object with name [not_supported_field]: parser not found", ex.getMessage());
807808
}
808809
}
810+
811+
public void testContextBuilder() throws IOException {
812+
ObjectParser<AtomicReference<String>, String> parser = ObjectParser.fromBuilder("test", AtomicReference::new);
813+
String context = randomAlphaOfLength(5);
814+
AtomicReference<String> parsed = parser.parse(createParser(JsonXContent.jsonXContent, "{}"), context);
815+
assertThat(parsed.get(), equalTo(context));
816+
}
809817
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ public static Request fromXContent(XContentParser parser, String index) throws I
264264
return request;
265265
}
266266

267-
private static final ObjectParser<Request, Void> PARSER = new ObjectParser<>("analyze_request", null);
267+
private static final ObjectParser<Request, Void> PARSER = new ObjectParser<>("analyze_request");
268268
static {
269269
PARSER.declareStringArray(Request::text, new ParseField("text"));
270270
PARSER.declareString(Request::analyzer, new ParseField("analyzer"));

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@
4545
*/
4646
public class ResizeRequest extends AcknowledgedRequest<ResizeRequest> implements IndicesRequest, ToXContentObject {
4747

48-
public static final ObjectParser<ResizeRequest, Void> PARSER = new ObjectParser<>("resize_request", null);
48+
public static final ObjectParser<ResizeRequest, Void> PARSER = new ObjectParser<>("resize_request");
4949
static {
5050
PARSER.declareField((parser, request, context) -> request.getTargetIndexRequest().settings(parser.map()),
5151
new ParseField("settings"), ObjectParser.ValueType.OBJECT);

server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregationBuilder.java

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,14 +42,13 @@
4242
public class AvgAggregationBuilder extends ValuesSourceAggregationBuilder.LeafOnly<ValuesSource.Numeric, AvgAggregationBuilder> {
4343
public static final String NAME = "avg";
4444

45-
private static final ObjectParser<AvgAggregationBuilder, Void> PARSER;
45+
private static final ObjectParser<AvgAggregationBuilder, String> PARSER = ObjectParser.fromBuilder(NAME, AvgAggregationBuilder::new);
4646
static {
47-
PARSER = new ObjectParser<>(AvgAggregationBuilder.NAME);
4847
ValuesSourceParserHelper.declareNumericFields(PARSER, true, true, false);
4948
}
5049

5150
public static AggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException {
52-
return PARSER.parse(parser, new AvgAggregationBuilder(aggregationName), null);
51+
return PARSER.parse(parser, aggregationName);
5352
}
5453

5554
public AvgAggregationBuilder(String name) {

server/src/main/java/org/elasticsearch/search/rescore/QueryRescorerBuilder.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ public class QueryRescorerBuilder extends RescorerBuilder<QueryRescorerBuilder>
4545
private static final ParseField RESCORE_QUERY_WEIGHT_FIELD = new ParseField("rescore_query_weight");
4646
private static final ParseField SCORE_MODE_FIELD = new ParseField("score_mode");
4747

48-
private static final ObjectParser<InnerBuilder, Void> QUERY_RESCORE_PARSER = new ObjectParser<>(NAME, null);
48+
private static final ObjectParser<InnerBuilder, Void> QUERY_RESCORE_PARSER = new ObjectParser<>(NAME);
4949
static {
5050
QUERY_RESCORE_PARSER.declareObject(InnerBuilder::setQueryBuilder, (p, c) -> {
5151
try {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public class CompletionSuggestionBuilder extends SuggestionBuilder<CompletionSug
7575
* "payload" : STRING_ARRAY
7676
* }
7777
*/
78-
private static final ObjectParser<CompletionSuggestionBuilder.InnerBuilder, Void> PARSER = new ObjectParser<>(SUGGESTION_NAME, null);
78+
private static final ObjectParser<CompletionSuggestionBuilder.InnerBuilder, Void> PARSER = new ObjectParser<>(SUGGESTION_NAME);
7979
static {
8080
PARSER.declareField((parser, completionSuggestionContext, context) -> {
8181
if (parser.currentToken() == XContentParser.Token.VALUE_BOOLEAN) {

server/src/main/java/org/elasticsearch/search/suggest/completion/context/CategoryQueryContext.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ public int hashCode() {
9595
return result;
9696
}
9797

98-
private static final ObjectParser<Builder, Void> CATEGORY_PARSER = new ObjectParser<>(NAME, null);
98+
private static final ObjectParser<Builder, Void> CATEGORY_PARSER = new ObjectParser<>(NAME);
9999
static {
100100
CATEGORY_PARSER.declareField(Builder::setCategory, XContentParser::text, new ParseField(CONTEXT_VALUE),
101101
ObjectParser.ValueType.VALUE);

server/src/main/java/org/elasticsearch/search/suggest/completion/context/GeoQueryContext.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ public static Builder builder() {
112112
return new Builder();
113113
}
114114

115-
private static final ObjectParser<GeoQueryContext.Builder, Void> GEO_CONTEXT_PARSER = new ObjectParser<>(NAME, null);
115+
private static final ObjectParser<GeoQueryContext.Builder, Void> GEO_CONTEXT_PARSER = new ObjectParser<>(NAME);
116116
static {
117117
GEO_CONTEXT_PARSER.declareField((parser, geoQueryContext,
118118
geoContextMapping) -> geoQueryContext.setGeoPoint(GeoUtils.parseGeoPoint(parser)),

0 commit comments

Comments
 (0)