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

Support {httr2} #237

Closed
dpprdan opened this issue Sep 27, 2021 · 19 comments
Closed

Support {httr2} #237

dpprdan opened this issue Sep 27, 2021 · 19 comments
Milestone

Comments

@dpprdan
Copy link
Member

dpprdan commented Sep 27, 2021

Now that {httr2} is on CRAN and supports mocking, could {vcr} support it as well?

@sckott
Copy link
Collaborator

sckott commented Sep 28, 2021

That's the idea! To have support for it we need support for httr2 in webmockr as well as here. Any extra time to contribute to these changes?

@dpprdan
Copy link
Member Author

dpprdan commented Oct 1, 2021

Unfortunately I don't. I am not familiar with the webmockr/vcr codebase, so I'd need a lot of extra time too, I'm afraid.

@sckott
Copy link
Collaborator

sckott commented Oct 2, 2021

Okay, no problem. Will try to get to it soon unless someone else has time

@maelle
Copy link
Member

maelle commented Nov 9, 2021

@sckott just wondering if you had any time to look into this? No worries if not, of course! (I would not know how 😬)

@sckott
Copy link
Collaborator

sckott commented Nov 9, 2021

not much yet unfortunately. i started a while back on webmockr ropensci/webmockr@master...httr2 but nothing useable yet.

@sckott
Copy link
Collaborator

sckott commented Oct 28, 2023

Almost there, left to do:

  • The request body using a cassette is always character instaed of raw. Should this be fixed?
vcr::use_cassette("stuff", {
    res_vcr1 <- request("https://google.com") %>% req_perform()
})
vcr::use_cassette("stuff", {
    res_vcr2 <- request("https://google.com") %>% req_perform()
})

res_vcr1
#> <httr2_response>
#> GET https://google.com/
#> Status: 200 OK
#> Body: In memory (7070 bytes)

res_vcr2
#> <httr2_response>
#> GET https://google.com/
#> Status: 200 OK
#> Body: In memory (1 bytes)
  • A warning arises when using httr2 with vcr, via use of httr2::local_mock in wemockr::httr2_mock. I'm not sure if this is a problem or not
Setting global deferred event(s).
i These will be run:
  * Automatically, when the R session ends.
  * On demand, if you call `withr::deferred_run()`.
i Use `withr::deferred_clear()` to clear them without executing.

@maelle
Copy link
Member

maelle commented Oct 30, 2023

this is great! is there a public package where you're using httr2+vcr?

@sckott
Copy link
Collaborator

sckott commented Oct 30, 2023

is there a public package where you're using httr2+vcr?

No, not yet. I shjould make a silly small pkg just to demonstrate though.

@maelle do you have a sense about that warning above from withr? does that raise a red flag for you? I'm not sure how to suppress it, but i'm just not very familiar with withr

@maelle
Copy link
Member

maelle commented Oct 30, 2023

do you get that warning when running the tests or else?

I actually blogged about deferred_run() recently which made me realize I had ignored that message forever no matter where I saw it 😂 https://masalmon.eu/2023/10/16/test-local-mess-reset/

@sckott
Copy link
Collaborator

sckott commented Oct 30, 2023

When you run use_cassette() that warning is thrown. It comes from here which uses httr2::local_mock, which uses withr::local_options, which uses withr::defer. That's the "hook" mechanism hadley built into httr2. I'll have a look at your blog post

@sckott
Copy link
Collaborator

sckott commented Oct 30, 2023

wrt that webmockr code, I cringe about having to change stuff in the global env, but that's the only way it works with the hook mechanism hadley used, 🤷🏽 🤷🏽 🤷🏽

@sckott
Copy link
Collaborator

sckott commented Nov 3, 2023

@maelle
Copy link
Member

maelle commented Nov 10, 2023

Would looking at https://github.com/nealrichardson/httptest2/tree/main/R help? I'm not familiar with how it works under the hood.

@sckott
Copy link
Collaborator

sckott commented Jan 16, 2024

(There was an issue I thought was related to the httr2 integration here, but was a redirect issue that's not germaine to httr2, see #267)

@sckott
Copy link
Collaborator

sckott commented Jan 17, 2024

In testing this I don't see the withr warnings when using vcr in another package. So i'm thinking it's all good, but will keep an eye out

@sckott
Copy link
Collaborator

sckott commented Jan 17, 2024

The test package for testing the httr2/vcr integration is up on my account https://github.com/sckott/rrr

@sckott
Copy link
Collaborator

sckott commented Jan 17, 2024

wowsa - this thing about httr2 turning HTTP error codes into R errors is really painful - i'm surprised that decision was made - definitely a pkg made for end users and not developers

@sckott
Copy link
Collaborator

sckott commented Feb 5, 2024

A few things i'm wokring on now that will hopefully be the last things:

  • Not capturing response body and reponse headers for some reason on requests that have cassettes already, probably something easy, just hard to find where it is
  • How to handle httr2's error behavior - that http errors are turned into R errors automatically. Considerations:
    • right now I'm adding httr2::req_error(is_error = function(resp) FALSE) to the intercepted real request and then running req_perform.
    • If we don't change anything about httr2 behavior we'd then have to use last_response to get the response after a 400/500 series response, which would probably work, will try that
    • On requests with an existing cassette for requests that have a 400/500 series status code you don't get the same behavior of http error -> R error because the request/response is mocked, it's not actuallly real. Could deal with this by seeing if we can trigger that http error -> R error behavior manually. Or leave this behavior as is and let people know?
    • I'd prefer httr2 didn't have this behavior at all, but given it's there, it's more important to match the behavior of the pkg as close as possible

Pretty sure above 2 are taken care of now with current state on httr2 bfranch

@sckott sckott closed this as completed Jul 19, 2024
sckott added a commit that referenced this issue Jul 19, 2024
* started httr2 integration work #237

* export RequestHandlerHttr2

* bmp version

* import httr2

* remove --no-build-vignettes in R CMD INSTALL

* add depends R >= 3.5 for data

* add RequestHandlerHttr2 to pkgdown yaml

* add req_error to httr2 handler so that httr2 doesnt turn a http errror status code into an R error

* compatabiilty for httr2 here and there

* udpate docs to talk about all 3 http pkgs

* update readm.md

* woops, errant debug line

* tickle R check

* update man file for Cassette class

* httr2: get last response with last_response after req_perform

* httr2: fix status code retrieval

* httr2 response handling in Cassette class

* uncomment commented out tests that now work

* fix a bunch of httr2 tests

* update roxygen2 version

* REVDEPCHECK

* REVDEPCHECK - use gh pat
@sckott
Copy link
Collaborator

sckott commented Jul 25, 2024

@maelle @dpprdan if either of you get a chance, would love any bug reports/etc. on the vcr/httr2 integration

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

3 participants