You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by ho...@apache.org on 2014/12/01 19:09:56 UTC

svn commit: r1642727 - in /lucene/dev/trunk/solr: ./ contrib/extraction/src/test-files/extraction/solr/collection1/conf/ contrib/extraction/src/test/org/apache/solr/handler/extraction/ core/src/test-files/solr/collection1/conf/ core/src/test/org/apache...

Author: hossman
Date: Mon Dec  1 18:09:56 2014
New Revision: 1642727

URL: http://svn.apache.org/r1642727
Log:
SOLR-6780: Fixed a bug in how default/appends/invariants params were affecting the set of all keys found in the request parameters, resulting in some key=value param pairs being duplicated.

Modified:
    lucene/dev/trunk/solr/CHANGES.txt
    lucene/dev/trunk/solr/contrib/extraction/src/test-files/extraction/solr/collection1/conf/solrconfig.xml
    lucene/dev/trunk/solr/contrib/extraction/src/test/org/apache/solr/handler/extraction/ExtractingRequestHandlerTest.java
    lucene/dev/trunk/solr/core/src/test-files/solr/collection1/conf/solrconfig.xml
    lucene/dev/trunk/solr/core/src/test/org/apache/solr/request/SimpleFacetsTest.java
    lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/common/params/DefaultSolrParams.java
    lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/common/util/IteratorChain.java
    lucene/dev/trunk/solr/solrj/src/test/org/apache/solr/common/params/SolrParamTest.java

Modified: lucene/dev/trunk/solr/CHANGES.txt
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/CHANGES.txt?rev=1642727&r1=1642726&r2=1642727&view=diff
==============================================================================
--- lucene/dev/trunk/solr/CHANGES.txt (original)
+++ lucene/dev/trunk/solr/CHANGES.txt Mon Dec  1 18:09:56 2014
@@ -487,9 +487,19 @@ Bug Fixes
 * SOLR-6510: The collapse QParser would throw a NPE when used on a DocValues field on
   an empty segment/index. (Christine Poerschke, David Smiley)
 
+* SOLR-6780: Fixed a bug in how default/appends/invariants params were affecting the set 
+  of all "keys" found in the request parameters, resulting in some key=value param pairs 
+  being duplicated.  This was noticably affecting some areas of the code where iteration 
+  was done over the set of all params: 
+    * literal.* in ExtractingRequestHandler
+    * facet.* in FacetComponent
+    * spellcheck.[dictionary name].* and spellcheck.collateParam.* in SpellCheckComponent
+    * olap.* in AnalyticsComponent
+  (Alexandre Rafalovitch & hossman)
+
 ==================  4.10.2 ==================
 
-Bug Fixes
+Bug FixesAnalyticsComponent
 ----------------------
 
 * SOLR-6509: Solr start scripts interactive mode doesn't honor -z argument (Timothy Potter)

Modified: lucene/dev/trunk/solr/contrib/extraction/src/test-files/extraction/solr/collection1/conf/solrconfig.xml
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/contrib/extraction/src/test-files/extraction/solr/collection1/conf/solrconfig.xml?rev=1642727&r1=1642726&r2=1642727&view=diff
==============================================================================
--- lucene/dev/trunk/solr/contrib/extraction/src/test-files/extraction/solr/collection1/conf/solrconfig.xml (original)
+++ lucene/dev/trunk/solr/contrib/extraction/src/test-files/extraction/solr/collection1/conf/solrconfig.xml Mon Dec  1 18:09:56 2014
@@ -194,6 +194,18 @@
   
   <requestHandler name="/update/extract" class="org.apache.solr.handler.extraction.ExtractingRequestHandler"/>
 
+  <requestHandler name="/update/extract/lit-def" class="org.apache.solr.handler.extraction.ExtractingRequestHandler">
+    <lst name="defaults">
+      <str name="literal.foo_s">x</str>
+    </lst>
+    <lst name="appends">
+      <str name="literal.bar_s">y</str>
+    </lst>
+    <lst name="invariants">
+      <str name="literal.zot_s">z</str>
+      <str name="uprefix">ignored_</str>
+    </lst>
+  </requestHandler>
 
   <highlighting>
    <!-- Configure the standard fragmenter -->

