Skip to content

SDIT-2021 Cater for retries to the mapping creation by performing an upsert #369

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

Merged
merged 1 commit into from
Oct 31, 2024
Merged

SDIT-2021 Cater for retries to the mapping creation by performing an upsert #369

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
2 changes: 1 addition & 1 deletion ...tice/digital/hmpps/nomisvisitsmappingservice/prisonperson/PrisonPersonMigrationMapping.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ data class PrisonPersonMigrationMapping(

val migrationType: PrisonPersonMigrationType,

val dpsIds: String,
var dpsIds: String,

val label: String,

Expand Down
1 change: 1 addition & 0 deletions ...al/hmpps/nomisvisitsmappingservice/prisonperson/PrisonPersonMigrationMappingRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import org.springframework.stereotype.Repository
@Repository
interface PrisonPersonMigrationMappingRepository : CoroutineCrudRepository<PrisonPersonMigrationMapping, String> {
suspend fun findByNomisPrisonerNumberAndMigrationType(nomisPrisonerNumber: String, migrationType: PrisonPersonMigrationType): PrisonPersonMigrationMapping?
suspend fun findByNomisPrisonerNumberAndMigrationTypeAndLabel(nomisPrisonerNumber: String, migrationType: PrisonPersonMigrationType, label: String): PrisonPersonMigrationMapping?
fun findAllByLabelOrderByNomisPrisonerNumberAsc(label: String, pageable: Pageable): Flow<PrisonPersonMigrationMapping>
suspend fun countAllByLabel(label: String): Long
}
44 changes: 44 additions & 0 deletions ...tice/digital/hmpps/nomisvisitsmappingservice/prisonperson/PrisonPersonMigrationService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,24 @@ package uk.gov.justice.digital.hmpps.nomisvisitsmappingservice.prisonperson
import kotlinx.coroutines.async
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.flow.toList
import kotlinx.coroutines.reactor.awaitSingle
import org.springframework.data.domain.Page
import org.springframework.data.domain.PageImpl
import org.springframework.data.domain.Pageable
import org.springframework.data.r2dbc.core.R2dbcEntityTemplate
import org.springframework.data.r2dbc.core.insert
import org.springframework.data.r2dbc.core.update
import org.springframework.data.relational.core.query.Criteria.where
import org.springframework.data.relational.core.query.Query.query
import org.springframework.data.relational.core.query.Update.update
import org.springframework.stereotype.Service
import org.springframework.transaction.annotation.Transactional

@Service
class PrisonPersonMigrationService(
private val repository: PrisonPersonMigrationMappingRepository,
private val prisonPersonMigrationMappingRepository: PrisonPersonMigrationMappingRepository,
private val template: R2dbcEntityTemplate,
) {

suspend fun create(mappingRequest: PrisonPersonMigrationMappingRequest) {
Expand All @@ -25,6 +34,41 @@ class PrisonPersonMigrationService(
)
}

@Transactional
suspend fun upsert(mappingRequest: PrisonPersonMigrationMappingRequest): PrisonPersonMigrationMapping =
mappingRequest.find()
?.let { mappingRequest.update() }
?: let { mappingRequest.insert() }

private suspend fun PrisonPersonMigrationMappingRequest.find() =
repository.findByNomisPrisonerNumberAndMigrationTypeAndLabel(nomisPrisonerNumber, migrationType, label)

private suspend fun PrisonPersonMigrationMappingRequest.update(): PrisonPersonMigrationMapping =
template.update<PrisonPersonMigrationMapping>()
.inTable("prison_person_migration_mapping")
.matching(
query(
where("nomis_prisoner_number").`is`(nomisPrisonerNumber)
.and(where("migration_type").`is`(migrationType))
.and(where("label").`is`(label)),
),
)
.apply(update("dps_ids", dpsIds.toString()))
.awaitSingle()
.let { find()!! }

private suspend fun PrisonPersonMigrationMappingRequest.insert(): PrisonPersonMigrationMapping =
template.insert<PrisonPersonMigrationMapping>()
.using(
PrisonPersonMigrationMapping(
nomisPrisonerNumber = nomisPrisonerNumber,
migrationType = migrationType,
dpsIds = dpsIds.toString(),
label = label,
),
)
.awaitSingle()
Comment on lines +46 to +70
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spring data R2DBC still doesn't support composite entity keys so we needed another way. I thought I'd try out the template instead.


suspend fun find(nomisPrisonerNumber: String, migrationType: PrisonPersonMigrationType) = repository.findByNomisPrisonerNumberAndMigrationType(nomisPrisonerNumber, migrationType)

suspend fun getMappings(
Expand Down
27 changes: 27 additions & 0 deletions .../gov/justice/digital/hmpps/nomisvisitsmappingservice/prisonperson/PrisonPersonResource.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import org.springframework.validation.annotation.Validated
import org.springframework.web.bind.annotation.GetMapping
import org.springframework.web.bind.annotation.PathVariable
import org.springframework.web.bind.annotation.PostMapping
import org.springframework.web.bind.annotation.PutMapping
import org.springframework.web.bind.annotation.RequestBody
import org.springframework.web.bind.annotation.RequestMapping
import org.springframework.web.bind.annotation.ResponseStatus
Expand All @@ -32,6 +33,7 @@ class PrisonPersonResource(private val service: PrisonPersonMigrationService) {

@PostMapping("/migration")
@ResponseStatus(HttpStatus.CREATED)
@Deprecated("Use the PUT endpoint which is idempotent")
@Operation(
summary = "Creates a new prison person migration mapping",
description = "Creates a mapping between nomis alert ids and dps alert id. Requires ROLE_NOMIS_PRISONPERSON",
Expand Down Expand Up @@ -76,6 +78,31 @@ class PrisonPersonResource(private val service: PrisonPersonMigrationService) {
)
}

@PutMapping("/migration")
@Operation(
summary = "Creates or updates a prison person migration mapping",
description = "Creates or updates a mapping between nomis prisoner numbers and prison person history ids. Requires ROLE_NOMIS_PRISONPERSON",
Comment on lines +81 to +84
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When testing we found that sometimes a prisoner migration is retried despite all records being created in DPS and the mapping service. As DPS is idempotent and just overwrites the previous migration we need the same behaviour in the mapping service, hence switching to a PUT.

requestBody = io.swagger.v3.oas.annotations.parameters.RequestBody(
content = [Content(mediaType = "application/json", schema = Schema(implementation = PrisonPersonMigrationMappingRequest::class))],
),
responses = [
ApiResponse(responseCode = "200", description = "Mapping created or updated"),
ApiResponse(
responseCode = "401",
description = "Unauthorized to access this endpoint",
content = [Content(mediaType = "application/json", schema = Schema(implementation = ErrorResponse::class))],
),
ApiResponse(
responseCode = "403",
description = "Access forbidden for this endpoint",
content = [Content(mediaType = "application/json", schema = Schema(implementation = ErrorResponse::class))],
),
],
)
suspend fun upsertMapping(
@RequestBody @Valid mappingRequest: PrisonPersonMigrationMappingRequest,
) = service.upsert(mappingRequest)

@GetMapping("/migration/migration-id/{migrationId}")
@Operation(
summary = "get paged mappings by migration id",
Expand Down
55 changes: 24 additions & 31 deletions ...ice/digital/hmpps/nomisvisitsmappingservice/prisonerperson/PrisonPersonResourceIntTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class PrisonPersonResourceIntTest : IntegrationTestBase() {
@Nested
inner class CreateMigrationMapping {
@AfterEach
fun teatDown() = runTest {
fun tearDown() = runTest {
repository.deleteAll()
}

Expand Down Expand Up @@ -71,42 +71,35 @@ class PrisonPersonResourceIntTest : IntegrationTestBase() {
inner class HappyPath {
@Test
fun `should create migration mapping`() = runTest {
webTestClient.createMigrationMapping(request("A1234AA", PROFILE_DETAILS_PHYSICAL_ATTRIBUTES, listOf(1, 2, 3), "label"))
.expectStatus().isCreated

val mapping = repository.findByNomisPrisonerNumberAndMigrationType("A1234AA", PROFILE_DETAILS_PHYSICAL_ATTRIBUTES)
assertThat(mapping?.nomisPrisonerNumber).isEqualTo("A1234AA")
assertThat(mapping?.migrationType).isEqualTo(PROFILE_DETAILS_PHYSICAL_ATTRIBUTES)
assertThat(mapping?.dpsIds).isEqualTo("[1, 2, 3]")
assertThat(mapping?.label).isEqualTo("label")
assertThat(mapping?.whenCreated?.toLocalDate()).isEqualTo(LocalDate.now())
}
}

@Nested
inner class Validation {
@Test
fun `should return 409 if migration mapping already exists`() = runTest {
webTestClient.createMigrationMapping(request("A1234AA", PHYSICAL_ATTRIBUTES, listOf(1), "label"))
.expectStatus().isCreated

webTestClient.createMigrationMapping(request("A1234AA", PHYSICAL_ATTRIBUTES, listOf(1), "label"))
.expectStatus().isEqualTo(409)
webTestClient.upsertMigrationMapping(request("A1234AA", PROFILE_DETAILS_PHYSICAL_ATTRIBUTES, listOf(1, 2, 3), "label"))
.expectStatus().isOk

val mapping = repository.findByNomisPrisonerNumberAndMigrationType("A1234AA", PROFILE_DETAILS_PHYSICAL_ATTRIBUTES)!!
assertThat(mapping.nomisPrisonerNumber).isEqualTo("A1234AA")
assertThat(mapping.migrationType).isEqualTo(PROFILE_DETAILS_PHYSICAL_ATTRIBUTES)
assertThat(mapping.dpsIds).isEqualTo("[1, 2, 3]")
assertThat(mapping.label).isEqualTo("label")
assertThat(mapping.whenCreated?.toLocalDate()).isEqualTo(LocalDate.now())
}

// The migration is idempotent and sometimes we want to migrate the same entity multiple times
@Test
fun `should return OK if migration mapping already exists for different migration run`() = runTest {
webTestClient.createMigrationMapping(request("A1234AA", PHYSICAL_ATTRIBUTES, listOf(1), "first-migration"))
.expectStatus().isCreated

webTestClient.createMigrationMapping(request("A1234AA", PHYSICAL_ATTRIBUTES, listOf(1), "second-migration"))
.expectStatus().isCreated
fun `should update migration mapping`() = runTest {
webTestClient.upsertMigrationMapping(request("A1234AA", PROFILE_DETAILS_PHYSICAL_ATTRIBUTES, listOf(1, 2, 3), "label"))
.expectStatus().isOk
webTestClient.upsertMigrationMapping(request("A1234AA", PROFILE_DETAILS_PHYSICAL_ATTRIBUTES, listOf(4, 5, 6), "label"))
.expectStatus().isOk

val mapping = repository.findByNomisPrisonerNumberAndMigrationType("A1234AA", PROFILE_DETAILS_PHYSICAL_ATTRIBUTES)!!
assertThat(mapping.nomisPrisonerNumber).isEqualTo("A1234AA")
assertThat(mapping.migrationType).isEqualTo(PROFILE_DETAILS_PHYSICAL_ATTRIBUTES)
assertThat(mapping.dpsIds).isEqualTo("[4, 5, 6]")
assertThat(mapping.label).isEqualTo("label")
assertThat(mapping.whenCreated?.toLocalDate()).isEqualTo(LocalDate.now())
}
}

private fun WebTestClient.createMigrationMapping(request: PrisonPersonMigrationMappingRequest) =
post()
private fun WebTestClient.upsertMigrationMapping(request: PrisonPersonMigrationMappingRequest) =
put()
.uri("/mapping/prisonperson/migration")
.headers(setAuthorisation(roles = listOf("ROLE_NOMIS_PRISONPERSON")))
.contentType(MediaType.APPLICATION_JSON)
Expand Down