Removing handling of null lucene query where we catch this at parse time

This commit is contained in:
Christoph Büscher 2016-05-25 11:18:40 +02:00
parent 359f45988f
commit cb145aec68
10 changed files with 11 additions and 46 deletions

View File

@ -50,7 +50,6 @@ import org.elasticsearch.index.engine.EngineFactory;
import org.elasticsearch.index.fielddata.IndexFieldDataCache; import org.elasticsearch.index.fielddata.IndexFieldDataCache;
import org.elasticsearch.index.fielddata.IndexFieldDataService; import org.elasticsearch.index.fielddata.IndexFieldDataService;
import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.query.ParsedQuery;
import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.index.shard.IndexEventListener; import org.elasticsearch.index.shard.IndexEventListener;
@ -604,11 +603,10 @@ public final class IndexService extends AbstractIndexComponent implements IndexC
byte[] filterSource = alias.filter().uncompressed(); byte[] filterSource = alias.filter().uncompressed();
try (XContentParser parser = XContentFactory.xContent(filterSource).createParser(filterSource)) { try (XContentParser parser = XContentFactory.xContent(filterSource).createParser(filterSource)) {
Optional<QueryBuilder> innerQueryBuilder = shardContext.newParseContext(parser).parseInnerQueryBuilder(); Optional<QueryBuilder> innerQueryBuilder = shardContext.newParseContext(parser).parseInnerQueryBuilder();
ParsedQuery parsedFilter = null;
if (innerQueryBuilder.isPresent()) { if (innerQueryBuilder.isPresent()) {
parsedFilter = shardContext.toFilter(innerQueryBuilder.get()); return shardContext.toFilter(innerQueryBuilder.get()).query();
} }
return parsedFilter == null ? null : parsedFilter.query(); return null;
} }
} catch (IOException ex) { } catch (IOException ex) {
throw new AliasFilterParsingException(shardContext.index(), alias.getAlias(), "Invalid alias filter", ex); throw new AliasFilterParsingException(shardContext.index(), alias.getAlias(), "Invalid alias filter", ex);

View File

@ -441,9 +441,7 @@ public class BoolQueryBuilder extends AbstractQueryBuilder<BoolQueryBuilder> {
luceneQuery = query.toFilter(context); luceneQuery = query.toFilter(context);
break; break;
} }
if (luceneQuery != null) { booleanQueryBuilder.add(new BooleanClause(luceneQuery, occurs));
booleanQueryBuilder.add(new BooleanClause(luceneQuery, occurs));
}
} }
} }

View File

@ -208,12 +208,6 @@ public class BoostingQueryBuilder extends AbstractQueryBuilder<BoostingQueryBuil
protected Query doToQuery(QueryShardContext context) throws IOException { protected Query doToQuery(QueryShardContext context) throws IOException {
Query positive = positiveQuery.toQuery(context); Query positive = positiveQuery.toQuery(context);
Query negative = negativeQuery.toQuery(context); Query negative = negativeQuery.toQuery(context);
// make upstream queries ignore this query by returning `null`
// if either inner query builder returns null
if (positive == null || negative == null) {
return null;
}
return new BoostingQuery(positive, negative, negativeBoost); return new BoostingQuery(positive, negative, negativeBoost);
} }

View File

@ -146,10 +146,6 @@ public class ConstantScoreQueryBuilder extends AbstractQueryBuilder<ConstantScor
@Override @Override
protected Query doToQuery(QueryShardContext context) throws IOException { protected Query doToQuery(QueryShardContext context) throws IOException {
Query innerFilter = filterBuilder.toFilter(context); Query innerFilter = filterBuilder.toFilter(context);
if (innerFilter == null ) {
// return null so that parent queries (e.g. bool) also ignore this
return null;
}
return new ConstantScoreQuery(innerFilter); return new ConstantScoreQuery(innerFilter);
} }

View File

@ -168,8 +168,8 @@ public class DisMaxQueryBuilder extends AbstractQueryBuilder<DisMaxQueryBuilder>
} }
} }
if (!queriesFound) { if (!queriesFound || queries.isEmpty()) {
throw new ParsingException(parser.getTokenLocation(), "[dis_max] requires 'queries' field"); throw new ParsingException(parser.getTokenLocation(), "[dis_max] requires 'queries' field with at least one clause");
} }
DisMaxQueryBuilder disMaxQuery = new DisMaxQueryBuilder(); DisMaxQueryBuilder disMaxQuery = new DisMaxQueryBuilder();
@ -186,10 +186,6 @@ public class DisMaxQueryBuilder extends AbstractQueryBuilder<DisMaxQueryBuilder>
protected Query doToQuery(QueryShardContext context) throws IOException { protected Query doToQuery(QueryShardContext context) throws IOException {
// return null if there are no queries at all // return null if there are no queries at all
Collection<Query> luceneQueries = toQueries(queries, context); Collection<Query> luceneQueries = toQueries(queries, context);
if (luceneQueries.isEmpty()) {
return null;
}
return new DisjunctionMaxQuery(luceneQueries, tieBreaker); return new DisjunctionMaxQuery(luceneQueries, tieBreaker);
} }

View File