Modified: lucene/dev/trunk/solr/contrib/extraction/src/test/org/apache/solr/handler/extraction/ExtractingRequestHandlerTest.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/contrib/extraction/src/test/org/apache/solr/handler/extraction/ExtractingRequestHandlerTest.java?rev=1642727&r1=1642726&r2=1642727&view=diff
==============================================================================
--- lucene/dev/trunk/solr/contrib/extraction/src/test/org/apache/solr/handler/extraction/ExtractingRequestHandlerTest.java (original)
+++ lucene/dev/trunk/solr/contrib/extraction/src/test/org/apache/solr/handler/extraction/ExtractingRequestHandlerTest.java Mon Dec  1 18:09:56 2014
@@ -288,6 +288,74 @@ public class ExtractingRequestHandlerTes
 
   }
 
+  public void testLiteralDefaults() throws Exception {
+
+    // sanity check config
+    loadLocalFromHandler("/update/extract/lit-def",
+                         "extraction/simple.html",
+                         "literal.id", "lit-def-simple");
+    assertU(commit());
+    assertQ(req("q", "id:lit-def-simple")
+            , "//*[@numFound='1']"
+            , "count(//arr[@name='foo_s']/str)=1"
+            , "//arr[@name='foo_s']/str[.='x']"
+            , "count(//arr[@name='bar_s']/str)=1"
+            , "//arr[@name='bar_s']/str[.='y']"
+            , "count(//arr[@name='zot_s']/str)=1"
+            , "//arr[@name='zot_s']/str[.='z']"
+            ); 
+    
+    // override the default foo_s
+    loadLocalFromHandler("/update/extract/lit-def",
+                         "extraction/simple.html",
+                         "literal.foo_s", "1111",
+                         "literal.id", "lit-def-simple");
+    assertU(commit());
+    assertQ(req("q", "id:lit-def-simple")
+            , "//*[@numFound='1']"
+            , "count(//arr[@name='foo_s']/str)=1"
+            , "//arr[@name='foo_s']/str[.='1111']"
+            , "count(//arr[@name='bar_s']/str)=1"
+            , "//arr[@name='bar_s']/str[.='y']"
+            , "count(//arr[@name='zot_s']/str)=1"
+            , "//arr[@name='zot_s']/str[.='z']"
+            ); 
+
+    // pre-pend the bar_s
+    loadLocalFromHandler("/update/extract/lit-def",
+                         "extraction/simple.html",
+                         "literal.bar_s", "2222",
+                         "literal.id", "lit-def-simple");
+    assertU(commit());
+    assertQ(req("q", "id:lit-def-simple")
+            , "//*[@numFound='1']"
+            , "count(//arr[@name='foo_s']/str)=1"
+            , "//arr[@name='foo_s']/str[.='x']"
+            , "count(//arr[@name='bar_s']/str)=2"
+            , "//arr[@name='bar_s']/str[.='2222']"
+            , "//arr[@name='bar_s']/str[.='y']"
+            , "count(//arr[@name='zot_s']/str)=1"
+            , "//arr[@name='zot_s']/str[.='z']"
+            ); 
+
+    // invariant zot_s can not be changed
+    loadLocalFromHandler("/update/extract/lit-def",
+                         "extraction/simple.html",
+                         "literal.zot_s", "3333",
+                         "literal.id", "lit-def-simple");
+    assertU(commit());
+    assertQ(req("q", "id:lit-def-simple")
+            , "//*[@numFound='1']"
+            , "count(//arr[@name='foo_s']/str)=1"
+            , "//arr[@name='foo_s']/str[.='x']"
+            , "count(//arr[@name='bar_s']/str)=1"
+            , "//arr[@name='bar_s']/str[.='y']"
+            , "count(//arr[@name='zot_s']/str)=1"
+            , "//arr[@name='zot_s']/str[.='z']"
+            ); 
+    
+  }
+
   @Test
   public void testPlainTextSpecifyingMimeType() throws Exception {
     ExtractingRequestHandler handler = (ExtractingRequestHandler) h.getCore().getRequestHandler("/update/extract");
@@ -612,7 +680,9 @@ public class ExtractingRequestHandlerTes
     assertQ(req("wdf_nocase:\"Test password protected word doc\""), "//*[@numFound='2']");
   }
   
