SOLR-6011: ComplexPhraseQuery hashCode/equals fix for inOrder

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1589812 13f79535-47bb-0310-9956-ffa450edef68

lucene/CHANGES.txt

@@ -319,6 +319,10 @@ Bug fixes
 * LUCENE-5624: Ensure NativeFSLockFactory does not leak file handles if it is unable
   to obtain the lock. (Uwe Schindler, Robert Muir)
 
+* SOLR-6011: ComplexPhraseQueryParser produced Query objects that did not correctly
+  implement hashCode and equals (inOrder was ignored), causing issues for any
+  system using Query objects as keys. (yonik)
+
 Test Framework
 
 * LUCENE-5592: Incorrectly reported uncloseable files. (Dawid Weiss)

lucene/queryparser/src/java/org/apache/lucene/queryparser/complexPhrase/ComplexPhraseQueryParser.java

@@ -208,11 +208,11 @@ public class ComplexPhraseQueryParser extends QueryParser {
    */
   static class ComplexPhraseQuery extends Query {
 
-    String field;
+    final String field;
 
-    String phrasedQueryStringContents;
+    final String phrasedQueryStringContents;
 
-    int slopFactor;
+    final int slopFactor;
 
     private final boolean inOrder;
 
@@ -394,6 +394,7 @@ public class ComplexPhraseQueryParser extends QueryParser {
           + ((phrasedQueryStringContents == null) ? 0
               : phrasedQueryStringContents.hashCode());
       result = prime * result + slopFactor;
+      result = prime * result + (inOrder ? 1 : 0);
       return result;
     }
 
@@ -422,7 +423,7 @@ public class ComplexPhraseQueryParser extends QueryParser {
         return false;
       if (slopFactor != other.slopFactor)
         return false;
-      return true;
+      return inOrder == other.inOrder;
     }
   }
 }

lucene/queryparser/src/test/org/apache/lucene/queryparser/complexPhrase/TestComplexPhraseQuery.java

@@ -139,6 +139,31 @@ public class TestComplexPhraseQuery extends LuceneTestCase {
     checkMatches("+role:developer +name:jack*", "");
     checkMatches("name:\"john smith\"~2 AND role:designer AND id:3", "3");
   }
