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

Change tutorial to use ujson integr. #2876

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Change tutorial to use ujson integr. #2876

wants to merge 1 commit into from

Conversation

szymon-rd
Copy link
Member

No description provided.

@szymon-rd szymon-rd requested a review from adpi2 July 26, 2023 10:42
@bishabosha
Copy link
Member

bishabosha commented Jul 26, 2023

originally I explored this a lot and it seems like maybe its too much magic (particularly when parsing the response body) - because now it returns either rather than throw exception,

maybe its more acceptable for writing the request body?

@adpi2
Copy link
Member

adpi2 commented Jul 26, 2023

maybe its more acceptable for writing the request body?

It is indeed more acceptable for writing the request body. But then I don't like that it is not symmetrical: we use usjson.read to read the response body but we don't use ujson.write to write the request body.

IMO the current documentation is "simple" enough and clear. It's symmetrical between request and response, and we can compare it with writing and reading to files too. There is no oslib-upickle integration so why would we need an sttp-upickle integration. It does not add anything really useful.

I don't think we should merge this PR.

@szymon-rd
Copy link
Member Author

If we merged that, it would also make sense to use the .response(asJson[...]) mechanism, like in the example for sttp-ujson integration:

basicRequest
  .post(uri"...")
  .body(requestPayload)
  .response(asJson[ResponsePayload])
  .send(backend)

@bishabosha
Copy link
Member

bishabosha commented Jul 26, 2023

If we merged that, it would also make sense to use the .response(asJson[...]) mechanism, like in the example for sttp-ujson integration:

basicRequest
  .post(uri"...")
  .body(requestPayload)
  .response(asJson[ResponsePayload])
  .send(backend)

this also changes the body to be Either[..., ResponsePayload], which is something that could be surprising.

see softwaremill/sttp#1771 for discussion

@adamw
Copy link

adamw commented Jul 27, 2023

I think that since we do have sttp<->ujson integration, we should be using that, as that's the way people should use both libraries together. I see how breaking this down might be educational, but then is the goal showing how to best use the toolkit libraries, or is the goal to explain the underlying mechanics? I think it's the former, but of course I might be wrong :)

Also I agree that then we should use the integration fully. Error handling is still something I'd like to spend some time on, and possibly refine it in sttp4. But it's a tricky thing to get right ;).

For now if we want exceptions to be thrown upon a non-2xx response, or a deserialization problem, that's the way you would do it with sttp:

basicRequest
  .post(uri"...")
  .body(requestPayload)
  .response(asJson[ResponsePayload].getRight)
  .send(backend)

Not saying it's the perfect way or that it can't be improved, but that's where the API is right now :)

@adpi2
Copy link
Member

adpi2 commented Jul 28, 2023

We have two APIs:

  1. the ujson/upickle API
val response = quickRequest
  .post(...)
  .send()

val json = ujson.read(response.body)
  1. the sttp-upickle API
val response = quickRequest
  .post(...)
  .response(asJson[ujson.Value].getRight)
  .send()

val json = response.body

In the context of the Toolkit, which is scripting and prototyping, I don't think that 2 is significantly better than 1.

More importantly, I think we should better explain that sttp and upickle combine well together rather than showing a specific API for handling JSON in HTTP requests. Otherwise the learner will get confused: "Why do I have to learn 2 different APIs, one general API to handle JSON (upickle/ujson) and one more specific for HTTP requests (sttp-upickle)?"

@adamw
Copy link

adamw commented Jul 28, 2023

Good point, you are right that in the context of quickRequest where we always read the response into a string and not care about non-2xx responses, ujson.read(response.body) is the simpler thing to do.

Though I think you are wrong about scoping toolkit to scripting/prototyping only. Since there is an "official" set of libaries - the toolkit - people will be using that in all kinds of scenarios.

Maybe that's planned, but "advanced" tutorials which would cover - for example - error handling in http requests using response descriptions (asJson etc.) would make sense as well>

@adpi2
Copy link
Member

adpi2 commented Jul 28, 2023

Maybe that's planned, but "advanced" tutorials which would cover - for example - error handling in http requests using response descriptions (asJson etc.) would make sense as well

We do not intend to cover all the features of the Toolkit libraries in the Toolkit tutorials. The Toolkit tutorials focus mostly on straightforward solutions for scripting and prototyping. There is What else can sttp do? where we can add a section about error handling that would redirect to the sttp documentation.

@adamw
Copy link

adamw commented Jul 28, 2023

Right, rewriting the whole docs would be quite pointless :) Adding a section on error handling to the tutorial you mention sounds great.

Though I would still think about the toolkit docs more like a starter and tutorial introducing these tools, not necessarily focused only on scripting (which is a very niche Scala use-case atm) and prototyping. I always considered it a great idea for "normal" projects as well.

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

Successfully merging this pull request may close these issues.

None yet

4 participants