-  SolrQueryResponse loadLocal(String filename, String... args) throws Exception {
+  SolrQueryResponse loadLocalFromHandler(String handler, String filename, 
+                                         String... args) throws Exception {
+                              
     LocalSolrQueryRequest req = (LocalSolrQueryRequest) req(args);
     try {
       // TODO: stop using locally defined streams once stream.file and
@@ -620,11 +690,15 @@ public class ExtractingRequestHandlerTes
       List<ContentStream> cs = new ArrayList<>();
       cs.add(new ContentStreamBase.FileStream(getFile(filename)));
       req.setContentStreams(cs);
-      return h.queryAndResponse("/update/extract", req);
+      return h.queryAndResponse(handler, req);
     } finally {
       req.close();
     }
   }
 
+  SolrQueryResponse loadLocal(String filename, String... args) throws Exception {
+    return loadLocalFromHandler("/update/extract", filename, args);
+  }
+
 
 }

Modified: lucene/dev/trunk/solr/core/src/test-files/solr/collection1/conf/solrconfig.xml
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/core/src/test-files/solr/collection1/conf/solrconfig.xml?rev=1642727&r1=1642726&r2=1642727&view=diff
==============================================================================
--- lucene/dev/trunk/solr/core/src/test-files/solr/collection1/conf/solrconfig.xml (original)
+++ lucene/dev/trunk/solr/core/src/test-files/solr/collection1/conf/solrconfig.xml Mon Dec  1 18:09:56 2014
@@ -482,6 +482,21 @@
     </lst>
   </requestHandler>
 
+  <requestHandler name="/search-facet-def" class="solr.SearchHandler" >
+    <lst name="defaults">
+      <str name="facet.field">foo_s</str>
+    </lst>
+    <lst name="appends">
+      <str name="facet.query">foo_s:bar</str>
+    </lst>
+  </requestHandler>
+  <requestHandler name="/search-facet-invariants" class="solr.SearchHandler" >
+    <lst name="invariants">
+      <str name="facet.field">foo_s</str>
+      <str name="facet.query">foo_s:bar</str>
+    </lst>
+  </requestHandler>
+
   <admin>
     <defaultQuery>solr</defaultQuery>
     <gettableFiles>solrconfig.xml schema.xml admin-extra.html</gettableFiles>

Modified: lucene/dev/trunk/solr/core/src/test/org/apache/solr/request/SimpleFacetsTest.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/core/src/test/org/apache/solr/request/SimpleFacetsTest.java?rev=1642727&r1=1642726&r2=1642727&view=diff
==============================================================================
--- lucene/dev/trunk/solr/core/src/test/org/apache/solr/request/SimpleFacetsTest.java (original)
+++ lucene/dev/trunk/solr/core/src/test/org/apache/solr/request/SimpleFacetsTest.java Mon Dec  1 18:09:56 2014
@@ -126,6 +126,59 @@ public class SimpleFacetsTest extends So
     add_doc("id", "2004", "hotel_s1", "b", "airport_s1", "ams", "duration_i1", "5");
   }
 
