SOLR-12275: fix caching for \{!filters} and {{filters}} in \{!parent} and \{!child}

solr/CHANGES.txt

@@ -191,6 +191,9 @@ Bug Fixes
   get an error that's it's a member of that alias.  This check now ensures the alias state is sync()'ed with ZK first.
   (David Smiley)
 
+* SOLR-12275: wrong caching for {!filters} as well as for `filters` local param in {!parent} and {!child}
+  (David Smiley, Mikhail Khluldnev)
+
 Optimizations
 ----------------------
 

solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java

@@ -17,8 +17,6 @@
 
 package org.apache.solr.search.join;
 
-import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.IdentityHashMap;
@@ -36,31 +34,10 @@ import org.apache.solr.common.util.StrUtils;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.search.QParser;
 import org.apache.solr.search.QueryParsing;
-import org.apache.solr.search.SolrConstantScoreQuery;
 import org.apache.solr.search.SyntaxError;
 
 public class FiltersQParser extends QParser {
 
-  private static final class FiltersQuery extends SolrConstantScoreQuery {
-    
-    private Set<Query> filters;
-
-    private FiltersQuery(SolrQueryRequest req, Set<Query> filters) throws IOException {
-      super(req.getSearcher().getDocSet(new ArrayList<>(filters)).getTopFilter());
-      this.filters = filters;
-    }
-
-    @Override
-    public boolean equals(Object other) {
-      return sameClassAs(other) && filters.equals(((FiltersQuery)other).filters);
-    }
-    
-    @Override
-    public int hashCode() {
-      return 31 * classHash() + filters.hashCode();
-    }
-  }
-
   protected String getFiltersParamName() {
     return "param";
   }
@@ -104,21 +81,14 @@ public class FiltersQParser extends QParser {
 
   /** @return number of added clauses */
   protected int addFilters(BooleanQuery.Builder builder, Map<Query,Occur> clauses) throws SyntaxError {
-    Set<Query> filters = new HashSet<>();
+    int count=0;
     for (Map.Entry<Query, Occur> clause: clauses.entrySet()) {
       if (clause.getValue() == Occur.FILTER) {
-        filters.add(clause.getKey());
+        builder.add( clause.getKey(), Occur.FILTER);
+        count++;
       }
     }
-    if (!filters.isEmpty()) {
-      try {
-        final SolrConstantScoreQuery intersQuery = new FiltersQuery(req, filters);
-        builder.add( intersQuery, Occur.FILTER);
-      } catch (IOException e) {
-        throw new SyntaxError("Exception occurs while parsing " + stringIncludingLocalParams, e);
-      }
-    }
-    return filters.size();
+    return count;
   }
 
   protected void exclude(Map<Query,Occur> clauses) {

solr/core/src/test/org/apache/solr/search/join/BJQParserTest.java

@@ -18,6 +18,7 @@ package org.apache.solr.search.join;
 
 import javax.xml.xpath.XPathConstants;
 import java.io.IOException;
+import java.lang.invoke.MethodHandles;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -26,18 +27,26 @@ import java.util.ListIterator;
 import java.util.Locale;
 import java.util.Map;
 
+import org.apache.lucene.search.Query;
+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;
+import org.apache.solr.search.QParser;
+import org.apache.solr.search.SyntaxError;
 import org.apache.solr.util.BaseTestHarness;
 import org.junit.BeforeClass;
 import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class BJQParserTest extends SolrTestCaseJ4 {
   
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
   private static final String[] klm = new String[] {"k", "l", "m"};
   private static final List<String> xyz = Arrays.asList("x", "y", "z");
   private static final String[] abcdef = new String[] {"a", "b", "c", "d", "e", "f"};
@@ -389,8 +398,8 @@ public class BJQParserTest extends SolrTestCaseJ4 {
   }
 
   private final static String elChild[] = new String[]{"//*[@numFound='1']",
-      "//doc[" +
-          "arr[@name=\"child_s\"]/str='l' and child::arr[@name=\"childparent_s\"]/str='e']"};
+      "//doc[" + 
+          "arr[@name=\"child_s\"]/str='l' and arr[@name=\"childparent_s\"]/str='e']"};
 
 
   @Test
@@ -441,10 +450,52 @@ public class BJQParserTest extends SolrTestCaseJ4 {
        "child.fq", "{!tag=secondTag}child_s:l", // 6 ls remains
        "gchq", "{!tag=top}childparent_s:e"), "//*[@numFound='6']");
     
-    assertQ(req("q", // top and filter are excluded, got zero result, but is it right? 
+    assertQ(req("q", // top and filter are excluded, got all results
         "{!filters excludeTags=bot,secondTag,top v=$gchq}" ,
          "child.fq", "{!tag=secondTag}child_s:l",
          "gchq", "{!tag=top}childparent_s:e"), "//*[@numFound='42']");
   }
+  
+  @Test
+  public void testFiltersCache() throws SyntaxError, IOException {
+    final String [] elFilterQuery = new String[] {"q", "{!filters param=$child.fq v=$gchq}",
+        "child.fq", "childparent_s:e",
+        "child.fq", "child_s:l",
+        "gchq", "child_s:[* TO *]"};
+    assertQ(req(elFilterQuery), elChild);
+    final Query query;
+    {
+      final SolrQueryRequest req = req(elFilterQuery);
+      try {
+        QParser parser = QParser.getParser(req.getParams().get("q"), null, req);
+        query = parser.getQuery();
+        final TopDocs topDocs = req.getSearcher().search(query, 10);
+        assertEquals(1, topDocs.totalHits);
+      }finally {
+        req.close();
+      }
+    }
+    assertU(adoc("id", "12275", 
+        "child_s", "l", "childparent_s", "e"));
+    assertU(commit());
+    try {
+      assertQ("here we rely on autowarming for cathing cache leak",  //cache=false
+          req(elFilterQuery), "//*[@numFound='2']");
+      final SolrQueryRequest req = req();
+      try {
+        final TopDocs topDocs = req.getSearcher().search(query, 10);
+        assertEquals("expecting new doc is visible to old query", 2, topDocs.totalHits);
+      }finally {
+        req.close();
+      }
+    }finally {
+      try {
+        assertU(delI("12275"));
+        assertU(commit());
+      } catch(Throwable t) {
+        log.error("ignoring exception occuring in compensation routine", t);
+      }
+    }
+  }
 }