Add types to error responses

[Leitsi]

Relates to HSLdevcom/jore4#1584

---

This change is 

[Leitsi]

Reviewable status: 0 of 27 files reviewed, all discussions resolved

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt line 135 at r1 (raw file):

            )

            return errorMatchingConditions.find { it.second() }?.first ?: HasuraErrorType.TransactionSystemError

Note: I initially implemented this with when, like so:

return when {  
   errorMessage.contains("passing times and their matching stop points must be in same order") -> HasuraErrorType.PassingTimeStopPointMatchingOrderError  
   errorMessage.contains("first passing time must not have arrival\_time set") -> HasuraErrorType.PassingTimeFirstArrivalTimeError  
   errorMessage.contains("last passing time must not have departure\_time set") -> HasuraErrorType.PassingTimeLastDepartureTimeError  
   errorMessage.contains("all passing time that are not first or last in the sequence must have both departure and arrival time defined") -> HasuraErrorType.PassingTimeNullError  
   errorMessage.contains("inconsistent journey\_pattern\_ref within vehicle journey, all timetabled\_passing\_times must reference same journey\_pattern\_ref as the vehicle\_journey") -> HasuraErrorType.PassingTimesMixedJourneyPatternRefsError  
   errorMessage.contains("validate\_queued\_schedules\_uniqueness") -> HasuraErrorType.ConflictingSchedulesError  
   errorMessage.contains("validate\_service\_sequential\_integrity") -> HasuraErrorType.SequentialIntegrityError  
   else \-> HasuraErrorType.TransactionSystemError  
}

What bothered me with that was that the resulting HasuraErrorType was at the end of each line. I think that hurt readability. So I implemented it with this errorMatchingConditions approach instead, to move the error types to start of line. Let me know if you disagree or have better suggestions.

[culka]

Reviewed 27 of 27 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Leitsi)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt line 135 at r1 (raw file):

Previously, Leitsi (Tommi Leinamo) wrote…

Note: I initially implemented this with when, like so:

return when {  
   errorMessage.contains("passing times and their matching stop points must be in same order") -> HasuraErrorType.PassingTimeStopPointMatchingOrderError  
   errorMessage.contains("first passing time must not have arrival\_time set") -> HasuraErrorType.PassingTimeFirstArrivalTimeError  
   errorMessage.contains("last passing time must not have departure\_time set") -> HasuraErrorType.PassingTimeLastDepartureTimeError  
   errorMessage.contains("all passing time that are not first or last in the sequence must have both departure and arrival time defined") -> HasuraErrorType.PassingTimeNullError  
   errorMessage.contains("inconsistent journey\_pattern\_ref within vehicle journey, all timetabled\_passing\_times must reference same journey\_pattern\_ref as the vehicle\_journey") -> HasuraErrorType.PassingTimesMixedJourneyPatternRefsError  
   errorMessage.contains("validate\_queued\_schedules\_uniqueness") -> HasuraErrorType.ConflictingSchedulesError  
   errorMessage.contains("validate\_service\_sequential\_integrity") -> HasuraErrorType.SequentialIntegrityError  
   else \-> HasuraErrorType.TransactionSystemError  
}

What bothered me with that was that the resulting HasuraErrorType was at the end of each line. I think that hurt readability. So I implemented it with this errorMatchingConditions approach instead, to move the error types to start of line. Let me know if you disagree or have better suggestions.

Few suggestions. One is to use SQL_ERROR_MESSAGE constants or similar to move them away from these lines for a cleaner look.

You can also move the return value to the next line, like