@ -330,9 +330,6 @@ public class HasChildQueryBuilder extends AbstractQueryBuilder<HasChildQueryBuil
context.setTypes(previousTypes); context.setTypes(previousTypes);
} }
if (innerQuery == null) {
return null;
}
DocumentMapper childDocMapper = context.getMapperService().documentMapper(type); DocumentMapper childDocMapper = context.getMapperService().documentMapper(type);
if (childDocMapper == null) { if (childDocMapper == null) {
if (ignoreUnmapped) { if (ignoreUnmapped) {

View File

@ -162,9 +162,6 @@ public class HasParentQueryBuilder extends AbstractQueryBuilder<HasParentQueryBu
context.setTypes(previousTypes); context.setTypes(previousTypes);
} }
if (innerQuery == null) {
return null;
}
DocumentMapper parentDocMapper = context.getMapperService().documentMapper(type); DocumentMapper parentDocMapper = context.getMapperService().documentMapper(type);
if (parentDocMapper == null) { if (parentDocMapper == null) {
if (ignoreUnmapped) { if (ignoreUnmapped) {

View File

@ -255,9 +255,6 @@ public class NestedQueryBuilder extends AbstractQueryBuilder<NestedQueryBuilder>
try { try {
context.nestedScope().nextLevel(nestedObjectMapper); context.nestedScope().nextLevel(nestedObjectMapper);
innerQuery = this.query.toQuery(context); innerQuery = this.query.toQuery(context);
if (innerQuery == null) {
return null;
}
} finally { } finally {
context.nestedScope().previousLevel(); context.nestedScope().previousLevel();
} }

View File

@ -20,10 +20,10 @@
package org.elasticsearch.search.aggregations.bucket.filter; package org.elasticsearch.search.aggregations.bucket.filter;
import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.index.query.MatchAllQueryBuilder;
import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.QueryParseContext; import org.elasticsearch.index.query.QueryParseContext;
import org.elasticsearch.search.aggregations.AbstractAggregationBuilder; import org.elasticsearch.search.aggregations.AbstractAggregationBuilder;
@ -84,8 +84,7 @@ public class FilterAggregationBuilder extends AbstractAggregationBuilder<FilterA
} }
public static FilterAggregationBuilder parse(String aggregationName, QueryParseContext context) throws IOException { public static FilterAggregationBuilder parse(String aggregationName, QueryParseContext context) throws IOException {
QueryBuilder filter = context.parseInnerQueryBuilder().orElseThrow(() -> new ParsingException(context.parser().getTokenLocation(), QueryBuilder filter = context.parseInnerQueryBuilder().orElse(new MatchAllQueryBuilder());
"filter cannot be null in filter aggregation [{}]", aggregationName));
return new FilterAggregationBuilder(aggregationName, filter); return new FilterAggregationBuilder(aggregationName, filter);
} }

View File

@ -25,6 +25,7 @@ import org.apache.lucene.search.DisjunctionMaxQuery;
import org.apache.lucene.search.PrefixQuery; import org.apache.lucene.search.PrefixQuery;
import org.apache.lucene.search.Query; import org.apache.lucene.search.Query;
import org.elasticsearch.common.ParseFieldMatcher; import org.elasticsearch.common.ParseFieldMatcher;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.test.AbstractQueryTestCase; import org.elasticsearch.test.AbstractQueryTestCase;
import java.io.IOException; import java.io.IOException;
@ -88,25 +89,17 @@ public class DisMaxQueryBuilderTests extends AbstractQueryTestCase<DisMaxQueryBu
return alternateVersions; return alternateVersions;
} }
/**
* test `null`return value for missing inner queries
*/
public void testNoInnerQueries() throws IOException {
DisMaxQueryBuilder disMaxBuilder = new DisMaxQueryBuilder();
assertNull(disMaxBuilder.toQuery(createShardContext()));
}
/** /**
* Test with empty inner query body, this should be ignored upstream. * Test with empty inner query body, this should be ignored upstream.
* To test this, we use inner {@link ConstantScoreQueryBuilder} with empty inner filter. * To test this, we use inner {@link ConstantScoreQueryBuilder} with empty inner filter.
*/ */
public void testInnerQueryEmptyIgnored() throws IOException { public void testInnerQueryEmptyException() throws IOException {
String queryString = "{ \"" + DisMaxQueryBuilder.NAME + "\" :" String queryString = "{ \"" + DisMaxQueryBuilder.NAME + "\" :"
+ " { \"queries\" : [ {\"" + ConstantScoreQueryBuilder.NAME + "\" : { \"filter\" : { } } } ] " + " { \"queries\" : [ {\"" + ConstantScoreQueryBuilder.NAME + "\" : { \"filter\" : { } } } ] "
+ " }" + " }"
+ " }"; + " }";
DisMaxQueryBuilder builder = (DisMaxQueryBuilder) parseQuery(queryString, ParseFieldMatcher.EMPTY); ParsingException ex = expectThrows(ParsingException.class, () -> parseQuery(queryString, ParseFieldMatcher.EMPTY));
assertTrue("the inner query has an empty body, so it should be ignored", builder.innerQueries().isEmpty()); assertEquals("[dis_max] requires 'queries' field with at least one clause", ex.getMessage());
} }
public void testIllegalArguments() { public void testIllegalArguments() {