Skip to content

Commit

Permalink
feat: return a standard problem detail instead of a custom error format
Browse files Browse the repository at this point in the history
  • Loading branch information
@fengelniederhammer
fengelniederhammer committed Oct 17, 2023
1 parent 7f6153d commit 5f89b50
Show file tree
Hide file tree
Showing 7 changed files with 54 additions and 77 deletions.
10 changes: 5 additions & 5 deletions lapis2/src/main/kotlin/org/genspectrum/lapis/auth/DataOpennessAuthorizationFilter.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ import org.genspectrum.lapis.controller.ACCESS_KEY_PROPERTY
import org.genspectrum.lapis.controller.AGGREGATED_ROUTE
import org.genspectrum.lapis.controller.AMINO_ACID_INSERTIONS_ROUTE
import org.genspectrum.lapis.controller.AMINO_ACID_MUTATIONS_ROUTE
import org.genspectrum.lapis.controller.LapisError
import org.genspectrum.lapis.controller.LapisErrorResponse
import org.genspectrum.lapis.controller.NUCLEOTIDE_INSERTIONS_ROUTE
import org.genspectrum.lapis.controller.NUCLEOTIDE_MUTATIONS_ROUTE
import org.genspectrum.lapis.util.CachedBodyHttpServletRequest
import org.springframework.http.HttpStatus
import org.springframework.http.MediaType
import org.springframework.http.ProblemDetail
import org.springframework.stereotype.Component
import org.springframework.web.filter.OncePerRequestFilter

