fix: PR comments + suggestions

stellio-hub · Jan 9, 2025 · 1de5901 · 1de5901
1 parent 6661df9
commit 1de5901
Show file tree

Hide file tree

Showing 10 changed files with 70 additions and 82 deletions.
diff --git a/search-service/src/main/kotlin/com/egm/stellio/search/temporal/util/TemporalQueryUtils.kt b/search-service/src/main/kotlin/com/egm/stellio/search/temporal/util/TemporalQueryUtils.kt
@@ -190,26 +190,29 @@ fun buildTimerelAndTime(
         "'timerel' and 'time' must be used in conjunction".left()
     }
 
-fun extractTemporalRepresentation(
+private fun extractTemporalRepresentation(
     queryParams: MultiValueMap<String, String>
 ): Either<APIException, TemporalRepresentation> = either {
     val optionsParam = Optional.ofNullable(queryParams.getFirst(QueryParameter.OPTIONS.key))
     val formatParam = queryParams.getFirst(QueryParameter.FORMAT.key)
     if (formatParam != null) {
         return TemporalRepresentation.fromString(formatParam)
     } else {
-        if (!optionsParam.isEmpty) {
+        return if (!optionsParam.isEmpty) {
             val hasTemporal = hasValueInOptionsParam(optionsParam, OptionsValue.TEMPORAL_VALUES).bind()
             val hasAggregated = hasValueInOptionsParam(optionsParam, OptionsValue.AGGREGATED_VALUES).bind()
             when {
                 hasTemporal && hasAggregated ->
-                    return BadRequestDataException("Only one temporal representation can be present").left()
-                hasTemporal -> return TemporalRepresentation.TEMPORAL_VALUES.right()
-                hasAggregated -> return TemporalRepresentation.AGGREGATED_VALUES.right()
-                else -> return TemporalRepresentation.NORMALIZED.right()
+                    BadRequestDataException(
+                        "Found different temporal representations in options query parameter," +
+                            " only one can be provided"
+                    ).left()
+                hasTemporal -> TemporalRepresentation.TEMPORAL_VALUES.right()
+                hasAggregated -> TemporalRepresentation.AGGREGATED_VALUES.right()
+                else -> TemporalRepresentation.NORMALIZED.right()
             }
         } else
-            return TemporalRepresentation.NORMALIZED.right()
+            TemporalRepresentation.NORMALIZED.right()
     }
 }
 

diff --git a/.../src/test/kotlin/com/egm/stellio/search/temporal/service/AttributeInstanceServiceTests.kt b/.../src/test/kotlin/com/egm/stellio/search/temporal/service/AttributeInstanceServiceTests.kt
@@ -200,8 +200,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
             buildDefaultTestTemporalQuery(
                 timerel = Timerel.AFTER,
                 timeAt = now.minusHours(1)
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
         attributeInstanceService.search(temporalEntitiesQuery, incomingAttribute)
             .shouldSucceedWith {
@@ -226,8 +225,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
                 timerel = Timerel.AFTER,
                 timeAt = now.minusHours(1),
                 timeproperty = AttributeInstance.TemporalProperty.CREATED_AT
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
         attributeInstanceService.search(temporalEntitiesQuery, incomingAttribute)
             .shouldSucceedWith {
@@ -252,8 +250,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
                 timerel = Timerel.AFTER,
                 timeAt = now.minusHours(1),
                 timeproperty = AttributeInstance.TemporalProperty.CREATED_AT
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
         attributeInstanceService.search(temporalEntitiesQuery, incomingAttribute)
             .shouldSucceedWith {
@@ -278,8 +275,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
                 timerel = Timerel.AFTER,
                 timeAt = now.minusHours(1),
                 timeproperty = AttributeInstance.TemporalProperty.MODIFIED_AT
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
         attributeInstanceService.search(temporalEntitiesQuery, incomingAttribute)
             .shouldSucceedWith {
@@ -298,8 +294,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
             buildDefaultTestTemporalQuery(
                 timerel = Timerel.AFTER,
                 timeAt = now.minusHours(1)
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
         attributeInstanceService.search(temporalEntitiesQuery, incomingAttribute)
             .shouldSucceedWith {
@@ -315,7 +310,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
         }
 
         attributeInstanceService.search(
-            gimmeTemporalEntitiesQuery(buildDefaultTestTemporalQuery(), TemporalRepresentation.NORMALIZED),
+            gimmeTemporalEntitiesQuery(buildDefaultTestTemporalQuery()),
             incomingAttribute
         )
             .shouldSucceedWith {
@@ -415,8 +410,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
                 timerel = Timerel.AFTER,
                 timeAt = ZonedDateTime.parse("2022-07-01T00:00:00Z"),
                 instanceLimit = 5
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
 
         attributeInstanceService.search(temporalEntitiesQuery, incomingAttribute)
@@ -442,8 +436,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
                 timeAt = ZonedDateTime.parse("2022-07-01T00:00:00Z"),
                 endTimeAt = ZonedDateTime.parse("2022-07-05T00:00:00Z"),
                 instanceLimit = 5
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
 
         attributeInstanceService.search(temporalEntitiesQuery, incomingAttribute)
@@ -470,8 +463,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
                 timeAt = now.minusHours(1),
                 instanceLimit = 5,
                 lastN = 5
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
         attributeInstanceService.search(temporalEntitiesQuery, incomingAttribute)
             .shouldSucceedWith {
@@ -533,8 +525,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
             buildDefaultTestTemporalQuery(
                 timerel = Timerel.AFTER,
                 timeAt = now.minusHours(1)
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
         attributeInstanceService.search(temporalEntitiesQuery, incomingAttribute)
             .shouldSucceedWith { results ->
@@ -556,8 +547,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
             buildDefaultTestTemporalQuery(
                 timerel = Timerel.AFTER,
                 timeAt = now.minusHours(1)
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
         attributeInstanceService.search(
             temporalEntitiesQuery,
@@ -578,8 +568,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
             buildDefaultTestTemporalQuery(
                 timerel = Timerel.AFTER,
                 timeAt = now.plusHours(1)
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
         attributeInstanceService.search(temporalEntitiesQuery, incomingAttribute)
             .shouldSucceedWith {
@@ -820,8 +809,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
             buildDefaultTestTemporalQuery(
                 timerel = Timerel.AFTER,
                 timeAt = ZonedDateTime.parse("1970-01-01T00:00:00Z")
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
 
         attributeInstanceService.modifyAttributeInstance(
@@ -859,8 +847,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
             buildDefaultTestTemporalQuery(
                 timerel = Timerel.AFTER,
                 timeAt = ZonedDateTime.parse("1970-01-01T00:00:00Z")
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
 
         attributeInstanceService.modifyAttributeInstance(
@@ -899,8 +886,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
             buildDefaultTestTemporalQuery(
                 timerel = Timerel.AFTER,
                 timeAt = ZonedDateTime.parse("1970-01-01T00:00:00Z")
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
 
         attributeInstanceService.modifyAttributeInstance(
@@ -944,8 +930,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
             buildDefaultTestTemporalQuery(
                 timerel = Timerel.AFTER,
                 timeAt = ZonedDateTime.parse("1970-01-01T00:00:00Z")
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
 
         attributeInstanceService.modifyAttributeInstance(
@@ -984,7 +969,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
         ).shouldSucceed()
 
         attributeInstanceService.search(
-            gimmeTemporalEntitiesQuery(buildDefaultTestTemporalQuery(), TemporalRepresentation.NORMALIZED),
+            gimmeTemporalEntitiesQuery(buildDefaultTestTemporalQuery()),
             incomingAttribute
         )
             .shouldSucceedWith {
@@ -1058,8 +1043,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
             buildDefaultTestTemporalQuery(
                 timerel = Timerel.AFTER,
                 timeAt = now.minusHours(1)
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
         attributeInstanceService.search(temporalEntitiesQuery, incomingAttribute)
             .shouldSucceedWith { assertThat(it).isEmpty() }
@@ -1069,8 +1053,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
                 timerel = Timerel.AFTER,
                 timeAt = now.minusHours(1),
                 timeproperty = AttributeInstance.TemporalProperty.CREATED_AT
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
 
         attributeInstanceService.search(temporalEntitiesAuditQuery, incomingAttribute)
@@ -1096,8 +1079,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
             buildDefaultTestTemporalQuery(
                 timerel = Timerel.AFTER,
                 timeAt = now.minusHours(1)
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
         attributeInstanceService.search(temporalEntitiesQuery, outgoingAttribute)
             .shouldSucceedWith { assertThat(it).hasSize(5) }
@@ -1124,8 +1106,7 @@ class AttributeInstanceServiceTests : WithTimescaleContainer, WithKafkaContainer
             buildDefaultTestTemporalQuery(
                 timerel = Timerel.AFTER,
                 timeAt = now.minusHours(1)
-            ),
-            TemporalRepresentation.NORMALIZED
+            )
         )
         attributeInstanceService.search(temporalEntitiesQuery, outgoingAttribute)
             .shouldSucceedWith { assertThat(it).hasSize(5) }

diff --git a/...h-service/src/test/kotlin/com/egm/stellio/search/temporal/util/TemporalQueryUtilsTests.kt b/...h-service/src/test/kotlin/com/egm/stellio/search/temporal/util/TemporalQueryUtilsTests.kt
@@ -111,7 +111,10 @@ class TemporalQueryUtilsTests {
             true
         ).shouldFail {
             assertInstanceOf(BadRequestDataException::class.java, it)
-            assertEquals("Only one temporal representation can be present", it.message)
+            assertEquals(
+                "Found different temporal representations in options query parameter, only one can be provided",
+                it.message
+            )
         }
     }
 
@@ -151,7 +154,7 @@ class TemporalQueryUtilsTests {
             true
         ).shouldFail {
             assertInstanceOf(InvalidRequestException::class.java, it)
-            assertEquals("'invalidOptions' is not a valid options value", it.message)
+            assertEquals("'invalidOptions' is not a valid value for the options query parameter", it.message)
         }
     }
 

diff --git a/...service/src/test/kotlin/com/egm/stellio/search/temporal/web/TemporalEntityHandlerTests.kt b/...service/src/test/kotlin/com/egm/stellio/search/temporal/web/TemporalEntityHandlerTests.kt
@@ -440,7 +440,7 @@ class TemporalEntityHandlerTests : TemporalEntityHandlerTestCommon() {
                 """
                 {
                     "type": "https://uri.etsi.org/ngsi-ld/errors/BadRequestData",
-                    "title": "Only one temporal representation can be present",
+                    "title": "Found different temporal representations in options query parameter, only one can be provided",
                     "detail": "$DEFAULT_DETAIL"
                 } 
                 """
@@ -482,7 +482,7 @@ class TemporalEntityHandlerTests : TemporalEntityHandlerTestCommon() {
                 """
                 {
                     "type": "https://uri.etsi.org/ngsi-ld/errors/InvalidRequest",
-                    "title": "'invalidOptions' is not a valid options value",
+                    "title": "'invalidOptions' is not a valid value for the options query parameter",
                     "detail": "$DEFAULT_DETAIL"
                 } 
                 """

diff --git a/shared/src/main/kotlin/com/egm/stellio/shared/model/NgsiLdDataRepresentation.kt b/shared/src/main/kotlin/com/egm/stellio/shared/model/NgsiLdDataRepresentation.kt
@@ -1,7 +1,6 @@
 package com.egm.stellio.shared.model
 
 import arrow.core.Either
-import arrow.core.left
 import arrow.core.raise.either
 import arrow.core.right
 import com.egm.stellio.shared.queryparameter.FormatValue
@@ -34,23 +33,20 @@ data class NgsiLdDataRepresentation(
         ): Either<APIException, NgsiLdDataRepresentation> = either {
             val optionsParam = queryParams.getOrDefault(QueryParameter.OPTIONS.key, emptyList())
                 .flatMap { it.split(",") }
-            val formatParam = queryParams.getFirst(QueryParameter.FORMAT.key)
-            if (formatParam != null && FormatValue.fromString(formatParam) == null) {
-                return InvalidRequestException("'$formatParam' is not a valid format value").left()
-            }
-
-            optionsParam.forEach { option ->
-                OptionsValue.fromString(option).bind()
+                .map { OptionsValue.fromString(it).bind() }
+            val formatParam = queryParams.getFirst(QueryParameter.FORMAT.key)?.let {
+                FormatValue.fromString(it).bind()
             }
             val attributeRepresentation = when {
-                formatParam == FormatValue.KEY_VALUES.value ||
-                    formatParam == FormatValue.SIMPLIFIED.value -> AttributeRepresentation.SIMPLIFIED
-                formatParam == FormatValue.NORMALIZED.value -> AttributeRepresentation.NORMALIZED
-                optionsParam.contains(FormatValue.KEY_VALUES.value) ||
-                    optionsParam.contains(FormatValue.SIMPLIFIED.value) -> AttributeRepresentation.SIMPLIFIED
+                formatParam == FormatValue.KEY_VALUES || formatParam == FormatValue.SIMPLIFIED ->
+                    AttributeRepresentation.SIMPLIFIED
+                formatParam == FormatValue.NORMALIZED ->
+                    AttributeRepresentation.NORMALIZED
+                optionsParam.contains(OptionsValue.KEY_VALUES) || optionsParam.contains(OptionsValue.SIMPLIFIED) ->
+                    AttributeRepresentation.SIMPLIFIED
                 else -> AttributeRepresentation.NORMALIZED
             }
-            val includeSysAttrs = optionsParam.contains(OptionsValue.SYS_ATTRS.value)
+            val includeSysAttrs = optionsParam.contains(OptionsValue.SYS_ATTRS)
             val languageFilter = queryParams.getFirst(QueryParameter.LANG.key)
             val entityRepresentation = EntityRepresentation.forMediaType(acceptMediaType)
             val geometryProperty =

diff --git a/shared/src/main/kotlin/com/egm/stellio/shared/queryparameter/FormatValue.kt b/shared/src/main/kotlin/com/egm/stellio/shared/queryparameter/FormatValue.kt
@@ -1,13 +1,21 @@
 package com.egm.stellio.shared.queryparameter
 
+import arrow.core.Either
+import arrow.core.left
+import arrow.core.raise.either
+import com.egm.stellio.shared.model.APIException
+import com.egm.stellio.shared.model.InvalidRequestException
+
 enum class FormatValue(val value: String) {
     KEY_VALUES("keyValues"),
     SIMPLIFIED("simplified"),
     NORMALIZED("normalized"),
     TEMPORAL_VALUES("temporalValues"),
     AGGREGATED_VALUES("aggregatedValues");
     companion object {
-        fun fromString(key: String): FormatValue? =
+        fun fromString(key: String): Either<APIException, FormatValue> = either {
             FormatValue.entries.find { it.value == key }
+                ?: return InvalidRequestException("'$key' is not a valid value for the format query parameter").left()
+        }
     }
 }
diff --git a/shared/src/main/kotlin/com/egm/stellio/shared/queryparameter/OptionsValue.kt b/shared/src/main/kotlin/com/egm/stellio/shared/queryparameter/OptionsValue.kt
@@ -20,7 +20,7 @@ enum class OptionsValue(val value: String) {
     companion object {
         fun fromString(key: String): Either<APIException, OptionsValue> = either {
             OptionsValue.entries.find { it.value == key }
-                ?: return InvalidRequestException("'$key' is not a valid options value").left()
+                ?: return InvalidRequestException("'$key' is not a valid value for the options query parameter").left()
         }
     }
 }
diff --git a/shared/src/main/kotlin/com/egm/stellio/shared/util/ApiUtils.kt b/shared/src/main/kotlin/com/egm/stellio/shared/util/ApiUtils.kt
@@ -170,17 +170,14 @@ internal fun canExpandJsonLdKeyFromCore(contexts: List<String>): Boolean {
 }
 
 fun hasValueInOptionsParam(
-    queryParam: Optional<String>,
-    optionsParamValue: OptionsValue
+    optionsParam: Optional<String>,
+    optionsValue: OptionsValue
 ): Either<APIException, Boolean> = either {
-    val optionsValue = queryParam
+    optionsParam
         .map { it.split(",") }
         .orElse(emptyList())
-
-    optionsValue.forEach { option ->
-        OptionsValue.fromString(option).bind()
-    }
-    optionsValue.any { option -> option == optionsParamValue.value }
+        .map { OptionsValue.fromString(it).bind() }
+        .any { it == optionsValue }
 }
 
 fun parseQueryParameter(queryParam: String?): Set<String> =