raboof commented on code in PR #799:
URL: https://github.com/apache/pekko-connectors/pull/799#discussion_r1757166610
##########
google-common/src/main/scala/org/apache/pekko/stream/connectors/google/ResumableUpload.scala:
##########
@@ -93,10 +94,16 @@ private[connectors] object ResumableUpload {
import implicits._
implicit val um: FromResponseUnmarshaller[Uri] =
- Unmarshaller.withMaterializer { implicit ec => implicit mat =>
(response: HttpResponse) =>
- response.discardEntityBytes().future.map { _ =>
- response.header[Location].fold(throw
InvalidResponseException(ErrorInfo("No Location header")))(_.uri)
- }
+ Unmarshaller.withMaterializer { _ => implicit mat => (response:
HttpResponse) =>
+ if (response.status.isSuccess())
+ response.discardEntityBytes().future.map { _ =>
+ response.header[Location].fold(throw
InvalidResponseException(ErrorInfo("No Location header")))(_.uri)
+ }(ExecutionContexts.parasitic)
+ else
+ response.entity.dataBytes.runWith(Sink.fold(ByteString.empty)(_ ++
_)).flatMap { body =>
+ Future.failed(InvalidResponseException(
+ ErrorInfo(s"Bad response, status code:
${response.status.intValue()} and body: $body")))
Review Comment:
I agree having the status code in the summary is fine (and I do think it's a
lot less misleading than the 'missing header' error).
For the case where the error bubbles up to a Pekko HTTP endpoint it's kind
of already configurable with `pekko.http.routing.verbose-error-messages` or by
configuring the HTTP error handling. I guess we could consider also logging the
body here, so that can be enabled/disabled with the log configuration - though
that also already happens when it bubbles up to Pekko HTTP
(https://github.com/apache/pekko-http/blob/main/http/src/main/scala/org/apache/pekko/http/scaladsl/server/ExceptionHandler.scala#L71).
We might even want to make *that* configurable actually...
I do think we generally want to err on the safe side wrt exposing details
(e.g.
https://pekko.apache.org/docs/pekko-http/1.1/routing-dsl/exception-handling.html#including-sensitive-data-in-exceptions)
Unmarshalling the error response to safely get the 'real' error is kinda
neat but sounds like overkill.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
