-
Notifications
You must be signed in to change notification settings - Fork 310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sttp2: response-as changes #284
Comments
In some http protocols, when there is an error, there is a json formatted error body that contains the error information in addition to the status code being 4xx or 5xx, etc. It was not clear that the proposal above makes that an easy case to code for. I'd prefer to have a type error but one that potentially has a different hierarchy than Throwable. Unless you are explicitly supporting an effect that allows a non-Throwable hierarchy in its error channel or have some other clever approach for non-effectful backends, it seems a left string really does not make it much easier since I would still have to run a json parser against it. |
@aappddeevv good point! That case would be representable as well, but you'd to adjust the json-deserializing code a bit. Maybe we could improve the hierarchy to make this easier. So the default hierarchy as it is now is:
Alternatively, it could be sth like:
The
And we could introduce a variant to decode errors as well:
What do you think? By the way, there's always an escape hatch. You can always use |
The above would essentially make
|
I think as long as your effect is optional e.g. not async, and you don't have an explicit error channel in your effect you will have a poor man's either floating around. You may want to figure out if most people are sync or async then optimize for the most frequent usage. I don't have a good answer for this, all I know is that it is hard. |
That's yet another category of errors: connection exceptions which are propagated through the effect. I suppose most people use async, but both use-cases need to be covered. However the main difference here is that sync throws exceptions (when there are connection problems), while async wraps them in the effect |
@aappddeevv after some consideration I think I'm going to leave the hierarchy as-is. But I've restructured the code so that it should be easy to re-use the existing parts. For the errors-as-json case, you would have to create a custom This would look sth like this: import com.softwaremill.sttp.json.circe._
asStringAlways.mapWithMetadata { (body, metadata) =>
if (metadata.isSuccess) {
eitherToCustomJsonSuccess(deserializeJson[SUCCESS_BODY](body))
} else if (metadata.isClientError) {
eitherToCustomJsonError(deserializeJson[ERROR_BODY](body))
} else {
CustomHttpError(body)
}
} Of course that would go into the docs :) |
Hmmm..I'm not sure that's the right answer. It feels highly imperative...which I'm not against though. I think there was some cats article on their site about hierarchy of errors. |
@aappddeevv do you mean https://typelevel.org/blog/2018/08/25/http4s-error-handling-mtl.html ? Combining errors is a tricky subject (see also ZIO). I think union types will help a lot, but until then :) I would say that the above is not declarative. But then, success/error criteria vary and are hard to capture declaratively. |
It was some article the cats github docs site...somewhere in https://typelevel/cats. I can't seem to find it now. Anyway, maybe its best not to try anything clever quite yet because its so hard as you mention. |
👍 👍 from me. This is the one of the main reasons why we found using sttp a bit cumbersome (but it's the best of the bunch!), as the types were being too restrictive/opinionated, resulting in a lot of "postprocessing" that we had to do to make the return type actually what we want. Good to see that this is being addressed! |
Released in 2.0.0-M5 |
The main pain point of sttp1, and a common source of confusion is the way errors are represented in the response body. For example, when parsing a response as json using circe, we get a cryptic type:
It's far from clear what the nested eithers correspond to (the outside either corresponds to http errors, while the inner one to json parsing errors).
Further, the way errors are handled is currently hard-coded: in case of a non-2xx response, the body is read as a byte array. There are some ways to change that (using
request.parseResponseIf
), but they are still limited.Hence the proposed change for sttp2, which is a major breaking change (source code will need to be adjusted).
The basic idea is to remove the assumptions on how errors should be handled. A
Request[T, ...]
will now result in a response, where the body has typeT
, regardless of the status code. Second, apart from mapping over response specifications, it will be possible to dynamically decide (basing on the headers & status code) on how to read the response. This brings a lot of flexibility.However, what about the common-case? Usually, when sending a request, the error and success cases should be handled separately. So for most requests, we still want the response body to have type
Either[String, T]
, whereString
corresponds to the error case, andT
to the successful one.That's why the basic response specifications are now changed. For example,
asString: ResponseAs[Either[String, String]]
,asFile: ResponseAs[Either[String, File]]
etc. Errors are represented at the top-level, right in the request specification. This has one important implication: the type ofsttp
, as well as many requests derived from it will change. What was before aRequest[T, ...]
, will now most probably becomeRequest[Either[String, T], ...]
.That's the problematic part. What do we gain?
sttp.response(asStringAlways)
, and then the reponse's body will be a plainString
:response.body: String
. Same for byte arrays, streams etc.ResponseAs[Either[ResponseError[E], T]
. The top-levelEither
remains to signal if the request was successful or not. However, both kinds of errors (http and deserialization) are represented as a left value, without the nesting:ResponseError
has two implementations,HttpError
andDeserializationError
.What do you think? Any ideas to further improve response handling? Or should the current way remain?
Related commit: 6d59d73
The text was updated successfully, but these errors were encountered: