From 5ef70c188deafd8b8f7907c08bcdad57f84c69e6 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 | 21 +++- .../timetables/api/util/HasuraErrorType.kt | 7 ++ .../api/TimetablesCombineApiTest.kt | 7 +- .../api/TimetablesReplaceApiTest.kt | 7 +- .../util/TransactionSystemExtensionsTest.kt | 116 ++++++++++++++++++ .../service/CombineTimetablesServiceTest.kt | 38 ++++++ 6 files changed, 193 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..21ba8b1 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,27 @@ 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. + + // Attempt to detect from the error message (because there's no other data really) which error case triggered the failure. + // Mainly using either the triggering validation function name or some known part of the error message for this. + val errorTypesWithMatchingSubstrings = listOf( + HasuraErrorType.PassingTimeStopPointMatchingOrderError to "passing times and their matching stop points must be in same order", + HasuraErrorType.PassingTimeFirstArrivalTimeError to "first passing time must not have arrival_time set", + HasuraErrorType.PassingTimeLastDepartureTimeError to "last passing time must not have departure_time set", + HasuraErrorType.PassingTimeNullError to "all passing time that are not first or last in the sequence must have both departure and arrival time defined", + HasuraErrorType.PassingTimesMixedJourneyPatternRefsError to "inconsistent journey_pattern_ref within vehicle journey, all timetabled_passing_times must reference same journey_pattern_ref as the vehicle_journey", + HasuraErrorType.ConflictingSchedulesError to "validate_queued_schedules_uniqueness", + HasuraErrorType.SequentialIntegrityError to "validate_service_sequential_integrity" + ) + + return errorTypesWithMatchingSubstrings.find { it.second in errorMessage }?.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..02d9020 100644 --- a/src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesCombineApiTest.kt +++ b/src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesCombineApiTest.kt @@ -124,6 +124,11 @@ class TimetablesCombineApiTest(@Autowired val mockMvc: MockMvc) { @Test fun `throws a 409 when service transaction fails to commit`() { + // Note: there are many kinds of transaction system errors (see `TransactionSystemException`). + // Just using `ConflictingSchedulesError` here as an example instead of plain `TransactionSystemError` + // to get better test coverage by involving error type detection. + // Other transaction system errors are handled similarly, + // type detection for each error case is tested in more detail in a separate suite. val sqlErrorMessage = "ERROR: conflicting schedules detected: vehicle schedule frame Where: PL/pgSQL function vehicle_schedule.validate_queued_schedules_uniqueness()" every { @@ -146,7 +151,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..907fcbc 100644 --- a/src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesReplaceApiTest.kt +++ b/src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesReplaceApiTest.kt @@ -132,6 +132,11 @@ class TimetablesReplaceApiTest(@Autowired val mockMvc: MockMvc) { @Test fun `throws a 409 when service transaction fails to commit`() { + // Note: there are many kinds of transaction system errors (see `TransactionSystemException`). + // Just using `ConflictingSchedulesError` here as an example instead of plain `TransactionSystemError` + // to get better test coverage by involving error type detection. + // Other transaction system errors are handled similarly, + // type detection for each error case is tested in more detail in a separate suite. val sqlErrorMessage = "ERROR: conflicting schedules detected: vehicle schedule frame Where: PL/pgSQL function vehicle_schedule.validate_queued_schedules_uniqueness()" every { @@ -154,7 +159,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..1259f55 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,40 @@ class CombineTimetablesServiceTest @Autowired constructor( assertEquals(listOf(UUID.fromString("bb2abc90-8e91-4b0b-a7e4-751a04e81ba3")), result) assertNull(vehicleScheduleFrameRepository.fetchOneByVehicleScheduleFrameId(stagingFrameId)) } + + @Test + fun `fails when combining would result in invalid data`() { + // There are not too many cases that can produce a commit fail here, + // since the frames have already been (mostly) validated when created as staging. + // Setup for one such case here: + // Change target2 validity times to differ from staging -> not detected as target. + // But since we have another target ("target" in dataset), we can still combine. + // However, since the staging frame overlaps with target2 (timetables active for same routes on same days), + // the commit will fail on DB constraints. + // There might be more realistic cases to reproduce similar error with, I think, + // but this is easiest with current datasets. + 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