Skip to content

Commit

Permalink
Add fine grained exceptions for known commit error situations
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Leitsi committed Nov 16, 2023
1 parent b334519 commit 5ef70c1
Show file tree
Hide file tree
Showing 6 changed files with 193 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -146,7 +151,7 @@ class TimetablesCombineApiTest(@Autowired val mockMvc: MockMvc) {
"message": "JDBC Commit Failed",
"extensions": {
"code": 409,
"type": "TransactionSystemError",
"type": "ConflictingSchedulesError",
"sqlErrorMessage": "$sqlErrorMessage"
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -154,7 +159,7 @@ class TimetablesReplaceApiTest(@Autowired val mockMvc: MockMvc) {
"message": "JDBC Commit Failed",
"extensions": {
"code": 409,
"type": "TransactionSystemError",
"type": "ConflictingSchedulesError",
"sqlErrorMessage": "$sqlErrorMessage"
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,22 @@ 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
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,
Expand Down Expand Up @@ -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<TransactionSystemException> {
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
Expand Down

0 comments on commit 5ef70c1

Please sign in to comment.