Cleanup and throw IllegalArgumentException on invalid path

4 files changed, 68 insertions, 58 deletions

diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/SearchPath.java b/container-search/src/main/java/com/yahoo/search/dispatch/SearchPath.java
index 568c9f117d7..bd13c04e8b5 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/SearchPath.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/SearchPath.java
@@ -20,49 +20,16 @@ import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 
 /**
- * Utility class for parsing model.searchPath and filtering a search cluster
- * based on it.
+ * A subset of nodes and groups to which a query should be sent.
+ * See https://docs.vespa.ai/en/reference/query-api-reference.html#model.searchpath
  *
  * @author ollivir
  */
 public class SearchPath {
 
-    /**
-     * Parses the search path and select nodes from the given cluster based on it.
-     *
-     * @param searchPath unparsed search path expression (see: model.searchPath in Search API reference)
-     * @param cluster the search cluster from which nodes are selected
-     * @throws InvalidSearchPathException if the searchPath is malformed
-     * @return list of nodes chosen with the search path, or an empty list in which
-     *         case some other node selection logic should be used
-     */
-    public static List<Node> selectNodes(String searchPath, SearchGroups cluster) {
-        Optional<SearchPath> sp = SearchPath.fromString(searchPath);
-        if (sp.isPresent()) {
-            return sp.get().mapToNodes(cluster);
-        } else {
-            return List.of();
-        }
-    }
-
-    static Optional<SearchPath> fromString(String path) {
-        if (path == null || path.isEmpty()) {
-            return Optional.empty();
-        }
-        if (path.indexOf(';') >= 0) {
-            return Optional.empty(); // multi-level not supported at this time
-        }
-        try {
-            SearchPath sp = parseElement(path);
-            if (sp.isEmpty()) {
-                return Optional.empty();
-            } else {
-                return Optional.of(sp);
-            }
-        } catch (NumberFormatException | InvalidSearchPathException e) {
-            throw new InvalidSearchPathException("Invalid search path: " + path, e);
-        }
-    }
+    // An asterisk or forward slash or an empty string followed by a comma or the end of the string
+    private static final Pattern NODE_WILDCARD = Pattern.compile("^\\*?(?:,|$|/$)");
+    private static final Pattern NODE_RANGE = Pattern.compile("^\\[(\\d+),(\\d+)>(?:,|$)");
 
     private final List<Selection> nodes;
     private final List<Selection> groups;
@@ -73,6 +40,17 @@ public class SearchPath {
         this.groups = groups;
     }
 
+    @Override
+    public String toString() {
+        StringBuilder sb = new StringBuilder();
+        selectionToString(sb, nodes);
+        if ( ! groups.isEmpty()) {
+            sb.append('/');
+            selectionToString(sb, groups);
+        }
+        return sb.toString();
+    }
+
     private List<Node> mapToNodes(SearchGroups cluster) {
         if (cluster.isEmpty()) {
             return List.of();
@@ -112,7 +90,7 @@ public class SearchPath {
                     groupIds.remove(index);
                 }
             } else {
-                throw new InvalidSearchPathException("Invalid searchPath, cluster does not have " + (groupId + 1) + " groups");
+                throw new InvalidSearchPathException("Invalid search path: Cluster does not have " + (groupId + 1) + " groups");
             }
         }
         return cluster.get(groupIds.get(0));
@@ -133,6 +111,43 @@ public class SearchPath {
         return selectRandomGroupWithSufficientCoverage(cluster, new ArrayList<>(cluster.keys()));
     }
 
+    /**
+     * Parses a search path and select nodes from the given cluster based on it.
+     *
+     * @param searchPath unparsed search path expression (see: model.searchPath in Search API reference)
+     * @param cluster the search cluster from which nodes are selected
+     * @throws InvalidSearchPathException if the searchPath is malformed
+     * @return list of nodes chosen with the search path, or an empty list in which
+     *         case some other node selection logic should be used
+     */
+    public static List<Node> selectNodes(String searchPath, SearchGroups cluster) {
+        Optional<SearchPath> sp = SearchPath.fromString(searchPath);
+        if (sp.isPresent()) {
+            return sp.get().mapToNodes(cluster);
+        } else {
+            return List.of();
+        }
+    }
+
+    static Optional<SearchPath> fromString(String path) {
+        if (path == null || path.isEmpty()) {
+            return Optional.empty();
+        }
+        if (path.indexOf(';') >= 0) {
+            return Optional.empty(); // multi-level not supported at this time
+        }
+        try {
+            SearchPath sp = parseElement(path);
+            if (sp.isEmpty()) {
+                return Optional.empty();
+            } else {
+                return Optional.of(sp);
+            }
+        } catch (NumberFormatException | InvalidSearchPathException e) {
+            throw new InvalidSearchPathException("Invalid search path '" + path + "'", e);
+        }
+    }
+
     private static SearchPath parseElement(String element) {
         Pair<String, String> nodesAndGroups = halveAt('/', element);
         List<Selection> nodes = parseSelection(nodesAndGroups.getFirst());
@@ -156,15 +171,10 @@ public class SearchPath {
         return ret;
     }
 
-    // An asterisk or forward slash or an empty string followed by a comma or the end of the string
-    private static final Pattern NODE_WILDCARD = Pattern.compile("^\\*?(?:,|$|/$)");
-
     private static boolean isWildcard(String node) {
         return NODE_WILDCARD.matcher(node).lookingAt();
     }
 
-    private static final Pattern NODE_RANGE = Pattern.compile("^\\[(\\d+),(\\d+)>(?:,|$)");
-
     private static String parseNodeRange(String nodes, List<Selection> into) {
         Matcher m = NODE_RANGE.matcher(nodes);
         if (m.find()) {
@@ -209,18 +219,8 @@ public class SearchPath {
         }
     }
 
-    @Override
-    public String toString() {
-        StringBuilder sb = new StringBuilder();
-        selectionToString(sb, nodes);
-        if ( ! groups.isEmpty()) {
-            sb.append('/');
-            selectionToString(sb, groups);
-        }
-        return sb.toString();
-    }
-
     private static class Selection {
+
         private final int from;
         private final int to;
 
@@ -247,7 +247,8 @@ public class SearchPath {
         }
     }
 
-    public static class InvalidSearchPathException extends RuntimeException {
+    public static class InvalidSearchPathException extends IllegalArgumentException {
+
         public InvalidSearchPathException(String message) {
             super(message);
         }
@@ -255,6 +256,7 @@ public class SearchPath {
         public InvalidSearchPathException(String message, Throwable cause) {
             super(message, cause);
         }
+
     }
 
 }
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Node.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Node.java
index d9c0198a472..73a3c3742cc 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Node.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Node.java
@@ -55,7 +55,11 @@ public class Node {
 
     public String hostname() { return hostname; }
 
-    /** Returns the id of the group this node belongs to */
+    /**
+     * Returns the index of the group this node belongs to.
+     * This is a 0-base continuous integer id, not necessarily the same as the group id assigned by the
+     * application/node repo.
+     */
     public int group() { return group; }
 
     public void setWorking(boolean working) {
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchGroupsImpl.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchGroupsImpl.java
index f5ce987c64c..906268c5904 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchGroupsImpl.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchGroupsImpl.java
@@ -8,16 +8,20 @@ import java.util.Set;
 import java.util.stream.Collectors;
 
 public class SearchGroupsImpl implements SearchGroups {
+
     private final Map<Integer, Group> groups;
     private final double minActivedocsPercentage;
+
     public SearchGroupsImpl(Map<Integer, Group> groups, double minActivedocsPercentage) {
         this.groups = Map.copyOf(groups);
         this.minActivedocsPercentage = minActivedocsPercentage;
     }
+
     @Override public Group get(int id) { return groups.get(id); }
     @Override public Set<Integer> keys() { return groups.keySet();}
     @Override public Collection<Group> groups() { return groups.values(); }
     @Override public int size() { return groups.size(); }
+
     @Override
     public boolean isPartialGroupCoverageSufficient(Collection<Node> nodes) {
         if (size() == 1)
diff --git a/container-search/src/main/java/com/yahoo/search/query/Model.java b/container-search/src/main/java/com/yahoo/search/query/Model.java
index 63fc386963a..190ad675015 100644
--- a/container-search/src/main/java/com/yahoo/search/query/Model.java
+++ b/container-search/src/main/java/com/yahoo/search/query/Model.java
@@ -211,7 +211,7 @@ public class Model implements Cloneable {
         this.encoding = toLowerCase(encoding);
     }
 
-    /** Set the path for which backend nodes to forward the search too. */
+    /** Set the path for which content nodes this query should go to - see  */
     public void setSearchPath(String searchPath) { this.searchPath = searchPath; }
 
     public String getSearchPath() { return searchPath; }