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