add test and prevent null values on task name setup (#1147)

* add test and prevent null values on task name setup

* refine tests and prevent falsy values on osmIdProperty

app/org/maproulette/provider/ChallengeProvider.scala

@@ -282,85 +282,89 @@ class ChallengeProvider @Inject() (
     }
   }
 
+  // Helper function to retrieve a non-null, non-empty string from a field
+  // Supports both string and numeric IDs.
+  def getNonNullString(value: JsValue, fieldName: String): Option[String] = {
+    (value \ fieldName).asOpt[JsValue].flatMap {
+      case JsString(str) if str.nonEmpty => Some(str)
+      case JsNumber(num)                 => Some(num.toString)
+      case _                             => None
+    }
+  }
+
   /**
-    * Extracts the OSM id from the given JsValue based on the `osmIdProperty`
+    * Checks for the first valid ID in the specified list of field names.
+    */
+  def findName(value: JsValue, fields: List[String]): Option[String] = {
+    fields.iterator
+      .flatMap { fieldName =>
+        val result = getNonNullString(value, fieldName)
+        result
+      }
+      .toList
+      .headOption
+  }
+
+  /**
+    * Extracts the OSM ID from the given JsValue based on the `osmIdProperty`
     * challenge field. Returns None if either the challenge has not specified an
     * osmIdProperty or if the JsValue contains neither a field nor property with
     * the specified name. If the JsValue represents a collection of features,
-    * each feature will be checked and the first OSM id found returned
+    * each feature will be checked and the first OSM ID found will be returned.
     */
