Rework connector logging to avoid logging response bodies#332
Rework connector logging to avoid logging response bodies#332DavidGregory084 wants to merge 1 commit into
Conversation
| .recover { case NonFatal(ex) => | ||
| logger.warn(EISErrorMessage(createdDateTime, ex.getMessage, correlationId, messageType), ex) | ||
| .recover { | ||
| case _: JacksonException => |
There was a problem hiding this comment.
JacksonException is the subtype of exceptions we can get from play-json's Json.parse, which is what HttpReads does under the hood
| logger.error(EISErrorMessage.parseError(createdDateTime, correlationId, messageType)) | ||
| internalError(timestamp, correlationId) | ||
|
|
||
| case _: JsResultException => |
There was a problem hiding this comment.
JsResultException is the kind of exception we can get from .as[A], .validate[A] and HttpReads' automatic JSON deserialization.
| ) | ||
| case NonFatal(ex) => | ||
| // Something else | ||
| logger.error(EISErrorMessage(createdDateTime, ex.getMessage, correlationId, messageType), ex) |
There was a problem hiding this comment.
I am in two minds about the logging of ex.getMessage here - I think that we have covered all of the typical cases where user data could be involved above
| logger.error(EISErrorMessage.readError(createdDateTime, correlationId, "PreValidateTrader")) | ||
| Left(InternalServerError(Json.toJson(internalError(timestamp, correlationId)))) | ||
|
|
||
| case response: UpstreamErrorResponse => |
There was a problem hiding this comment.
I have essentially inlined the logic of the PreValidateTraderHttpReader here.
I don't think that splitting up this logic was helpful as it just spread all of the possible error handling outcomes across multiple files.
| correlationId | ||
| ) | ||
|
|
||
| Left(Status(response.statusCode)(Json.toJson(ourErrorResponse))) |
There was a problem hiding this comment.
I don't think that we should pass through the response.statusCode here; it would be better if we passed through only 404 responses and otherwise used response.reportAs, which converts client errors to 500 status and server errors to 502 status.
However, I don't really want to change the behaviour of the service in production as users are presumably used to its quirks now.
| logger.error(EISErrorMessage.readError(createdDateTime, correlationId, "PreValidateTrader")) | ||
| Left(InternalServerError(Json.toJson(internalError(timestamp, correlationId)))) | ||
|
|
||
| case response: UpstreamErrorResponse => |
There was a problem hiding this comment.
This is the logic of PreValidateTraderETDSHttpReader inlined here
|
| ) | ||
| } | ||
|
|
||
| Left(Status(response.statusCode)(Json.toJson(ourErrorResponse))) |
There was a problem hiding this comment.
Same comment as above about response.statusCode
| private val validResponse = getPreValidateTraderSuccessResponse | ||
| private val businessError = getPreValidateTraderErrorResponse | ||
| private val validResponse = getPreValidateTraderSuccessEISResponse | ||
| private val businessError = getPreValidateTraderErrorEISResponse |
There was a problem hiding this comment.
This test suite originally used the wrong types everywhere and only got away with it due to Mockito and the fact that generic types don't really exist at the JVM level
| emcsCorrelationId | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The original response values here were a lie - the real connector couldn't actually produce responses that contained the response body.
It only appeared like this was possible because we're mocking away the HttpReads logic, which was originally producing the error responses.
2 participants
The current connector logic can be tricked into logging things it really shouldn't via response bodies or via JSON parsing errors.
This reworks connector logging so that it only logs exceptions in the case that they really are exceptions and not something to do with response data.