SOLR-8059: debug=results&distrib.singlePass=true can NPE

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

solr/CHANGES.txt

@@ -289,6 +289,9 @@ Bug Fixes
 * SOLR-8422: When authentication enabled, requests fail if sent to a node that doesn't host
   the collection (noble)
 
+* SOLR-8059: &debug=results for distributed search when distrib.singlePass (sometimes activated
+  automatically) could result in an NPE. (David Smiley, Markus Jelsma)
+
 Other Changes
 ----------------------
 

solr/core/src/java/org/apache/solr/handler/component/DebugComponent.java

@@ -24,6 +24,7 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -230,13 +231,7 @@ public class DebugComponent extends SearchComponent
             hasGetDebugResponses = true;
             if (rb.isDebugResults()) {
               NamedList sexplain = (NamedList)sdebug.get("explain");
-              for (int i = 0; i < sexplain.size(); i++) {
+              SolrPluginUtils.copyNamedListIntoArrayByDocPosInResponse(sexplain, rb.resultIds, arr);
-                String id = sexplain.getName(i);
-                // TODO: lookup won't work for non-string ids... String vs Float
-                ShardDoc sdoc = rb.resultIds.get(id);
-                int idx = sdoc.positionInResponse;
-                arr[idx] = new NamedList.NamedListEntry<>(id, sexplain.getVal(i));
-              }
             }
           }
         }
@@ -295,7 +290,7 @@ public class DebugComponent extends SearchComponent
     return namedList;
   }
 
