From 109c6c271772c21659ef3ad8fe8d4250250ae923 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Fri, 22 Sep 2017 07:22:05 +0200 Subject: [PATCH] aggs: Do not delegate a null scorer to LeafBucketCollectors Closes #26611 --- .../aggregations/AggregatorFactory.java | 6 +- .../MultiBucketAggregatorWrapperTests.java | 93 +++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 core/src/test/java/org/elasticsearch/search/aggregations/MultiBucketAggregatorWrapperTests.java diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactory.java b/core/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactory.java index 4ea3f59a416..9a47635416a 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactory.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactory.java @@ -130,7 +130,11 @@ public abstract class AggregatorFactory> { aggregators.set(bucket, aggregator); } collector = aggregator.getLeafCollector(ctx); - collector.setScorer(scorer); + if (scorer != null) { + // Passing a null scorer can cause unexpected NPE at a later time, + // which can't not be directly linked to the fact that a null scorer has been supplied. + collector.setScorer(scorer); + } collectors.set(bucket, collector); } collector.collect(doc, 0); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/MultiBucketAggregatorWrapperTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/MultiBucketAggregatorWrapperTests.java new file mode 100644 index 00000000000..0a83b2ec5c7 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/search/aggregations/MultiBucketAggregatorWrapperTests.java @@ -0,0 +1,93 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.search.aggregations; + +import org.apache.lucene.analysis.MockAnalyzer; +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.index.memory.MemoryIndex; +import org.apache.lucene.search.Scorer; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.BigArrays; +import org.elasticsearch.common.util.MockBigArrays; +import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; +import org.elasticsearch.search.internal.SearchContext; +import org.elasticsearch.test.ESTestCase; + +import java.io.IOException; +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.same; +import static org.mockito.Mockito.verifyNoMoreInteractions; + +public class MultiBucketAggregatorWrapperTests extends ESTestCase { + + public void testNoNullScorerIsDelegated() throws Exception { + LeafReaderContext leafReaderContext = MemoryIndex.fromDocument(Collections.emptyList(), new MockAnalyzer(random())) + .createSearcher().getIndexReader().leaves().get(0); + BigArrays bigArrays = new MockBigArrays(Settings.EMPTY, new NoneCircuitBreakerService()); + SearchContext searchContext = mock(SearchContext.class); + when(searchContext.bigArrays()).thenReturn(bigArrays); + + Aggregator aggregator = mock(Aggregator.class); + AggregatorFactory aggregatorFactory = new TestAggregatorFactory(searchContext, aggregator); + LeafBucketCollector wrappedCollector = mock(LeafBucketCollector.class); + when(aggregator.getLeafCollector(leafReaderContext)).thenReturn(wrappedCollector); + Aggregator wrapper = AggregatorFactory.asMultiBucketAggregator(aggregatorFactory, searchContext, null); + + LeafBucketCollector collector = wrapper.getLeafCollector(leafReaderContext); + + collector.collect(0, 0); + // setScorer should not be invoked as it has not been set + // Only collect should be invoked: + verify(wrappedCollector).collect(0, 0); + verifyNoMoreInteractions(wrappedCollector); + + reset(wrappedCollector); + Scorer scorer = mock(Scorer.class); + collector.setScorer(scorer); + collector.collect(0, 1); + verify(wrappedCollector).setScorer(same(scorer)); + verify(wrappedCollector).collect(0, 0); + verifyNoMoreInteractions(wrappedCollector); + wrapper.close(); + } + + static class TestAggregatorFactory extends AggregatorFactory { + + private final Aggregator aggregator; + + TestAggregatorFactory(SearchContext context, Aggregator aggregator) throws IOException { + super("_name", context, null, new AggregatorFactories.Builder(), Collections.emptyMap()); + this.aggregator = aggregator; + } + + @Override + protected Aggregator createInternal(Aggregator parent, boolean collectsFromSingleBucket, List list, + Map metaData) throws IOException { + return aggregator; + } + } + +}