diff --git a/solr/core/src/java/org/apache/solr/schema/AbstractSpatialFieldType.java b/solr/core/src/java/org/apache/solr/schema/AbstractSpatialFieldType.java index c47e6fa418b..b8c8afc547e 100644 --- a/solr/core/src/java/org/apache/solr/schema/AbstractSpatialFieldType.java +++ b/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 extend protected SpatialContext ctx; protected SpatialArgsParser argsParser; - private final ConcurrentHashMap fieldStrategyMap = new ConcurrentHashMap(); + private final Cache fieldStrategyCache = CacheBuilder.newBuilder().build(); @Override protected void init(IndexSchema schema, Map args) { @@ -86,20 +91,20 @@ public abstract class AbstractSpatialFieldType 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 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 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 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 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() { + @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 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."); } } diff --git a/solr/core/src/test/org/apache/solr/search/TestSolr4Spatial.java b/solr/core/src/test/org/apache/solr/search/TestSolr4Spatial.java index 8f03a6a410d..e2dc0cb1857 100644 --- a/solr/core/src/test/org/apache/solr/search/TestSolr4Spatial.java +++ b/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"));