-  Object merge(Object source, Object dest, Set<String> exclude) {
+  protected Object merge(Object source, Object dest, Set<String> exclude) {
     if (source == null) return dest;
     if (dest == null) {
       if (source instanceof NamedList) {
@@ -306,6 +301,10 @@ public class DebugComponent extends SearchComponent
     } else {
 
       if (dest instanceof Collection) {
+        // merge as Set
+        if (!(dest instanceof Set)) {
+          dest = new LinkedHashSet<>((Collection<?>) dest);
+        }
         if (source instanceof Collection) {
           ((Collection)dest).addAll((Collection)source);
         } else {
@@ -337,7 +336,7 @@ public class DebugComponent extends SearchComponent
       NamedList<Object> dl = (NamedList<Object>)dest;
       for (int i=0; i<sl.size(); i++) {
         String skey = sl.getName(i);
-        if (exclude != null && exclude.contains(skey)) continue;
+        if (exclude.contains(skey)) continue;
         Object sval = sl.getVal(i);
         int didx = -1;
 
@@ -354,9 +353,9 @@ public class DebugComponent extends SearchComponent
         }
 
         if (didx == -1) {
-          tmp.add(skey, merge(sval, null, null));
+          tmp.add(skey, merge(sval, null, Collections.emptySet()));
         } else {
-          dl.setVal(didx, merge(sval, dl.getVal(didx), null));
+          dl.setVal(didx, merge(sval, dl.getVal(didx), Collections.emptySet()));
         }
       }
       dl.addAll(tmp);

solr/core/src/java/org/apache/solr/handler/component/HighlightComponent.java

@@ -175,7 +175,7 @@ public class HighlightComponent extends SearchComponent implements PluginInfoIni
   public void finishStage(ResponseBuilder rb) {
     if (rb.doHighlights && rb.stage == ResponseBuilder.STAGE_GET_FIELDS) {
 
-      Map.Entry<String, Object>[] arr = new NamedList.NamedListEntry[rb.resultIds.size()];
+      NamedList.NamedListEntry[] arr = new NamedList.NamedListEntry[rb.resultIds.size()];
 
       // TODO: make a generic routine to do automatic merging of id keyed data
       for (ShardRequest sreq : rb.finished) {
@@ -187,21 +187,12 @@ public class HighlightComponent extends SearchComponent implements PluginInfoIni
             continue;
           }
           NamedList hl = (NamedList)srsp.getSolrResponse().getResponse().get("highlighting");
-          for (int i=0; i<hl.size(); i++) {
+          SolrPluginUtils.copyNamedListIntoArrayByDocPosInResponse(hl, rb.resultIds, arr);
-            String id = hl.getName(i);
-            ShardDoc sdoc = rb.resultIds.get(id);
-            // sdoc maybe null
-            if (sdoc == null) {
-                continue;
-            }
-            int idx = sdoc.positionInResponse;
-            arr[idx] = new NamedList.NamedListEntry<>(id, hl.getVal(i));
-          }
         }
       }
 
       // remove nulls in case not all docs were able to be retrieved
-      rb.rsp.add("highlighting", SolrPluginUtils.removeNulls(arr, new SimpleOrderedMap<Object>()));      
+      rb.rsp.add("highlighting", SolrPluginUtils.removeNulls(arr, new SimpleOrderedMap<>()));
     }
   }
 

solr/core/src/java/org/apache/solr/handler/component/ResponseBuilder.java

@@ -251,6 +251,10 @@ public class ResponseBuilder
     return this.mergeStrategies;
   }
 
+  public RankQuery getRankQuery() {
+    return rankQuery;
+  }
+
   public void setRankQuery(RankQuery rankQuery) {
     this.rankQuery = rankQuery;
   }
@@ -428,7 +432,8 @@ public class ResponseBuilder
     return cmd;
   }
 
-  Query wrap(Query q) {
+  /** Calls {@link RankQuery#wrap(Query)} if there's a rank query, otherwise just returns the query. */
+  public Query wrap(Query q) {
     if(this.rankQuery != null) {
       return this.rankQuery.wrap(q);
     } else {

solr/core/src/java/org/apache/solr/handler/component/TermVectorComponent.java

@@ -457,14 +457,7 @@ public class TermVectorComponent extends SearchComponent implements SolrCoreAwar
           }
 
           // UniqueKey data
-          for (int i=0; i < nl.size(); i++) {
+          SolrPluginUtils.copyNamedListIntoArrayByDocPosInResponse(nl, rb.resultIds, arr);
-            String key = nl.getName(i);
-            ShardDoc sdoc = rb.resultIds.get(key);
-            if (sdoc != null) {// can be null when rb.onePassDistributedQuery
-              int idx = sdoc.positionInResponse;
-              arr[idx] = new NamedList.NamedListEntry<>(key, nl.getVal(i));
-            }
-          }
         }
       }
       // remove nulls in case not all docs were able to be retrieved

solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java

@@ -57,6 +57,7 @@ import org.apache.solr.core.RequestParams;
 import org.apache.solr.core.SolrCore;
 import org.apache.solr.handler.component.HighlightComponent;
 import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
 import org.apache.solr.handler.component.ShardRequest;
 import org.apache.solr.highlight.SolrHighlighter;
 import org.apache.solr.parser.QueryParser;
@@ -811,6 +812,25 @@ public class SolrPluginUtils {
     return dest;
   }
 
+  /** Copies the given {@code namedList} assumed to have doc uniqueKey keyed data into {@code destArr}
+   * at the position of the document in the response.  destArr is assumed to be the same size as
+   * {@code resultIds} is.  {@code resultIds} comes from {@link ResponseBuilder#resultIds}.  If the doc key
+   * isn't in {@code resultIds} then it is ignored.
+   * Note: most likely you will call {@link #removeNulls(Map.Entry[], NamedList)} sometime after calling this. */
+  public static void copyNamedListIntoArrayByDocPosInResponse(NamedList namedList, Map<Object, ShardDoc> resultIds,
+                                                              Map.Entry<String, Object>[] destArr) {
+    assert resultIds.size() == destArr.length;
+    for (int i = 0; i < namedList.size(); i++) {
+      String id = namedList.getName(i);
+      // TODO: lookup won't work for non-string ids... String vs Float
+      ShardDoc sdoc = resultIds.get(id);
+      if (sdoc != null) { // maybe null when rb.onePassDistributedQuery
+        int idx = sdoc.positionInResponse;
+        destArr[idx] = new NamedList.NamedListEntry<>(id, namedList.getVal(i));
+      }
+    }
+  }
+
   /**
    * A subclass of SolrQueryParser that supports aliasing fields for
    * constructing DisjunctionMaxQueries.

solr/core/src/test/org/apache/solr/handler/component/DistributedDebugComponentTest.java

@@ -346,11 +346,17 @@ public class DistributedDebugComponentTest extends SolrJettyTestBase {
   
   public void testCompareWithNonDistributedRequest() throws SolrServerException, IOException {
     SolrQuery query = new SolrQuery();
-    query.setQuery("id:1");
+    query.setQuery("id:1 OR id:2");
-    query.setFilterQueries("id:[0 TO 10]");
+    query.setFilterQueries("id:[0 TO 10]", "id:[0 TO 5]");
+    query.setRows(1);
+    query.setSort("id", SolrQuery.ORDER.asc); // thus only return id:1 since rows 1
     query.set("debug",  "true");
     query.set("distrib", "true");
-    query.setFields("id", "text");
+    query.setFields("id");
+    if (random().nextBoolean()) { // can affect rb.onePassDistributedQuery
+      query.addField("text");
+    }
+    query.set(ShardParams.DISTRIB_SINGLE_PASS, random().nextBoolean());
     query.set("shards", shard1 + "," + shard2);
     QueryResponse distribResponse = collection1.query(query);