GEOMESA-3516 Check user data parsing before updateSchema (#3410)

Author: elahrvivaz

geomesa-index-api/src/main/scala/org/locationtech/geomesa/index/geotools/MetadataBackedDataStore.scala

@@ -138,13 +138,18 @@ abstract class MetadataBackedDataStore(config: NamespaceConfig) extends DataStor
           // set the enabled indices
           preSchemaCreate(sft)
 
+          // compute the metadata values - IMPORTANT: encode type has to be called after all user data is set
+          val encodedAttributes = SimpleFeatureTypes.encodeType(sft, includeUserData = true)
+          // validate we can read out the encoded sft - invalid user data keys can break the parsing
+          try { SimpleFeatureTypes.createType("", encodedAttributes) } catch {
+            case NonFatal(e) => throw new IllegalArgumentException("Invalid schema:", e)
+          }
+          val metadataMap = Map(
+            AttributesKey      -> encodedAttributes,
+            StatsGenerationKey -> GeoToolsDateFormat.format(Instant.now().atOffset(ZoneOffset.UTC))
+          )
           try {
             // write out the metadata to the catalog table
-            // compute the metadata values - IMPORTANT: encode type has to be called after all user data is set
-            val metadataMap = Map(
-              AttributesKey      -> SimpleFeatureTypes.encodeType(sft, includeUserData = true),
-              StatsGenerationKey -> GeoToolsDateFormat.format(Instant.now().atOffset(ZoneOffset.UTC))
-            )
             metadata.insert(sft.getTypeName, metadataMap)
 
             // reload the sft so that we have any default metadata,
@@ -252,6 +257,12 @@ abstract class MetadataBackedDataStore(config: NamespaceConfig) extends DataStor
       // validation and normalization of the schema
       preSchemaUpdate(sft, previousSft)
 
+      val encodedAttributes = SimpleFeatureTypes.encodeType(sft, includeUserData = true)
+      // validate we can read out the encoded sft - invalid user data keys can break the parsing
+      try { SimpleFeatureTypes.createType("", encodedAttributes) } catch {
+        case NonFatal(e) => throw new IllegalArgumentException("Invalid schema:", e)
+      }
+
       // if all is well, update the metadata - first back it up
       if (FastConverter.convertOrElse[java.lang.Boolean](sft.getUserData.get(Configs.UpdateBackupMetadata), true)) {
         metadata.backup(typeName.getLocalPart)
@@ -265,7 +276,7 @@ abstract class MetadataBackedDataStore(config: NamespaceConfig) extends DataStor
         }
       }
       // now insert the new spec string
-      metadata.insert(sft.getTypeName, AttributesKey, SimpleFeatureTypes.encodeType(sft, includeUserData = true))
+      metadata.insert(sft.getTypeName, AttributesKey, encodedAttributes)
 
       onSchemaUpdated(sft, previousSft)
     } finally {

geomesa-index-api/src/test/scala/org/locationtech/geomesa/index/geotools/GeoMesaDataStoreAlterSchemaTest.scala

@@ -222,6 +222,27 @@ class GeoMesaDataStoreAlterSchemaTest extends Specification {
 
       ds.checkSchemaCompatibility("test", toSft(missingConfig)) mustEqual SchemaCompatibility.Unchanged
     }
+    "prevent creating a schema with corrupt metadata" in {
+      val ds = new TestGeoMesaDataStore(true)
+      val spec = "name:String,age:Integer,dtg:Date,*geom:Point:srid=4326"
+      val sft = SimpleFeatureTypes.createType("test", spec)
+      sft.getUserData.put("geomesa.foo=bar", "baz")
+      ds.createSchema(sft) must throwAn[IllegalArgumentException]
+      ds.getSchema("test") must beNull
+    }
+    "prevent updates that would corrupt the schema metadata" in {
+      val ds = new TestGeoMesaDataStore(true)
+      val spec = "name:String,age:Integer,dtg:Date,*geom:Point:srid=4326"
+      ds.createSchema(SimpleFeatureTypes.createType("test", spec))
+
+      val sft = SimpleFeatureTypes.mutable(ds.getSchema("test"))
+      sft.getUserData.put("geomesa.foo=bar", "baz")
+
+      ds.updateSchema("test", sft) must throwAn[IllegalArgumentException]
+
+      ds.getSchema("test") must not(beNull)
+      SimpleFeatureTypes.encodeType(ds.getSchema("test")) mustEqual spec
+    }
   }
 }
 