-  private def featureOSMId(value: JsValue, challenge: Challenge): Option[String] = {
-    challenge.extra.osmIdProperty match {
-      case Some(osmIdName) =>
-        // Whether `value` represents multiple features or just one, process as List
-        val features = (value \ "features").asOpt[List[JsValue]].getOrElse(List(value))
-        features
-          .map(feature =>
-            // First look for a matching field on the feature itself. If not found, then
-            // look at the feature's properties
-            (feature \ osmIdName).asOpt[String] match {
-              case Some(matchingIdField) => Some(matchingIdField)
-              case None =>
-                (feature \ "properties").asOpt[JsObject] match {
-                  case Some(properties) =>
-                    (properties \ osmIdName).asOpt[String] match {
-                      case Some(matchingIdProperty) => Some(matchingIdProperty)
-                      case None                     => None // feature doesn't have the id property
-                    }
-                  case None => None // feature doesn't have any properties
-                }
-            }
-          )
-          .find(_.isDefined) match { // first feature that has a match
-          case Some(featureWithId) => featureWithId
-          case None                => None // No features found with matching id field or property
+  def featureOSMId(value: JsValue, challenge: Challenge): Option[String] = {
+    challenge.extra.osmIdProperty.flatMap { osmIdName =>
+      // Whether `value` represents multiple features or just one, process as List
+      val features = (value \ "features").asOpt[List[JsValue]].getOrElse(List(value))
+
+      features.flatMap { feature =>
+        // First look for a matching field on the feature itself
+        getNonNullString(feature, osmIdName).orElse {
+          // If not found on the feature, then look at the feature's properties
+          (feature \ "properties").asOpt[JsObject].flatMap { properties =>
+            getNonNullString(properties, osmIdName)
+          }
         }
-      case None => None // No osmIdProperty defined on challenge
+      }.headOption // Get the first non-empty match
     }
   }
 
   /**
     * Extracts an appropriate task name from the given JsValue, looking for any
-    * of multiple suitable id fields, or finally defaulting to a random UUID if
-    * no acceptable field is found
+    * of multiple suitable ID fields, or finally defaulting to a random UUID if
+    * no acceptable field is found.
     */
-  private def taskNameFromJsValue(value: JsValue, challenge: Challenge): String = {
+  def taskNameFromJsValue(value: JsValue, challenge: Challenge): String = {
     // Use field/property specified by challenge, if available. Otherwise look
-    // for commonly used id fields/properties
-    if (!challenge.extra.osmIdProperty.getOrElse("").isEmpty) {
-      return featureOSMId(value, challenge) match {
-        case Some(osmId) => osmId
-        case None        => UUID.randomUUID().toString // task does not contain id property
-      }
+    // for commonly used ID fields/properties
+    if (challenge.extra.osmIdProperty.exists(_.nonEmpty)) {
+      return featureOSMId(value, challenge).getOrElse(UUID.randomUUID().toString)
     }
 
-    val featureList = (value \ "features").asOpt[List[JsValue]]
-    if (featureList.isDefined) {
-      taskNameFromJsValue(featureList.get.head, challenge) // Base name on first feature
-    } else {
-      val nameKeys = List.apply("id", "@id", "osmid", "osm_id", "name")
-      nameKeys.collectFirst {
-        case x if (value \ x).asOpt[JsValue].isDefined =>
-          // Support both string and numeric ids. If it's a string, use it.
-          // Otherwise convert the value to a string
-          (value \ x).asOpt[String] match {
-            case Some(stringValue) => stringValue
-            case None              => (value \ x).asOpt[JsValue].get.toString
-          }
-      } match {
-        case Some(n) => n
-        case None =>
-          (value \ "properties").asOpt[JsObject] match {
-            // See if we can find an id field on the feature properties
-            case Some(properties) => taskNameFromJsValue(properties, challenge)
-            case None             =>
+    (value \ "features")
+      .asOpt[List[JsValue]]
+      .flatMap(_.headOption)
+      .flatMap { firstFeature =>
+        val name = taskNameFromJsValue(firstFeature, challenge)
+        if (name.nonEmpty) Some(name) else None
+      }
+      .getOrElse {
+        val nameKeys = List("id", "@id", "osmid", "osm_id", "name")
+        findName(value, nameKeys).getOrElse {
+          (value \ "properties")
+            .asOpt[JsObject]
+            .flatMap { properties =>
+              val name = taskNameFromJsValue(properties, challenge)
+              if (name.nonEmpty) Some(name) else None
+            }
+            .getOrElse {
               // if we still don't find anything, create a UUID for it. The
               // caveat to this is that if you upload the same file again, it
               // will create duplicate tasks
               UUID.randomUUID().toString
-          }
+            }
+        }
       }
-    }
   }
 
   /**

test/org/maproulette/provider/ChallengeProviderSpec.scala

@@ -0,0 +1,219 @@
+import org.joda.time.DateTime
+import org.maproulette.framework.model._
+import org.maproulette.provider.ChallengeProvider
+import org.scalatestplus.play.PlaySpec
+import org.scalatestplus.mockito.MockitoSugar
+import play.api.libs.json._
+import java.util.UUID
+
+class ChallengeProviderSpec extends PlaySpec with MockitoSugar {
+  val repository: ChallengeProvider = new ChallengeProvider(null, null, null, null, null)
+
+  val challengeWithOsmId = Challenge(
+    1,
+    "ChallengeWithOsmId",
+    DateTime.now(),
+    DateTime.now(),
+    None,
+    false,
+    None,
+    ChallengeGeneral(101, 1, ""),
+    ChallengeCreation(),
+    ChallengePriority(),
+    ChallengeExtra(osmIdProperty = Some("custom_osm_id"))
+  )
+
+  val challengeWithoutOsmId = Challenge(
+    1,
+    "ChallengeWithoutOsmId",
+    DateTime.now(),
+    DateTime.now(),
+    None,
+    false,
+    None,
+    ChallengeGeneral(101, 1, ""),
+    ChallengeCreation(),
+    ChallengePriority(),
+    ChallengeExtra()
+  )
+
+  "getNonNullString" should {
+    "return Some string for a non-empty string field" in {
+      val json = Json.obj("field" -> "validString")
+      repository.getNonNullString(json, "field") mustEqual Some("validString")
+    }
+
+    "return Some string for a numeric field" in {
+      val json = Json.obj("field" -> 12345)
+      repository.getNonNullString(json, "field") mustEqual Some("12345")
+    }
+
+    "return None for an empty string field" in {
+      val json = Json.obj("field" -> "")
+      repository.getNonNullString(json, "field") mustEqual None
+    }
+
+    "return None for a null field" in {
+      val json = Json.obj("field" -> JsNull)
+      repository.getNonNullString(json, "field") mustEqual None
+    }
+
+    "return None for a missing field" in {
+      val json = Json.obj()
+      repository.getNonNullString(json, "field") mustEqual None
+    }
+
+    "return None for a non-string and non-numeric field" in {
+      val json = Json.obj("field" -> Json.obj("nestedField" -> "value"))
+      repository.getNonNullString(json, "field") mustEqual None
+    }
+  }
+
+  "findName" should {
+    "return the first non-null, non-empty string from the list of fields" in {
+      val json = Json.obj("field1" -> "first", "field2" -> "second")
+      repository.findName(json, List("field1", "field2")) mustEqual Some("first")
+    }
+
+    "return None if none of the fields have valid values" in {
+      val json = Json.obj("field1" -> "", "field2" -> JsNull)
+      repository.findName(json, List("field1", "field2")) mustEqual None
+    }
+
+    "return None if the list of fields is empty" in {
+      val json = Json.obj("field" -> "value")
+      repository.findName(json, List()) mustEqual None
+    }
+
+    "return None if the fields do not exist in the JSON" in {
+      val json = Json.obj()
+      repository.findName(json, List("field1", "field2")) mustEqual None
+    }
+  }
+
+  "featureOSMId" should {
+    "return OSM ID from root if present and specified in challenge" in {
+      val json = Json.obj("custom_osm_id" -> "singleFeatureId")
+      repository.featureOSMId(json, challengeWithOsmId) mustEqual Some("singleFeatureId")
+    }
+
+    "return OSM ID from root if present and specified in challenge (numeric)" in {
+      val json = Json.obj("custom_osm_id" -> 9999999)
+      repository.featureOSMId(json, challengeWithOsmId) mustEqual Some("9999999")
+    }
+
+    "return OSM ID from properties if specified in challenge" in {
+      val json = Json.obj("properties" -> Json.obj("custom_osm_id" -> "propertyId"))
+      repository.featureOSMId(json, challengeWithOsmId) mustEqual Some("propertyId")
+    }
+
+    "return OSM ID from properties if specified in challenge (numeric)" in {
+      val json = Json.obj("properties" -> Json.obj("custom_osm_id" -> 9999999))
+      repository.featureOSMId(json, challengeWithOsmId) mustEqual Some("9999999")
+    }
+
+    "return OSM ID from first feature in list if specified in challenge" in {
+      val json = Json.obj("features" -> Json.arr(Json.obj("custom_osm_id" -> "featureId1")))
+      repository.featureOSMId(json, challengeWithOsmId) mustEqual Some("featureId1")
+    }
+
+    "return OSM ID from nested features if specified in challenge" in {
+      val json = Json.obj(
+        "features" -> Json.arr(
+          Json.obj(
+            "properties" -> Json.obj("custom_osm_id" -> "nestedFeatureId1")
+          )
+        )
+      )
+      repository.featureOSMId(json, challengeWithOsmId) mustEqual Some("nestedFeatureId1")
+    }
+
+    "return None if OSM ID not found in root or properties" in {
+      val json = Json.obj("otherField" -> "value")
+      repository.featureOSMId(json, challengeWithOsmId) mustEqual None
+    }
+
+    "return None if features do not contain specified OSM ID field" in {
+      val json = Json.obj("features" -> Json.arr(Json.obj("otherField" -> "value1")))
+      repository.featureOSMId(json, challengeWithOsmId) mustEqual None
+    }
+
+    "return None if challenge does not specify OSM ID property" in {
+      val json = Json.obj("features" -> Json.arr(Json.obj("custom_osm_id" -> "featureId1")))
+      repository.featureOSMId(json, challengeWithoutOsmId) mustEqual None
+    }
+
+    "return None if JSON has no features and challenge does not specify OSM ID property" in {
+      val json = Json.obj("custom_osm_id" -> null, "fillerProperty" -> "string")
+      repository.featureOSMId(json, challengeWithoutOsmId) mustEqual None
+    }
+
+    "return None if properties object is empty and challenge does not specify OSM ID property" in {
+      val json =
+        Json.obj("properties" -> Json.obj("custom_osm_id" -> null, "fillerProperty" -> "string"))
+      repository.featureOSMId(json, challengeWithoutOsmId) mustEqual None
+    }
+
+    "return None if features array is empty and challenge specifies OSM ID property" in {
+      val json = Json.obj("features" -> Json.arr())
+      repository.featureOSMId(json, challengeWithOsmId) mustEqual None
+    }
+  }
+
+  "taskNameFromJsValue" should {
+    "return OSM ID from root object if present and specified in challenge" in {
+      val json = Json.obj("custom_osm_id" -> "12345")
+      repository.taskNameFromJsValue(json, challengeWithOsmId) mustEqual "12345"
+    }
+
+    "return feature id from properties if ID field is null and other fields are valid" in {
+      val json = Json.obj(
+        "id" -> null,
+        "properties" -> Json
+          .obj("name" -> "testName", "id" -> "idstring", "fillerProperty" -> "string")
+      )
+      repository.taskNameFromJsValue(json, challengeWithoutOsmId) mustEqual "idstring"
+    }
+
+    "return feature id from properties if ID field is null and properties contain valid IDs" in {
+      val json = Json.obj(
+        "properties" -> Json.obj("fillerProperty" -> "string", "id" -> null, "name" -> "testName")
+      )
+      repository.taskNameFromJsValue(json, challengeWithoutOsmId) mustEqual "testName"
+    }
+
+    "return feature id from the first feature if the ID field is null and challenge specifies valid feature IDs" in {
+      val json = Json.obj(
+        "features" -> Json.arr(
+          Json.obj("id" -> null, "properties" -> Json.obj("name" -> "featureName")),
+          Json.obj("id" -> null, "properties" -> Json.obj("name" -> "otherFeatureName"))
+        )
+      )
+      repository.taskNameFromJsValue(json, challengeWithoutOsmId) mustEqual "featureName"
+    }
+
+    "return random UUID if OSM ID field is specified but not found" in {
+      val json   = Json.obj("otherField" -> "value")
+      val result = repository.taskNameFromJsValue(json, challengeWithOsmId)
+      assert(UUID.fromString(result).toString == result)
+    }
+
+    "return random UUID if no valid ID fields are found" in {
+      val json   = Json.obj()
+      val result = repository.taskNameFromJsValue(json, challengeWithoutOsmId)
+      assert(UUID.fromString(result).toString == result)
+    }
+
+    "return random UUID if features array is empty" in {
+      val json   = Json.obj("features" -> Json.arr())
+      val result = repository.taskNameFromJsValue(json, challengeWithoutOsmId)
+      assert(UUID.fromString(result).toString == result)
+    }
+
+    "return random UUID if JSON features array has objects with null or empty names" in {
+      val json   = Json.obj("features" -> Json.arr(Json.obj("name" -> ""), Json.obj("name" -> null)))
+      val result = repository.taskNameFromJsValue(json, challengeWithoutOsmId)
+      assert(UUID.fromString(result).toString == result)
+    }
+  }
+}