From 43b901109d61e18b448d38b0c9598170a91d5e0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20B=C3=B8gh=20K=C3=B6ster?= Date: Wed, 14 Feb 2024 14:59:02 +0100 Subject: [PATCH] [SOLR-10059] a new approach to tackle appended fq parameters Problems with duplicate parameter appending was the inability to expand macros in shard requests. This fix will sanitize all macros from `fq` parameters in shard requests (as they should have been expanded on the coordinator) --- .../solr/handler/RequestHandlerBase.java | 3 +- .../apache/solr/request/json/RequestUtil.java | 8 +- .../solr/request/macro/MacroSanitizer.java | 55 +++++++++ ...archHandlerAppendsWithMacrosCloudTest.java | 107 ++++++++++++++++++ .../request/macro/MacroSanitizerTest.java | 62 ++++++++++ 5 files changed, 233 insertions(+), 2 deletions(-) create mode 100644 solr/core/src/java/org/apache/solr/request/macro/MacroSanitizer.java create mode 100644 solr/core/src/test/org/apache/solr/handler/component/SearchHandlerAppendsWithMacrosCloudTest.java create mode 100644 solr/core/src/test/org/apache/solr/request/macro/MacroSanitizerTest.java diff --git a/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java b/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java index e700428f1a4..6f94bfa6119 100644 --- a/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java +++ b/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java @@ -218,8 +218,9 @@ public void handleRequest(SolrQueryRequest req, SolrQueryResponse rsp) { Timer.Context timer = metrics.requestTimes.time(); try { TestInjection.injectLeaderTragedy(req.getCore()); - if (pluginInfo != null && pluginInfo.attributes.containsKey(USEPARAM)) + if (pluginInfo != null && pluginInfo.attributes.containsKey(USEPARAM)) { req.getContext().put(USEPARAM, pluginInfo.attributes.get(USEPARAM)); + } SolrPluginUtils.setDefaults(this, req, defaults, appends, invariants); req.getContext().remove(USEPARAM); rsp.setHttpCaching(httpCaching); diff --git a/solr/core/src/java/org/apache/solr/request/json/RequestUtil.java b/solr/core/src/java/org/apache/solr/request/json/RequestUtil.java index e6367612951..591b7f2c2ab 100644 --- a/solr/core/src/java/org/apache/solr/request/json/RequestUtil.java +++ b/solr/core/src/java/org/apache/solr/request/json/RequestUtil.java @@ -24,6 +24,7 @@ import java.util.List; import java.util.Map; import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.CommonParams; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.params.MultiMapSolrParams; import org.apache.solr.common.params.SolrParams; @@ -33,6 +34,7 @@ import org.apache.solr.request.SolrQueryRequest; import org.apache.solr.request.SolrRequestHandler; import org.apache.solr.request.macro.MacroExpander; +import org.apache.solr.request.macro.MacroSanitizer; import org.apache.solr.search.QueryParsing; import org.noggit.JSONParser; import org.noggit.ObjectBuilder; @@ -164,7 +166,11 @@ public static void processParams( newMap.putAll(MultiMapSolrParams.asMultiMap(invariants)); } - if (!isShard) { // Don't expand macros in shard requests + if (isShard) { + // sanitize all macros from fq parameters as they + // might corrupt handler local params + newMap = MacroSanitizer.sanitize(CommonParams.FQ, newMap); + } else { // Don't expand macros in shard requests String[] doMacrosStr = newMap.get("expandMacros"); boolean doMacros = true; if (doMacrosStr != null) { diff --git a/solr/core/src/java/org/apache/solr/request/macro/MacroSanitizer.java b/solr/core/src/java/org/apache/solr/request/macro/MacroSanitizer.java new file mode 100644 index 00000000000..746390bc3a0 --- /dev/null +++ b/solr/core/src/java/org/apache/solr/request/macro/MacroSanitizer.java @@ -0,0 +1,55 @@ +/* + * 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.request.macro; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Map; + +public class MacroSanitizer { + + /** + * Sanitizes macros for the given parameter in the given params set if present + * + * @param param the parameter whose values we should sanitzize + * @param params the parameter set + * @return the sanitized parameter set + */ + public static Map sanitize(String param, Map params) { + // quick peek into the values to check for macros + final boolean needsSanitizing = + params.containsKey(param) + && Arrays.stream(params.get(param)) + .anyMatch(s -> s.contains(MacroExpander.MACRO_START)); + + if (needsSanitizing) { + final String[] fqs = params.get(param); + final List sanitizedFqs = new ArrayList<>(fqs.length); + + for (int i = 0; i < fqs.length; i++) { + if (!fqs[i].contains(MacroExpander.MACRO_START)) { + sanitizedFqs.add(fqs[i]); + } + } + + params.put(param, sanitizedFqs.toArray(new String[] {})); + } + + return params; + } +} diff --git a/solr/core/src/test/org/apache/solr/handler/component/SearchHandlerAppendsWithMacrosCloudTest.java b/solr/core/src/test/org/apache/solr/handler/component/SearchHandlerAppendsWithMacrosCloudTest.java new file mode 100644 index 00000000000..735b6dc6720 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/handler/component/SearchHandlerAppendsWithMacrosCloudTest.java @@ -0,0 +1,107 @@ +/* + * 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.handler.component; + +import org.apache.solr.client.solrj.SolrQuery; +import org.apache.solr.client.solrj.request.CollectionAdminRequest; +import org.apache.solr.client.solrj.request.QueryRequest; +import org.apache.solr.client.solrj.request.UpdateRequest; +import org.apache.solr.client.solrj.response.QueryResponse; +import org.apache.solr.cloud.AbstractDistribZkTestBase; +import org.apache.solr.cloud.ConfigRequest; +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.cloud.ZkStateReader; +import org.apache.solr.common.params.CommonParams; +import org.junit.BeforeClass; +import org.junit.Test; + +public class SearchHandlerAppendsWithMacrosCloudTest extends SolrCloudTestCase { + + private static String COLLECTION; + private static int NUM_SHARDS; + private static int NUM_REPLICAS; + + @BeforeClass + public static void setupCluster() throws Exception { + + // decide collection name ... + COLLECTION = "collection" + (1 + random().nextInt(100)); + // ... and shard/replica/node numbers + NUM_SHARDS = (2 + random().nextInt(2)); // 0..2 + NUM_REPLICAS = (1 + random().nextInt(2)); // 0..2 + + // create and configure cluster + configureCluster(NUM_SHARDS * NUM_REPLICAS /* nodeCount */) + .addConfig("conf", configset("cloud-dynamic")) + .configure(); + + // create an empty collection + CollectionAdminRequest.createCollection(COLLECTION, "conf", NUM_SHARDS, NUM_REPLICAS) + .processAndWait(cluster.getSolrClient(), DEFAULT_TIMEOUT); + AbstractDistribZkTestBase.waitForRecoveriesToFinish( + COLLECTION, ZkStateReader.from(cluster.getSolrClient()), false, true, DEFAULT_TIMEOUT); + } + + @Test + public void test() throws Exception { + + // field names + final String id = "id"; + final String bee_si = "bee_sI"; + final String forage_t = "forage_t"; + final String handlerName = "/custom-select"; + + // add custom handlers (the exact custom handler names should not matter) + cluster + .getSolrClient() + .request( + new ConfigRequest( + "{\n" + + " 'add-requesthandler': {\n" + + " 'name' : '" + + handlerName + + "',\n" + + " 'class' : 'org.apache.solr.handler.component.SearchHandler',\n" + + " 'appends' : { 'fq' : '{!collapse tag=collapsing field=" + + bee_si + + " sort=\"${collapseSort}\" }' }, \n" + + " }\n" + + "}"), + COLLECTION); + + // add some documents + { + new UpdateRequest() + .add(sdoc(id, 1, bee_si, "bumble bee", forage_t, "nectar")) + .add(sdoc(id, 2, bee_si, "honey bee", forage_t, "propolis")) + .add(sdoc(id, 3, bee_si, "solitary bee", forage_t, "pollen")) + .commit(cluster.getSolrClient(), COLLECTION); + } + + // compose the query + final SolrQuery solrQuery = new SolrQuery(bee_si + ":bee"); + solrQuery.setParam(CommonParams.QT, handlerName); + solrQuery.setParam(CommonParams.SORT, "id desc"); + solrQuery.setParam("collapseSort", "id asc"); + + // make the query + // the query wouold break with macros in shard response + final QueryResponse queryResponse = + new QueryRequest(solrQuery).process(cluster.getSolrClient(), COLLECTION); + assertNotNull(queryResponse); + } +} diff --git a/solr/core/src/test/org/apache/solr/request/macro/MacroSanitizerTest.java b/solr/core/src/test/org/apache/solr/request/macro/MacroSanitizerTest.java new file mode 100644 index 00000000000..ec8c88ae011 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/request/macro/MacroSanitizerTest.java @@ -0,0 +1,62 @@ +/* + * 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.request.macro; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; + +import java.util.HashMap; +import java.util.Map; +import org.apache.solr.common.params.CommonParams; +import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; +import org.junit.Test; + +public class MacroSanitizerTest { + + @Test + public void shouldReturnSameInstanceWhenNotSanitizing() { + // given + Map params = new HashMap<>(); + + // when + Map sanitized = MacroSanitizer.sanitize(CommonParams.FQ, params); + + // then + assertSame(params, sanitized); + } + + @Test + public void shouldNotSanitizeNonMacros() { + // given + Map params = new HashMap<>(); + params.put( + CommonParams.FQ, + new String[] { + "bee:up", "look:left", "{!collapse tag=collapsing field=bee sort=${collapseSort}}" + }); + params.put("q", new String[] {"bee:honey"}); + + // when + Map sanitized = MacroSanitizer.sanitize(CommonParams.FQ, params); + + // then + assertEquals(2, sanitized.size()); + assertEquals(2, sanitized.get("fq").length); + MatcherAssert.assertThat(sanitized.get("fq"), Matchers.arrayContaining("bee:up", "look:left")); + } +}