return when {
    errorMessage.contains("passing times and their matching stop points must be in same order")
    -> HasuraErrorType.PassingTimeStopPointMatchingOrderError
    errorMessage.contains("first passing time must not have arrival\_time set")
    -> HasuraErrorType.PassingTimeFirstArrivalTimeError
    else
    -> HasuraErrorType.TransactionSystemError

---

src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesCombineApiTest.kt line 126 at r1 (raw file):

    @Test
    fun `throws a 409 when service transaction fails to commit`() {

The test name says it tests when a commit fails, yet tests for a specific case.

I think this should use a generic case like TransactionSystemError, or either rename test to when there is a conflicting schedule.

---

src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesReplaceApiTest.kt line 134 at r1 (raw file):

    @Test
    fun `throws a 409 when service transaction fails to commit`() {

The test name says it tests when a commit fails, yet tests for a specific case.

I think this should use a generic case like TransactionSystemError, or either rename test to when there is a conflicting schedule.

[Leitsi]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @culka)

---

src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesCombineApiTest.kt line 126 at r1 (raw file):

Previously, culka (Teemu Mäkinen) wrote…

The test name says it tests when a commit fails, yet tests for a specific case.

I think this should use a generic case like TransactionSystemError, or either rename test to when there is a conflicting schedule.

Chose to use one of the more specific errors rather than an unrecognized transaction error so we test that the type is picked up correctly as well. I don't think it makes sense to test for every possible TransactionSystemError here either, that's why I created the separate TransactionSystemExtensionsTest suite for those.

I could add separate tests for plain TransactionSystemError and some more specific type, but again I don't think that provides much value. The plain TransactionSystemError test wouldn't really cover anything that this current one or the TransactionSystemExtensionsTest don't cover.

Maybe just slap a comment here we're using a "subset" of transaction errors here to check that type is set correctly as well?

[jarkkoka]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @culka and @Leitsi)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt line 135 at r1 (raw file):

Previously, culka (Teemu Mäkinen) wrote…

Few suggestions. One is to use SQL_ERROR_MESSAGE constants or similar to move them away from these lines for a cleaner look.

You can also move the return value to the next line, like

return when {
    errorMessage.contains("passing times and their matching stop points must be in same order")
    -> HasuraErrorType.PassingTimeStopPointMatchingOrderError
    errorMessage.contains("first passing time must not have arrival\_time set")
    -> HasuraErrorType.PassingTimeFirstArrivalTimeError
    else
    -> HasuraErrorType.TransactionSystemError

I'd refactor out redundant errorMessage.contains(...) structure,

[Leitsi]

Reviewable status: 23 of 27 files reviewed, 2 unresolved discussions (waiting on @culka)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt line 135 at r1 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

I'd refactor out redundant errorMessage.contains(...) structure,

Refactored a bit but left this list + find approach. Mainly moved the contains check inside the find and replaced it with in operator. Also added a comment.

---

src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesCombineApiTest.kt line 126 at r1 (raw file):

Previously, Leitsi (Tommi Leinamo) wrote…

Chose to use one of the more specific errors rather than an unrecognized transaction error so we test that the type is picked up correctly as well. I don't think it makes sense to test for every possible TransactionSystemError here either, that's why I created the separate TransactionSystemExtensionsTest suite for those.

I could add separate tests for plain TransactionSystemError and some more specific type, but again I don't think that provides much value. The plain TransactionSystemError test wouldn't really cover anything that this current one or the TransactionSystemExtensionsTest don't cover.

Maybe just slap a comment here we're using a "subset" of transaction errors here to check that type is set correctly as well?

Added the comment here and on the other similar test.

---

src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesReplaceApiTest.kt line 134 at r1 (raw file):

Previously, culka (Teemu Mäkinen) wrote…

The test name says it tests when a commit fails, yet tests for a specific case.

I think this should use a generic case like TransactionSystemError, or either rename test to when there is a conflicting schedule.

Solved in same way as the above comment in same issue.

[culka]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Leitsi)

---

src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesCombineApiTest.kt line 126 at r1 (raw file):

Previously, Leitsi (Tommi Leinamo) wrote…

Added the comment here and on the other similar test.

👌

---

src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesReplaceApiTest.kt line 134 at r1 (raw file):

Previously, Leitsi (Tommi Leinamo) wrote…

Solved in same way as the above comment in same issue.

👍

[jarkkoka]

Reviewable status: 26 of 27 files reviewed, 1 unresolved discussion (waiting on @culka and @Leitsi)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt line 131 at r3 (raw file):

                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",

This list could be held in a const member of companion object. Not needed to construct it all over again.

[Leitsi]

Reviewable status: 25 of 27 files reviewed, 1 unresolved discussion (waiting on @culka and @jarkkoka)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt line 131 at r3 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

This list could be held in a const member of companion object. Not needed to construct it all over again.

Lists can't be const members it looks like (only primitives & strings can), but I moved it and marked as private, should be fine.

[jarkkoka]

Reviewed 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @culka and @Leitsi)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt line 135 at r1 (raw file):

