From 44a19c22fee8d4711f72962ec8dc951b77a20368 Mon Sep 17 00:00:00 2001 From: Tommi Leinamo Date: Mon, 13 Nov 2023 09:39:30 +0200 Subject: [PATCH] Add fine grained exceptions for known commit error situations To allow better error messages in UI. Note: most of these shouldn't actually be possible in replace or combine APIs, because the staging frames would run into these constraints already when being created in Hastus import. In addition there are lots of plain DB constraints, from simple value checks to foreign key constraints etc, but I don't see it as beneficial to add custom exceptions for all of these. Can always add these if users actually encounter them. --- .../api/util/HasuraErrorExtensions.kt | 17 ++- .../timetables/api/util/HasuraErrorType.kt | 7 ++ .../api/TimetablesCombineApiTest.kt | 2 +- .../api/TimetablesReplaceApiTest.kt | 2 +- .../util/TransactionSystemExtensionsTest.kt | 116 ++++++++++++++++++ .../service/CombineTimetablesServiceTest.kt | 29 +++++ 6 files changed, 170 insertions(+), 3 deletions(-) create mode 100644 src/test/kotlin/fi/hsl/jore4/timetables/api/util/TransactionSystemExtensionsTest.kt diff --git a/src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt b/src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt index 0e98e0a..b6cafc7 100644 --- a/src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt +++ b/src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt @@ -116,8 +116,23 @@ data class TransactionSystemExtensions( companion object { fun from(ex: TransactionSystemException) = TransactionSystemExtensions( HttpStatus.CONFLICT.value(), - HasuraErrorType.TransactionSystemError, + resolveHasuraErrorType(ex), ex.cause?.message ?: "unknown error on transaction commit or rollback" ) + + private fun resolveHasuraErrorType(ex: TransactionSystemException): HasuraErrorType { + val errorMessage = ex.cause?.message ?: "" // Should always be defined though. + val errorMatchingConditions = listOf( + HasuraErrorType.PassingTimeStopPointMatchingOrderError to { errorMessage.contains("passing times and their matching stop points must be in same order") }, + HasuraErrorType.PassingTimeFirstArrivalTimeError to { errorMessage.contains("first passing time must not have arrival_time set") }, + HasuraErrorType.PassingTimeLastDepartureTimeError to { errorMessage.contains("last passing time must not have departure_time set") }, + HasuraErrorType.PassingTimeNullError to { errorMessage.contains("all passing time that are not first or last in the sequence must have both departure and arrival time defined") }, + HasuraErrorType.PassingTimesMixedJourneyPatternRefsError to { errorMessage.contains("inconsistent journey_pattern_ref within vehicle journey, all timetabled_passing_times must reference same journey_pattern_ref as the vehicle_journey") }, + HasuraErrorType.ConflictingSchedulesError to { errorMessage.contains("validate_queued_schedules_uniqueness") }, + HasuraErrorType.SequentialIntegrityError to { errorMessage.contains("validate_service_sequential_integrity") } + ) + + return errorMatchingConditions.find { it.second() }?.first ?: HasuraErrorType.TransactionSystemError + } } } diff --git a/src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorType.kt b/src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorType.kt index c9f60eb..169807d 100644 --- a/src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorType.kt +++ b/src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorType.kt @@ -2,8 +2,15 @@ package fi.hsl.jore4.timetables.api.util enum class HasuraErrorType { UnknownError, + ConflictingSchedulesError, InvalidTargetPriorityError, MultipleTargetFramesFoundError, + PassingTimeFirstArrivalTimeError, + PassingTimeLastDepartureTimeError, + PassingTimeNullError, + PassingTimeStopPointMatchingOrderError, + PassingTimesMixedJourneyPatternRefsError, + SequentialIntegrityError, StagingVehicleScheduleFrameNotFoundError, TargetPriorityParsingError, TargetVehicleScheduleFrameNotFoundError, diff --git a/src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesCombineApiTest.kt b/src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesCombineApiTest.kt index 832fe4a..b2c4c13 100644 --- a/src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesCombineApiTest.kt +++ b/src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesCombineApiTest.kt @@ -146,7 +146,7 @@ class TimetablesCombineApiTest(@Autowired val mockMvc: MockMvc) { "message": "JDBC Commit Failed", "extensions": { "code": 409, - "type": "TransactionSystemError", + "type": "ConflictingSchedulesError", "sqlErrorMessage": "$sqlErrorMessage" } } diff --git a/src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesReplaceApiTest.kt b/src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesReplaceApiTest.kt index 6932e68..2501287 100644 --- a/src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesReplaceApiTest.kt +++ b/src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesReplaceApiTest.kt @@ -154,7 +154,7 @@ class TimetablesReplaceApiTest(@Autowired val mockMvc: MockMvc) { "message": "JDBC Commit Failed", "extensions": { "code": 409, - "type": "TransactionSystemError", + "type": "ConflictingSchedulesError", "sqlErrorMessage": "$sqlErrorMessage" } } diff --git a/src/test/kotlin/fi/hsl/jore4/timetables/api/util/TransactionSystemExtensionsTest.kt b/src/test/kotlin/fi/hsl/jore4/timetables/api/util/TransactionSystemExtensionsTest.kt new file mode 100644 index 0000000..ff07503 --- /dev/null +++ b/src/test/kotlin/fi/hsl/jore4/timetables/api/util/TransactionSystemExtensionsTest.kt @@ -0,0 +1,116 @@ +package fi.hsl.jore4.timetables.api.util + +import org.junit.jupiter.api.Test +import org.springframework.transaction.TransactionSystemException +import kotlin.test.assertEquals + +class TransactionSystemExtensionsTest { + + private fun createTransactionSystemExceptionWithCause(message: String): TransactionSystemException { + return TransactionSystemException("test exception", Exception(message)) + } + + @Test + fun `resolves PassingTimeStopPointMatchingOrderError`() { + val exception = TransactionSystemExtensions.from( + createTransactionSystemExceptionWithCause( + """ + ERROR: passing times and their matching stop points must be in same order + Where: PL/pgSQL function passing_times.validate_passing_time_sequences() + """.trimIndent() + ) + ) + assertEquals(exception.type, HasuraErrorType.PassingTimeStopPointMatchingOrderError) + } + + @Test + fun `resolves PassingTimeFirstArrivalTimeError`() { + val exception = TransactionSystemExtensions.from( + createTransactionSystemExceptionWithCause( + """ + ERROR: first passing time must not have arrival_time set + Where: PL/pgSQL function passing_times.validate_passing_time_sequences() + """.trimIndent() + ) + ) + assertEquals(exception.type, HasuraErrorType.PassingTimeFirstArrivalTimeError) + } + + @Test + fun `resolves PassingTimeLastDepartureTimeError`() { + val exception = TransactionSystemExtensions.from( + createTransactionSystemExceptionWithCause( + """ + ERROR: last passing time must not have departure_time set + Where: PL/pgSQL function passing_times.validate_passing_time_sequences() + """.trimIndent() + ) + ) + assertEquals(exception.type, HasuraErrorType.PassingTimeLastDepartureTimeError) + } + + @Test + fun `resolves PassingTimeNullError`() { + val exception = TransactionSystemExtensions.from( + createTransactionSystemExceptionWithCause( + """ + ERROR: all passing time that are not first or last in the sequence must have both departure and arrival time defined + Where: PL/pgSQL function passing_times.validate_passing_time_sequences() + """.trimIndent() + ) + ) + assertEquals(exception.type, HasuraErrorType.PassingTimeNullError) + } + + @Test + fun `resolves PassingTimesMixedJourneyPatternRefsError`() { + val exception = TransactionSystemExtensions.from( + createTransactionSystemExceptionWithCause( + """ + ERROR: inconsistent journey_pattern_ref within vehicle journey, all timetabled_passing_times must reference same journey_pattern_ref as the vehicle_journey + Where: PL/pgSQL function passing_times.validate_passing_time_sequences() + """.trimIndent() + ) + ) + assertEquals(exception.type, HasuraErrorType.PassingTimesMixedJourneyPatternRefsError) + } + + @Test + fun `resolves ConflictingSchedulesError`() { + val exception = TransactionSystemExtensions.from( + createTransactionSystemExceptionWithCause( + """ + ERROR: conflicting schedules detected: vehicle schedule frame + Where: PL/pgSQL function vehicle_schedule.validate_queued_schedules_uniqueness() + """.trimIndent() + ) + ) + assertEquals(exception.type, HasuraErrorType.ConflictingSchedulesError) + } + + @Test + fun `resolves SequentialIntegrityError`() { + val exception = TransactionSystemExtensions.from( + createTransactionSystemExceptionWithCause( + """ + ERROR: Sequential integrity issues detected: + Where: PL/pgSQL function vehicle_service.validate_service_sequential_integrity() + """.trimIndent() + ) + ) + assertEquals(exception.type, HasuraErrorType.SequentialIntegrityError) + } + + @Test + fun `resolves an unknown error as TransactionSystemError`() { + val exception = TransactionSystemExtensions.from( + createTransactionSystemExceptionWithCause( + """ + ERROR: something else: + Where: PL/pgSQL function somewhere_else() + """.trimIndent() + ) + ) + assertEquals(exception.type, HasuraErrorType.TransactionSystemError) + } +} diff --git a/src/test/kotlin/fi/hsl/jore4/timetables/service/CombineTimetablesServiceTest.kt b/src/test/kotlin/fi/hsl/jore4/timetables/service/CombineTimetablesServiceTest.kt index 3aa8a50..583c9cb 100644 --- a/src/test/kotlin/fi/hsl/jore4/timetables/service/CombineTimetablesServiceTest.kt +++ b/src/test/kotlin/fi/hsl/jore4/timetables/service/CombineTimetablesServiceTest.kt @@ -8,11 +8,13 @@ import fi.hsl.jore4.timetables.extensions.deepClone import fi.hsl.jore4.timetables.extensions.getNested import fi.hsl.jore4.timetables.repository.VehicleScheduleFrameRepository import fi.hsl.jore4.timetables.repository.VehicleServiceRepository +import mu.KotlinLogging import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.DisplayName import org.junit.jupiter.api.Nested import org.junit.jupiter.api.Test import org.springframework.beans.factory.annotation.Autowired +import org.springframework.transaction.TransactionSystemException import java.util.UUID import kotlin.test.assertContains import kotlin.test.assertEquals @@ -20,6 +22,8 @@ import kotlin.test.assertFailsWith import kotlin.test.assertNotNull import kotlin.test.assertNull +private val LOGGER = KotlinLogging.logger {} + @IntTest class CombineTimetablesServiceTest @Autowired constructor( val combineTimetablesService: CombineTimetablesService, @@ -332,6 +336,31 @@ class CombineTimetablesServiceTest @Autowired constructor( assertEquals(listOf(UUID.fromString("bb2abc90-8e91-4b0b-a7e4-751a04e81ba3")), result) assertNull(vehicleScheduleFrameRepository.fetchOneByVehicleScheduleFrameId(stagingFrameId)) } + + @Test + fun `fails when combininga would result in invalid data`() { + val testData = TimetablesDataset.createFromResource("datasets/combine_multiple_targets.json") + testData.getNested("_vehicle_schedule_frames.target2")["validity_start"] = "2022-07-15" + testData.getNested("_vehicle_schedule_frames.target2")["validity_end"] = "2023-05-15" + + timetablesDataInserterRunner.truncateAndInsertDataset(testData.toJSONString()) + + val stagingFrameId = UUID.fromString("e8d07c0d-575f-4cbe-bddb-ead5b2943638") + val exception = assertFailsWith { + combineTimetablesService.combineTimetables( + listOf(stagingFrameId), + TimetablesPriority.STANDARD + ) + } + + // Check that the error messages are somewhat in the format we expect. TransactionSystemExtensions depends on these. + assertEquals(exception.message, "JDBC commit failed") + val causeMessage = exception?.cause?.message + assertNotNull(causeMessage) + assertContains(causeMessage, "ERROR: conflicting schedules detected: vehicle schedule frame") + assertContains(causeMessage, "Where: PL/pgSQL function vehicle_schedule.validate_queued_schedules_uniqueness()") + assertContains(causeMessage, "SQL statement \"SELECT vehicle_schedule.validate_queued_schedules_uniqueness()") + } } @Nested