SOLR-4231: AbstractSpatialFieldType extensibility, and throw 400 code for invalid shapes

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

solr/core/src/java/org/apache/solr/schema/AbstractSpatialFieldType.java

@@ -17,8 +17,12 @@ package org.apache.solr.schema;
  * limitations under the License.
  */
 
+import com.google.common.base.Throwables;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
 import com.spatial4j.core.context.SpatialContext;
 import com.spatial4j.core.context.SpatialContextFactory;
+import com.spatial4j.core.exception.InvalidShapeException;
 import com.spatial4j.core.shape.Point;
 import com.spatial4j.core.shape.Rectangle;
 import com.spatial4j.core.shape.Shape;
@@ -45,7 +49,8 @@ import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
 
 /**
  * Abstract base class for Solr FieldTypes based on a Lucene 4 {@link SpatialStrategy}.
@@ -61,7 +66,7 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
   protected SpatialContext ctx;
   protected SpatialArgsParser argsParser;
 
-  private final ConcurrentHashMap<String, T> fieldStrategyMap = new ConcurrentHashMap<String,T>();
+  private final Cache<String, T> fieldStrategyCache = CacheBuilder.newBuilder().build();
 
   @Override
   protected void init(IndexSchema schema, Map<String, String> args) {
@@ -86,20 +91,20 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
 
   @Override
   public final Field createField(SchemaField field, Object val, float boost) {
-    throw new IllegalStateException("should be calling createFields because isPolyField() is true");
+    throw new IllegalStateException("instead call createFields() because isPolyField() is true");
   }
 
   @Override
-  public final Field[] createFields(SchemaField field, Object val, float boost) {
+  public Field[] createFields(SchemaField field, Object val, float boost) {
     String shapeStr = null;
     Shape shape = null;
     if (val instanceof Shape) {
       shape = ((Shape) val);
     } else {
       shapeStr = val.toString();
-      shape = ctx.readShape(shapeStr);
+      shape = parseShape(shapeStr);
     }
-    if( shape == null ) {
+    if (shape == null) {
       log.debug("Field {}: null shape for input: {}", field, val);
       return null;
     }
@@ -131,6 +136,14 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
     }
   }
 
+  protected Shape parseShape(String shapeStr) {
+    try {
+      return ctx.readShape(shapeStr);
+    } catch (InvalidShapeException e) {
+      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e);
+    }
+  }
+
   protected String shapeToString(Shape shape) {
     return ctx.toString(shape);
   }
@@ -151,8 +164,8 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
   public Query getRangeQuery(QParser parser, SchemaField field, String part1, String part2, boolean minInclusive, boolean maxInclusive) {
     if (!minInclusive || !maxInclusive)
       throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Both sides of spatial range query must be inclusive: " + field.getName());
-    Shape shape1 = ctx.readShape(part1);
-    Shape shape2 = ctx.readShape(part2);
+    Shape shape1 = parseShape(part1);
+    Shape shape2 = parseShape(part2);
     if (!(shape1 instanceof Point) || !(shape2 instanceof Point))
       throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Both sides of spatial range query must be points: " + field.getName());
     Point p1 = (Point) shape1;
@@ -165,14 +178,22 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
   @Override
   public ValueSource getValueSource(SchemaField field, QParser parser) {
     //This is different from Solr 3 LatLonType's approach which uses the MultiValueSource concept to directly expose
-    // the an x & y pair of FieldCache value sources.
+    // the x & y pair of FieldCache value sources.
     throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
         "A ValueSource isn't directly available from this field. Instead try a query using the distance as the score.");
   }
 
   @Override
   public Query getFieldQuery(QParser parser, SchemaField field, String externalVal) {
-    return getQueryFromSpatialArgs(parser, field, argsParser.parse(externalVal, ctx));
+    return getQueryFromSpatialArgs(parser, field, parseSpatialArgs(externalVal));
+  }
+
+  protected SpatialArgs parseSpatialArgs(String externalVal) {
+    try {
+      return argsParser.parse(externalVal, ctx);
+    } catch (Exception e) {
+      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e);
+    }
   }
 
   private Query getQueryFromSpatialArgs(QParser parser, SchemaField field, SpatialArgs spatialArgs) {
@@ -208,18 +229,16 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
    * @return Non-null.
    */
   public T getStrategy(final String fieldName) {
-    T strategy = fieldStrategyMap.get(fieldName);
-    //double-checked locking idiom
-    if (strategy == null) {
-      synchronized (fieldStrategyMap) {
-        strategy = fieldStrategyMap.get(fieldName);
-        if (strategy == null) {
-          strategy = newSpatialStrategy(fieldName);
-          fieldStrategyMap.put(fieldName,strategy);
+    try {
+      return fieldStrategyCache.get(fieldName, new Callable<T>() {
+        @Override
+        public T call() throws Exception {
+          return newSpatialStrategy(fieldName);
         }
-      }
+      });
+    } catch (ExecutionException e) {
+      throw Throwables.propagate(e.getCause());
     }
-    return strategy;
   }
 
   @Override
@@ -229,7 +248,8 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
 
   @Override
   public SortField getSortField(SchemaField field, boolean top) {
-    throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Sorting not supported on SpatialField: " + field.getName());
+    throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Sorting not supported on SpatialField: " + field.getName()+
+      ", instead try sorting by query.");
   }
 }
 

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

@@ -22,6 +22,7 @@ import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
 import com.spatial4j.core.context.SpatialContext;
 import com.spatial4j.core.distance.DistanceUtils;
 import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrException;
 import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
@@ -60,6 +61,26 @@ public class TestSolr4Spatial extends SolrTestCaseJ4 {
     assertU(commit());
   }
 
+  @Test
+  public void testBadShapeParse400() {
+    assertQEx(null, req(
+        "fl", "id," + fieldName, "q", "*:*", "rows", "1000",
+        "fq", "{!field f="+fieldName+"}Intersects(NonexistentShape(89.9,-130 d=9))"), 400);
+    assertQEx(null, req(
+        "fl", "id," + fieldName, "q", "*:*", "rows", "1000",
+        "fq", "{!field f="+fieldName+"}Intersects(NonexistentShape(89.9,-130 d=9"), 400);//missing parens
+    assertQEx(null, req(
+        "fl", "id," + fieldName, "q", "*:*", "rows", "1000",
+        "fq", "{!field f="+fieldName+"}Intersectssss"), 400);
+
+    try {
+      assertU(adoc("id", "-1", fieldName, "NonexistentShape"));
+      fail();
+    } catch (SolrException e) {
+      assertEquals(400, e.code());
+    }
+  }
+
   private void setupDocs() {
     assertU(adoc("id", "1", fieldName, "32.7693246, -79.9289094"));
     assertU(adoc("id", "2", fieldName, "33.7693246, -80.9289094"));