From bba1701d19b4f1e29324105405132207822375c7 Mon Sep 17 00:00:00 2001 From: Marcus Aspin Date: Fri, 5 Jul 2024 09:13:16 +0100 Subject: [PATCH] PI-2228 Remove synchronized block from prison staff creation (#3990) --- .../hmpps/service/AssignmentService.kt | 9 +++++- .../digital/hmpps/service/StaffService.kt | 32 ++++++------------- .../hmpps/service/AssignmentServiceTest.kt | 31 ++++++++++++++++++ .../hmpps/service/OfficerCodeGenerator.kt | 25 --------------- 4 files changed, 49 insertions(+), 48 deletions(-) delete mode 100644 projects/resettlement-passport-and-delius/src/main/kotlin/uk/gov/justice/digital/hmpps/service/OfficerCodeGenerator.kt diff --git a/libs/prison-staff/src/main/kotlin/uk/gov/justice/digital/hmpps/service/AssignmentService.kt b/libs/prison-staff/src/main/kotlin/uk/gov/justice/digital/hmpps/service/AssignmentService.kt index 8d0314c825..dc64381ba8 100644 --- a/libs/prison-staff/src/main/kotlin/uk/gov/justice/digital/hmpps/service/AssignmentService.kt +++ b/libs/prison-staff/src/main/kotlin/uk/gov/justice/digital/hmpps/service/AssignmentService.kt @@ -1,6 +1,7 @@ package uk.gov.justice.digital.hmpps.service import org.springframework.stereotype.Service +import org.springframework.util.ConcurrentReferenceHashMap import uk.gov.justice.digital.hmpps.entity.PrisonStaff import uk.gov.justice.digital.hmpps.exception.NotFoundException import uk.gov.justice.digital.hmpps.exceptions.InvalidEstablishmentCodeException @@ -9,6 +10,8 @@ import uk.gov.justice.digital.hmpps.repository.PrisonProbationAreaRepository import uk.gov.justice.digital.hmpps.repository.PrisonTeamRepository import uk.gov.justice.digital.hmpps.retry.retry import java.time.ZonedDateTime +import java.util.concurrent.locks.ReentrantLock +import kotlin.concurrent.withLock @Service class AssignmentService( @@ -16,6 +19,10 @@ class AssignmentService( private val teamRepository: PrisonTeamRepository, private val staffService: StaffService ) { + companion object { + private val mutexMap = ConcurrentReferenceHashMap() + private fun getMutex(key: Long) = mutexMap.compute(key) { _, v -> v ?: ReentrantLock() }!! + } fun findAssignment(establishmentCode: String, staffName: StaffName): Triple { if (establishmentCode.length < 3) throw InvalidEstablishmentCodeException(establishmentCode) @@ -36,7 +43,7 @@ class AssignmentService( teamId: Long, staffName: StaffName, allocationDate: ZonedDateTime? = null - ): PrisonStaff { + ): PrisonStaff = getMutex(probationAreaId).withLock { val findStaff = { staffService.findStaff(probationAreaId, staffName) } return retry(3) { findStaff() ?: staffService.create(probationAreaId, probationAreaCode, teamId, staffName, allocationDate) diff --git a/libs/prison-staff/src/main/kotlin/uk/gov/justice/digital/hmpps/service/StaffService.kt b/libs/prison-staff/src/main/kotlin/uk/gov/justice/digital/hmpps/service/StaffService.kt index 410d033647..4688570e87 100644 --- a/libs/prison-staff/src/main/kotlin/uk/gov/justice/digital/hmpps/service/StaffService.kt +++ b/libs/prison-staff/src/main/kotlin/uk/gov/justice/digital/hmpps/service/StaffService.kt @@ -3,7 +3,6 @@ package uk.gov.justice.digital.hmpps.service import org.springframework.stereotype.Service import org.springframework.transaction.annotation.Propagation import org.springframework.transaction.annotation.Transactional -import org.springframework.util.ConcurrentReferenceHashMap import uk.gov.justice.digital.hmpps.entity.PrisonStaff import uk.gov.justice.digital.hmpps.entity.PrisonStaffTeam import uk.gov.justice.digital.hmpps.model.StaffName @@ -19,33 +18,22 @@ class StaffService( private val staffTeamRepository: PrisonStaffTeamRepository ) { - companion object { - private val mutexMap = ConcurrentReferenceHashMap() - private fun getMutex(key: Long) { - mutexMap.compute(key) { _, v -> v ?: Any() } - } - } - @Transactional(propagation = Propagation.REQUIRES_NEW) fun create( - paId: Long, - paCode: String, + probationAreaId: Long, + probationAreaCode: String, teamId: Long, staffName: StaffName, startDate: ZonedDateTime? = null - ): PrisonStaff = synchronized(getMutex(paId)) { - val staff = staffRepository.save( - PrisonStaff( - forename = staffName.forename, - surname = staffName.surname, - probationAreaId = paId, - code = officerCodeGenerator.generateFor(paCode), - startDate = startDate ?: ZonedDateTime.now() - ) + ) = staffRepository.save( + PrisonStaff( + forename = staffName.forename, + surname = staffName.surname, + probationAreaId = probationAreaId, + code = officerCodeGenerator.generateFor(probationAreaCode), + startDate = startDate ?: ZonedDateTime.now() ) - staffTeamRepository.save(PrisonStaffTeam(staff.id, teamId)) - return staff - } + ).also { staffTeamRepository.save(PrisonStaffTeam(it.id, teamId)) } fun findStaff(probationAreaId: Long, staffName: StaffName) = staffRepository.findTopByProbationAreaIdAndForenameIgnoreCaseAndSurnameIgnoreCase( diff --git a/libs/prison-staff/src/test/kotlin/uk/gov/justice/digital/hmpps/service/AssignmentServiceTest.kt b/libs/prison-staff/src/test/kotlin/uk/gov/justice/digital/hmpps/service/AssignmentServiceTest.kt index 03dec9caec..70a2daa06c 100644 --- a/libs/prison-staff/src/test/kotlin/uk/gov/justice/digital/hmpps/service/AssignmentServiceTest.kt +++ b/libs/prison-staff/src/test/kotlin/uk/gov/justice/digital/hmpps/service/AssignmentServiceTest.kt @@ -20,6 +20,8 @@ import uk.gov.justice.digital.hmpps.exceptions.InvalidEstablishmentCodeException import uk.gov.justice.digital.hmpps.model.StaffName import uk.gov.justice.digital.hmpps.repository.PrisonProbationAreaRepository import uk.gov.justice.digital.hmpps.repository.PrisonTeamRepository +import java.util.concurrent.CompletableFuture.allOf +import java.util.concurrent.CompletableFuture.runAsync @ExtendWith(MockitoExtension::class) class AssignmentServiceTest { @@ -169,6 +171,35 @@ class AssignmentServiceTest { ) } } + + @Test + fun `prevents creation of duplicate staff`() { + val probationArea = ProbationAreaGenerator.DEFAULT + val team = TeamGenerator.DEFAULT + val newStaff = PrisonStaff( + code = "C12A001", + forename = staffName.forename, + surname = staffName.surname, + probationAreaId = probationArea.id + ) + + whenever(probationAreaRepository.findByInstitutionNomisCode("LEI")) + .thenReturn(probationArea) + whenever(teamRepository.findByCode(team.code)).thenReturn(team) + whenever(staffService.findStaff(probationArea.id, staffName)) + .thenReturn(null) + whenever(staffService.create(probationArea.id, probationArea.code, team.id, staffName, null)) + .thenAnswer { + whenever(staffService.findStaff(probationArea.id, staffName)).thenReturn(newStaff) + newStaff + } + + allOf(*List(3) { + runAsync { assignmentService.findAssignment("LEI", staffName) } + }.toTypedArray()).join() + + verify(staffService, times(1)).create(probationArea.id, probationArea.code, team.id, staffName) + } } object ProbationAreaGenerator { diff --git a/projects/resettlement-passport-and-delius/src/main/kotlin/uk/gov/justice/digital/hmpps/service/OfficerCodeGenerator.kt b/projects/resettlement-passport-and-delius/src/main/kotlin/uk/gov/justice/digital/hmpps/service/OfficerCodeGenerator.kt deleted file mode 100644 index 2a7e11abdb..0000000000 --- a/projects/resettlement-passport-and-delius/src/main/kotlin/uk/gov/justice/digital/hmpps/service/OfficerCodeGenerator.kt +++ /dev/null @@ -1,25 +0,0 @@ -package uk.gov.justice.digital.hmpps.service - -import org.springframework.stereotype.Component -import uk.gov.justice.digital.hmpps.entity.StaffRepository -import uk.gov.justice.digital.hmpps.exception.InvalidRequestException - -@Component -class OfficerCodeGenerator(private val staffRepository: StaffRepository) { - private val alphabet = ('A'..'Z').toList() - - fun generateFor(probationAreaCode: String, index: Int = 0): String { - if (index == alphabet.size) { - throw InvalidRequestException("Cannot generate another staff id for probationAreaCode ${probationAreaCode}") - } - val prefix = probationAreaCode.substring(0, 3) + alphabet[index] - val latest = staffRepository.getLatestStaffReference("^$prefix\\d{3}$") - val number = latest?.substring(latest.length - 3)?.toInt()?.plus(1) ?: 1 - return if (number > 999) { - generateFor(probationAreaCode, index + 1) - } else { - val suffix = number.toString().padStart(3, '0') - prefix + suffix - } - } -}