+
+  public void testHashcodeEquals() throws Exception {
+    ComplexPhraseQueryParser qp = new ComplexPhraseQueryParser(TEST_VERSION_CURRENT, defaultFieldName, analyzer);
+    qp.setInOrder(true);
+    qp.setFuzzyPrefixLength(1);
+
+    String qString = "\"aaa* bbb*\"";
+
+    Query q = qp.parse(qString);
+    Query q2 = qp.parse(qString);
+
+    assertEquals(q.hashCode(), q2.hashCode());
+    assertEquals(q, q2);
+
+    qp.setInOrder(false); // SOLR-6011
+
+    q2 = qp.parse(qString);
+
+    // although the general contract of hashCode can't guarantee different values, if we only change one thing
+    // about a single query, it normally should result in a different value (and will with the current
+    // implementation in ComplexPhraseQuery)
+    assertTrue(q.hashCode() != q2.hashCode());
+    assertTrue(!q.equals(q2));
+    assertTrue(!q2.equals(q));
+  }
   
   @Override
   public void setUp() throws Exception {

solr/core/src/test/org/apache/solr/search/TestComplexPhraseQParserPlugin.java

@@ -30,7 +30,7 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase {
 
   @BeforeClass
   public static void beforeClass() throws Exception {
-    initCore("solrconfig-query-parser-init.xml","schema-complex-phrase.xml");
+    initCore("solrconfig.xml","schema15.xml");
   }
 
   @Override
@@ -54,43 +54,43 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase {
 
     assertQ(req("q", "{!complexphrase} \"john smith\"")
             , "//result[@numFound='1']"
-            , "//doc[./int[@name='id']='1']"
+            , "//doc[./str[@name='id']='1']"
     );
 
     assertQ(req("q", "{!complexphrase} \"j* smyth~\"")
             , "//result[@numFound='2']"
-            , "//doc[./int[@name='id']='1']"
+            , "//doc[./str[@name='id']='1']"
-            , "//doc[./int[@name='id']='2']"
+            , "//doc[./str[@name='id']='2']"
     );
 
     assertQ(req("q", "{!complexphrase} \"(jo* -john) smith\"")
             , "//result[@numFound='1']"
-            , "//doc[./int[@name='id']='2']"
+            , "//doc[./str[@name='id']='2']"
     );
 
     assertQ(req("q", "{!complexphrase} \"jo* smith\"~2")
             , "//result[@numFound='3']"
-            , "//doc[./int[@name='id']='1']"
+            , "//doc[./str[@name='id']='1']"
-            , "//doc[./int[@name='id']='2']"
+            , "//doc[./str[@name='id']='2']"
-            , "//doc[./int[@name='id']='3']"
+            , "//doc[./str[@name='id']='3']"
     );
 
     assertQ(req("q", "{!complexphrase} \"jo* [sma TO smz]\"")
             , "//result[@numFound='2']"
-            , "//doc[./int[@name='id']='1']"
+            , "//doc[./str[@name='id']='1']"
-            , "//doc[./int[@name='id']='2']"
+            , "//doc[./str[@name='id']='2']"
     );
 
     assertQ(req("q", "{!complexphrase} \"john\"")
             , "//result[@numFound='2']"
-            , "//doc[./int[@name='id']='1']"
+            , "//doc[./str[@name='id']='1']"
-            , "//doc[./int[@name='id']='3']"
+            , "//doc[./str[@name='id']='3']"
     );
 
     assertQ(req("q", "{!complexphrase} \"(john johathon) smith\"")
             , "//result[@numFound='2']"
-            , "//doc[./int[@name='id']='1']"
+            , "//doc[./str[@name='id']='1']"
-            , "//doc[./int[@name='id']='2']"
+            , "//doc[./str[@name='id']='2']"
     );
 
   }
@@ -113,55 +113,55 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase {
 
     assertQ("Simple multi-term still works",
             sumLRF.makeRequest("name:\"john smith\""),
-            "//doc[./int[@name='id']='1']",
+            "//doc[./str[@name='id']='1']",
             "//result[@numFound='1']"
     );
 
     assertQ(req("q", "{!complexphrase} name:\"john smith\""),
-            "//doc[./int[@name='id']='1']",
+            "//doc[./str[@name='id']='1']",
             "//result[@numFound='1']"
     );
 
 
     assertQ("wildcards and fuzzies are OK in phrases",
             sumLRF.makeRequest("name:\"j* smyth~\""),
-            "//doc[./int[@name='id']='1']",
+            "//doc[./str[@name='id']='1']",
-            "//doc[./int[@name='id']='2']",
+            "//doc[./str[@name='id']='2']",
             "//result[@numFound='2']"
     );
 
     assertQ("boolean logic works",
             sumLRF.makeRequest("name:\"(jo* -john) smith\""),
-            "//doc[./int[@name='id']='2']",
+            "//doc[./str[@name='id']='2']",
             "//result[@numFound='1']"
     );
 
     assertQ("position logic works",
             sumLRF.makeRequest("name:\"jo*  smith\"~2"),
-            "//doc[./int[@name='id']='1']",
+            "//doc[./str[@name='id']='1']",
-            "//doc[./int[@name='id']='2']",
+            "//doc[./str[@name='id']='2']",
-            "//doc[./int[@name='id']='3']",
+            "//doc[./str[@name='id']='3']",
             "//result[@numFound='3']"
     );
 
     assertQ("range queries supported",
             sumLRF.makeRequest("name:\"jo* [sma TO smz]\""),
-            "//doc[./int[@name='id']='1']",
+            "//doc[./str[@name='id']='1']",
-            "//doc[./int[@name='id']='2']",
+            "//doc[./str[@name='id']='2']",
             "//result[@numFound='2']"
     );
 
     assertQ("Simple single-term still works",
             sumLRF.makeRequest("name:\"john\""),
-            "//doc[./int[@name='id']='1']",
+            "//doc[./str[@name='id']='1']",
-            "//doc[./int[@name='id']='3']",
+            "//doc[./str[@name='id']='3']",
             "//result[@numFound='2']"
     );
 
     assertQ("OR inside phrase works",
             sumLRF.makeRequest("name:\"(john johathon) smith\""),
-            "//doc[./int[@name='id']='1']",
+            "//doc[./str[@name='id']='1']",
-            "//doc[./int[@name='id']='2']",
+            "//doc[./str[@name='id']='2']",
             "//result[@numFound='2']"
     );
 
@@ -192,9 +192,9 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase {
 
     assertQ("range queries supported",
             sumLRF.makeRequest("name:[sma TO smz]"),
-            "//doc[./int[@name='id']='1']",
+            "//doc[./str[@name='id']='1']",
-            "//doc[./int[@name='id']='2']",
+            "//doc[./str[@name='id']='2']",
-            "//doc[./int[@name='id']='3']",
+            "//doc[./str[@name='id']='3']",
             "//result[@numFound='3']"
     );
 
@@ -246,29 +246,29 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase {
 
     assertQ(req("q", "{!complexphrase} name:\"protein digest\" AND text:\"dna rules\"")
         , "//result[@numFound='1']"
-        , "//doc[./int[@name='id']='3']"
+        , "//doc[./str[@name='id']='3']"
     );
 
     assertQ(req("q", "{!complexphrase} name:\"prot* dige*\" AND text:\"d* r*\"")
         , "//result[@numFound='1']"
-        , "//doc[./int[@name='id']='3']"
+        , "//doc[./str[@name='id']='3']"
     );
 
     assertQ(req("q", "{!complexphrase inOrder=\"false\"} name:\"dna* rule*\" AND text:\"prot* diges*\"")
         , "//result[@numFound='1']"
-        , "//doc[./int[@name='id']='1']"
+        , "//doc[./str[@name='id']='1']"
     );
 
-    assertQ(req("q", "{!unorderedcomplexphrase} name:\"protein digest\" AND text:\"dna rules\"~2")
+    assertQ(req("q", "{!complexphrase inOrder=false} name:\"protein digest\" AND text:\"dna rules\"~2")
         , "//result[@numFound='2']"
-        , "//doc[./int[@name='id']='3']"
+        , "//doc[./str[@name='id']='3']"
-        , "//doc[./int[@name='id']='4']"
+        , "//doc[./str[@name='id']='4']"
     );
 
 
-    assertQ(req("q", "{!unorderedcomplexphrase inOrder=\"true\"} name:\"protein digest\" AND text:\"dna rules\"")
+    assertQ(req("q", "{!complexphrase inOrder=\"true\"} name:\"protein digest\" AND text:\"dna rules\"")
         , "//result[@numFound='1']"
-        , "//doc[./int[@name='id']='3']"
+        , "//doc[./str[@name='id']='3']"
     );
 
   }
@@ -290,49 +290,49 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase {
      */
     assertQ(req("q", "{!complexphrase} \"protein digest\"")
             , "//result[@numFound='1']"
-            , "//doc[./int[@name='id']='1']"
+            , "//doc[./str[@name='id']='1']"
     );
 
     assertQ(req("q", "{!complexphrase} \"pro* di*\"")
             , "//result[@numFound='1']"
-            , "//doc[./int[@name='id']='1']"
+            , "//doc[./str[@name='id']='1']"
     );
 
     assertQ(req("q", "{!complexphrase} name:\"protein digest\"")
             , "//result[@numFound='1']"
-            , "//doc[./int[@name='id']='3']"
+            , "//doc[./str[@name='id']='3']"
     );
 
     assertQ(req("q", "{!complexphrase} name:\"pro* di*\"")
             , "//result[@numFound='1']"
-            , "//doc[./int[@name='id']='3']"
+            , "//doc[./str[@name='id']='3']"
     );
 
     /**
      * unordered phrase query returns two documents.
      */
-    assertQ(req("q", "{!unorderedcomplexphrase} \"digest protein\"")
+    assertQ(req("q", "{!complexphrase inOrder=false} \"digest protein\"")
             , "//result[@numFound='2']"
-            , "//doc[./int[@name='id']='1']"
+            , "//doc[./str[@name='id']='1']"
-            , "//doc[./int[@name='id']='2']"
+            , "//doc[./str[@name='id']='2']"
     );
 
-    assertQ(req("q", "{!unorderedcomplexphrase} \"di* pro*\"")
+    assertQ(req("q", "{!complexphrase inOrder=false} \"di* pro*\"")
             , "//result[@numFound='2']"
-            , "//doc[./int[@name='id']='1']"
+            , "//doc[./str[@name='id']='1']"
-            , "//doc[./int[@name='id']='2']"
+            , "//doc[./str[@name='id']='2']"
     );
 
-    assertQ(req("q", "{!unorderedcomplexphrase} name:\"digest protein\"")
+    assertQ(req("q", "{!complexphrase inOrder=false} name:\"digest protein\"")
             , "//result[@numFound='2']"
-            , "//doc[./int[@name='id']='3']"
+            , "//doc[./str[@name='id']='3']"
-            , "//doc[./int[@name='id']='4']"
+            , "//doc[./str[@name='id']='4']"
     );
 
-    assertQ(req("q", "{!unorderedcomplexphrase} name:\"di* pro*\"")
+    assertQ(req("q", "{!complexphrase inOrder=false} name:\"di* pro*\"")
             , "//result[@numFound='2']"
-            , "//doc[./int[@name='id']='3']"
+            , "//doc[./str[@name='id']='3']"
-            , "//doc[./int[@name='id']='4']"
+            , "//doc[./str[@name='id']='4']"
     );
 
     /**
@@ -340,8 +340,13 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase {
      */
     assertQ(req("q", "{!complexphrase inOrder=false} \"di* pro*\"")
         , "//result[@numFound='2']"
-        , "//doc[./int[@name='id']='1']"
+        , "//doc[./str[@name='id']='1']"
-        , "//doc[./int[@name='id']='2']"
+        , "//doc[./str[@name='id']='2']"
+    );
+
+
+    assertQ(req("q", "{!complexphrase inOrder=true} \"di* pro*\"")
+          , "//result[@numFound='1']"
     );
 
     /**
@@ -349,8 +354,8 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase {
      */
     assertQ(req("q", "{!complexphrase inOrder=false df=name} \"di* pro*\"")
         , "//result[@numFound='2']"
-        , "//doc[./int[@name='id']='3']"
+        , "//doc[./str[@name='id']='3']"
-        , "//doc[./int[@name='id']='4']"
+        , "//doc[./str[@name='id']='4']"
     );
   }
   /**
@@ -369,14 +374,13 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase {
 
     assertQ(req("q", "{!complexphrase} \"sulfur-reducing bacteria\"")
             , "//result[@numFound='2']"
-            , "//doc[./int[@name='id']='1']"
+            , "//doc[./str[@name='id']='1']"
-            , "//doc[./int[@name='id']='2']"
+            , "//doc[./str[@name='id']='2']"
     );
 
+    // the analysis for "name" currently does not break on "-" (only whitespace) and thus only matches one doc
     assertQ(req("q", "{!complexphrase} name:\"sulfur-reducing bacteria\"")
-            , "//result[@numFound='2']"
+            , "//result[@numFound='1']"
-            , "//doc[./int[@name='id']='3']"
-            , "//doc[./int[@name='id']='4']"
     );
   }
 }