This is an automated email from the ASF dual-hosted git repository.
gerlowskija pushed a commit to branch branch_9x
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/branch_9x by this push:
new 0ade74477d2 SOLR-17677: Ensure DBQ is safe before running (#3203)
0ade74477d2 is described below
commit 0ade74477d2ceef1de244e6462d3769d7c00cd7a
Author: Jason Gerlowski <[email protected]>
AuthorDate: Fri Feb 21 21:21:21 2025 -0500
SOLR-17677: Ensure DBQ is safe before running (#3203)
Portions of the DBQ codepath execute the deletion query with a standard
Lucene IndexSearcher, which upsets some Solr Query implementations that
have been built to assume SolrIndexSearcher in (e.g.)
Query.createWeight.
This commit adds checks for this case, which work by using a Lucene
"QueryVisitor" to iterate over a parsed query tree and detect whether
any of the individual queries require SolrIndexSearcher.
(The QueryVisitor implementation has been added as 'lucene.
experimental', to leave us free to modify or remove it in future as
desired.)
---
.../src/java/org/apache/solr/search/JoinQuery.java | 8 +-
.../search/SolrSearcherRequirementDetector.java | 65 +++++++++++++++
.../apache/solr/search/SolrSearcherRequirer.java | 27 +++++++
.../org/apache/solr/search/TopLevelJoinQuery.java | 3 +-
.../solr/search/join/CrossCollectionJoinQuery.java | 3 +-
.../org/apache/solr/search/join/GraphQuery.java | 3 +-
.../apache/solr/search/join/HashRangeQuery.java | 3 +-
.../apache/solr/update/DirectUpdateHandler2.java | 18 ++++-
.../apache/solr/cloud/TestCloudDeleteByQuery.java | 40 ++++++++++
.../SolrSearcherRequirementDetectorTest.java | 93 ++++++++++++++++++++++
.../pages/indexing-with-update-handlers.adoc | 6 +-
11 files changed, 258 insertions(+), 11 deletions(-)
diff --git a/solr/core/src/java/org/apache/solr/search/JoinQuery.java
b/solr/core/src/java/org/apache/solr/search/JoinQuery.java
index 3ec6b85b94c..ede3edaddc7 100644
--- a/solr/core/src/java/org/apache/solr/search/JoinQuery.java
+++ b/solr/core/src/java/org/apache/solr/search/JoinQuery.java
@@ -29,6 +29,7 @@ import org.apache.lucene.index.MultiPostingsEnum;
import org.apache.lucene.index.PostingsEnum;
import org.apache.lucene.index.Terms;
import org.apache.lucene.index.TermsEnum;
+import org.apache.lucene.search.BooleanClause;
import org.apache.lucene.search.ConstantScoreScorer;
import org.apache.lucene.search.ConstantScoreWeight;
import org.apache.lucene.search.DocIdSetIterator;
@@ -54,7 +55,7 @@ import org.apache.solr.search.join.GraphPointsCollector;
import org.apache.solr.util.RTimer;
import org.apache.solr.util.RefCounted;
-class JoinQuery extends Query {
+class JoinQuery extends Query implements SolrSearcherRequirer {
String fromField;
String toField;
// TODO: name is missleading here compared to JoinQParserPlugin usage - here
it must be a core
@@ -86,7 +87,10 @@ class JoinQuery extends Query {
}
@Override
- public void visit(QueryVisitor visitor) {}
+ public void visit(QueryVisitor visitor) {
+ QueryVisitor sub = visitor.getSubVisitor(BooleanClause.Occur.MUST, this);
+ q.visit(sub);
+ }
@Override
public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode,
float boost)
diff --git
a/solr/core/src/java/org/apache/solr/search/SolrSearcherRequirementDetector.java
b/solr/core/src/java/org/apache/solr/search/SolrSearcherRequirementDetector.java
new file mode 100644
index 00000000000..9abc601fa6d
--- /dev/null
+++
b/solr/core/src/java/org/apache/solr/search/SolrSearcherRequirementDetector.java
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.solr.search;
+
+import org.apache.lucene.search.BooleanClause;
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.QueryVisitor;
+
+/**
+ * Detects whether a query can be run using a standard Lucene {@link
+ * org.apache.lucene.search.IndexSearcher}
+ *
+ * <p>Some Solr {@link Query} implementations are written to assume access to
a {@link
+ * SolrIndexSearcher}. But these objects aren't always available: some
code-paths (e.g. when
+ * executing a "delete-by-query") execute the query using the standard {@link
+ * org.apache.lucene.search.IndexSearcher} available in Lucene. This {@link
QueryVisitor} allows
+ * code to detect whether a given Query requires SolrIndexSearcher or not.
+ *
+ * <p>Instances should not be reused for multiple query-tree inspections.
+ *
+ * @see SolrSearcherRequirer
+ * @lucene.experimental
+ */
+public class SolrSearcherRequirementDetector extends QueryVisitor {
+
+ private boolean requiresSolrSearcher = false;
+
+ @Override
+ public QueryVisitor getSubVisitor(BooleanClause.Occur occur, Query parent) {
+ // This method is primarily intended to swap out visitors when descending
through the Query
+ // tree, but it's also the only place to put visiting logic for non-leaf
nodes, since the
+ // QueryVisitor interface largely assumes that only leaf-nodes are worth
visiting. See
+ // LUCENE-????? for more details - this can be restructured if that ticket
is ever addressed.
+ if (parent instanceof SolrSearcherRequirer) {
+ requiresSolrSearcher = true;
+ }
+
+ return this;
+ }
+
+ @Override
+ public void visitLeaf(Query query) {
+ if (query instanceof SolrSearcherRequirer) {
+ requiresSolrSearcher = true;
+ }
+ }
+
+ public boolean getRequiresSolrSearcher() {
+ return requiresSolrSearcher;
+ }
+}
diff --git
a/solr/core/src/java/org/apache/solr/search/SolrSearcherRequirer.java
b/solr/core/src/java/org/apache/solr/search/SolrSearcherRequirer.java
new file mode 100644
index 00000000000..888060a4231
--- /dev/null
+++ b/solr/core/src/java/org/apache/solr/search/SolrSearcherRequirer.java
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.solr.search;
+
+import org.apache.lucene.search.IndexSearcher;
+
+/**
+ * Marker interface indicating that the tagged class assumes access to {@link
SolrIndexSearcher}
+ * functionality and cannot be used by a vanilla Lucene {@link IndexSearcher}
+ *
+ * @lucene.experimental
+ */
+public interface SolrSearcherRequirer {}
diff --git a/solr/core/src/java/org/apache/solr/search/TopLevelJoinQuery.java
b/solr/core/src/java/org/apache/solr/search/TopLevelJoinQuery.java
index ea7b55a2e75..ad3fd963574 100644
--- a/solr/core/src/java/org/apache/solr/search/TopLevelJoinQuery.java
+++ b/solr/core/src/java/org/apache/solr/search/TopLevelJoinQuery.java
@@ -46,7 +46,7 @@ import org.slf4j.LoggerFactory;
* {@link JoinQuery} implementation using global (top-level) DocValues
ordinals to efficiently
* compare values in the "from" and "to" fields.
*/
-public class TopLevelJoinQuery extends JoinQuery {
+public class TopLevelJoinQuery extends JoinQuery implements
SolrSearcherRequirer {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public TopLevelJoinQuery(String fromField, String toField, String coreName,
Query subQuery) {
@@ -60,6 +60,7 @@ public class TopLevelJoinQuery extends JoinQuery {
log.debug(
"Falling back to JoinQueryWeight because searcher [{}] is not the
required SolrIndexSearcher",
searcher);
+ // TODO This check no longer makes sense as super.createWeight *also*
requires a SIS
return super.createWeight(searcher, scoreMode, boost);
}
diff --git
a/solr/core/src/java/org/apache/solr/search/join/CrossCollectionJoinQuery.java
b/solr/core/src/java/org/apache/solr/search/join/CrossCollectionJoinQuery.java
index b0b396cf6cf..619025800ac 100644
---
a/solr/core/src/java/org/apache/solr/search/join/CrossCollectionJoinQuery.java
+++
b/solr/core/src/java/org/apache/solr/search/join/CrossCollectionJoinQuery.java
@@ -60,8 +60,9 @@ import org.apache.solr.search.BitDocSet;
import org.apache.solr.search.DocSet;
import org.apache.solr.search.DocSetUtil;
import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.SolrSearcherRequirer;
-public class CrossCollectionJoinQuery extends Query {
+public class CrossCollectionJoinQuery extends Query implements
SolrSearcherRequirer {
protected final String query;
protected final String zkHost;
diff --git a/solr/core/src/java/org/apache/solr/search/join/GraphQuery.java
b/solr/core/src/java/org/apache/solr/search/join/GraphQuery.java
index 3dddedb755f..051c3f639f5 100644
--- a/solr/core/src/java/org/apache/solr/search/join/GraphQuery.java
+++ b/solr/core/src/java/org/apache/solr/search/join/GraphQuery.java
@@ -46,6 +46,7 @@ import org.apache.solr.schema.SchemaField;
import org.apache.solr.search.BitDocSet;
import org.apache.solr.search.DocSet;
import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.SolrSearcherRequirer;
/**
* GraphQuery - search for nodes and traverse edges in an index.
@@ -58,7 +59,7 @@ import org.apache.solr.search.SolrIndexSearcher;
*
* @lucene.experimental
*/
-public class GraphQuery extends Query {
+public class GraphQuery extends Query implements SolrSearcherRequirer {
/** The inital node matching query */
private Query q;
diff --git a/solr/core/src/java/org/apache/solr/search/join/HashRangeQuery.java
b/solr/core/src/java/org/apache/solr/search/join/HashRangeQuery.java
index d0bdc326747..df70e7c9bf0 100644
--- a/solr/core/src/java/org/apache/solr/search/join/HashRangeQuery.java
+++ b/solr/core/src/java/org/apache/solr/search/join/HashRangeQuery.java
@@ -38,8 +38,9 @@ import org.apache.lucene.util.BytesRef;
import org.apache.solr.common.util.Hash;
import org.apache.solr.search.SolrCache;
import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.SolrSearcherRequirer;
-public class HashRangeQuery extends Query {
+public class HashRangeQuery extends Query implements SolrSearcherRequirer {
protected final String field;
protected final int lower;
diff --git
a/solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java
b/solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java
index 2704ef4e027..4c55eaab9b5 100644
--- a/solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java
+++ b/solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java
@@ -65,6 +65,7 @@ import org.apache.solr.search.FunctionRangeQuery;
import org.apache.solr.search.QParser;
import org.apache.solr.search.QueryUtils;
import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.SolrSearcherRequirementDetector;
import org.apache.solr.search.SyntaxError;
import org.apache.solr.search.function.ValueSourceRangeFilter;
import org.apache.solr.util.RefCounted;
@@ -79,6 +80,8 @@ import org.slf4j.LoggerFactory;
public class DirectUpdateHandler2 extends UpdateHandler
implements SolrCoreState.IndexWriterCloser, SolrMetricProducer {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
private static final int NO_FILE_SIZE_UPPER_BOUND_PLACEHOLDER = -1;
protected final SolrCoreState solrCoreState;
@@ -115,8 +118,6 @@ public class DirectUpdateHandler2 extends UpdateHandler
this.commitWithinSoftCommit = value;
}
- private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
-
public DirectUpdateHandler2(SolrCore core) {
super(core, null, false);
@@ -572,12 +573,23 @@ public class DirectUpdateHandler2 extends UpdateHandler
deleteByQueryCommandsCumulative.mark();
boolean madeIt = false;
try {
+ Query q = getQuery(cmd);
+
+ // Parts of the DBQ codepath run the query using a standard Lucene
IndexSearcher, so block any
+ // queries that we know require a SolrIndexSearcher
+ final var unsupportedQDetector = new SolrSearcherRequirementDetector();
+ q.visit(unsupportedQDetector);
+ if (unsupportedQDetector.getRequiresSolrSearcher()) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "Query [" + cmd.getQuery() + "] is not supported in
delete-by-query operations");
+ }
+
if ((cmd.getFlags() & UpdateCommand.IGNORE_INDEXWRITER) != 0) {
if (ulog != null) ulog.deleteByQuery(cmd);
madeIt = true;
return;
}
- Query q = getQuery(cmd);
boolean delAll = MatchAllDocsQuery.class == q.getClass();
diff --git
a/solr/core/src/test/org/apache/solr/cloud/TestCloudDeleteByQuery.java
b/solr/core/src/test/org/apache/solr/cloud/TestCloudDeleteByQuery.java
index f2a6d7c1d76..1e6a5c04b5c 100644
--- a/solr/core/src/test/org/apache/solr/cloud/TestCloudDeleteByQuery.java
+++ b/solr/core/src/test/org/apache/solr/cloud/TestCloudDeleteByQuery.java
@@ -16,11 +16,15 @@
*/
package org.apache.solr.cloud;
+import static org.hamcrest.Matchers.containsString;
+
+import java.lang.invoke.MethodHandles;
import java.net.URL;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
+import java.util.UUID;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.impl.CloudSolrClient;
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
@@ -40,9 +44,14 @@ import org.apache.solr.embedded.JettySolrRunner;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class TestCloudDeleteByQuery extends SolrCloudTestCase {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
private static final int NUM_SHARDS = 2;
private static final int REPLICATION_FACTOR = 2;
private static final int NUM_SERVERS = 5;
@@ -255,6 +264,37 @@ public class TestCloudDeleteByQuery extends
SolrCloudTestCase {
testMalformedDBQ(NO_COLLECTION_CLIENT);
}
+ // See SOLR-17677 for context
+ @Test
+ public void testDBQWithUnsupportedQueryReturns400() throws Exception {
+ final var unsupportedQueryExamples =
+ new String[] {
+ "{!join from=expected_shard_s to=expected_shard_s
v=\"expected_shard_s:5\"}",
+ "{!graph from=expected_shard_s to=expected_shard_s
v=\"expected_shard_s:5\"}",
+ "{!hash_range f=\"foo_i\" l=\"0\" u=\"12345\"}"
+ };
+
+ update(params()).add(doc(f("id",
UUID.randomUUID().toString()))).process(COLLECTION_CLIENT);
+ for (String queryStr : unsupportedQueryExamples) {
+ log.info("Testing unsupported DBQ query: {}", queryStr);
+ SolrException e =
+ expectThrows(
+ SolrException.class,
+ () -> {
+
update(params()).deleteByQuery(queryStr).process(COLLECTION_CLIENT);
+ });
+ assertEquals("Unexpected status code for DBQ with query " + queryStr,
400, e.code());
+ final var expectedStr =
+ "Query [" + queryStr + "] is not supported in delete-by-query
operations";
+ assertThat(e.getMessage(), containsString(expectedStr));
+ }
+
+ final var acceptableJoin =
+ "{!join method=dvWithScore score=None from=expected_shard_s
to=expected_shard_s v=\"expected_shard_s:5\"}";
+ final var response =
update(params()).deleteByQuery(acceptableJoin).process(COLLECTION_CLIENT);
+ assertEquals(0, response.getStatus());
+ }
+
public static UpdateRequest update(SolrParams params, SolrInputDocument...
docs) {
UpdateRequest r = new UpdateRequest();
r.setParams(new ModifiableSolrParams(params));
diff --git
a/solr/core/src/test/org/apache/solr/search/SolrSearcherRequirementDetectorTest.java
b/solr/core/src/test/org/apache/solr/search/SolrSearcherRequirementDetectorTest.java
new file mode 100644
index 00000000000..526d9a23a26
--- /dev/null
+++
b/solr/core/src/test/org/apache/solr/search/SolrSearcherRequirementDetectorTest.java
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.solr.search;
+
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.not;
+
+import org.apache.lucene.index.Term;
+import org.apache.lucene.search.BooleanClause;
+import org.apache.lucene.search.BooleanQuery;
+import org.apache.lucene.search.TermQuery;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.search.join.GraphQuery;
+import org.junit.Test;
+
+/** Unit tests for {@link SolrSearcherRequirementDetector} */
+public class SolrSearcherRequirementDetectorTest extends SolrTestCase {
+
+ @Test
+ public void testDetectsWhenQueriesDontRequireSolrSearcher() {
+ final var termQuery = new TermQuery(new Term("someField",
"someFieldValue"));
+ final var termQuery2 = new TermQuery(new Term("someField",
"someOtherFieldValue"));
+
+ var needsSearcherDetector = new SolrSearcherRequirementDetector();
+ termQuery.visit(needsSearcherDetector);
+ assertFalse(needsSearcherDetector.getRequiresSolrSearcher());
+
+ final var boolQuery =
+ new BooleanQuery.Builder()
+ .add(termQuery, BooleanClause.Occur.MUST)
+ .add(termQuery2, BooleanClause.Occur.SHOULD)
+ .build();
+ needsSearcherDetector = new SolrSearcherRequirementDetector();
+ boolQuery.visit(needsSearcherDetector);
+ assertFalse(needsSearcherDetector.getRequiresSolrSearcher());
+ }
+
+ @Test
+ public void testDetectsWhenQueriesDoRequireSolrSearcher_TopLevel() {
+ final var termQuery = new TermQuery(new Term("someField",
"someFieldValue"));
+
+ var needsSearcherDetector = new SolrSearcherRequirementDetector();
+ final var joinQuery = new JoinQuery("fromField", "toField",
"someCoreName", termQuery);
+ assertThat(
+ joinQuery,
+ instanceOf(SolrSearcherRequirer.class)); // Ensure that JoinQuery
still requires SIS
+ joinQuery.visit(needsSearcherDetector);
+ assertTrue(needsSearcherDetector.getRequiresSolrSearcher());
+
+ needsSearcherDetector = new SolrSearcherRequirementDetector();
+ final var graphQuery = new GraphQuery(termQuery, "fromField", "toField");
+ assertThat(
+ graphQuery,
+ instanceOf(SolrSearcherRequirer.class)); // Ensure that GraphQuery
still requires SIS
+ graphQuery.visit(needsSearcherDetector);
+ assertTrue(needsSearcherDetector.getRequiresSolrSearcher());
+ }
+
+ @Test
+ public void testDeteectsWhenQueriesDoRequireSolrSearcher_Nested() {
+ final var termQuery = new TermQuery(new Term("someField",
"someFieldValue"));
+ final var termQuery2 = new TermQuery(new Term("someField",
"someOtherFieldValue"));
+ final var joinQuery = new JoinQuery("fromField", "toField",
"someCoreName", termQuery);
+ final var boolQuery =
+ new BooleanQuery.Builder()
+ .add(new BooleanClause(termQuery, BooleanClause.Occur.MUST))
+ .add(new BooleanClause(termQuery2, BooleanClause.Occur.SHOULD))
+ .add(new BooleanClause(joinQuery, BooleanClause.Occur.SHOULD))
+ .build();
+
+ final var needsSearcherDetector = new SolrSearcherRequirementDetector();
+ // Top level query and some leaves don't require SIS, but JoinQuery does.
+ assertThat(boolQuery, not(instanceOf(SolrSearcherRequirer.class)));
+ assertThat(termQuery, not(instanceOf(SolrSearcherRequirer.class)));
+ assertThat(joinQuery, instanceOf(SolrSearcherRequirer.class));
+ boolQuery.visit(needsSearcherDetector);
+ assertTrue(needsSearcherDetector.getRequiresSolrSearcher());
+ }
+}
diff --git
a/solr/solr-ref-guide/modules/indexing-guide/pages/indexing-with-update-handlers.adoc
b/solr/solr-ref-guide/modules/indexing-guide/pages/indexing-with-update-handlers.adoc
index 92b7bedcdec..44aa38a85a7 100644
---
a/solr/solr-ref-guide/modules/indexing-guide/pages/indexing-with-update-handlers.adoc
+++
b/solr/solr-ref-guide/modules/indexing-guide/pages/indexing-with-update-handlers.adoc
@@ -181,8 +181,10 @@ A single delete message can contain multiple delete
operations.
[IMPORTANT]
====
-When using the Join query parser in a Delete By Query, you should use the
`score` parameter with a value of "none" to avoid a `ClassCastException`.
-See the section on the xref:query-guide:join-query-parser.adoc[] for more
details on the `score` parameter.
+Some queries, including many `{!join}` and `{!graph}` queries, are not
supported in delete operations and will return a 400 error.
+
+Users interested in using the Join query parser in a Delete By Query may do so
by specifying a `score` parameter with the value "none" (and ensuring no
`method` parameter is provided).
+See the section on the xref:query-guide:join-query-parser.adoc[] for more
details on the `score` parameter and its usage.
====
