Skip to content

Commit

Permalink
Don't expand default_field in query_string before required
Browse files Browse the repository at this point in the history
Currently QueryStringQueryParser already checks if the field limit is breached
at construction time, which e.g. leads to errors if the default field is set to
"*" or the default is used and there are more fields than the limit, even if the
query itself does not use all these fields.
This change moves this check to happen after query parsing. QueryStringQueryParser now
keeps track of the fields that are actually resolved while parsing. The size of
that set is later used to check against the limit set by the
`indices.query.bool.max_clause_count` setting.

Closes elastic#53789
  • Loading branch information
Christoph Büscher committed Apr 14, 2020
1 parent 7375d85 commit 7710744
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 74 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public static Map<String, Float> resolveMappingFields(QueryShardContext context,
* The original name of the field is kept if adding the suffix to the field name does not point to a valid field
* in the mapping.
*/
public static Map<String, Float> resolveMappingFields(QueryShardContext context,
static Map<String, Float> resolveMappingFields(QueryShardContext context,
Map<String, Float> fieldsAndWeights,
String fieldSuffix) {
Map<String, Float> resolvedFields = new HashMap<>();
Expand All @@ -97,26 +97,10 @@ public static Map<String, Float> resolveMappingFields(QueryShardContext context,
resolvedFields.put(field.getKey(), boost);
}
}

checkForTooManyFields(resolvedFields, context);
checkForTooManyFields(resolvedFields.size(), context);
return resolvedFields;
}

/**
* Resolves the provided pattern or field name from the {@link QueryShardContext} and return a map of
* the expanded fields with their original boost.
* @param context The context of the query
* @param fieldOrPattern The field name or the pattern to resolve
* @param weight The weight for the field
* @param acceptAllTypes Whether all field type should be added when a pattern is expanded.
* If false, only searchable field types are added.
* @param acceptMetadataField Whether metadata fields should be added when a pattern is expanded.
*/
public static Map<String, Float> resolveMappingField(QueryShardContext context, String fieldOrPattern, float weight,
boolean acceptAllTypes, boolean acceptMetadataField) {
return resolveMappingField(context, fieldOrPattern, weight, acceptAllTypes, acceptMetadataField, null);
}

/**
* Resolves the provided pattern or field name from the {@link QueryShardContext} and return a map of
* the expanded fields with their original boost.
Expand All @@ -130,7 +114,7 @@ public static Map<String, Float> resolveMappingField(QueryShardContext context,
* The original name of the field is kept if adding the suffix to the field name does not point to a valid field
* in the mapping.
*/
public static Map<String, Float> resolveMappingField(QueryShardContext context, String fieldOrPattern, float weight,
static Map<String, Float> resolveMappingField(QueryShardContext context, String fieldOrPattern, float weight,
boolean acceptAllTypes, boolean acceptMetadataField, String fieldSuffix) {
Set<String> allFields = context.simpleMatchToIndexNames(fieldOrPattern);
Map<String, Float> fields = new HashMap<>();
Expand Down Expand Up @@ -170,15 +154,13 @@ public static Map<String, Float> resolveMappingField(QueryShardContext context,
float w = fields.getOrDefault(fieldName, 1.0F);
fields.put(fieldName, w * weight);
}

checkForTooManyFields(fields, context);
return fields;
}

private static void checkForTooManyFields(Map<String, Float> fields, QueryShardContext context) {
static void checkForTooManyFields(int numberOfFields, QueryShardContext context) {
Integer limit = SearchModule.INDICES_MAX_CLAUSE_COUNT_SETTING.get(context.getIndexSettings().getSettings());
if (fields.size() > limit) {
throw new IllegalArgumentException("field expansion matches too many fields, limit: " + limit + ", got: " + fields.size());
if (numberOfFields > limit) {
throw new IllegalArgumentException("field expansion matches too many fields, limit: " + limit + ", got: " + numberOfFields);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,15 @@
import java.time.ZoneId;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

import static org.elasticsearch.common.lucene.search.Queries.fixNegativeQueryIfNeeded;
import static org.elasticsearch.common.lucene.search.Queries.newLenientFieldQuery;
import static org.elasticsearch.common.lucene.search.Queries.newUnmappedFieldQuery;
import static org.elasticsearch.index.search.QueryParserHelper.checkForTooManyFields;
import static org.elasticsearch.index.search.QueryParserHelper.resolveMappingField;
import static org.elasticsearch.index.search.QueryParserHelper.resolveMappingFields;

Expand Down Expand Up @@ -97,6 +100,9 @@ public class QueryStringQueryParser extends XQueryParser {
private MultiTermQuery.RewriteMethod fuzzyRewriteMethod;
private boolean fuzzyTranspositions = FuzzyQuery.defaultTranspositions;

// set of field names this parser targets, used to check limits on expanded field names
private Set<String> queriedFields = new HashSet<>();

/**
* @param context The query shard context.
* @param defaultField The default field for query terms.
Expand Down Expand Up @@ -132,13 +138,13 @@ public QueryStringQueryParser(QueryShardContext context, Map<String, Float> fiel
}

/**
* Defaults to all queryiable fields extracted from the mapping for query terms
* Defaults to all queryable fields extracted from the mapping for query terms
* @param context The query shard context
* @param lenient If set to `true` will cause format based failures (like providing text to a numeric field) to be ignored.
*/
public QueryStringQueryParser(QueryShardContext context, boolean lenient) {
this(context, "*",
resolveMappingField(context, "*", 1.0f, false, false),
resolveMappingField(context, "*", 1.0f, false, false, null),
lenient, context.getMapperService().searchAnalyzer());
}

Expand Down Expand Up @@ -268,21 +274,24 @@ private Query applyBoost(Query q, Float boost) {
}

private Map<String, Float> extractMultiFields(String field, boolean quoted) {
Map<String, Float> extractedFields;
if (field != null) {
boolean allFields = Regex.isMatchAllPattern(field);
if (allFields && this.field != null && this.field.equals(field)) {
// "*" is the default field
return fieldsAndWeights;
extractedFields = fieldsAndWeights;
}
boolean multiFields = Regex.isSimpleMatchPattern(field);
// Filters unsupported fields if a pattern is requested
// Filters metadata fields if all fields are requested
return resolveMappingField(context, field, 1.0f, !allFields, !multiFields, quoted ? quoteFieldSuffix : null);
extractedFields = resolveMappingField(context, field, 1.0f, !allFields, !multiFields, quoted ? quoteFieldSuffix : null);
} else if (quoted && quoteFieldSuffix != null) {
return resolveMappingFields(context, fieldsAndWeights, quoteFieldSuffix);
extractedFields = resolveMappingFields(context, fieldsAndWeights, quoteFieldSuffix);
} else {
return fieldsAndWeights;
extractedFields = fieldsAndWeights;
}
this.queriedFields.addAll(extractedFields.keySet());
return extractedFields;
}

@Override
Expand Down Expand Up @@ -789,6 +798,8 @@ public Query parse(String query) throws ParseException {
if (query.trim().isEmpty()) {
return Queries.newMatchNoDocsQuery("Matching no documents because no terms present");
}
return super.parse(query);
Query result = super.parse(query);
checkForTooManyFields(this.queriedFields.size(), this.context);
return result;
}
}
103 changes: 71 additions & 32 deletions server/src/test/java/org/elasticsearch/search/query/QueryStringIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -263,38 +263,6 @@ public void testAllFieldsWithSpecifiedLeniency() throws IOException {
assertThat(e.getCause().getMessage(), containsString("unit [D] not supported for date math [-2D]"));
}

public void testLimitOnExpandedFields() throws Exception {
XContentBuilder builder = jsonBuilder();
builder.startObject();
builder.startObject("_doc");
builder.startObject("properties");
for (int i = 0; i < CLUSTER_MAX_CLAUSE_COUNT + 1; i++) {
builder.startObject("field" + i).field("type", "text").endObject();
}
builder.endObject(); // properties
builder.endObject(); // type1
builder.endObject();

assertAcked(prepareCreate("toomanyfields")
.setSettings(Settings.builder().put(MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(),
CLUSTER_MAX_CLAUSE_COUNT + 100))
.setMapping(builder));

client().prepareIndex("toomanyfields").setId("1").setSource("field1", "foo bar baz").get();
refresh();

Exception e = expectThrows(Exception.class, () -> {
QueryStringQueryBuilder qb = queryStringQuery("bar");
if (randomBoolean()) {
qb.defaultField("*");
}
client().prepareSearch("toomanyfields").setQuery(qb).get();
});
assertThat(ExceptionsHelper.unwrap(e, IllegalArgumentException.class).getMessage(),
containsString("field expansion matches too many fields, limit: " + CLUSTER_MAX_CLAUSE_COUNT + ", got: "
+ (CLUSTER_MAX_CLAUSE_COUNT + 1)));
}

// The only expectation for this test is to not throw exception
public void testLimitOnExpandedFieldsButIgnoreUnmappedFields() throws Exception {
XContentBuilder builder = jsonBuilder();
Expand Down Expand Up @@ -324,6 +292,77 @@ public void testLimitOnExpandedFieldsButIgnoreUnmappedFields() throws Exception
client().prepareSearch("ignoreunmappedfields").setQuery(qb).get();
}

public void testLimitOnExpandedFields() throws Exception {
XContentBuilder builder = jsonBuilder();
builder.startObject();
{
builder.startObject("_doc");
{
builder.startObject("properties");
{
for (int i = 0; i < CLUSTER_MAX_CLAUSE_COUNT; i++) {
String fieldName = (i % 2 == 0) ? "field.A" + i : "field.B" + i;
builder.startObject(fieldName).field("type", "text").endObject();
}
builder.startObject("field.C").field("type", "text").endObject();
builder.startObject("field.D").field("type", "text").endObject();
builder.endObject();
}
builder.endObject();
}
builder.endObject();
}

assertAcked(prepareCreate("testindex")
.setSettings(Settings.builder().put(MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(),
CLUSTER_MAX_CLAUSE_COUNT + 100))
.setMapping(builder));

client().prepareIndex("testindex").setId("1").setSource("field.A0", "foo bar baz").get();
refresh();

// single fields shouldn't trigger the limit
doAssertOneHitForQueryString("field.A0:foo");
doAssertOneHitForQueryString("field.A0:foo field.A2:bar");
// expanding to half of the limit should work
doAssertOneHitForQueryString("field.A\\*:foo");
// expanding to all fields inside the limit should still work
doAssertOneHitForQueryString("field.A\\*:foo field.B\\*:bar");
// adding a non-existing field on top shouldn't increase towards the limit
doAssertOneHitForQueryString("field.A\\*:foo field.B\\*:bar unmapped:something");

// the following should all exceed the limit
doAssertLimitExceededException("field.A\\*:foo field.B\\*:bar field.C:something", CLUSTER_MAX_CLAUSE_COUNT + 1);
doAssertLimitExceededException("field.A\\*:foo field.B\\*:bar field.C:something field.D:something", CLUSTER_MAX_CLAUSE_COUNT + 2);
doAssertLimitExceededException("field.A\\*:foo field.B\\*:bar field.C:something field.D:>=10", CLUSTER_MAX_CLAUSE_COUNT + 2);
doAssertLimitExceededException("field.A\\*:foo field.B\\*:bar something", CLUSTER_MAX_CLAUSE_COUNT +2);
doAssertLimitExceededException("*:something", CLUSTER_MAX_CLAUSE_COUNT + 2);
doAssertLimitExceededException("something", CLUSTER_MAX_CLAUSE_COUNT + 2);
}

private void doAssertOneHitForQueryString(String queryString) {
QueryStringQueryBuilder qb = queryStringQuery(queryString);
if (randomBoolean()) {
qb.defaultField("*");
}
SearchResponse response = client().prepareSearch("testindex").setQuery(qb).get();
assertHitCount(response, 1);
}

private void doAssertLimitExceededException(String queryString, int exceedingFieldCount) {
Exception e = expectThrows(Exception.class, () -> {
QueryStringQueryBuilder qb = queryStringQuery(queryString);
if (randomBoolean()) {
qb.defaultField("*");
}
client().prepareSearch("testindex").setQuery(qb).get();
});
assertThat(
ExceptionsHelper.unwrap(e, IllegalArgumentException.class).getMessage(),
containsString("field expansion matches too many fields, limit: " + CLUSTER_MAX_CLAUSE_COUNT + ", got: " + exceedingFieldCount)
);
}

public void testFieldAlias() throws Exception {
List<IndexRequestBuilder> indexRequests = new ArrayList<>();
indexRequests.add(client().prepareIndex("test").setId("1").setSource("f3", "text", "f2", "one"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.apache.lucene.analysis.TokenFilter;
import org.apache.lucene.analysis.TokenStream;
import org.apache.lucene.analysis.tokenattributes.CharTermAttribute;
import org.elasticsearch.ExceptionsHelper;
import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder;
import org.elasticsearch.action.index.IndexRequestBuilder;
import org.elasticsearch.action.search.SearchPhaseExecutionException;
Expand All @@ -36,7 +37,7 @@
import org.elasticsearch.index.query.BoolQueryBuilder;
import org.elasticsearch.index.query.Operator;
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.index.query.SimpleQueryStringBuilder;
import org.elasticsearch.index.query.QueryStringQueryBuilder;
import org.elasticsearch.index.query.SimpleQueryStringFlag;
import org.elasticsearch.plugins.AnalysisPlugin;
import org.elasticsearch.plugins.Plugin;
Expand Down Expand Up @@ -586,16 +587,20 @@ public void testLimitOnExpandedFields() throws Exception {
client().prepareIndex("toomanyfields").setId("1").setSource("field1", "foo bar baz").get();
refresh();

SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, () -> {
SimpleQueryStringBuilder qb = simpleQueryStringQuery("bar");
if (randomBoolean()) {
qb.field("*");
}
client().prepareSearch("toomanyfields").setQuery(qb).get();
});
assertThat(e.getDetailedMessage(),
containsString("field expansion matches too many fields, limit: " + CLUSTER_MAX_CLAUSE_COUNT + ", got: "
+ (CLUSTER_MAX_CLAUSE_COUNT + 1)));
doAssertLimitExceededException("*", CLUSTER_MAX_CLAUSE_COUNT + 1);
doAssertLimitExceededException("field*", CLUSTER_MAX_CLAUSE_COUNT + 1);
}

private void doAssertLimitExceededException(String field, int exceedingFieldCount) {
Exception e = expectThrows(Exception.class, () -> {
QueryStringQueryBuilder qb = queryStringQuery("bar");
qb.field(field);
client().prepareSearch("toomanyfields").setQuery(qb).get();
});
assertThat(
ExceptionsHelper.unwrap(e, IllegalArgumentException.class).getMessage(),
containsString("field expansion matches too many fields, limit: " + CLUSTER_MAX_CLAUSE_COUNT + ", got: " + exceedingFieldCount)
);
}

public void testFieldAlias() throws Exception {
Expand Down

0 comments on commit 7710744

Please sign in to comment.