throw error for missing sort direction, sort on unindexed field: SOLR-99 SOLR-9

git-svn-id: https://svn.apache.org/repos/asf/incubator/solr/trunk@495772 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -61,6 +61,9 @@ Bug Fixes
  2. SOLR-92: DOMUtils.getText (used when parsing config files) did not 
     work properly with many DOM implementations when dealing with 
     "Attributes". (Ryan McKinley via hossman)
+ 3. SOLR-9,SOLR-99: Tighten up sort specification error checking, throw
+    exceptions for missing sort specifications or a sort on a non-indexed
+    field. (Ryan McKinley via yonik)
   
 Other Changes
  1.

src/java/org/apache/solr/search/QueryParsing.java

@@ -131,110 +131,82 @@ public class QueryParsing {
   }
 
 
-  private static Pattern sortSeparator = Pattern.compile("[\\s,]+");
+  private static Pattern sortSep = Pattern.compile(",");
 
   /**
-   * Returns null if the sortSpec string doesn't look like a sort specification,
-   * or if the sort specification couldn't be converted into a Lucene Sort
-   * (because of a field not being indexed or undefined, etc).
+   * Returns null if the sortSpec is the standard sort desc.
    *
    * <p>
    * The form of the sort specification string currently parsed is:
    * </p>
    * <pre>>
-   * SortSpec ::= SingleSort [, SingleSort]* <number>?
+   * SortSpec ::= SingleSort [, SingleSort]*
    * SingleSort ::= <fieldname> SortDirection
    * SortDirection ::= top | desc | bottom | asc
    * </pre>
    * Examples:
    * <pre>
-   *   top 10                        #take the top 10 by score
-   *   desc 10                       #take the top 10 by score
-   *   score desc 10                 #take the top 10 by score
-   *   weight bottom 10              #sort by weight ascending and take the first 10
-   *   weight desc                   #sort by weight descending
-   *   height desc,weight desc       #sort by height descending, and use weight descending to break any ties
-   *   height desc,weight asc top 20 #sort by height descending, using weight ascending as a tiebreaker
+   *   score desc               #normal sort by score (will return null)
+   *   weight bottom            #sort by weight ascending 
+   *   weight desc              #sort by weight descending
+   *   height desc,weight desc  #sort by height descending, and use weight descending to break any ties
+   *   height desc,weight asc   #sort by height descending, using weight ascending as a tiebreaker
    * </pre>
    *
    */
   public static SortSpec parseSort(String sortSpec, IndexSchema schema) {
     if (sortSpec==null || sortSpec.length()==0) return null;
 
-    // I wonder how fast the regex is??? as least we cache the pattern.
-    String[] parts = sortSeparator.split(sortSpec.trim(),0);
+    String[] parts = sortSep.split(sortSpec.trim());
     if (parts.length == 0) return null;
 
-    ArrayList<SortField> lst = new ArrayList<SortField>();
-    int num=-1;
-
-    int pos=0;
-    String fn;
-    boolean top=true;
-    boolean normalSortOnScore=false;
-
-    while (pos < parts.length) {
-      String str=parts[pos];
-      if ("top".equals(str) || "bottom".equals(str) || "asc".equals(str) || "desc".equals(str)) {
-        // if the field name seems to be missing, default to "score".
-        // note that this will mess up a field name that has the same name
-        // as a sort direction specifier.
-        fn="score";
-      } else {
-        fn=str;
-        pos++;
+    SortField[] lst = new SortField[parts.length];
+    for( int i=0; i<parts.length; i++ ) {
+      String part = parts[i].trim();
+      boolean top=true;
+        
+      int idx = part.indexOf( ' ' );
+      if( idx > 0 ) {
+        String order = part.substring( idx+1 ).trim();
+    	if( "desc".equals( order ) || "top".equals(order) ) {
+    	  top = true;
+    	}
+    	else if ("asc".equals(order) || "bottom".equals(order)) {
+    	  top = false;
+    	}
+    	else {
+    	  throw new SolrException( 400, "Unknown sort order: "+order);
+    	}
+    	part = part.substring( 0, idx ).trim();
       }
-
-      // get the direction of the sort
-      str=parts[pos];
-      if ("top".equals(str) || "desc".equals(str)) {
-        top=true;
-      } else if ("bottom".equals(str) || "asc".equals(str)) {
-        top=false;
-      }  else {
-        return null;  // must not be a sort command
+      else {
+		throw new SolrException( 400, "Missing sort order." );
       }
-
-      // get the field to sort on
-      // hmmm - should there be a fake/pseudo-field named "score" in the schema?
-      if ("score".equals(fn)) {
+    	
+      if( "score".equals(part) ) {
         if (top) {
-          normalSortOnScore=true;
-          lst.add(SortField.FIELD_SCORE);
-        } else {
-          lst.add(new SortField(null, SortField.SCORE, true));
+          // If thre is only one thing in the list, just do the regular thing...
+          if( parts.length == 1 ) {
+            return null; // do normal scoring...
+          }
+          lst[i] = SortField.FIELD_SCORE;
         }
-      } else {
+        else {
+          lst[i] = new SortField(null, SortField.SCORE, true);
+        }
+      } 
+      else {
         // getField could throw an exception if the name isn't found
-        try {
-          SchemaField f = schema.getField(fn);
-          if (f == null || !f.indexed()) return null;
-          lst.add(f.getType().getSortField(f,top));
-        } catch (Exception e) {
-          return null;
+        SchemaField f = schema.getField(part);
+        if (f == null || !f.indexed()){
+          throw new SolrException( 400, "can not sort on unindexed field: "+part );
         }
-      }
-      pos++;
-
-      // If there is a leftover part, assume it is a count
-      if (pos+1 == parts.length) {
-        try {
-          num = Integer.parseInt(parts[pos]);
-        } catch (Exception e) {
-          return null;
-        }
-        pos++;
+        lst[i] = f.getType().getSortField(f,top);
       }
     }
-
-    Sort sort;
-    if (normalSortOnScore && lst.size() == 1) {
-      // Normalize the default sort on score descending to sort=null
-      sort=null;
-    } else {
-      sort = new Sort((SortField[]) lst.toArray(new SortField[lst.size()]));
-    }
-    return new SortSpec(sort,num);
+    // For more info on the 'num' field, -1, 
+    // see: https://issues.apache.org/jira/browse/SOLR-99
+    return new SortSpec( new Sort(lst),-1);
   }
 
 

src/java/org/apache/solr/util/AbstractSolrTestCase.java

@@ -18,6 +18,7 @@
 
 package org.apache.solr.util;
 
+import org.apache.solr.core.SolrException;
 import org.apache.solr.request.*;
 import org.apache.solr.util.TestHarness;
 
@@ -163,6 +164,19 @@ public abstract class AbstractSolrTestCase extends TestCase {
     }
   }
 
+  /** Makes sure a query throws a SolrException with the listed response code */
+  public void assertQEx(String message, SolrQueryRequest req, int code ) {
+    try {
+      h.query(req);
+      fail( message );
+    } catch (SolrException sex) {
+      assertEquals( code, sex.code() );
+    } catch (Exception e2) {
+      throw new RuntimeException("Exception during query", e2);
+    }
+  }
+
+  
   /**
    * @see TestHarness#optimize
    */

src/test/org/apache/solr/ConvertedLegacyTest.java

@@ -22,6 +22,7 @@ import org.apache.solr.util.*;
 
 import java.util.*;
 import java.io.IOException;
+
     
 /**
  * This tests was converted from a legacy testing system.
@@ -777,7 +778,20 @@ public class ConvertedLegacyTest extends AbstractSolrTestCase {
             ,"//doc[2]/int[.='1000']  "
             ,"//doc[3]/int[.='1001']"
             );
+    
+    // Sort parsing exception tests.  (SOLR-6, SOLR-99)
+    assertQEx( "can not sort unindexed fields",
+        req( "id_i:1000; shouldbeunindexed asc" ), 400 );
+    
+    assertQEx( "invalid query format",
+        req( "id_i:1000; nullfirst" ), 400 );
 
+    assertQEx( "unknown sort field",
+        req( "id_i:1000; abcde12345 asc" ), 1 ); 
+
+    assertQEx( "unknown sort order",
+        req( "id_i:1000; nullfirst aaa" ), 400 ); 
+        
     // test prefix query
 
     assertU("<delete><query>val_s:[* TO *]</query></delete>");