SOLR-8859: Fix AbstractSpatialFieldType to not cycle through all Spatial4j provided formats.

And Fix RptWithGeometrySpatialField to be less brittle on init()

solr/CHANGES.txt

@@ -81,8 +81,8 @@ New Features
   https://github.com/locationtech/spatial4j/blob/master/FORMATS.md
   To return the FeatureCollection as the root element, add '&omitHeader=true" (ryan)
 
-* SOLR-8859: AbstractSpatialFieldType will now convert Shapes to/from Strings
-  using the SpatialContext.  (ryan)
+* SOLR-8859: Spatial fields like RPT can now be configured to use Spatial4j registered shape formats
+  e.g. via format="GeoJSON".  (ryan, David Smiley)
 
 * SOLR-445: new ToleranteUpdateProcessorFactory to support skipping update commands that cause
   failures when sending multiple updates in a single request.

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

@@ -26,9 +26,11 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.TreeSet;
-import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 
+import com.google.common.base.Throwables;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
 import org.apache.lucene.document.Field;
 import org.apache.lucene.document.StoredField;
 import org.apache.lucene.index.IndexableField;
@@ -42,7 +44,6 @@ import org.apache.lucene.spatial.SpatialStrategy;
 import org.apache.lucene.spatial.query.SpatialArgs;
 import org.apache.lucene.spatial.query.SpatialArgsParser;
 import org.apache.lucene.spatial.query.SpatialOperation;
-import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.response.TextResponseWriter;
@@ -64,10 +65,6 @@ import org.locationtech.spatial4j.shape.Shape;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import com.google.common.base.Throwables;
-import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
-
 /**
  * Abstract base class for Solr FieldTypes based on a Lucene 4 {@link SpatialStrategy}.
  *
@@ -141,24 +138,21 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
     }
 
     final SupportedFormats fmts = ctx.getFormats();
-    final String format = args.remove(FORMAT);
-    if (format != null) {
-      shapeWriter = fmts.getWriter(format);
-      shapeReader = fmts.getReader(format);
-      if(shapeWriter==null) {
-        throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
-            "Unknown Shape Format: "+ format);
-      }
-      if(shapeReader==null) {
-        throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
-            "Unknown Shape Format: "+ format);
-      }
+    String format = args.remove(FORMAT);
+    if (format == null) {
+      format = "WKT";
     }
-    else {
-      // Otherwise, pick the first supported reader/writer
-      shapeWriter = fmts.getWriters().get(0);
-      shapeReader = fmts.getReaders().get(0);
+    shapeWriter = fmts.getWriter(format);
+    shapeReader = fmts.getReader(format);
+    if(shapeWriter==null) {
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+          "Unknown Shape Format: "+ format);
     }
+    if(shapeReader==null) {
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+          "Unknown Shape Format: "+ format);
+    }
+
     argsParser = newSpatialArgsParser();
   }
 
@@ -234,23 +228,29 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
 
   /** Create a {@link Shape} from the input string */
   public Shape parseShape(String str) {
+    str = str.trim();
     if (str.length() == 0)
       throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "empty string shape");
 
-    Shape shape = null;
-    if(shapeReader!=null) {
-      shape = shapeReader.readIfSupported(str);
+    // If the first char is promising, try to parse with SpatialUtils.parsePoint
+    char firstChar = str.charAt(0);
+    if (firstChar == '+' || firstChar == '-' || (firstChar >= '0' && firstChar <= '9')) {
+      try {
+        return SpatialUtils.parsePoint(str, ctx);
+      } catch (Exception e) {//ignore
+      }
     }
 
-    if(shape==null) {
-      // Try all supported formats
-      shape = ctx.getFormats().read(str);
+    try {
+      return shapeReader.read(str);
+    } catch (Exception e) {
+      String msg = "Unable to parse shape given formats" +
+          " \"lat,lon\", \"x y\" or as " + shapeReader.getFormatName() + " because " + e;
+      if (!msg.contains(str)) {
+        msg += " input: " + str;
+      }
+      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, msg, e);
     }
-
-    if(shape==null) {
-       throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Unable to parse shape from: "+str);
-    }
-    return shape;
   }
 
   /**
@@ -259,11 +259,7 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
    * The format can be selected using the initParam <code>format={WKT|GeoJSON}</code>
    */
   public String shapeToString(Shape shape) {
-    if(shapeWriter!=null) {
-      return shapeWriter.toString(shape);
-    }
-    // This will only happen if the context does not have any writers
-    throw new SolrException(ErrorCode.SERVER_ERROR, "ShapeWriter not configured");
+    return shapeWriter.toString(shape);
   }
 
   /** Called from {@link #getStrategy(String)} upon first use by fieldName. } */

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

@@ -18,11 +18,9 @@ package org.apache.solr.schema;
 
 import java.io.IOException;
 import java.lang.ref.WeakReference;
+import java.util.HashMap;
 import java.util.Map;
 
-import org.locationtech.spatial4j.context.SpatialContext;
-import org.locationtech.spatial4j.shape.Shape;
-import org.locationtech.spatial4j.shape.jts.JtsGeometry;
 import org.apache.lucene.analysis.Analyzer;
 import org.apache.lucene.index.LeafReaderContext;
 import org.apache.lucene.queries.function.FunctionValues;
@@ -36,6 +34,9 @@ import org.apache.solr.common.SolrException;
 import org.apache.solr.core.SolrCore;
 import org.apache.solr.request.SolrRequestInfo;
 import org.apache.solr.search.SolrCache;