+
+  public void testDefaultsAndAppends() throws Exception {
+    // all defaults
+    assertQ( req("indent","true", "q","*:*", "rows","0", "facet","true", "qt","/search-facet-def")
+             // only one default facet.field
+             ,"//lst[@name='facet_fields']/lst[@name='foo_s']"
+             ,"count(//lst[@name='facet_fields']/lst[@name='foo_s'])=1"
+             ,"count(//lst[@name='facet_fields']/lst)=1"
+             // only one default facet.query
+             ,"//lst[@name='facet_queries']/int[@name='foo_s:bar']"
+             ,"count(//lst[@name='facet_queries']/int[@name='foo_s:bar'])=1"
+             ,"count(//lst[@name='facet_queries']/int)=1"
+             );
+
+    // override default & pre-pend to appends
+    assertQ( req("indent","true", "q","*:*", "rows","0", "facet","true", "qt","/search-facet-def",
+                 "facet.field", "bar_s",
+                 "facet.query", "bar_s:yak"
+                 )
+             // override single default facet.field
+             ,"//lst[@name='facet_fields']/lst[@name='bar_s']"
+             ,"count(//lst[@name='facet_fields']/lst[@name='bar_s'])=1"
+             ,"count(//lst[@name='facet_fields']/lst)=1"
+             // add an additional facet.query
+             ,"//lst[@name='facet_queries']/int[@name='foo_s:bar']"
+             ,"//lst[@name='facet_queries']/int[@name='bar_s:yak']"
+             ,"count(//lst[@name='facet_queries']/int[@name='foo_s:bar'])=1"
+             ,"count(//lst[@name='facet_queries']/int[@name='bar_s:yak'])=1"
+             ,"count(//lst[@name='facet_queries']/int)=2"
+             );
+  }
+
+  public void testInvariants() throws Exception {
+    // no matter if we try to use facet.field or facet.query, results shouldn't change
+    for (String ff : new String[] { "facet.field", "bogus" }) {
+      for (String fq : new String[] { "facet.query", "bogus" }) {
+        assertQ( req("indent","true", "q", "*:*", "rows","0", "facet","true", 
+                     "qt","/search-facet-invariants",
+                     ff, "bar_s",
+                     fq, "bar_s:yak")
+                 // only one invariant facet.field
+                 ,"//lst[@name='facet_fields']/lst[@name='foo_s']"
+                 ,"count(//lst[@name='facet_fields']/lst[@name='foo_s'])=1"
+                 ,"count(//lst[@name='facet_fields']/lst)=1"
+                 // only one invariant facet.query
+                 ,"//lst[@name='facet_queries']/int[@name='foo_s:bar']"
+                 ,"count(//lst[@name='facet_queries']/int[@name='foo_s:bar'])=1"
+                 ,"count(//lst[@name='facet_queries']/int)=1"
+                 );
+      }
+    }
+  }
+
   @Test
   public void testCachingBigTerms() throws Exception {
     assertQ( req("indent","true", "q", "id:[42 TO 47]",

Modified: lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/common/params/DefaultSolrParams.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/common/params/DefaultSolrParams.java?rev=1642727&r1=1642726&r2=1642727&view=diff
==============================================================================
--- lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/common/params/DefaultSolrParams.java (original)
+++ lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/common/params/DefaultSolrParams.java Mon Dec  1 18:09:56 2014
@@ -18,8 +18,7 @@
 package org.apache.solr.common.params;
 
 import java.util.Iterator;
-
-import org.apache.solr.common.util.IteratorChain;
+import java.util.LinkedHashSet;
 
 /**
  *
@@ -52,10 +51,17 @@ public class DefaultSolrParams extends S
 
   @Override
   public Iterator<String> getParameterNamesIterator() {
-    final IteratorChain<String> c = new IteratorChain<>();
-    c.addIterator(defaults.getParameterNamesIterator());
-    c.addIterator(params.getParameterNamesIterator());
-    return c;
+    // We need to compute the set of all param names in advance 
+    // So we don't wind up with an iterator that returns the same
+    // String more then once (SOLR-6780)
+    LinkedHashSet<String> allKeys = new LinkedHashSet<>();
+    for (SolrParams p : new SolrParams [] {params, defaults}) {
+      Iterator<String> localKeys = p.getParameterNamesIterator();
+      while (localKeys.hasNext()) {
+        allKeys.add(localKeys.next());
+      }
+    }
+    return allKeys.iterator();
   }
 
   @Override

Modified: lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/common/util/IteratorChain.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/common/util/IteratorChain.java?rev=1642727&r1=1642726&r2=1642727&view=diff
==============================================================================
--- lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/common/util/IteratorChain.java (original)
+++ lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/common/util/IteratorChain.java Mon Dec  1 18:09:56 2014
@@ -23,8 +23,10 @@ import java.util.List;
 
 /** Chain several Iterators, so that this iterates
  *  over all of them in sequence.
+ *
+ * @deprecated This class is no longer used by Solr, and may be removed in future versions
  */
-
+@Deprecated
 public class IteratorChain<E> implements Iterator<E> {
 
   private final List<Iterator<E>> iterators = new ArrayList<>();

Modified: lucene/dev/trunk/solr/solrj/src/test/org/apache/solr/common/params/SolrParamTest.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/solrj/src/test/org/apache/solr/common/params/SolrParamTest.java?rev=1642727&r1=1642726&r2=1642727&view=diff
==============================================================================
--- lucene/dev/trunk/solr/solrj/src/test/org/apache/solr/common/params/SolrParamTest.java (original)
+++ lucene/dev/trunk/solr/solrj/src/test/org/apache/solr/common/params/SolrParamTest.java Mon Dec  1 18:09:56 2014
@@ -19,14 +19,93 @@ package org.apache.solr.common.params;
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.List;
+import java.util.ArrayList;
+import java.util.Iterator;
 
 import org.apache.lucene.util.LuceneTestCase;
 import org.apache.solr.common.SolrException;
 
 /**
  */
-public class SolrParamTest extends LuceneTestCase 
-{  
+public class SolrParamTest extends LuceneTestCase {  
+
+  public void testParamIterators() {
+
+    ModifiableSolrParams aaa = new ModifiableSolrParams();
+    aaa.add("foo", "a1");
+    aaa.add("foo", "a2");
+
+    assertIterSize("aaa: foo", 1, aaa);
+    assertIterSize("required aaa: foo", 1, aaa.required());
+
+    assertEquals(new String[] { "a1", "a2" }, aaa.getParams("foo"));
+
+    aaa.add("yak", "a3");
+
+    assertIterSize("aaa: foo & yak", 2, aaa);
+    assertIterSize("required aaa: foo & yak", 2, aaa.required());
+
+    assertEquals(new String[] { "a1", "a2" }, aaa.getParams("foo"));
+    assertEquals(new String[] { "a3" }, aaa.getParams("yak"));
+
+    ModifiableSolrParams bbb = new ModifiableSolrParams();
+    bbb.add("foo", "b1");
+    bbb.add("foo", "b2");
+    bbb.add("zot", "b3");
+
+    assertIterSize("bbb: foo & zot", 2, bbb);
+    assertIterSize("required bbb: foo & zot", 2, bbb.required());
+
+    assertEquals(new String[] { "b1", "b2" }, bbb.getParams("foo"));
+    assertEquals(new String[] { "b3" }, bbb.getParams("zot"));
+
+    SolrParams def = SolrParams.wrapDefaults(aaa, bbb);
+
+    assertIterSize("def: aaa + bbb", 3, def);
+    assertIterSize("required def: aaa + bbb", 3, def.required());
+
+    assertEquals(new String[] { "a1", "a2" }, def.getParams("foo"));
+    assertEquals(new String[] { "a3" }, def.getParams("yak"));
+    assertEquals(new String[] { "b3" }, def.getParams("zot"));
+
+    SolrParams append = SolrParams.wrapAppended(aaa, bbb);
+
+    assertIterSize("append: aaa + bbb", 3, append);
+    assertIterSize("required appended: aaa + bbb", 3, append.required());
+
+    assertEquals(new String[] { "a1", "a2", "b1", "b2", }, append.getParams("foo"));
+    assertEquals(new String[] { "a3" }, append.getParams("yak"));
+    assertEquals(new String[] { "b3" }, append.getParams("zot"));
+
+  }
+
+  public void testModParamAddParams() {
+
+    ModifiableSolrParams aaa = new ModifiableSolrParams();
+    aaa.add("foo", "a1");
+    aaa.add("foo", "a2");
+    aaa.add("yak", "a3");
+    
+    ModifiableSolrParams bbb = new ModifiableSolrParams();
+    bbb.add("foo", "b1");
+    bbb.add("foo", "b2");
+    bbb.add("zot", "b3");
+    
+    SolrParams def = SolrParams.wrapDefaults(aaa, bbb);
+    assertEquals(new String[] { "a1", "a2" }, def.getParams("foo"));
+    assertEquals(new String[] { "a3" }, def.getParams("yak"));
+    assertEquals(new String[] { "b3" }, def.getParams("zot"));
+
+    ModifiableSolrParams combined = new ModifiableSolrParams();
+    combined.add(def);
+
+    assertEquals(new String[] { "a1", "a2" }, combined.getParams("foo"));
+    assertEquals(new String[] { "a3" }, combined.getParams("yak"));
+    assertEquals(new String[] { "b3" }, combined.getParams("zot"));
+
+  }
+
   public void testGetParams() {
     Map<String,String> pmap = new HashMap<>();
     pmap.put( "str"        , "string"   );
@@ -194,4 +273,18 @@ public class SolrParamTest extends Lucen
     }
     return 200;
   }
+
+  static <T> List<T> iterToList(Iterator<T> iter) {
+    List<T> result = new ArrayList<>();
+    while (iter.hasNext()) {
+      result.add(iter.next());
+    }
+    return result;
+  }
+
+  static void assertIterSize(String msg, int expectedSize, SolrParams p) {
+    List<String> keys = iterToList(p.getParameterNamesIterator());
+    assertEquals(msg + " " + keys.toString(), expectedSize, keys.size());
+  }
+
 }