Skip to content
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

Add a Variant of asJson that always throws #1771

Open
bishabosha opened this issue Mar 16, 2023 · 15 comments
Open

Add a Variant of asJson that always throws #1771

bishabosha opened this issue Mar 16, 2023 · 15 comments

Comments

@bishabosha
Copy link

bishabosha commented Mar 16, 2023

version affected

4.0.0-M1

Describe the issue

To me it seems strange that asJsonAlways merges success and error bodies for http errors, but still wraps the body in Either in case of deserialisation problems, which still requires the user to explicitly handle deserialisation errors.

In the spirit of import sttp.client4.quick.* it seems it would make sense to have a variant of asJson that extends asJsonAlways but also throws in the case of deserialisation errors, so that for scripting, assuming a happy path has no friction from handling errors.

e.g. imagine a script like

import sttp.client4.quick.*
import sttp.client4.upicklejson.*

def allNotes: ujson.Value =
  val resp = quickRequest
    .get(uri"http://localhost:8080/api/notes/all")
    .response(asJsonOrThrow[ujson.Value])
    .send()
  resp.body

allNotes.arr.foreach(println)
@bishabosha
Copy link
Author

bishabosha commented Mar 16, 2023

the workaround currently is to not use asJson and use ujson.read directly:

import sttp.client4.quick.*

def allNotes: ujson.Value =
  val resp = quickRequest
    .get(uri"http://localhost:8080/api/notes/all")
    .send()
  ujson.read(resp.body)

allNotes.arr.foreach(println)

@adpi2
Copy link
Contributor

adpi2 commented Mar 16, 2023

or you can do:

import sttp.client4.quick.*

def allNotes: ujson.Value =
  val resp = quickRequest
    .get(uri"http://localhost:8080/api/notes/all")
    .mapResponse(ujson.read(_))
    .send()
    .body

allNotes.arr.foreach(println)

@adamw
Copy link
Member

adamw commented Mar 16, 2023

I think .getRight does what you want: https://sttp.softwaremill.com/en/stable/responses/body.html#failing-when-the-response-code-is-not-2xx

Not saying that the defaults shouldn't change, this might be the case :)

@bishabosha
Copy link
Author

Thanks, I didn't know about .getRight before, that's really useful!

@bishabosha
Copy link
Author

bishabosha commented Mar 16, 2023

@adamw what do you think about adding another variant of quickRequest (or changing quickRequest) to use asString.getRight rather than asStringAlways? Otherwise manually overriding the response for an empty body on a get request is a bit awkward.

@adamw
Copy link
Member

adamw commented Mar 17, 2023

Otherwise manually overriding the response for an empty body on a get request is a bit awkward.

@bishabosha Not sure I understand that :) Can you give an example?

In general, I think always getting a String when using quickRequest is what you'd expect - you make a request, you get back the string content as a response, regardless of the response code. Exceptions would probably be more expected when you do some kind of parsing.

@bishabosha
Copy link
Author

bishabosha commented Mar 17, 2023

I think my core frustration is just wanting a simple way to ignore errors for a beginner (where the only imports come from stdlib + Scala Toolkit) - so they do not need to consider Either[SomeException, T] - and just get the T. So far the standard library only gives us .toTry.get which is annoying to explain.

The suggestion to use .response(asJson[T].getRight) could work for that case, but then all the cases where we do not need to deserialise a json body now have different error handling behaviour (unless we explicitly add .response(asString.getRight) - also I think that there is not a throwing example of .response(ignore)).

At the end of the day maybe this is actually a goal we shouldn't aim for and its better to actually show explicit error handling to a beginner (im unsure!).

@bishabosha
Copy link
Author

Another proposal - an extension method to ResponseAs[DeserialisationException[E], T] that throws:

extension [E, T](t: ResponseAs[Either[DeserializationException[E], T]])
  def simple: ResponseAs[T] = t.map(_.fold(throw _, identity))
import sttp.client4.quick.*

def allNotes: ujson.Value =
  val resp = quickRequest
    .get(uri"http://localhost:8080/api/notes/all")
    .response(asJsonAlways[ujson.Value].simple)
    .send()
    .body

allNotes.arr.foreach(println)

@adamw
Copy link
Member

adamw commented Mar 17, 2023

@bishabosha wouldn't this be the same as .getRight?

def getRight: ResponseAs[B, R] =

@bishabosha
Copy link
Author

No because the simple extension would only throw for deserialisation errors, not for http status codes

@adamw
Copy link
Member

adamw commented Mar 17, 2023

So what would happen then if the response is a 404 or a 500 (which probably wouldn't bo formatted as a Value json)?

@bishabosha
Copy link
Author

So what would happen then if the response is a 404 or a 500 (which probably wouldn't bo formatted as a Value json)?

If that is the most likely case, then again I guess to have a consistent error model for asJson.getRight what is missing is a default request that uses asString.getRight?

e.g. if you were making some NotesRepo object with several responses that can return json body, some responses that return no body, but you want the same throwing behaviour for 404/502 etc on the empty responses.

@adamw
Copy link
Member

adamw commented Mar 19, 2023

This seems as a quite specialised case, if you want to have specific JSON parsing for some status code, but not for others, I think you should simply do asString.mapWithMetadata(...).

The default requests in sttp don't throw, they return an Either[String, String] (the basicRequest).

But coming back to the original issue, I did consider adding a asJsonOrThrow, but in the end decided on adding a .getRight method which has less code duplication (you don't have to define the same method for every JSON integration). But it might be better to pay that price

@bishabosha
Copy link
Author

bishabosha commented Mar 19, 2023

I think then for consistency in the Scala Toolkit tutorials we should then recommend handling Either explicitly for deserialising JSON, and also to use basicRequest, so Right always means success. I think it could be confusing to switch between error handling models depending on the format of expected response.

@adamw
Copy link
Member

adamw commented Mar 22, 2023

Well we could have a asStringOrThrow, just as we can have asJsonOrThrow. Both are possible today, but they require asString.getRight or asJson.getRight. I'll reopen this to re-evaluate if it's maybe better to add the explicit response specifications(at the expense of more code), or somehow document .getRight better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants