Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: incorrect behaviors in Batch Entity Upsert endpoint #1290

Merged
merged 1 commit into from
Dec 20, 2024
Merged

fix: incorrect behaviors in Batch Entity Upsert endpoint #1290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 11 additions & 15 deletions ...h-service/src/main/kotlin/com/egm/stellio/search/entity/service/EntityOperationService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
) {

/**
Expand Down Expand Up @@ -150,7 +150,8 @@ class EntityOperationService(
@Transactional
suspend fun upsert(
entities: List<JsonLdNgsiLdEntity>,
options: String?,
disallowOverwrite: Boolean,
updateMode: Boolean,
sub: Sub?
): Pair<BatchOperationResult, List<URI>> {
val (existingEntities, newEntities) = splitEntitiesByExistence(entities)
Expand All @@ -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)
Expand Down Expand Up @@ -208,8 +209,7 @@ class EntityOperationService(
entities: List<JsonLdNgsiLdEntity>,
disallowOverwrite: Boolean = false,
sub: Sub?,
processor:
suspend (JsonLdNgsiLdEntity, Boolean, Sub?) -> Either<APIException, UpdateResult>
processor: suspend (JsonLdNgsiLdEntity, Boolean, Sub?) -> Either<APIException, UpdateResult>
): BatchOperationResult =
entities.map {
processEntity(it, disallowOverwrite, sub, processor)
Expand All @@ -227,8 +227,7 @@ class EntityOperationService(
entity: JsonLdNgsiLdEntity,
disallowOverwrite: Boolean = false,
sub: Sub?,
processor:
suspend (JsonLdNgsiLdEntity, Boolean, Sub?) -> Either<APIException, UpdateResult>
processor: suspend (JsonLdNgsiLdEntity, Boolean, Sub?) -> Either<APIException, UpdateResult>
): Either<BatchEntityError, BatchEntitySuccess> =
kotlin.runCatching {
either {
Expand All @@ -250,19 +249,16 @@ class EntityOperationService(
onSuccess = { it }
)

@SuppressWarnings("UnusedParameter")
suspend fun replaceEntity(
entity: JsonLdNgsiLdEntity,
disallowOverwrite: Boolean,
sub: Sub?
): Either<APIException, UpdateResult> = 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(
Expand Down
11 changes: 0 additions & 11 deletions search-service/src/main/kotlin/com/egm/stellio/search/entity/web/BatchAPIResponses.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,6 @@ data class BatchOperationResult(
entities.forEach {
errors.add(BatchEntityError(it.first.toUri(), arrayListOf(it.second.message)))
}

@JsonIgnore
fun addEntitiesToErrors(entities: List<NgsiLdEntity>, errorMessage: String) =
addIdsToErrors(entities.map { it.id }, errorMessage)

@JsonIgnore
fun addIdsToErrors(entitiesIds: List<URI>, errorMessage: String) =
errors.addAll(
entitiesIds.map { BatchEntityError(it, arrayListOf(errorMessage)) }
)
}

data class BatchEntitySuccess(
Expand All @@ -53,7 +43,6 @@ data class BatchEntityError(

typealias JsonLdNgsiLdEntity = Pair<ExpandedEntity, NgsiLdEntity>

fun List<JsonLdNgsiLdEntity>.extractNgsiLdEntities(): List<NgsiLdEntity> = 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
Expand Down
14 changes: 8 additions & 6 deletions search-service/src/main/kotlin/com/egm/stellio/search/entity/web/EntityOperationHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,9 @@ class EntityOperationHandler(
)
@RequestParam queryParams: MultiValueMap<String, String>
): 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
Copy link
Contributor

@thomasBousselin thomasBousselin Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a no_overwrite option for upsert in the specification. (5.6.8 and 6.15)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see the in-progress doc with misc fixes to contribute to the spec

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed, aligned with the figures of operations from 1.6.1. Spec needs clarification, to be discussed with ISG-CIM.

val updateMode = options?.any { it == OptionsValue.UPDATE_MODE.value } == true
val sub = getSubFromSecurityContext()

val (parsedEntities, unparsableEntities) = prepareEntitiesFromRequestBody(requestBody, httpHeaders).bind()
Expand All @@ -121,7 +123,8 @@ class EntityOperationHandler(
val newUniqueEntities = if (parsedEntities.isNotEmpty()) {
val (updateOperationResult, newUniqueEntities) = entityOperationService.upsert(
parsedEntities,
options,
disallowOverwrite,
updateMode,
sub.getOrNull()
)

Expand Down Expand Up @@ -155,12 +158,12 @@ class EntityOperationHandler(
@RequestParam queryParams: MultiValueMap<String, String>
): 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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
29 changes: 10 additions & 19 deletions ...vice/src/test/kotlin/com/egm/stellio/search/entity/service/EntityOperationServiceTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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(
Expand All @@ -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
Expand Down Expand Up @@ -457,7 +444,8 @@ class EntityOperationServiceTests {
firstExpandedEntity to firstEntity,
secondExpandedEntity to secondEntity
),
null,
disallowOverwrite = false,
updateMode = false,
sub
)

Expand All @@ -484,7 +472,8 @@ class EntityOperationServiceTests {
firstExpandedEntity to firstEntity,
secondExpandedEntity to secondEntity
),
"update",
disallowOverwrite = false,
updateMode = true,
sub
)

Expand Down Expand Up @@ -523,7 +512,8 @@ class EntityOperationServiceTests {
firstExpandedEntity to firstEntity,
secondExpandedEntity to secondEntity
),
"update",
disallowOverwrite = false,
updateMode = true,
sub
)

Expand Down Expand Up @@ -567,7 +557,8 @@ class EntityOperationServiceTests {
firstExpandedEntity to firstEntity,
secondExpandedEntity to secondEntity
),
null,
disallowOverwrite = false,
updateMode = false,
sub
)

Expand Down
26 changes: 23 additions & 3 deletions ...-service/src/test/kotlin/com/egm/stellio/search/entity/web/EntityOperationHandlerTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
)

Expand All @@ -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()
Expand All @@ -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
Expand Down Expand Up @@ -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")
Expand Down
4 changes: 3 additions & 1 deletion shared/src/main/kotlin/com/egm/stellio/shared/queryparameter/OptionsValue.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Loading