From f7711d712472528b567ab975d0ed677bbd30ac12 Mon Sep 17 00:00:00 2001
From: Alan Woodward <romseygeek@apache.org>
Date: Wed, 16 Oct 2019 16:03:44 +0100
Subject: [PATCH] LUCENE-9005: BooleanQuery.visit() pulls subvisitors from a
 top-level MUST visitor

---
 lucene/CHANGES.txt                            |  6 ++++++
 .../apache/lucene/search/BooleanQuery.java    | 21 ++++++++++++-------
 .../lucene/search/TestQueryVisitor.java       | 18 ++++++++++++++++
 3 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt
index 6fea1351170..fd49a73f68d 100644
--- a/lucene/CHANGES.txt
+++ b/lucene/CHANGES.txt
@@ -217,6 +217,12 @@ Bug Fixes
 * LUCENE-8755: spatial-extras quad and packed quad prefix trees could throw a
   NullPointerException for certain cell edge coordinates (Chongchen Chen, David Smiley)
 
+* LUCENE-9005: BooleanQuery.visit() would pull subVisitors from its parent visitor, rather
+  than from a visitor for its own specific query.  This could cause problems when BQ was
+  nested under another BQ. Instead, we now pull a MUST subvisitor, pass it to any MUST
+  subclauses, and then pull SHOULD, MUST_NOT and FILTER visitors from it rather than from
+  the parent.  (Alan Woodward)
+
 Other
 
 * LUCENE-8778 LUCENE-8911 LUCENE-8957: Define analyzer SPI names as static final fields and document the names in Javadocs.
diff --git a/lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java b/lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java
index def245585d9..2e2d81b53de 100644
--- a/lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java
+++ b/lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java
@@ -506,14 +506,19 @@ public class BooleanQuery extends Query implements Iterable<BooleanClause> {
 
   @Override
   public void visit(QueryVisitor visitor) {
-    for (Map.Entry<Occur, Collection<Query>> entry : clauseSets.entrySet()) {
-      Occur clauseOccur = entry.getKey();
-      Collection<Query> clauseQueries = entry.getValue();
-
-      if (clauseQueries.size() > 0) {
-        QueryVisitor v = visitor.getSubVisitor(clauseOccur, this);
-        for (Query q : clauseQueries) {
-          q.visit(v);
+    QueryVisitor sub = visitor.getSubVisitor(Occur.MUST, this);
+    for (BooleanClause.Occur occur : clauseSets.keySet()) {
+      if (clauseSets.get(occur).size() > 0) {
+        if (occur == Occur.MUST) {
+          for (Query q : clauseSets.get(occur)) {
+            q.visit(sub);
+          }
+        }
+        else {
+          QueryVisitor v = sub.getSubVisitor(occur, this);
+          for (Query q : clauseSets.get(occur)) {
+            q.visit(v);
+          }
         }
       }
     }
diff --git a/lucene/core/src/test/org/apache/lucene/search/TestQueryVisitor.java b/lucene/core/src/test/org/apache/lucene/search/TestQueryVisitor.java
index f1a43105474..27254d8579f 100644
--- a/lucene/core/src/test/org/apache/lucene/search/TestQueryVisitor.java
+++ b/lucene/core/src/test/org/apache/lucene/search/TestQueryVisitor.java
@@ -328,6 +328,24 @@ public class TestQueryVisitor extends LuceneTestCase {
     minimumTermSet.clear();
     extractor.collectTerms(minimumTermSet);
     assertThat(minimumTermSet, equalTo(expected2));
+
+    BooleanQuery bq = new BooleanQuery.Builder()
+        .add(new BooleanQuery.Builder()
+            .add(new TermQuery(new Term("f", "1")), BooleanClause.Occur.MUST)
+            .add(new TermQuery(new Term("f", "61")), BooleanClause.Occur.MUST)
+            .add(new TermQuery(new Term("f", "211")), BooleanClause.Occur.FILTER)
+            .add(new TermQuery(new Term("f", "5")), BooleanClause.Occur.SHOULD)
+            .build(), BooleanClause.Occur.SHOULD)
+        .add(new PhraseQuery("f", "3333", "44444"), BooleanClause.Occur.SHOULD)
+        .build();
+    QueryNode ex2 = new ConjunctionNode();
+    bq.visit(ex2);
+    Set<Term> expected3 = new HashSet<>(Arrays.asList(new Term("f", "1"), new Term("f", "3333")));
+    minimumTermSet.clear();
+    ex2.collectTerms(minimumTermSet);
+    assertThat(minimumTermSet, equalTo(expected3));
+    ex2.getWeight(); // force sort order
+    assertThat(ex2.toString(), equalTo("AND(AND(OR(AND(TERM(f:3333),TERM(f:44444)),AND(TERM(f:1),TERM(f:61),AND(TERM(f:211))))))"));
   }
 
 }