+import org.locationtech.spatial4j.context.SpatialContext;
+import org.locationtech.spatial4j.shape.Shape;
+import org.locationtech.spatial4j.shape.jts.JtsGeometry;
 
 /** A Solr Spatial FieldType based on {@link CompositeSpatialStrategy}.
  * @lucene.experimental */
@@ -48,7 +49,8 @@ public class RptWithGeometrySpatialField extends AbstractSpatialFieldType<Compos
 
   @Override
   protected void init(IndexSchema schema, Map<String, String> args) {
-    // Do NOT call super.init(); instead we delegate to an RPT field. Admittedly this is error prone.
+    Map<String, String> origArgs = new HashMap<>(args); // clone so we can feed it to an aggregated field type
+    super.init(schema, origArgs);
 
     //TODO Move this check to a call from AbstractSpatialFieldType.createFields() so the type can declare
     // if it supports multi-valued or not. It's insufficient here; we can't see if you set multiValued on the field.
@@ -65,6 +67,7 @@ public class RptWithGeometrySpatialField extends AbstractSpatialFieldType<Compos
     rptFieldType.setTypeName(getTypeName());
     rptFieldType.properties = properties;
     rptFieldType.init(schema, args);
+
     rptFieldType.argsParser = argsParser = newSpatialArgsParser();
     this.ctx = rptFieldType.ctx;
     this.distanceUnits = rptFieldType.distanceUnits;

solr/core/src/test/org/apache/solr/schema/SpatialRPTFieldTypeTest.java

@@ -21,6 +21,7 @@ import java.util.HashMap;
 import java.util.Map;
 
 import org.apache.commons.io.FileUtils;
+import org.apache.solr.common.SolrException;
 import org.apache.solr.core.AbstractBadConfigTestBase;
 import org.junit.After;
 import org.junit.Before;
@@ -203,8 +204,8 @@ public class SpatialRPTFieldTypeTest extends AbstractBadConfigTestBase {
   }
 
   public void testShapeToFromStringWKT() throws Exception {
-    // Check WKT
-    setupRPTField("miles", "true", "WKT");
+    setupRPTField("miles", "true", "WKT", random().nextBoolean()
+        ? new SpatialRecursivePrefixTreeFieldType() : new RptWithGeometrySpatialField());
 
     AbstractSpatialFieldType ftype = (AbstractSpatialFieldType)
         h.getCore().getLatestSchema().getField("geo").getType();
@@ -214,11 +215,20 @@ public class SpatialRPTFieldTypeTest extends AbstractBadConfigTestBase {
     String out = ftype.shapeToString(shape);
 
     assertEquals(wkt, out);
+
+    //assert fails GeoJSON
+    try {
+      ftype.parseShape("{\"type\":\"Point\",\"coordinates\":[1,2]}");
+      fail("Should not parse GeoJSON if told format is WKT");
+    } catch (SolrException e) {// expected
+      System.out.println(e);
+    }
+
   }
 
   public void testShapeToFromStringGeoJSON() throws Exception {
-    // Check WKT
-    setupRPTField("miles", "true", "GeoJSON");
+    setupRPTField("miles", "true", "GeoJSON", random().nextBoolean()
+        ? new SpatialRecursivePrefixTreeFieldType() : new RptWithGeometrySpatialField());
 
     AbstractSpatialFieldType ftype = (AbstractSpatialFieldType)
         h.getCore().getLatestSchema().getField("geo").getType();
@@ -230,7 +240,7 @@ public class SpatialRPTFieldTypeTest extends AbstractBadConfigTestBase {
     assertEquals(json, out);
   }
 
-  private void setupRPTField(String distanceUnits, String geo, String format) throws Exception {
+  private void setupRPTField(String distanceUnits, String geo, String format, FieldType fieldType) throws Exception {
     deleteCore();
     File managedSchemaFile = new File(tmpConfDir, "managed-schema");
     Files.delete(managedSchemaFile.toPath()); // Delete managed-schema so it won't block parsing a new schema
@@ -243,7 +253,9 @@ public class SpatialRPTFieldTypeTest extends AbstractBadConfigTestBase {
     
     IndexSchema oldSchema = h.getCore().getLatestSchema();
 
-    SpatialRecursivePrefixTreeFieldType rptFieldType = new SpatialRecursivePrefixTreeFieldType();
+    if (fieldType == null) {
+      fieldType = new SpatialRecursivePrefixTreeFieldType();
+    }
     Map<String, String> rptMap = new HashMap<String,String>();
     if(distanceUnits!=null)
       rptMap.put("distanceUnits", distanceUnits);
@@ -252,9 +264,9 @@ public class SpatialRPTFieldTypeTest extends AbstractBadConfigTestBase {
     if(format!=null) {
       rptMap.put("format", format);
     }
-    rptFieldType.init(oldSchema, rptMap);
-    rptFieldType.setTypeName("location_rpt");
-    SchemaField newField = new SchemaField("geo", rptFieldType, SchemaField.STORED | SchemaField.INDEXED, null);
+    fieldType.init(oldSchema, rptMap);
+    fieldType.setTypeName("location_rpt");
+    SchemaField newField = new SchemaField("geo", fieldType, SchemaField.STORED | SchemaField.INDEXED, null);
     IndexSchema newSchema = oldSchema.addField(newField);
 
     h.getCore().setLatestSchema(newSchema);
@@ -263,6 +275,6 @@ public class SpatialRPTFieldTypeTest extends AbstractBadConfigTestBase {
   }
 
   private void setupRPTField(String distanceUnits, String geo) throws Exception {
-    setupRPTField(distanceUnits, geo, null);
+    setupRPTField(distanceUnits, geo, null, null);
   }
 }