From 99516e78da0de618fc893afcc2e17ee96b992ba7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20B=C3=B8gh=20K=C3=B6ster?= Date: Wed, 14 Feb 2024 13:05:37 +0100 Subject: [PATCH] [SOLR-10059] Formalities check --- .../solr/handler/RequestHandlerBase.java | 2 - .../apache/solr/request/json/RequestUtil.java | 6 +- .../solr/request/macro/MacroSanitizer.java | 71 ++++++++++------- ...archHandlerAppendsWithMacrosCloudTest.java | 10 ++- .../request/macro/MacroSanitizerTest.java | 76 ++++++++++++------- 5 files changed, 100 insertions(+), 65 deletions(-) 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 68c821178e1..b3784800258 100644 --- a/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java +++ b/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java @@ -28,9 +28,7 @@ import org.apache.solr.api.Api; import org.apache.solr.api.ApiBag; import org.apache.solr.api.ApiSupport; -import org.apache.solr.cluster.Shard; import org.apache.solr.common.SolrException; -import org.apache.solr.common.params.CommonParams; import org.apache.solr.common.params.ShardParams; import org.apache.solr.common.params.SolrParams; import org.apache.solr.common.util.NamedList; 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 e35940c18b1..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; @@ -166,8 +167,9 @@ public static void processParams( } if (isShard) { - // sanitize all macros from parameters - newMap = MacroSanitizer.sanitize(newMap); + // 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; 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 index 2e7796136d7..746390bc3a0 100644 --- a/solr/core/src/java/org/apache/solr/request/macro/MacroSanitizer.java +++ b/solr/core/src/java/org/apache/solr/request/macro/MacroSanitizer.java @@ -1,42 +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; -import java.util.Map.Entry; - -import org.apache.solr.common.util.CollectionUtil; public class MacroSanitizer { - - public static Map sanitize(Map params) { - Map sanitized = params; - - // quick peek into the values to check for macros - final boolean needsSanitizing = params.values().stream() - .flatMap(v -> Arrays.stream(v)) - .anyMatch(s -> s.contains(MacroExpander.MACRO_START)); - - if (needsSanitizing) { - sanitized = CollectionUtil.newHashMap(params.size()); - - for (Entry param : params.entrySet()) { - final String paramName = param.getKey(); - final String[] paramValues = param.getValue(); - final List sanitizedParamValues = new ArrayList<>(paramValues.length); - for (int i = 0; i < paramValues.length; i++) { - if (!paramValues[i].contains(MacroExpander.MACRO_START)) { - sanitizedParamValues.add(paramValues[i]); - } - } - - sanitized.put(paramName, sanitizedParamValues.toArray(new String[] {})); - } + /** + * 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]); } - - return sanitized; + } + + 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 index 80d1078abf8..735b6dc6720 100644 --- a/solr/core/src/test/org/apache/solr/handler/component/SearchHandlerAppendsWithMacrosCloudTest.java +++ b/solr/core/src/test/org/apache/solr/handler/component/SearchHandlerAppendsWithMacrosCloudTest.java @@ -72,9 +72,13 @@ public void test() throws Exception { new ConfigRequest( "{\n" + " 'add-requesthandler': {\n" - + " 'name' : '" + handlerName + "',\n" + + " 'name' : '" + + handlerName + + "',\n" + " 'class' : 'org.apache.solr.handler.component.SearchHandler',\n" - + " 'appends' : { 'fq' : '{!collapse tag=collapsing field=" + bee_si + " sort=\"${collapseSort}\" }' }, \n" + + " 'appends' : { 'fq' : '{!collapse tag=collapsing field=" + + bee_si + + " sort=\"${collapseSort}\" }' }, \n" + " }\n" + "}"), COLLECTION); @@ -88,8 +92,6 @@ public void test() throws Exception { .commit(cluster.getSolrClient(), COLLECTION); } - - // compose the query final SolrQuery solrQuery = new SolrQuery(bee_si + ":bee"); solrQuery.setParam(CommonParams.QT, handlerName); 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 index c9e5525f9d2..ec8c88ae011 100644 --- a/solr/core/src/test/org/apache/solr/request/macro/MacroSanitizerTest.java +++ b/solr/core/src/test/org/apache/solr/request/macro/MacroSanitizerTest.java @@ -1,3 +1,19 @@ +/* + * 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; @@ -5,38 +21,42 @@ 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(params); - - // then - assertSame(params, sanitized); - } - - @Test - public void shouldNotSanitizeNonMacros() { - // given - Map params = new HashMap<>(); - params.put("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(params); - - // then - assertEquals(2, sanitized.size()); - assertEquals(2, sanitized.get("fq").length); - MatcherAssert.assertThat(sanitized.get("fq"), Matchers.arrayContaining("bee:up", "look:left")); - } + @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")); + } }