From 003183f10c2025507f7ee8b3f3763d4135fd6114 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Thu, 29 Aug 2024 13:23:45 -0500 Subject: [PATCH] add test and prevent null values on task name setup --- .../provider/ChallengeProvider.scala | 75 +++++---- .../provider/ChallengeProviderSpec.scala | 145 ++++++++++++++++++ 2 files changed, 188 insertions(+), 32 deletions(-) create mode 100644 test/org/maproulette/provider/ChallengeProviderSpec.scala diff --git a/app/org/maproulette/provider/ChallengeProvider.scala b/app/org/maproulette/provider/ChallengeProvider.scala index 75222141..164d2b1f 100644 --- a/app/org/maproulette/provider/ChallengeProvider.scala +++ b/app/org/maproulette/provider/ChallengeProvider.scala @@ -289,7 +289,7 @@ class ChallengeProvider @Inject() ( * the specified name. If the JsValue represents a collection of features, * each feature will be checked and the first OSM id found returned */ - private def featureOSMId(value: JsValue, challenge: Challenge): Option[String] = { + 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 @@ -324,41 +324,52 @@ class ChallengeProvider @Inject() ( * 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 = { - // 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 + def taskNameFromJsValue(value: JsValue, challenge: Challenge): String = { + + // Helper function to retrieve a non-null, non-empty string from a field + // Supports both string and numeric ids. + def getNonNullString(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 } } - 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 => - // 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 + // Check for the first valid ID in the specified list of field names + def findName(fields: List[String]): Option[String] = { + fields.iterator.flatMap(fieldName => getNonNullString(fieldName)).toList.headOption + } + + // Use field/property specified by challenge, if available. Otherwise look + // for commonly used id fields/properties + challenge.extra.osmIdProperty.flatMap { osmIdProperty => + if (osmIdProperty.nonEmpty) { + featureOSMId(value, challenge) + } else { + None + } + } getOrElse { + (value \ "features").asOpt[List[JsValue]].flatMap(_.headOption).flatMap { firstFeature => + taskNameFromJsValue(firstFeature, challenge) match { + case "" => None + case id => Some(id) + } + } getOrElse { + val nameKeys = List("id", "@id", "osmid", "osm_id", "name") + findName(nameKeys).getOrElse { + (value \ "properties").asOpt[JsObject].flatMap { properties => + taskNameFromJsValue(properties, challenge) match { + case "" => None + case id => Some(id) + } + } 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 } + } } } } diff --git a/test/org/maproulette/provider/ChallengeProviderSpec.scala b/test/org/maproulette/provider/ChallengeProviderSpec.scala new file mode 100644 index 00000000..30cb8946 --- /dev/null +++ b/test/org/maproulette/provider/ChallengeProviderSpec.scala @@ -0,0 +1,145 @@ +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() + ) + + "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 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 None if OSM ID not found in root or properties" in { + val json = Json.obj("otherField" -> "value") + repository.featureOSMId(json, challengeWithOsmId) mustEqual None + } + + "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 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() + repository.featureOSMId(json, challengeWithoutOsmId) 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 OSM ID from first feature if available and specified in challenge" in { + val json = Json.obj("features" -> Json.arr(Json.obj("custom_osm_id" -> "featureId123"))) + repository.taskNameFromJsValue(json, challengeWithOsmId) mustEqual "featureId123" + } + + "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 feature ID is found and no challenge-specific ID is available" in { + val json = Json.obj("features" -> Json.arr(Json.obj("otherField" -> "value"))) + val result = repository.taskNameFromJsValue(json, challengeWithoutOsmId) + 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 properties object is empty and no ID fields are found" in { + val json = Json.obj("properties" -> Json.obj()) + val result = repository.taskNameFromJsValue(json, challengeWithoutOsmId) + assert(UUID.fromString(result).toString == result) + } + + "return ID field from root object or properties if available" in { + val jsonRoot = Json.obj("id" -> "testId") + val jsonProps = Json.obj("properties" -> Json.obj("id" -> "testId")) + repository.taskNameFromJsValue(jsonRoot, challengeWithoutOsmId) mustEqual "testId" + repository.taskNameFromJsValue(jsonProps, challengeWithoutOsmId) mustEqual "testId" + } + + "return UUID if ID field is 'null'" in { + val json = Json.obj("id" -> null) + val result = repository.taskNameFromJsValue(json, challengeWithoutOsmId) + assert(UUID.fromString(result).toString == result) + } + + "return field 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")) + repository.taskNameFromJsValue(json, challengeWithoutOsmId) mustEqual "idstring" + } + + "return field from properties if ID field is 'null' and other fields are valid" in { + val json = Json.obj( + "name" -> "string", + "properties" -> Json.obj("name" -> "testName", "id" -> "idstring") + ) + repository.taskNameFromJsValue(json, challengeWithoutOsmId) mustEqual "string" + } + } +}