Port all the underlying HTTP code to use httr2 #626
Conversation
This commit refactors `bigrquery`'s internals to use `httr2` instead of `httr`. I don't have a strong motivation for this change, other than the general belief that we should be trying to move packages to `httr2` over time, and because it made `bigrquery`'s internals a bit more modular. One of the more visible consequences is that the OAuth tokens returned by `bq_token()` can no longer take the form of an `httr` request object; now they are simple bearer tokens instead. This should make it much easier to implement viewer-based credentials in the future. However, it might break downstream packages that rely on the existing `bq_token()` return value. I believe that most of these changes should be covered by the existing test suite. Signed-off-by: Aaron Jacobs <[email protected]>
|
I haven't delved into this, but am just passing by to say that I'd be interested in considering this move in the context of migrating gargle to httr2, which is also desirable and in the vague near-term future. I realize that bigrquery only uses gargle for auth, but rolls its own request construction and response processing and that involves direct use of httr. gmailr is in a similar boat. googlesheets4 and googledrive use gargle for basically everything; they do depend on httr, but there's very little direct use. All of the tricky bits are mediated through gargle. I think it would be good to form a coordinated plan of attack. |
After going through one of these packages for this PR, my current thinking on this is that it will have to be done in multiple stages. Because of this, I think we'll need to do something like the following:
The other aspect of this is that I'd really like to get support for Posit Connect's viewer-based credentials into these packages, but I think a strategy that brings it to e.g. |
| req <- req_error(req, is_error = function(resp) { | ||
| resp_status(resp) != 404 && resp_status(resp) >= 400 | ||
| }) | ||
| resp <- req_perform(req) |
There was a problem hiding this comment.
In this case, I'd be tempted to drop the req_error() and instead use tryCatch(). Something like this:
tryCatch(
{
req_perform(req)
TRUE
},
httr2_http_404 = function(cnd) FALSE
})| ) | ||
| invisible(process_request(req)) | ||
| #' @importFrom httr2 req_body_json req_perform resp_body_json | ||
| bq_post <- function(url, body, query = NULL, token = bq_token()) { |
There was a problem hiding this comment.
Feels like we could probably weed out these specialised methods, since they're not so important with httr2's design. Probably not worth doing in this PR, but might be worth filing an issue so we remember to do it in the future?
| content | ||
| } else { | ||
| jsonlite::fromJSON(rawToChar(content), simplifyVector = FALSE) | ||
| #' @importFrom httr2 request req_user_agent req_url_path_append req_method req_auth_bearer_token req_url_query req_error req_perform |
There was a problem hiding this comment.
I don't have any clear principle here, but it feels like you should either import all of httr2 or none of it.
This commit refactors
bigrquery's internals to usehttr2instead ofhttr. I don't have a strong motivation for this change, other than the general belief that we should be trying to move packages tohttr2over time, and because it madebigrquery's internals a bit more modular.One of the more visible consequences is that the OAuth tokens returned by
bq_token()can no longer take the form of anhttrrequest object; now they are simple bearer tokens instead. This should make it much easier to implement viewer-based credentials in the future.However, it might break downstream packages that rely on the existing
bq_token()return value.I believe that most of these changes should be covered by the existing test suite.