Expand Down Expand Up @@ -54,10 +54,10 @@ abstract class DataOpennessAuthorizationFilter(protected val objectMapper: Objec
response.writer.write(
objectMapper.writeValueAsString(
LapisErrorResponse(
LapisError(
"Forbidden",
result.message,
),
ProblemDetail.forStatus(HttpStatus.FORBIDDEN).also {
it.title = HttpStatus.FORBIDDEN.reasonPhrase
it.detail = result.message
},
),
),
)
Expand Down
85 changes: 30 additions & 55 deletions lapis2/src/main/kotlin/org/genspectrum/lapis/controller/ExceptionHandler.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import mu.KotlinLogging
import org.genspectrum.lapis.model.SiloNotImplementedError
import org.genspectrum.lapis.silo.SiloException
import org.springframework.http.HttpStatus
import org.springframework.http.HttpStatusCode
import org.springframework.http.MediaType
import org.springframework.http.ProblemDetail
import org.springframework.http.ResponseEntity
import org.springframework.web.bind.annotation.ControllerAdvice
import org.springframework.web.bind.annotation.ExceptionHandler
Expand All @@ -20,91 +22,64 @@ class ExceptionHandler : ResponseEntityExceptionHandler() {
@ExceptionHandler(Throwable::class)
@ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR)
fun handleUnexpectedException(e: Throwable): ErrorResponse {
log.error(e) { "Caught unexpected exception: ${e.message}" }
log.warn(e) { "Caught unexpected exception: ${e.message}" }

return ResponseEntity
.status(HttpStatus.INTERNAL_SERVER_ERROR)
.contentType(MediaType.APPLICATION_JSON)
.body(
LapisErrorResponse(
LapisError(
"Unexpected error",
"${e.message}",
),
),
)
return responseEntity(HttpStatus.INTERNAL_SERVER_ERROR, e.message)
}

@ExceptionHandler(IllegalArgumentException::class)
@ResponseStatus(HttpStatus.BAD_REQUEST)
fun handleIllegalArgumentException(e: IllegalArgumentException): ErrorResponse {
log.error(e) { "Caught IllegalArgumentException: ${e.message}" }
log.warn(e) { "Caught IllegalArgumentException: ${e.message}" }

return ResponseEntity
.status(HttpStatus.BAD_REQUEST)
.contentType(MediaType.APPLICATION_JSON)
.body(
LapisErrorResponse(
LapisError(
"Bad request",
"${e.message}",
),
),
)
return responseEntity(HttpStatus.BAD_REQUEST, e.message)
}

@ExceptionHandler(AddForbiddenToOpenApiDocsHelper::class)
@ResponseStatus(HttpStatus.FORBIDDEN)
fun handleForbiddenException(e: AddForbiddenToOpenApiDocsHelper): ErrorResponse {
return ResponseEntity
.status(HttpStatus.FORBIDDEN)
.contentType(MediaType.APPLICATION_JSON)
.body(
LapisErrorResponse(
LapisError(
"Forbidden",
"${e.message}",
),
),
)
return responseEntity(HttpStatus.FORBIDDEN, e.message)
}

@ExceptionHandler(SiloException::class)
fun handleSiloException(e: SiloException): ErrorResponse {
log.error(e) { "Caught SiloException: ${e.statusCode} - ${e.message}" }
log.warn(e) { "Caught SiloException: ${e.statusCode} - ${e.message}" }

return ResponseEntity
.status(e.statusCode)
.contentType(MediaType.APPLICATION_JSON)
.body(
LapisErrorResponse(
LapisError(
e.title,
e.message,
),
),
)
return responseEntity(e.statusCode, e.title, e.message)
}

@ExceptionHandler(SiloNotImplementedError::class)
@ResponseStatus(HttpStatus.NOT_IMPLEMENTED)
fun handleNotImplementedError(e: SiloNotImplementedError): ErrorResponse {
log.error(e) { "Caught SiloNotImplementedError: ${e.message}" }
log.warn(e) { "Caught SiloNotImplementedError: ${e.message}" }

return responseEntity(HttpStatus.NOT_IMPLEMENTED, e.message)
}

private fun responseEntity(httpStatus: HttpStatus, detail: String?) =
responseEntity(httpStatus, httpStatus.reasonPhrase, detail)

private fun responseEntity(httpStatus: HttpStatusCode, title: String, detail: String?) =
responseEntity(httpStatus.value(), title, detail)

private fun responseEntity(
httpStatus: Int,
title: String,
detail: String?,
): ErrorResponse {
return ResponseEntity
.status(HttpStatus.NOT_IMPLEMENTED)
.status(httpStatus)
.contentType(MediaType.APPLICATION_JSON)
.body(
LapisErrorResponse(
LapisError(
"Not implemented",
"${e.message}",
),
ProblemDetail.forStatus(httpStatus).also {
it.title = title
it.detail = detail
},
),
)
}
}

/** This is not yet actually thrown, but makes "403 Forbidden" appear in OpenAPI docs. */
class AddForbiddenToOpenApiDocsHelper(message: String) : Exception(message)

data class LapisError(val title: String, val message: String)
4 changes: 3 additions & 1 deletion lapis2/src/main/kotlin/org/genspectrum/lapis/controller/LapisResponse.kt
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.genspectrum.lapis.controller

import org.springframework.http.ProblemDetail

data class LapisResponse<Data>(val data: Data)

