Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Aggregations parsing is too lenient. #5837

Closed
wants to merge 1 commit into from
Closed

Aggregations parsing is too lenient. #5837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
93 changes: 53 additions & 40 deletions src/main/java/org/elasticsearch/search/aggregations/AggregatorParsers.java
View file
Open in desktop
Expand Up @@ -79,58 +79,71 @@ public AggregatorFactories parseAggregators(XContentParser parser, SearchContext


private AggregatorFactories parseAggregators(XContentParser parser, SearchContext context, int level) throws IOException {
XContentParser.Token token = null;
String currentFieldName = null;

Matcher validAggMatcher = VALID_AGG_NAME.matcher("");

AggregatorFactories.Builder factories = new AggregatorFactories.Builder();

String aggregationName = null;
XContentParser.Token token = null;
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
aggregationName = parser.currentName();
} else if (token == XContentParser.Token.START_OBJECT) {
String aggregatorType = null;
AggregatorFactory factory = null;
AggregatorFactories subFactories = null;
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
} else if (token == XContentParser.Token.START_OBJECT) {
if ("aggregations".equals(currentFieldName) || "aggs".equals(currentFieldName)) {
subFactories = parseAggregators(parser, context, level+1);
} else if (aggregatorType != null) {
throw new SearchParseException(context, "Found two aggregation type definitions in [" + aggregationName + "]: [" + aggregatorType + "] and [" + currentFieldName + "]. Only one type is allowed.");
} else {
aggregatorType = currentFieldName;
Aggregator.Parser aggregatorParser = parser(aggregatorType);
if (aggregatorParser == null) {
throw new SearchParseException(context, "Could not find aggregator type [" + currentFieldName + "]");
}
if (!validAggMatcher.reset(aggregationName).matches()) {
throw new SearchParseException(context, "Invalid aggregation name [" + aggregationName + "]. Aggregation names must be alpha-numeric and can only contain '_' and '-'");
}
factory = aggregatorParser.parse(aggregationName, parser, context);
}
}
}
if (token != XContentParser.Token.FIELD_NAME) {
throw new SearchParseException(context, "Unexpected token " + token + " in [aggs]: aggregations definitions must start with the name of the aggregation.");
}
final String aggregationName = parser.currentName();
if (!validAggMatcher.reset(aggregationName).matches()) {
throw new SearchParseException(context, "Invalid aggregation name [" + aggregationName + "]. Aggregation names must be alpha-numeric and can only contain '_' and '-'");
}

token = parser.nextToken();
if (token != XContentParser.Token.START_OBJECT) {
throw new SearchParseException(context, "Aggregation definition for [" + aggregationName + " starts with a [" + token + "], expected a [" + XContentParser.Token.START_OBJECT + "].");
}

if (factory == null) {
// skipping the aggregation
continue;
AggregatorFactory factory = null;
AggregatorFactories subFactories = null;

while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token != XContentParser.Token.FIELD_NAME) {
throw new SearchParseException(context, "Expected [" + XContentParser.Token.FIELD_NAME + "] under a [" + XContentParser.Token.START_OBJECT + "], but got a [" + token + "] in [" + aggregationName + "]");
}
final String fieldName = parser.currentName();

if (subFactories != null) {
factory.subFactories(subFactories);
token = parser.nextToken();
if (token != XContentParser.Token.START_OBJECT) {
throw new SearchParseException(context, "Expected [" + XContentParser.Token.START_OBJECT + "] under [" + fieldName + "], but got a [" + token + "] in [" + aggregationName + "]");
}

if (level == 0) {
factory.validate();
switch (fieldName) {
case "aggregations":
case "aggs":
if (subFactories != null) {
throw new SearchParseException(context, "Found two sub aggregation definitions under [" + aggregationName + "]");
}
subFactories = parseAggregators(parser, context, level+1);
break;
default:
if (factory != null) {
throw new SearchParseException(context, "Found two aggregation type definitions in [" + aggregationName + "]: [" + factory.type + "] and [" + fieldName + "]");
}
Aggregator.Parser aggregatorParser = parser(fieldName);
if (aggregatorParser == null) {
throw new SearchParseException(context, "Could not find aggregator type [" + fieldName + "] in [" + aggregationName + "]");
}
factory = aggregatorParser.parse(aggregationName, parser, context);
}
}

factories.add(factory);
if (factory == null) {
throw new SearchParseException(context, "Missing definition for aggregation [" + aggregationName + "]");
}

if (subFactories != null) {
factory.subFactories(subFactories);
}

if (level == 0) {
factory.validate();
}

factories.add(factory);
}

return factories.build();
Expand Down
25 changes: 25 additions & 0 deletions src/test/java/org/elasticsearch/search/aggregations/ParsingTests.java
View file
Open in desktop
Expand Up @@ -33,6 +33,7 @@ public class ParsingTests extends ElasticsearchIntegrationTest {
@Test(expected=SearchPhaseExecutionException.class)
public void testTwoTypes() throws Exception {
createIndex("idx");
ensureGreen();
client().prepareSearch("idx").setAggregations(JsonXContent.contentBuilder()
.startObject()
.startObject("in_stock")
Expand Down Expand Up @@ -69,6 +70,7 @@ public void testInvalidAggregationName() throws Exception {
}

createIndex("idx");
ensureGreen();
client().prepareSearch("idx").setAggregations(JsonXContent.contentBuilder()
.startObject()
.startObject(name)
Expand All @@ -81,4 +83,27 @@ public void testInvalidAggregationName() throws Exception {
.endObject()
.endObject()).execute().actionGet();
}

@Test(expected=SearchPhaseExecutionException.class)
public void testMissingName() throws Exception {
createIndex("idx");
ensureGreen();
client().prepareSearch("idx").setAggregations(JsonXContent.contentBuilder()
.startObject()
.startObject("by_date")
.startObject("date_histogram")
.field("field", "timestamp")
.field("interval", "month")
.endObject()
.startObject("aggs")
// the aggregation name is missing
//.startObject("tag_count")
.startObject("cardinality")
.field("field", "tag")
.endObject()
//s.endObject()
.endObject()
.endObject()).execute().actionGet();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also add a test for:

  • agg type is missing
  • 2 agg types definitions
  • 2 "aggs" definitions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a test for "2 agg types", I'll add the 2 other ideas.

}