[solr] branch main updated: SOLR-16126: Allow FiltersQParser ({!filters param=[...]}) to tolerate empty param args (#777)

[ma...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

magibney pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git


The following commit(s) were added to refs/heads/main by this push:
     new 0e6d01cb571 SOLR-16126: Allow FiltersQParser ({!filters param=[...]}) to tolerate empty param args (#777)
0e6d01cb571 is described below

commit 0e6d01cb5715081529d59abf0d73845861c0176f
Author: Michael Gibney <mi...@michaelgibney.net>
AuthorDate: Wed Apr 20 12:26:09 2022 -0400

    SOLR-16126: Allow FiltersQParser ({!filters param=[...]}) to tolerate empty param args (#777)
---
 .../apache/solr/search/join/FiltersQParser.java    |  4 ++--
 .../solr/search/TestFiltersQueryCaching.java       | 23 ++++++++++++++++++++++
 .../org/apache/solr/search/join/BJQParserTest.java | 13 ++++++++----
 3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java b/solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java
index b2653d88080..64e3adef3bc 100644
--- a/solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java
+++ b/solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java
@@ -101,8 +101,8 @@ public class FiltersQParser extends QParser {
 
     for (String filter : params == null ? new String[0] : params) {
       if (filter == null || filter.length() == 0) {
-        throw new SyntaxError(
-            "Filter '" + filter + "' has been picked in " + stringIncludingLocalParams);
+        // it is ok to specify a filter param that is not present
+        continue;
       }
       // as a side effect, qparser is mapped by tags in req context
       QParser parser = subQuery(filter, null);
diff --git a/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java b/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java
index cf797fef425..72edc410184 100644
--- a/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java
+++ b/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java
@@ -16,6 +16,10 @@
  */
 package org.apache.solr.search;
 
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
 import java.util.Map;
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.core.SolrCore;
@@ -194,4 +198,23 @@ public class TestFiltersQueryCaching extends SolrTestCaseJ4 {
     assertEquals(3, (long) filterCacheMetrics.get("inserts")); // added top-level
     assertEquals(6, (long) filterCacheMetrics.get("hits"));
   }
+
+  @Test
+  public void testAbsentParams() throws Exception {
+    // no `fqs` at all
+    doTestAbsentParams(Collections.emptyList(), NUM_DOCS);
+    // simple term query `fqs`
+    doTestAbsentParams(List.of("fqs", "{!term f=field_s v='d0'}"), 1);
+    // empty `fqs`
+    doTestAbsentParams(List.of("fqs", ""), NUM_DOCS);
+  }
+
+  private static void doTestAbsentParams(Collection<String> fqsArgs, int expectNumFound)
+      throws Exception {
+    List<String> request = new ArrayList<>();
+    request.addAll(List.of("q", "*:*", "indent", "true", "fq", "{!filters param=$fqs}"));
+    request.addAll(fqsArgs);
+    assertJQ(
+        req(request.toArray(new String[request.size()])), "/response/numFound==" + expectNumFound);
+  }
 }
diff --git a/solr/core/src/test/org/apache/solr/search/join/BJQParserTest.java b/solr/core/src/test/org/apache/solr/search/join/BJQParserTest.java
index 46077112eec..bde3716177c 100644
--- a/solr/core/src/test/org/apache/solr/search/join/BJQParserTest.java
+++ b/solr/core/src/test/org/apache/solr/search/join/BJQParserTest.java
@@ -30,7 +30,6 @@ import org.apache.lucene.search.TopDocs;
 import org.apache.lucene.search.join.ScoreMode;
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.common.SolrException;
-import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.metrics.MetricsMap;
 import org.apache.solr.metrics.SolrMetricManager;
 import org.apache.solr.request.SolrQueryRequest;
@@ -568,12 +567,18 @@ public class BJQParserTest extends SolrTestCaseJ4 {
             "child_s:[* TO *]"),
         "//*[@numFound='18']");
 
-    assertQEx(
-        "expecting exception on weird param",
+    /*
+    The below is admittedly a weird request; but QParser param dereferencing provides no way to
+    distinguish between `{!filters param=}` and `{!filters param=$fqs}` where `fqs` param is
+    absent. Because we want to support the latter case, we must also support the former, even it
+    would arguably make sense to throw a syntax error in the former case.
+     */
+    assertQ(
+        "should support weird absent param spec",
         req(
             "q", "{!filters v=$gchq param=}\"",
             "gchq", "child_s:[* TO *]"),
-        ErrorCode.BAD_REQUEST);
+        "//*[@numFound='18']");
 
     assertQ( // omit main query
         req(