r-lib / httr2

Make HTTP requests and process their responses. A modern reimagining of httr.
https://httr2.r-lib.org
Other
235 stars 56 forks source link

Handle streaming errors like regular errors #492

Closed hadley closed 2 months ago

hadley commented 2 months ago

Fixes #479

@jwimberl how does this look to you?

jwimberl commented 2 months ago

Testing this branch, I see uniform behavior btw req_perform and req_perform_stream in one of the reprex cases from #479:

packageVersion("httr2")
#> [1] '1.0.1.9000'
library(httr2)
req <- request("https://api.github.com/asdfsdadf")
a <- req |> req_perform()
#> Error in `req_perform()`:
#> ! HTTP 404 Not Found.
last_response() |> resp_body_string()
#> [1] "{\n  \"message\": \"Not Found\",\n  \"documentation_url\": \"https://docs.github.com/rest\",\n  \"status\": \"404\"\n}\n"
b <- req |> req_perform_stream(callback = \(z) TRUE)
#> Error in `req_perform_stream()`:
#> ! HTTP 404 Not Found.
last_response() |> resp_body_string()
#> [1] "{\n  \"message\": \"Not Found\",\n  \"documentation_url\": \"https://docs.github.com/rest\",\n  \"status\": \"404\"\n}\n"

Created on 2024-07-12 with reprex v2.1.1

So, both lead to an R error, and last_response() contains the error body in both cases, which looks good to me, as this most likely how I would want to call endpoints like this.

I do see a slight difference behavior in the other reprex case:

packageVersion("httr2")
#> [1] '1.0.1.9000'
library(httr2)
req <- request("https://api.github.com/asdfsdadf")
a <- req |> req_error(is_error = \(z) FALSE) |> req_perform()
last_response() |> resp_body_string()
#> [1] "{\n  \"message\": \"Not Found\",\n  \"documentation_url\": \"https://docs.github.com/rest\",\n  \"status\": \"404\"\n}\n"
b <- req |> req_error(is_error = \(z) FALSE) |> req_perform_stream(callback = \(z) TRUE)
last_response() |> resp_body_string()
#> Error in `resp_body_raw()`:
#> ! Can't retrieve empty body.

Created on 2024-07-12 with reprex v2.1.1

but this makes sense too -- if I'm overriding things to tell it its not an error, of course it would leave the body to be handled by the callback. So, this seems consistent to me as well.

hadley commented 2 months ago

@jwimberl thanks for the thoughtful review. I really appreciate it 😄