Previously, Leitsi (Tommi Leinamo) wrote…

Refactored a bit but left this list + find approach. Mainly moved the contains check inside the find and replaced it with in operator. Also added a comment.

Well done!

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt line 19 at r3 (raw file):

    // Not required by Hasura, but we want to always pass one.
    val type: HasuraErrorType

I'd replace

"but we want to always pass one"

with

"but we want to provide one so that the UI can present nice detailed error messages".

To be clear, I would introduce another interface for the type property, since it's not related to Hasura. Also, the enum name is wrong IMO.

sealed interface HasuraErrorExtensions {

    // code must be 4xx
    val code: Int
}

sealed interface JoreErrorExtensions: HasuraErrorExtensions {

    // Not required by Hasura, but we want to always pass one.
    val type: HasuraErrorType
}

data class PlainStatusExtensions(
    override val code: Int,
    override val type: HasuraErrorType
) : JoreErrorExtensions {
...

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorType.kt line 3 at r4 (raw file):

package fi.hsl.jore4.timetables.api.util

enum class HasuraErrorType {

The name of this class is really bad. I'd like to rename it to "VehicleSchedulePersistenceErrorType" or something shorter but still descriptive.

These types of errors really have nothing to do with Hasura itself.

However, these are very well separated error cases. Props for that! 👍

[Leitsi]

Reviewable status: 23 of 28 files reviewed, 2 unresolved discussions (waiting on @culka and @jarkkoka)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt line 19 at r3 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

I'd replace

"but we want to always pass one"

with

"but we want to provide one so that the UI can present nice detailed error messages".

To be clear, I would introduce another interface for the type property, since it's not related to Hasura. Also, the enum name is wrong IMO.

sealed interface HasuraErrorExtensions {

    // code must be 4xx
    val code: Int
}

sealed interface JoreErrorExtensions: HasuraErrorExtensions {

    // Not required by Hasura, but we want to always pass one.
    val type: HasuraErrorType
}

data class PlainStatusExtensions(
    override val code: Int,
    override val type: HasuraErrorType
) : JoreErrorExtensions {
...

👌 done. Did similar change for HasuraErrorResponse, so the type is still required in the API responses.

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorType.kt line 3 at r4 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

The name of this class is really bad. I'd like to rename it to "VehicleSchedulePersistenceErrorType" or something shorter but still descriptive.

These types of errors really have nothing to do with Hasura itself.

However, these are very well separated error cases. Props for that! 👍

Good point. Renamed, but to TimetablesApiErrorType instead. These don't necessarily have to do with persistence, see eg. TargetPriorityParsingError.

[jarkkoka]

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @culka and @Leitsi)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/TimetablesController.kt line 122 at r5 (raw file):

    @ExceptionHandler(RuntimeException::class)
    fun handleRuntimeException(ex: RuntimeException): ResponseEntity<JoreErrorResponse> {
        val hasuraExtensions: JoreErrorExtensions = when (ex) {

Maybe hasuraExtensions -> errorExtensions would be better.

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorResponse.kt line 4 at r5 (raw file):

// See https://hasura.io/docs/latest/actions/action-handlers/#returning-an-error-response
open class HasuraErrorResponse(

As of now, the open could replaced by sealed.