From c685caf729c0092b8adedbbf49500051d4386d0b Mon Sep 17 00:00:00 2001 From: jimczi Date: Mon, 18 Feb 2019 20:53:14 +0100 Subject: [PATCH 1/4] Fix IndexSearcherWrapper visibility This change adds a wrapper for IndexSearcher that makes IndexSearcher#search(List, Weight, Collector) visible by sub-classes. The wrapper is used by the ContextIndexSearcher to call this protected method on a searcher created by a plugin. This ensures that an override of the protected method in an IndexSearcherWrapper plugin is called when a search is executed. Closes #30758 --- .../apache/lucene/search/XIndexSearcher.java | 27 ++++++++++ .../search/internal/ContextIndexSearcher.java | 10 ++-- .../shard/IndexSearcherWrapperTests.java | 53 +++++++++++++++++++ 3 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 server/src/main/java/org/apache/lucene/search/XIndexSearcher.java diff --git a/server/src/main/java/org/apache/lucene/search/XIndexSearcher.java b/server/src/main/java/org/apache/lucene/search/XIndexSearcher.java new file mode 100644 index 0000000000000..9a5e687007f49 --- /dev/null +++ b/server/src/main/java/org/apache/lucene/search/XIndexSearcher.java @@ -0,0 +1,27 @@ +package org.apache.lucene.search; + +import org.apache.lucene.index.LeafReaderContext; + +import java.io.IOException; +import java.util.List; + +/** + * A wrapper for {@link IndexSearcher} that makes {@link IndexSearcher#search(List, Weight, Collector)} + * visible by sub-classes. + */ +public class XIndexSearcher extends IndexSearcher { + private final IndexSearcher in; + + public XIndexSearcher(IndexSearcher in, QueryCache queryCache, QueryCachingPolicy queryCachingPolicy) { + super(in.getIndexReader()); + this.in = in; + setSimilarity(in.getSimilarity()); + setQueryCache(queryCache); + setQueryCachingPolicy(queryCachingPolicy); + } + + @Override + public void search(List leaves, Weight weight, Collector collector) throws IOException { + in.search(leaves, weight, collector); + } +} diff --git a/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java b/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java index 8e7c2ef013a43..96cb378fec6de 100644 --- a/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java +++ b/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java @@ -35,6 +35,7 @@ import org.apache.lucene.search.Scorer; import org.apache.lucene.search.TermStatistics; import org.apache.lucene.search.Weight; +import org.apache.lucene.search.XIndexSearcher; import org.elasticsearch.common.lease.Releasable; import org.elasticsearch.index.engine.Engine; import org.elasticsearch.search.dfs.AggregatedDfs; @@ -56,7 +57,7 @@ public class ContextIndexSearcher extends IndexSearcher implements Releasable { /** The wrapped {@link IndexSearcher}. The reason why we sometimes prefer delegating to this searcher instead of {@code super} is that * this instance may have more assertions, for example if it comes from MockInternalEngine which wraps the IndexSearcher into an * AssertingIndexSearcher. */ - private final IndexSearcher in; + private final XIndexSearcher in; private AggregatedDfs aggregatedDfs; @@ -67,11 +68,10 @@ public class ContextIndexSearcher extends IndexSearcher implements Releasable { private Runnable checkCancelled; - public ContextIndexSearcher(Engine.Searcher searcher, - QueryCache queryCache, QueryCachingPolicy queryCachingPolicy) { + public ContextIndexSearcher(Engine.Searcher searcher, QueryCache queryCache, QueryCachingPolicy queryCachingPolicy) { super(searcher.reader()); - in = searcher.searcher(); engineSearcher = searcher; + in = new XIndexSearcher(searcher.searcher(), queryCache, queryCachingPolicy); setSimilarity(searcher.searcher().getSimilarity()); setQueryCache(queryCache); setQueryCachingPolicy(queryCachingPolicy); @@ -174,7 +174,7 @@ public BulkScorer bulkScorer(LeafReaderContext context) throws IOException { } else { cancellableWeight = weight; } - super.search(leaves, cancellableWeight, collector); + in.search(leaves, cancellableWeight, collector); } @Override diff --git a/server/src/test/java/org/elasticsearch/index/shard/IndexSearcherWrapperTests.java b/server/src/test/java/org/elasticsearch/index/shard/IndexSearcherWrapperTests.java index 3ba62647b6be2..dc84f008884e8 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexSearcherWrapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexSearcherWrapperTests.java @@ -28,23 +28,32 @@ import org.apache.lucene.index.IndexWriter; import org.apache.lucene.index.IndexWriterConfig; import org.apache.lucene.index.LeafReader; +import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.index.Term; +import org.apache.lucene.search.Collector; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.TermQuery; import org.apache.lucene.search.TopDocs; +import org.apache.lucene.search.TotalHitCountCollector; +import org.apache.lucene.search.Weight; +import org.apache.lucene.search.XIndexSearcher; import org.apache.lucene.store.Directory; import org.elasticsearch.core.internal.io.IOUtils; import org.elasticsearch.common.lucene.index.ElasticsearchDirectoryReader; import org.elasticsearch.index.engine.Engine; import org.elasticsearch.index.engine.EngineException; +import org.elasticsearch.search.internal.ContextIndexSearcher; import org.elasticsearch.test.ESTestCase; import java.io.IOException; import java.util.Collections; +import java.util.List; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import static org.hamcrest.Matchers.equalTo; + public class IndexSearcherWrapperTests extends ESTestCase { public void testReaderCloseListenerIsCalled() throws IOException { @@ -159,6 +168,50 @@ public void testNoWrap() throws IOException { IOUtils.close(writer, dir); } + public void testWrapVisibility() throws IOException { + Directory dir = newDirectory(); + IndexWriterConfig iwc = newIndexWriterConfig(); + IndexWriter writer = new IndexWriter(dir, iwc); + Document doc = new Document(); + doc.add(new StringField("id", "1", random().nextBoolean() ? Field.Store.YES : Field.Store.NO)); + doc.add(new TextField("field", "doc", random().nextBoolean() ? Field.Store.YES : Field.Store.NO)); + writer.addDocument(doc); + DirectoryReader open = ElasticsearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "_na_", 1)); + IndexSearcher searcher = new IndexSearcher(open); + assertEquals(1, searcher.search(new TermQuery(new Term("field", "doc")), 1).totalHits.value); + IndexSearcherWrapper wrapper = new IndexSearcherWrapper() { + @Override + public DirectoryReader wrap(DirectoryReader reader) throws IOException { + return reader; + } + + @Override + public IndexSearcher wrap(IndexSearcher searcher) throws EngineException { + return new IndexSearcher(searcher.getIndexReader()) { + @Override + protected void search(List leaves, Weight weight, Collector collector) throws IOException { + throw new IllegalStateException("boum"); + } + }; + } + + }; + final AtomicBoolean closeCalled = new AtomicBoolean(false); + final Engine.Searcher wrap = wrapper.wrap(new Engine.Searcher("foo", searcher, () -> closeCalled.set(true))); + assertEquals(1, wrap.reader().getRefCount()); + ContextIndexSearcher contextSearcher = new ContextIndexSearcher(wrap, wrap.searcher().getQueryCache(), + wrap.searcher().getQueryCachingPolicy()); + IllegalStateException exc = expectThrows(IllegalStateException.class, + () -> contextSearcher.search(new TermQuery(new Term("field", "doc")), new TotalHitCountCollector())); + assertThat(exc.getMessage(), equalTo("boum")); + wrap.close(); + assertFalse("wrapped reader is closed", wrap.reader().tryIncRef()); + assertTrue(closeCalled.get()); + + IOUtils.close(open, writer, dir); + assertEquals(0, open.getRefCount()); + } + private static class FieldMaskingReader extends FilterDirectoryReader { private final String field; private final AtomicInteger closeCalls; From 60ae41feac664bcf0bf6446c06edcfa6d33f411d Mon Sep 17 00:00:00 2001 From: jimczi Date: Mon, 18 Feb 2019 21:54:02 +0100 Subject: [PATCH 2/4] fix style --- .../org/elasticsearch/index/shard/IndexSearcherWrapperTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/index/shard/IndexSearcherWrapperTests.java b/server/src/test/java/org/elasticsearch/index/shard/IndexSearcherWrapperTests.java index dc84f008884e8..7a422e82c2202 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexSearcherWrapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexSearcherWrapperTests.java @@ -36,7 +36,6 @@ import org.apache.lucene.search.TopDocs; import org.apache.lucene.search.TotalHitCountCollector; import org.apache.lucene.search.Weight; -import org.apache.lucene.search.XIndexSearcher; import org.apache.lucene.store.Directory; import org.elasticsearch.core.internal.io.IOUtils; import org.elasticsearch.common.lucene.index.ElasticsearchDirectoryReader; From 19e9361ba88176cac51668f9b66da2bdba572b9c Mon Sep 17 00:00:00 2001 From: jimczi Date: Mon, 18 Feb 2019 23:11:28 +0100 Subject: [PATCH 3/4] add license header --- .../apache/lucene/search/XIndexSearcher.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/server/src/main/java/org/apache/lucene/search/XIndexSearcher.java b/server/src/main/java/org/apache/lucene/search/XIndexSearcher.java index 9a5e687007f49..80d2951167fd9 100644 --- a/server/src/main/java/org/apache/lucene/search/XIndexSearcher.java +++ b/server/src/main/java/org/apache/lucene/search/XIndexSearcher.java @@ -1,3 +1,22 @@ +/* + * 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.apache.lucene.search; import org.apache.lucene.index.LeafReaderContext; From 9d26bcba4f5a29d88946755ea056ec277abcc82e Mon Sep 17 00:00:00 2001 From: jimczi Date: Tue, 19 Feb 2019 08:51:33 +0100 Subject: [PATCH 4/4] fix mock engine to support asserting index searcher --- .../apache/lucene/search/XIndexSearcher.java | 6 +++--- .../search/internal/ContextIndexSearcher.java | 2 +- .../test/engine/MockEngineSupport.java | 19 ++++++++++++++++++- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/apache/lucene/search/XIndexSearcher.java b/server/src/main/java/org/apache/lucene/search/XIndexSearcher.java index 80d2951167fd9..100c5f4944afe 100644 --- a/server/src/main/java/org/apache/lucene/search/XIndexSearcher.java +++ b/server/src/main/java/org/apache/lucene/search/XIndexSearcher.java @@ -31,12 +31,12 @@ public class XIndexSearcher extends IndexSearcher { private final IndexSearcher in; - public XIndexSearcher(IndexSearcher in, QueryCache queryCache, QueryCachingPolicy queryCachingPolicy) { + public XIndexSearcher(IndexSearcher in) { super(in.getIndexReader()); this.in = in; setSimilarity(in.getSimilarity()); - setQueryCache(queryCache); - setQueryCachingPolicy(queryCachingPolicy); + setQueryCache(in.getQueryCache()); + setQueryCachingPolicy(in.getQueryCachingPolicy()); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java b/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java index 96cb378fec6de..7c56796f3d24d 100644 --- a/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java +++ b/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java @@ -71,7 +71,7 @@ public class ContextIndexSearcher extends IndexSearcher implements Releasable { public ContextIndexSearcher(Engine.Searcher searcher, QueryCache queryCache, QueryCachingPolicy queryCachingPolicy) { super(searcher.reader()); engineSearcher = searcher; - in = new XIndexSearcher(searcher.searcher(), queryCache, queryCachingPolicy); + in = new XIndexSearcher(searcher.searcher()); setSimilarity(searcher.searcher().getSimilarity()); setQueryCache(queryCache); setQueryCachingPolicy(queryCachingPolicy); diff --git a/test/framework/src/main/java/org/elasticsearch/test/engine/MockEngineSupport.java b/test/framework/src/main/java/org/elasticsearch/test/engine/MockEngineSupport.java index 52b086db338f3..b3a6fe8490895 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/engine/MockEngineSupport.java +++ b/test/framework/src/main/java/org/elasticsearch/test/engine/MockEngineSupport.java @@ -24,9 +24,14 @@ import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.FilterDirectoryReader; import org.apache.lucene.index.IndexReader; +import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.search.AssertingIndexSearcher; +import org.apache.lucene.search.Collector; +import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.QueryCache; import org.apache.lucene.search.QueryCachingPolicy; +import org.apache.lucene.search.Weight; +import org.apache.lucene.search.XIndexSearcher; import org.apache.lucene.util.LuceneTestCase; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.settings.Setting; @@ -42,6 +47,7 @@ import java.io.IOException; import java.lang.reflect.Constructor; import java.util.IdentityHashMap; +import java.util.List; import java.util.Random; import java.util.concurrent.atomic.AtomicBoolean; @@ -145,8 +151,19 @@ public AssertingIndexSearcher newSearcher(Engine.Searcher searcher) throws Engin if (reader instanceof DirectoryReader && mockContext.wrapReader) { wrappedReader = wrapReader((DirectoryReader) reader); } + final IndexSearcher delegate = new IndexSearcher(wrappedReader); + delegate.setSimilarity(searcher.searcher().getSimilarity()); + delegate.setQueryCache(filterCache); + delegate.setQueryCachingPolicy(filterCachingPolicy); + final XIndexSearcher wrappedSearcher = new XIndexSearcher(delegate); // this executes basic query checks and asserts that weights are normalized only once etc. - final AssertingIndexSearcher assertingIndexSearcher = new AssertingIndexSearcher(mockContext.random, wrappedReader); + final AssertingIndexSearcher assertingIndexSearcher = new AssertingIndexSearcher(mockContext.random, wrappedReader) { + @Override + protected void search(List leaves, Weight weight, Collector collector) throws IOException { + // we cannot use the asserting searcher because the weight is created by the ContextIndexSearcher + wrappedSearcher.search(leaves, weight, collector); + } + }; assertingIndexSearcher.setSimilarity(searcher.searcher().getSimilarity()); assertingIndexSearcher.setQueryCache(filterCache); assertingIndexSearcher.setQueryCachingPolicy(filterCachingPolicy);