geomesa-utils-parent/geomesa-utils/src/main/scala/org/locationtech/geomesa/utils/geotools/SimpleFeatureTypes.scala

@@ -285,13 +285,12 @@ object SimpleFeatureTypes {
 
   def encodeUserData(sft: SimpleFeatureType): String = {
     val prefixes = sft.getUserDataPrefixes
-    val result = new StringBuilder(";")
-    sft.getUserData.asScala.foreach { case (k: AnyRef, v: AnyRef) =>
-      if (v != null && prefixes.exists(k.toString.startsWith)) {
-        result.append(encodeUserData(k, v)).append(",")
-      }
+    val toEncode = sft.getUserData.asScala.collect {
+      case (k, v) if v != null && prefixes.exists(k.toString.startsWith) => encodeUserData(k, v)
+    }
+    if (toEncode.isEmpty) { "" } else {
+      toEncode.toSeq.sorted.mkString(";", ",", "")
     }
-    if (result.lengthCompare(1) > 0) { result.substring(0, result.length - 1) } else { "" }
   }
 
   def encodeUserData(data: java.util.Map[_ <: AnyRef, _ <: AnyRef]): String = {

geomesa-utils-parent/geomesa-utils/src/main/scala/org/locationtech/geomesa/utils/geotools/sft/SimpleFeatureSpecParser.scala

@@ -36,25 +36,27 @@ object SimpleFeatureSpecParser {
 
   private def parse[T](spec: String, rule: Rule1[T], report: Boolean): T = {
     if (spec == null) {
-      throw new IllegalArgumentException("Invalid spec string: null")
+      throw new IllegalArgumentException("Invalid spec: null")
     }
     val runner = if (report) { ReportingParseRunner(rule) } else { BasicParseRunner(rule) }
     val parsing = runner.run(spec.stripMarginAndWhitespace())
-    parsing.result.getOrElse(throw new ParsingException(constructErrorMessage(parsing, report)))
+    parsing.result.getOrElse(throw new ParsingException(constructErrorMessage(parsing, report, spec)))
   }
 
-  private def constructErrorMessage[T](result: ParsingResult[T], report: Boolean): String = {
-    lazy val fallback = s"Invalid spec string: ${ErrorUtils.printParseErrors(result)}"
+  private def constructErrorMessage[T](result: ParsingResult[T], report: Boolean, spec: String): String = {
+    lazy val fallback = s"Invalid spec: ${ErrorUtils.printParseErrors(result)}\n$spec"
     if (!report) { fallback } else {
       result.parseErrors.collectFirst { case e: InvalidInputError =>
         import scala.collection.JavaConverters._
+        // add a caret pointing to the invalid location
+        val specAndIndex = s"$spec\n${Seq.fill(math.max(0, e.getStartIndex - 1))(" ").mkString}^"
         // determine what paths the parser partially matched
         val matchers = e.getFailedMatchers.asScala.map(getFailedMatcher).distinct
         if (matchers.isEmpty) {
-          s"Invalid spec string at index ${e.getStartIndex}."
+          s"Invalid spec at index ${e.getStartIndex}\n$specAndIndex"
         } else {
           val expected = if (matchers.lengthCompare(1) > 0) { s"one of: ${matchers.mkString(", ")}" } else { matchers.head }
-          s"Invalid spec string at index ${e.getStartIndex}. Expected $expected."
+          s"Invalid spec at index ${e.getStartIndex} - expected $expected\n$specAndIndex"
         }
       }.getOrElse(fallback)
     }

geomesa-utils-parent/geomesa-utils/src/test/scala/org/locationtech/geomesa/utils/geotools/SimpleFeatureTypesTest.scala

@@ -60,20 +60,20 @@ class SimpleFeatureTypesTest extends Specification {
       }
       "encode an sft properly with geomesa user data" >> {
         val encoded = SimpleFeatureTypes.encodeType(sft, includeUserData = true)
-        encoded must startWith("id:Integer,dtg:Date,*geom:Point:srid=4326;")
-        encoded must contain("geomesa.index.dtg='dtg'")
-        encoded must contain("geomesa.table.sharing='true'")
-        encoded must not(contain("hello="))
+        encoded mustEqual "id:Integer,dtg:Date,*geom:Point:srid=4326;geomesa.index.dtg='dtg',geomesa.table.sharing='true'"
       }
       "encode an sft properly with specified user data" >> {
         import org.locationtech.geomesa.utils.geotools.RichSimpleFeatureType.RichSimpleFeatureType
         sft.setUserDataPrefixes(Seq("hello"))
         val encoded = SimpleFeatureTypes.encodeType(sft, includeUserData = true)
-        encoded must startWith("id:Integer,dtg:Date,*geom:Point:srid=4326;")
-        encoded must contain("geomesa.user-data.prefix='hello'")
-        encoded must contain("geomesa.index.dtg='dtg'")
-        encoded must contain("geomesa.table.sharing='true'")
-        encoded must contain("hello='goodbye'")
+        encoded mustEqual "id:Integer,dtg:Date,*geom:Point:srid=4326;" +
+          "geomesa.index.dtg='dtg',geomesa.table.sharing='true',geomesa.user-data.prefix='hello',hello='goodbye'"
+      }
+      "encode an sft and preserve spaces in user data" >> {
+        val sft = SimpleFeatureTypes.createType("test", "name:String,dtg:Date,*geom:Point:srid=4326;geomesa.foo='1 day'")
+        sft.getUserData.get("geomesa.foo") mustEqual "1 day"
+        val encoded = SimpleFeatureTypes.encodeType(sft, includeUserData = true)
+        SimpleFeatureTypes.createType("test", encoded).getUserData.get("geomesa.foo") mustEqual "1 day"
       }
     }
 
@@ -362,16 +362,16 @@ class SimpleFeatureTypesTest extends Specification {
 
     "return meaningful error messages" >> {
       Try(SimpleFeatureTypes.createType("test", null)) must
-          beAFailedTry.withThrowable[IllegalArgumentException](Pattern.quote("Invalid spec string: null"))
+          beAFailedTry.withThrowable[IllegalArgumentException](Pattern.quote("Invalid spec: null"))
       val failures = Seq(
-        ("foo:Strong", "7. Expected attribute type binding"),
-        ("foo:String,*bar:String", "16. Expected geometry type binding"),
-        ("foo:String,bar:String;;", "22. Expected one of: specification, feature type option, end of spec"),
-        ("foo:String,bar,baz:String", "14. Expected one of: attribute name, attribute, attribute type binding, geometry type binding"),
-        ("foo:String:bar,baz:String", "14. Expected attribute option")
+        ("foo:Strong", "7 - expected attribute type binding"),
+        ("foo:String,*bar:String", "16 - expected geometry type binding"),
+        ("foo:String,bar:String;;", "22 - expected one of: specification, feature type option, end of spec"),
+        ("foo:String,bar,baz:String", "14 - expected one of: attribute name, attribute, attribute type binding, geometry type binding"),
+        ("foo:String:bar,baz:String", "14 - expected attribute option")
       )
       forall(failures) { case (spec, message) =>
-        val pattern = Pattern.quote(s"Invalid spec string at index $message.")
+        val pattern = Pattern.quote(s"Invalid spec at index $message\n$spec\n") + " *\\^"
         val result = Try(SimpleFeatureTypes.createType("test", spec))
         result must beAFailedTry.withThrowable[IllegalArgumentException](pattern)
       }
@@ -664,13 +664,5 @@ class SimpleFeatureTypesTest extends Specification {
         "geomesa.table.sharing.prefix" -> "\u0001"
       )
     }
-
-    "preserve spaces in user data" >> {
-      val sft = SimpleFeatureTypes.createType("test", "name:String,dtg:Date,*geom:Point:srid=4326;geomesa.foo='1 day'")
-      sft.getUserData.get("geomesa.foo") mustEqual "1 day"
-      val encoded = SimpleFeatureTypes.encodeType(sft, includeUserData = true)
-      SimpleFeatureTypes.createType("test", encoded).getUserData.get("geomesa.foo") mustEqual "1 day"
-    }
   }
-
 }