bigrquery icon indicating copy to clipboard operation
bigrquery copied to clipboard

Port all the underlying HTTP code to use httr2

Open atheriel opened this issue 1 year ago • 2 comments

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.

atheriel avatar Dec 03 '24 01:12 atheriel

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.

jennybc avatar Dec 03 '24 03:12 jennybc

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. httr2's model for tokens is totally incompatible with httr but aspects of how httr handles tokens (i.e. that they are stateful) are implicit in much of the design of gargle. We can't swap them out with the existing API contract.

Because of this, I think we'll need to do something like the following:

  1. Port downstream packages to use httr2 instead of httr, but leave their use of gargle largely intact.
  2. Add new abstraction mechanisms in gargle that align better with how httr2 sees the world and deprecate existing ones that strongly reflect httr's semantics. I'm thinking something like a new gargle::req_auth_google(...) function that looks at home in packages that use httr2's request-building APIs. Probably for some period of time gargle is going to have both httr and httr2 as direct dependencies.
  3. Port downstream packages to use these new auth APIs.
  4. Remove the old APIs from gargle and drop the use of httr entirely.

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. bigrquery first and then pushes some of the code into gargle later is more likely to be successful. Right now gargle strongly assumes that credentials are global to the R process, and unwinding that assumption will be nontrivial, I think.

atheriel avatar Dec 03 '24 16:12 atheriel