data class LapisErrorResponse(val error: LapisError)
data class LapisErrorResponse(val error: ProblemDetail)
6 changes: 3 additions & 3 deletions lapis2/src/test/kotlin/org/genspectrum/lapis/auth/ProtectedDataAuthorizationTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ private const val notAuthorizedToAccessEndpointError = """
{
"error" : {
"title": "Forbidden",
"message": "You are not authorized to access /aggregated."
"detail": "You are not authorized to access /aggregated."
}
}
"""
Expand Down Expand Up @@ -55,7 +55,7 @@ class ProtectedDataAuthorizationTest(@Autowired val mockMvc: MockMvc) {
{
"error" : {
"title": "Forbidden",
"message": "An access key is required to access /aggregated."
"detail": "An access key is required to access /aggregated."
}
}
""",
Expand All @@ -74,7 +74,7 @@ class ProtectedDataAuthorizationTest(@Autowired val mockMvc: MockMvc) {
{
"error" : {
"title": "Forbidden",
"message": "An access key is required to access /aggregated."
"detail": "An access key is required to access /aggregated."
}
}
""",
Expand Down
16 changes: 8 additions & 8 deletions lapis2/src/test/kotlin/org/genspectrum/lapis/controller/ExceptionHandlerTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ class ExceptionHandlerTest(@Autowired val mockMvc: MockMvc) {
"""
{
"error": {
"title": "Unexpected error",
"message": "SomeMessage"
"title": "Internal Server Error",
"detail": "SomeMessage"
}
}
""",
Expand All @@ -66,7 +66,7 @@ class ExceptionHandlerTest(@Autowired val mockMvc: MockMvc) {

@Test
fun `Passes through exception with status code from SILO`() {
every { validControllerCall() } throws SiloException(123, "SomeTitle", "SomeMessage",)
every { validControllerCall() } throws SiloException(123, "SomeTitle", "SomeMessage")

mockMvc.perform(get(validRoute))
.andExpect(status().`is`(123))
Expand All @@ -77,7 +77,7 @@ class ExceptionHandlerTest(@Autowired val mockMvc: MockMvc) {
{
"error": {
"title": "SomeTitle",
"message": "SomeMessage"
"detail": "SomeMessage"
}
}
""",
Expand All @@ -97,8 +97,8 @@ class ExceptionHandlerTest(@Autowired val mockMvc: MockMvc) {
"""
{
"error": {
"title": "Bad request",
"message": "SomeMessage"
"title": "Bad Request",
"detail": "SomeMessage"
}
}
""",
Expand All @@ -118,8 +118,8 @@ class ExceptionHandlerTest(@Autowired val mockMvc: MockMvc) {
"""
{
"error": {
"title": "Not implemented",
"message": "SomeMessage"
"title": "Not Implemented",
"detail": "SomeMessage"
}
}
""",
Expand Down
6 changes: 3 additions & 3 deletions lapis2/src/test/kotlin/org/genspectrum/lapis/controller/LapisControllerCommonFieldsTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ class LapisControllerCommonFieldsTest(

mockMvc.perform(request)
.andExpect(status().isBadRequest)
.andExpect(jsonPath("\$.error.message").value("orderByField must have a string property \"field\""))
.andExpect(jsonPath("\$.error.detail").value("orderByField must have a string property \"field\""))
}

@Test
Expand Down Expand Up @@ -209,7 +209,7 @@ class LapisControllerCommonFieldsTest(

mockMvc.perform(request)
.andExpect(status().isBadRequest)
.andExpect(jsonPath("\$.error.message").value("limit must be a number or null"))
.andExpect(jsonPath("\$.error.detail").value("limit must be a number or null"))
}

@Test
Expand Down Expand Up @@ -271,7 +271,7 @@ class LapisControllerCommonFieldsTest(

mockMvc.perform(request)
.andExpect(status().isBadRequest)
.andExpect(jsonPath("\$.error.message").value("offset must be a number or null"))
.andExpect(jsonPath("\$.error.detail").value("offset must be a number or null"))
}

@Test
Expand Down
4 changes: 2 additions & 2 deletions lapis2/src/test/kotlin/org/genspectrum/lapis/controller/LapisControllerTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,9 +234,9 @@ class LapisControllerTest(@Autowired val mockMvc: MockMvc) {

mockMvc.perform(request)
.andExpect(status().isBadRequest)
.andExpect(jsonPath("\$.error.title").value("Bad request"))
.andExpect(jsonPath("\$.error.title").value("Bad Request"))
.andExpect(
jsonPath("\$.error.message").value("minProportion must be a number"),
jsonPath("\$.error.detail").value("minProportion must be a number"),
)
}

Expand Down

0 comments on commit 5f89b50

Please sign in to comment.