From 93d8cb5851a85d6c8247869b9d9817a01ea42d92 Mon Sep 17 00:00:00 2001 From: Benoit Orihuela Date: Wed, 18 Dec 2024 18:11:36 +0100 Subject: [PATCH] fix: incorrect behaviors in Batch Entity Upsert endpoint - noOverwrite option is not handled whereas it should be passed when in update mode - replace mode was only replacing the attributes of the entity --- .../entity/service/EntityOperationService.kt | 26 +++++++---------- .../search/entity/web/BatchAPIResponses.kt | 11 ------- .../entity/web/EntityOperationHandler.kt | 14 +++++---- .../service/EntityOperationServiceTests.kt | 29 +++++++------------ .../entity/web/EntityOperationHandlerTests.kt | 26 +++++++++++++++-- .../shared/queryparameter/OptionsValue.kt | 4 ++- 6 files changed, 55 insertions(+), 55 deletions(-) diff --git a/search-service/src/main/kotlin/com/egm/stellio/search/entity/service/EntityOperationService.kt b/search-service/src/main/kotlin/com/egm/stellio/search/entity/service/EntityOperationService.kt index 5794c612f..833b90c2e 100644 --- a/search-service/src/main/kotlin/com/egm/stellio/search/entity/service/EntityOperationService.kt +++ b/search-service/src/main/kotlin/com/egm/stellio/search/entity/service/EntityOperationService.kt @@ -4,6 +4,7 @@ import arrow.core.Either import arrow.core.left import arrow.core.raise.either import arrow.core.right +import com.egm.stellio.search.entity.model.EMPTY_UPDATE_RESULT import com.egm.stellio.search.entity.model.UpdateResult import com.egm.stellio.search.entity.web.BatchEntityError import com.egm.stellio.search.entity.web.BatchEntitySuccess @@ -23,8 +24,7 @@ import java.net.URI @Component class EntityOperationService( private val entityService: EntityService, - private val entityQueryService: EntityQueryService, - private val entityAttributeService: EntityAttributeService, + private val entityQueryService: EntityQueryService ) { /** @@ -150,7 +150,8 @@ class EntityOperationService( @Transactional suspend fun upsert( entities: List, - options: String?, + disallowOverwrite: Boolean, + updateMode: Boolean, sub: Sub? ): Pair> { val (existingEntities, newEntities) = splitEntitiesByExistence(entities) @@ -168,7 +169,7 @@ class EntityOperationService( if (existingOrDuplicatedEntities.isNotEmpty()) { val updateOperationResult = - if (options == "update") update(existingOrDuplicatedEntities, false, sub) + if (updateMode) update(existingOrDuplicatedEntities, disallowOverwrite, sub) else replace(existingOrDuplicatedEntities, sub) batchOperationResult.errors.addAll(updateOperationResult.errors) @@ -208,8 +209,7 @@ class EntityOperationService( entities: List, disallowOverwrite: Boolean = false, sub: Sub?, - processor: - suspend (JsonLdNgsiLdEntity, Boolean, Sub?) -> Either + processor: suspend (JsonLdNgsiLdEntity, Boolean, Sub?) -> Either ): BatchOperationResult = entities.map { processEntity(it, disallowOverwrite, sub, processor) @@ -227,8 +227,7 @@ class EntityOperationService( entity: JsonLdNgsiLdEntity, disallowOverwrite: Boolean = false, sub: Sub?, - processor: - suspend (JsonLdNgsiLdEntity, Boolean, Sub?) -> Either + processor: suspend (JsonLdNgsiLdEntity, Boolean, Sub?) -> Either ): Either = kotlin.runCatching { either { @@ -250,19 +249,16 @@ class EntityOperationService( onSuccess = { it } ) + @SuppressWarnings("UnusedParameter") suspend fun replaceEntity( entity: JsonLdNgsiLdEntity, disallowOverwrite: Boolean, sub: Sub? ): Either = either { val (jsonLdEntity, ngsiLdEntity) = entity - entityAttributeService.deleteAttributes(ngsiLdEntity.id).bind() - entityService.appendAttributes( - ngsiLdEntity.id, - jsonLdEntity.getModifiableMembers(), - disallowOverwrite, - sub - ).bind() + entityService.replaceEntity(ngsiLdEntity.id, ngsiLdEntity, jsonLdEntity, sub).map { + EMPTY_UPDATE_RESULT + }.bind() } suspend fun updateEntity( diff --git a/search-service/src/main/kotlin/com/egm/stellio/search/entity/web/BatchAPIResponses.kt b/search-service/src/main/kotlin/com/egm/stellio/search/entity/web/BatchAPIResponses.kt index f1c2d3414..64c0ac380 100644 --- a/search-service/src/main/kotlin/com/egm/stellio/search/entity/web/BatchAPIResponses.kt +++ b/search-service/src/main/kotlin/com/egm/stellio/search/entity/web/BatchAPIResponses.kt @@ -27,16 +27,6 @@ data class BatchOperationResult( entities.forEach { errors.add(BatchEntityError(it.first.toUri(), arrayListOf(it.second.message))) } - - @JsonIgnore - fun addEntitiesToErrors(entities: List, errorMessage: String) = - addIdsToErrors(entities.map { it.id }, errorMessage) - - @JsonIgnore - fun addIdsToErrors(entitiesIds: List, errorMessage: String) = - errors.addAll( - entitiesIds.map { BatchEntityError(it, arrayListOf(errorMessage)) } - ) } data class BatchEntitySuccess( @@ -53,7 +43,6 @@ data class BatchEntityError( typealias JsonLdNgsiLdEntity = Pair -fun List.extractNgsiLdEntities(): List = this.map { it.second } fun JsonLdNgsiLdEntity.entityId(): URI = this.second.id // a temporary data class to hold the result of deserializing, expanding and transforming to NGSI-LD entities diff --git a/search-service/src/main/kotlin/com/egm/stellio/search/entity/web/EntityOperationHandler.kt b/search-service/src/main/kotlin/com/egm/stellio/search/entity/web/EntityOperationHandler.kt index 61f59326c..3d5c9913d 100644 --- a/search-service/src/main/kotlin/com/egm/stellio/search/entity/web/EntityOperationHandler.kt +++ b/search-service/src/main/kotlin/com/egm/stellio/search/entity/web/EntityOperationHandler.kt @@ -109,7 +109,9 @@ class EntityOperationHandler( ) @RequestParam queryParams: MultiValueMap ): ResponseEntity<*> = either { - val options = queryParams.getFirst(QP.OPTIONS.key) + val options = queryParams.getFirst(QP.OPTIONS.key)?.split(",") + val disallowOverwrite = options?.any { it == OptionsValue.NO_OVERWRITE.value } == true + val updateMode = options?.any { it == OptionsValue.UPDATE_MODE.value } == true val sub = getSubFromSecurityContext() val (parsedEntities, unparsableEntities) = prepareEntitiesFromRequestBody(requestBody, httpHeaders).bind() @@ -121,7 +123,8 @@ class EntityOperationHandler( val newUniqueEntities = if (parsedEntities.isNotEmpty()) { val (updateOperationResult, newUniqueEntities) = entityOperationService.upsert( parsedEntities, - options, + disallowOverwrite, + updateMode, sub.getOrNull() ) @@ -155,12 +158,12 @@ class EntityOperationHandler( @RequestParam queryParams: MultiValueMap ): ResponseEntity<*> = either { val options = queryParams.getFirst(QP.OPTIONS.key) + val disallowOverwrite = options?.let { it == OptionsValue.NO_OVERWRITE.value } == true + val sub = getSubFromSecurityContext() val (parsedEntities, unparsableEntities) = prepareEntitiesFromRequestBody(requestBody, httpHeaders).bind() - val disallowOverwrite = options?.let { it == OptionsValue.NO_OVERWRITE.value } ?: false - val batchOperationResult = BatchOperationResult().apply { addEntitiesToErrors(unparsableEntities) } @@ -201,8 +204,7 @@ class EntityOperationHandler( } if (parsedEntities.isNotEmpty()) { - val mergeOperationResult = - entityOperationService.merge(parsedEntities, sub.getOrNull()) + val mergeOperationResult = entityOperationService.merge(parsedEntities, sub.getOrNull()) batchOperationResult.errors.addAll(mergeOperationResult.errors) batchOperationResult.success.addAll(mergeOperationResult.success) } diff --git a/search-service/src/test/kotlin/com/egm/stellio/search/entity/service/EntityOperationServiceTests.kt b/search-service/src/test/kotlin/com/egm/stellio/search/entity/service/EntityOperationServiceTests.kt index eac22bdab..0cbb4a47d 100644 --- a/search-service/src/test/kotlin/com/egm/stellio/search/entity/service/EntityOperationServiceTests.kt +++ b/search-service/src/test/kotlin/com/egm/stellio/search/entity/service/EntityOperationServiceTests.kt @@ -40,9 +40,6 @@ class EntityOperationServiceTests { @MockkBean private lateinit var entityService: EntityService - @MockkBean(relaxed = true) - private lateinit var entityAttributeService: EntityAttributeService - @MockkBean private lateinit var entityQueryService: EntityQueryService @@ -282,11 +279,8 @@ class EntityOperationServiceTests { @Test fun `batch replace should ask to replace entities`() = runTest { coEvery { - entityAttributeService.deleteAttributes(any()) + entityService.replaceEntity(any(), any(), any(), any()) } returns Unit.right() - coEvery { - entityService.appendAttributes(any(), any(), any(), any()) - } returns EMPTY_UPDATE_RESULT.right() val batchOperationResult = entityOperationService.replace( listOf( @@ -302,14 +296,7 @@ class EntityOperationServiceTests { ) assertTrue(batchOperationResult.errors.isEmpty()) - coVerify { entityAttributeService.deleteAttributes(firstEntityURI) } - coVerify { entityAttributeService.deleteAttributes(secondEntityURI) } - coVerify { - entityService.appendAttributes(eq(firstEntityURI), any(), false, sub) - } - coVerify { - entityService.appendAttributes(eq(secondEntityURI), any(), false, sub) - } + coVerify { entityService.replaceEntity(firstEntityURI, any(), any(), any()) } } @Test @@ -457,7 +444,8 @@ class EntityOperationServiceTests { firstExpandedEntity to firstEntity, secondExpandedEntity to secondEntity ), - null, + disallowOverwrite = false, + updateMode = false, sub ) @@ -484,7 +472,8 @@ class EntityOperationServiceTests { firstExpandedEntity to firstEntity, secondExpandedEntity to secondEntity ), - "update", + disallowOverwrite = false, + updateMode = true, sub ) @@ -523,7 +512,8 @@ class EntityOperationServiceTests { firstExpandedEntity to firstEntity, secondExpandedEntity to secondEntity ), - "update", + disallowOverwrite = false, + updateMode = true, sub ) @@ -567,7 +557,8 @@ class EntityOperationServiceTests { firstExpandedEntity to firstEntity, secondExpandedEntity to secondEntity ), - null, + disallowOverwrite = false, + updateMode = false, sub ) diff --git a/search-service/src/test/kotlin/com/egm/stellio/search/entity/web/EntityOperationHandlerTests.kt b/search-service/src/test/kotlin/com/egm/stellio/search/entity/web/EntityOperationHandlerTests.kt index 2a995cec3..085563554 100644 --- a/search-service/src/test/kotlin/com/egm/stellio/search/entity/web/EntityOperationHandlerTests.kt +++ b/search-service/src/test/kotlin/com/egm/stellio/search/entity/web/EntityOperationHandlerTests.kt @@ -112,6 +112,8 @@ class EntityOperationHandlerTests { private val batchCreateEndpoint = "/ngsi-ld/v1/entityOperations/create" private val batchUpsertWithUpdateEndpoint = "/ngsi-ld/v1/entityOperations/upsert?options=update" + private val batchUpsertWithUpdateAndNoOverwriteEndpoint = + "/ngsi-ld/v1/entityOperations/upsert?options=update,noOverwrite" private val batchUpdateEndpoint = "/ngsi-ld/v1/entityOperations/update" private val batchUpdateEndpointWithNoOverwriteOption = "/ngsi-ld/v1/entityOperations/update?options=noOverwrite" private val batchDeleteEndpoint = "/ngsi-ld/v1/entityOperations/delete" @@ -324,7 +326,7 @@ class EntityOperationHandlerTests { updatedEntitiesIds.map { BatchEntitySuccess(it, mockkClass(UpdateResult::class)) }.toMutableList() ) - coEvery { entityOperationService.upsert(any(), any(), any()) } returns ( + coEvery { entityOperationService.upsert(any(), any(), any(), any()) } returns ( updatedBatchResult to createdEntitiesIds ) @@ -341,7 +343,7 @@ class EntityOperationHandlerTests { fun `upsert batch entity should return a 204 if it has only updated existing entities`() = runTest { val jsonLdFile = validJsonFile coEvery { - entityOperationService.upsert(any(), any(), any()) + entityOperationService.upsert(any(), any(), any(), any()) } returns (BatchOperationResult(success = mutableListOf(), errors = mutableListOf()) to emptyList()) webClient.post() @@ -360,7 +362,7 @@ class EntityOperationHandlerTests { BatchEntityError(dissolvedOxygenSensorUri, arrayListOf("Update unexpectedly failed.")) ) - coEvery { entityOperationService.upsert(any(), any(), any()) } returns ( + coEvery { entityOperationService.upsert(any(), any(), any(), any()) } returns ( BatchOperationResult( arrayListOf(BatchEntitySuccess(deviceUri, mockkClass(UpdateResult::class))), errors @@ -391,6 +393,24 @@ class EntityOperationHandlerTests { ) } + @Test + fun `upsert batch entity should update entities without overwriting if update and noOverwrite options`() = runTest { + coEvery { + entityOperationService.upsert(any(), any(), any(), any()) + } returns (BatchOperationResult(success = mutableListOf(), errors = mutableListOf()) to emptyList()) + + webClient.post() + .uri(batchUpsertWithUpdateAndNoOverwriteEndpoint) + .bodyValue(validJsonFile) + .exchange() + .expectStatus().isNoContent + .expectBody().isEmpty + + coVerify(exactly = 1) { + entityOperationService.upsert(any(), true, true, any()) + } + } + @Test fun `upsert batch entity should return a 400 if JSON-LD payload is not correct`() { shouldReturn400WithBadPayload("upsert") 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 index 283e13640..05072d108 100644 --- a/shared/src/main/kotlin/com/egm/stellio/shared/queryparameter/OptionsValue.kt +++ b/shared/src/main/kotlin/com/egm/stellio/shared/queryparameter/OptionsValue.kt @@ -3,5 +3,7 @@ package com.egm.stellio.shared.queryparameter enum class OptionsValue(val value: String) { SYS_ATTRS("sysAttrs"), KEY_VALUES("keyValues"), - NO_OVERWRITE("noOverwrite") + NO_OVERWRITE("noOverwrite"), + UPDATE_MODE("update"), + REPLACE_MODE("replace") }