Skip to content

Commit c392895

Browse files
nik9000nik9000
committed
Replace AggregatorParsers with namedObject (#22397)
Removes `AggregatorParsers`, replacing all of its functionality with `XContentParser#namedObject`. This is the third bit of payoff from #22003, one less thing to pass around the entire application.
1 parent 7a5974f commit c392895

File tree

29 files changed

+523
-657
lines changed

29 files changed

+523
-657
lines changed

‎core/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,7 @@ public static MultiSearchRequest parseRequest(RestRequest restRequest, boolean a
9292
parseMultiLineRequest(restRequest, multiRequest.indicesOptions(), allowExplicitIndex, (searchRequest, parser) -> {
9393
try {
9494
final QueryParseContext queryParseContext = new QueryParseContext(parser, parseFieldMatcher);
95-
searchRequest.source(SearchSourceBuilder.fromXContent(queryParseContext,
96-
searchRequestParsers.aggParsers, searchRequestParsers.suggesters));
95+
searchRequest.source(SearchSourceBuilder.fromXContent(queryParseContext, searchRequestParsers.suggesters));
9796
multiRequest.add(searchRequest);
9897
} catch (IOException e) {
9998
throw new ElasticsearchParseException("Exception when parsing search request", e);

‎core/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ public static void parseSearchRequest(SearchRequest searchRequest, RestRequest r
9696
searchRequest.indices(Strings.splitStringByCommaToArray(request.param("index")));
9797
if (requestContentParser != null) {
9898
QueryParseContext context = new QueryParseContext(requestContentParser, parseFieldMatcher);
99-
searchRequest.source().parseXContent(context, searchRequestParsers.aggParsers, searchRequestParsers.suggesters);
99+
searchRequest.source().parseXContent(context, searchRequestParsers.suggesters);
100100
}
101101

102102
// do not allow 'query_and_fetch' or 'dfs_query_and_fetch' search types

‎core/src/main/java/org/elasticsearch/search/SearchModule.java‎

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,8 @@
9797
import org.elasticsearch.plugins.SearchPlugin.SearchExtSpec;
9898
import org.elasticsearch.plugins.SearchPlugin.SearchExtensionSpec;
9999
import org.elasticsearch.search.aggregations.AggregationBuilder;
100-
import org.elasticsearch.search.aggregations.Aggregator;
101-
import org.elasticsearch.search.aggregations.AggregatorParsers;
100+
import org.elasticsearch.search.aggregations.AggregatorFactories;
101+
import org.elasticsearch.search.aggregations.BaseAggregationBuilder;
102102
import org.elasticsearch.search.aggregations.InternalAggregation;
103103
import org.elasticsearch.search.aggregations.PipelineAggregationBuilder;
104104
import org.elasticsearch.search.aggregations.bucket.children.ChildrenAggregationBuilder;
@@ -272,10 +272,6 @@ public class SearchModule {
272272
private final boolean transportClient;
273273
private final Map<String, Highlighter> highlighters;
274274
private final Map<String, Suggester<?>> suggesters;
275-
private final ParseFieldRegistry<Aggregator.Parser> aggregationParserRegistry = new ParseFieldRegistry<>("aggregation");
276-
private final ParseFieldRegistry<PipelineAggregator.Parser> pipelineAggregationParserRegistry = new ParseFieldRegistry<>(
277-
"pipline_aggregation");
278-
private final AggregatorParsers aggregatorParsers = new AggregatorParsers(aggregationParserRegistry, pipelineAggregationParserRegistry);
279275
private final ParseFieldRegistry<SignificanceHeuristicParser> significanceHeuristicParserRegistry = new ParseFieldRegistry<>(
280276
"significance_heuristic");
281277
private final ParseFieldRegistry<MovAvgModel.AbstractModelParser> movingAverageModelParserRegistry = new ParseFieldRegistry<>(
@@ -305,7 +301,7 @@ public SearchModule(Settings settings, boolean transportClient, List<SearchPlugi
305301
registerFetchSubPhases(plugins);
306302
registerSearchExts(plugins);
307303
registerShapes();
308-
searchRequestParsers = new SearchRequestParsers(aggregatorParsers, getSuggesters());
304+
searchRequestParsers = new SearchRequestParsers(getSuggesters());
309305
}
310306

311307
public List<NamedWriteableRegistry.Entry> getNamedWriteables() {
@@ -345,13 +341,6 @@ public ParseFieldRegistry<MovAvgModel.AbstractModelParser> getMovingAverageModel
345341
return movingAverageModelParserRegistry;
346342
}
347343

348-
/**
349-
* Parsers for {@link AggregationBuilder}s and {@link PipelineAggregationBuilder}s.
350-
*/
351-
public AggregatorParsers getAggregatorParsers() {
352-
return aggregatorParsers;
353-
}
354-
355344
private void registerAggregations(List<SearchPlugin> plugins) {
356345
registerAggregation(new AggregationSpec(AvgAggregationBuilder.NAME, AvgAggregationBuilder::new, AvgAggregationBuilder::parse)
357346
.addResultReader(InternalAvg::new));
@@ -437,7 +426,10 @@ private void registerAggregations(List<SearchPlugin> plugins) {
437426

438427
private void registerAggregation(AggregationSpec spec) {
439428
if (false == transportClient) {
440-
aggregationParserRegistry.register(spec.getParser(), spec.getName());
429+
namedXContents.add(new NamedXContentRegistry.Entry(BaseAggregationBuilder.class, spec.getName(), (p, c) -> {
430+
AggregatorFactories.AggParseContext context = (AggregatorFactories.AggParseContext) c;
431+
return spec.getParser().parse(context.name, context.queryParseContext);
432+
}));
441433
}
442434
namedWriteables.add(
443435
new NamedWriteableRegistry.Entry(AggregationBuilder.class, spec.getName().getPreferredName(), spec.getReader()));
@@ -531,7 +523,10 @@ private void registerPipelineAggregations(List<SearchPlugin> plugins) {
531523

532524
private void registerPipelineAggregation(PipelineAggregationSpec spec) {
533525
if (false == transportClient) {
534-
pipelineAggregationParserRegistry.register(spec.getParser(), spec.getName());
526+
namedXContents.add(new NamedXContentRegistry.Entry(BaseAggregationBuilder.class, spec.getName(), (p, c) -> {
527+
AggregatorFactories.AggParseContext context = (AggregatorFactories.AggParseContext) c;
528+
return spec.getParser().parse(context.name, context.queryParseContext);
529+
}));
535530
}
536531
namedWriteables.add(
537532
new NamedWriteableRegistry.Entry(PipelineAggregationBuilder.class, spec.getName().getPreferredName(), spec.getReader()));

‎core/src/main/java/org/elasticsearch/search/SearchRequestParsers.java‎

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,6 @@
1919

2020
package org.elasticsearch.search;
2121

22-
import org.elasticsearch.index.query.QueryParseContext;
23-
import org.elasticsearch.search.aggregations.AggregatorParsers;
2422
import org.elasticsearch.search.suggest.Suggesters;
2523

2624
/**
@@ -32,25 +30,14 @@ public class SearchRequestParsers {
3230
// methods split across RestSearchAction and SearchSourceBuilder should be moved here
3331
// TODO: make all members private once parsing functions are moved here
3432

35-
// TODO: AggregatorParsers should be removed and the underlying maps of agg
36-
// and pipeline agg parsers should be here
37-
/**
38-
* Agg and pipeline agg parsers that may be used in search requests.
39-
* @see org.elasticsearch.search.builder.SearchSourceBuilder#fromXContent(QueryParseContext, AggregatorParsers,
40-
* Suggesters)
41-
*/
42-
public final AggregatorParsers aggParsers;
43-
4433
// TODO: Suggesters should be removed and the underlying map moved here
4534
/**
4635
* Suggesters that may be used in search requests.
47-
* @see org.elasticsearch.search.builder.SearchSourceBuilder#fromXContent(QueryParseContext, AggregatorParsers,
48-
* Suggesters)
36+
* @see org.elasticsearch.search.builder.SearchSourceBuilder#fromXContent(QueryParseContext, Suggesters)
4937
*/
5038
public final Suggesters suggesters;
5139

52-
public SearchRequestParsers(AggregatorParsers aggParsers, Suggesters suggesters) {
53-
this.aggParsers = aggParsers;
40+
public SearchRequestParsers(Suggesters suggesters) {
5441
this.suggesters = suggesters;
5542
}
5643
}

‎core/src/main/java/org/elasticsearch/search/aggregations/AbstractAggregationBuilder.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@ public AB setMetaData(Map<String, Object> metaData) {
117117
return (AB) this;
118118
}
119119

120+
@Override
120121
public String getType() {
121122
return type.name();
122123
}

‎core/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import org.elasticsearch.common.ParseField;
2424
import org.elasticsearch.common.io.stream.NamedWriteable;
2525
import org.elasticsearch.common.xcontent.ToXContent;
26+
import org.elasticsearch.index.query.QueryParseContext;
2627
import org.elasticsearch.search.aggregations.InternalAggregation.Type;
2728
import org.elasticsearch.search.internal.SearchContext;
2829

@@ -34,7 +35,7 @@
3435
*/
3536
public abstract class AggregationBuilder
3637
extends ToXContentToBytes
37-
implements NamedWriteable, ToXContent {
38+
implements NamedWriteable, ToXContent, BaseAggregationBuilder {
3839

3940
protected final String name;
4041
protected final Type type;
@@ -66,6 +67,7 @@ public String getName() {
6667
protected abstract AggregatorFactory<?> build(SearchContext context, AggregatorFactory<?> parent) throws IOException;
6768

6869
/** Associate metadata with this {@link AggregationBuilder}. */
70+
@Override
6971
public abstract AggregationBuilder setMetaData(Map<String, Object> metaData);
7072

7173
/** Add a sub aggregation to this builder. */
@@ -77,13 +79,14 @@ public String getName() {
7779
/**
7880
* Internal: Registers sub-factories with this factory. The sub-factory will be
7981
* responsible for the creation of sub-aggregators under the aggregator
80-
* created by this factory. This is only for use by {@link AggregatorParsers}.
82+
* created by this factory. This is only for use by {@link AggregatorFactories#parseAggregators(QueryParseContext)}.
8183
*
8284
* @param subFactories
8385
* The sub-factories
8486
* @return this factory (fluent interface)
8587
*/
86-
protected abstract AggregationBuilder subAggregations(AggregatorFactories.Builder subFactories);
88+
@Override
89+
public abstract AggregationBuilder subAggregations(AggregatorFactories.Builder subFactories);
8790

8891
/** Common xcontent fields shared among aggregator builders */
8992
public static final class CommonFields extends ParseField.CommonFields {

‎core/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,13 @@
1919
package org.elasticsearch.search.aggregations;
2020

2121
import org.elasticsearch.action.support.ToXContentToBytes;
22+
import org.elasticsearch.common.ParsingException;
2223
import org.elasticsearch.common.io.stream.StreamInput;
2324
import org.elasticsearch.common.io.stream.StreamOutput;
2425
import org.elasticsearch.common.io.stream.Writeable;
2526
import org.elasticsearch.common.xcontent.XContentBuilder;
27+
import org.elasticsearch.common.xcontent.XContentParser;
28+
import org.elasticsearch.index.query.QueryParseContext;
2629
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
2730
import org.elasticsearch.search.aggregations.support.AggregationPath;
2831
import org.elasticsearch.search.aggregations.support.AggregationPath.PathElement;
@@ -40,11 +43,129 @@
4043
import java.util.Map;
4144
import java.util.Objects;
4245
import java.util.Set;
46+
import java.util.regex.Matcher;
47+
import java.util.regex.Pattern;
4348

4449
/**
4550
*
4651
*/
4752
public class AggregatorFactories {
53+
public static final Pattern VALID_AGG_NAME = Pattern.compile("[^\\[\\]>]+");
54+
55+
/**
56+
* Parses the aggregation request recursively generating aggregator factories in turn.
57+
*
58+
* @param parseContext The parse context.
59+
*
60+
* @return The parsed aggregator factories.
61+
*
62+
* @throws IOException When parsing fails for unknown reasons.
63+
*/
64+
public static AggregatorFactories.Builder parseAggregators(QueryParseContext parseContext) throws IOException {
65+
return parseAggregators(parseContext, 0);
66+
}
67+
68+
private static AggregatorFactories.Builder parseAggregators(QueryParseContext parseContext, int level) throws IOException {
69+
Matcher validAggMatcher = VALID_AGG_NAME.matcher("");
70+
AggregatorFactories.Builder factories = new AggregatorFactories.Builder();
71+
72+
XContentParser.Token token = null;
73+
XContentParser parser = parseContext.parser();
74+
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
75+
if (token != XContentParser.Token.FIELD_NAME) {
76+
throw new ParsingException(parser.getTokenLocation(),
77+
"Unexpected token " + token + " in [aggs]: aggregations definitions must start with the name of the aggregation.");
78+
}
79+
final String aggregationName = parser.currentName();
80+
if (!validAggMatcher.reset(aggregationName).matches()) {
81+
throw new ParsingException(parser.getTokenLocation(), "Invalid aggregation name [" + aggregationName
82+
+ "]. Aggregation names must be alpha-numeric and can only contain '_' and '-'");
83+
}
84+
85+
token = parser.nextToken();
86+
if (token != XContentParser.Token.START_OBJECT) {
87+
throw new ParsingException(parser.getTokenLocation(), "Aggregation definition for [" + aggregationName + " starts with a ["
88+
+ token + "], expected a [" + XContentParser.Token.START_OBJECT + "].");
89+
}
90+
91+
BaseAggregationBuilder aggBuilder = null;
92+
AggregatorFactories.Builder subFactories = null;
93+
94+
Map<String, Object> metaData = null;
95+
96+
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
97+
if (token != XContentParser.Token.FIELD_NAME) {
98+
throw new ParsingException(
99+
parser.getTokenLocation(), "Expected [" + XContentParser.Token.FIELD_NAME + "] under a ["
100+
+ XContentParser.Token.START_OBJECT + "], but got a [" + token + "] in [" + aggregationName + "]",
101+
parser.getTokenLocation());
102+
}
103+
final String fieldName = parser.currentName();
104+
105+
token = parser.nextToken();
106+
if (token == XContentParser.Token.START_OBJECT) {
107+
switch (fieldName) {
108+
case "meta":
109+
metaData = parser.map();
110+
break;
111+
case "aggregations":
112+
case "aggs":
113+
if (subFactories != null) {
114+
throw new ParsingException(parser.getTokenLocation(),
115+
"Found two sub aggregation definitions under [" + aggregationName + "]");
116+
}
117+
subFactories = parseAggregators(parseContext, level + 1);
118+
break;
119+
default:
120+
if (aggBuilder != null) {
121+
throw new ParsingException(parser.getTokenLocation(), "Found two aggregation type definitions in ["
122+
+ aggregationName + "]: [" + aggBuilder.getType() + "] and [" + fieldName + "]");
123+
}
124+
125+
aggBuilder = parser.namedObject(BaseAggregationBuilder.class, fieldName,
126+
new AggParseContext(aggregationName, parseContext));
127+
}
128+
} else {
129+
throw new ParsingException(parser.getTokenLocation(), "Expected [" + XContentParser.Token.START_OBJECT + "] under ["
130+
+ fieldName + "], but got a [" + token + "] in [" + aggregationName + "]");
131+
}
132+
}
133+
134+
if (aggBuilder == null) {
135+
throw new ParsingException(parser.getTokenLocation(), "Missing definition for aggregation [" + aggregationName + "]",
136+
parser.getTokenLocation());
137+
} else {
138+
if (metaData != null) {
139+
aggBuilder.setMetaData(metaData);
140+
}
141+
142+
if (subFactories != null) {
143+
aggBuilder.subAggregations(subFactories);
144+
}
145+
146+
if (aggBuilder instanceof AggregationBuilder) {
147+
factories.addAggregator((AggregationBuilder) aggBuilder);
148+
} else {
149+
factories.addPipelineAggregator((PipelineAggregationBuilder) aggBuilder);
150+
}
151+
}
152+
}
153+
154+
return factories;
155+
}
156+
157+
/**
158+
* Context to parse and aggregation. This should eventually be removed and replaced with a String.
159+
*/
160+
public static final class AggParseContext {
161+
public final String name;
162+
public final QueryParseContext queryParseContext;
163+
164+
public AggParseContext(String name, QueryParseContext queryParseContext) {
165+
this.name = name;
166+
this.queryParseContext = queryParseContext;
167+
}
168+
}
48169

49170
public static final AggregatorFactories EMPTY = new AggregatorFactories(null, new AggregatorFactory<?>[0],
50171
new ArrayList<PipelineAggregationBuilder>());

0 commit comments

Comments
 (0)