Closed mzeitlin11 closed 5 months ago
Attention: Patch coverage is 6.16875%
with 3681 lines
in your changes are missing coverage. Please review.
:exclamation: No coverage uploaded for pull request base (
next@ee601fd
). Click here to learn what that means.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Damn! Exciting. I will probably tackle this in a few rounds, starting now!
First lot of comments. Regarding release, Regarding release, I am going to create a new branch off master called next
. Please target that with this PR and once it is merged I will adjust the semantic release config to publish these as alpha releases.
Last 2 commits update the code generation docs, contributing guide, README, and migration guide. Still very WIP and possibly typo-filled but should make the changes more obvious.
Last 2 commits make some changes worth nothing:
Integration of search request pagination (https://github.com/arlyon/async-stripe/pull/414), extended to the general search request case. But there's still one big change from that PR - AFAIK that PR still uses starting_after
as the cursor for search pagination, this version reads the next_page
from the response and sets it into the page
parameter of the next request. Also added testing for more pagination scenarios since that logic gets tricky!
Fixing the UrlFinder
errors (seems like Stripe API docs just starting returning a different format :( ). I couldn't find a simple replacement of what we had before, so I thought a better approach would just be to stop fetching this data on-demand and store it. This has some advantages:
cc @arlyon also happy to fix the merge conflicts if it helps review, but holding off since each change to the generated code on master will gives a fresh merge conflict with the generated files.
Also 2 more breaking changes I'm curious if any thoughts on:
serde(default)
when deserializing List<T>
. I'd prefer Option<List<T>>
for a couple reasons (edit: diff for generated code can be seen in commit: https://github.com/arlyon/async-stripe/pull/452/commits/d8a2dec58f753d9465ee109024dd271daae30ac1)
Vec
Default
for List
does not make much sense (e.g. will have an empty url
parameter), so footgun for incorrect usage.serde
attributes complicate miniserde
integration since it does not support thatAsRef<str>
impl for enums. Seems not in the spirit of AsRef
and as_str
is clearer for that use case. Also just like generating less code.I am on board with those changes also. I have comments that have been piling up but I am waiting to flush them until I have covered more of the code. Stay tuned :)
I am on board with those changes also. I have comments that have been piling up but I am waiting to flush them until I have covered more of the code. Stay tuned :)
Great! No rush - I've been working on a (now mostly functional, but very ugly) miniserde
implementation. That adds a bunch of complexity because of manual derives though, so would plan that as a separate PR once this is done (but without promising too much, looks like exciting compile time improvements!).
One other question I'd like to improve in this PR is any thoughts on overall crate structure. I think instead of having a bunch of separate request crates, it would be nicer to reexport under a single crate so that features (like runtime, or serialization preference) only need to be specified in one place.
But that brings up some questions around keeping the crate namespace / feature set clean with so much exposed in that central crate. And whether or not we'd like a single stripe
crate containing (client + core types/traits (now stripe_types
) + requests (now generated/*
crates) or if those pieces should/can be split up at all. Will think more about this, but ideas very welcome
No immediate news, however I was experimenting with how to split the clients out:
#![feature(impl_trait_in_assoc_type)]
mod headers;
mod request_strategy;
use std::future::{self, Future, Ready};
use futures::FutureExt;
use headers::Headers;
use http_types::{convert::DeserializeOwned, headers::USER_AGENT, Body, Method, Request, Url};
use request_strategy::RequestStrategy;
use stripe_shared::version::VERSION;
use crate::headers::AppInfo;
pub trait Client {
type Response: ClientResponse;
type Fut: Future<Output = Self::Response>;
fn execute(&self, method: http_types::Request) -> Self::Fut;
}
pub trait ClientSync: Client<Fut = Ready<<Self as Client>::Response>> {
fn execute_sync(&self, method: http_types::Request) -> <Self as Client>::Response;
}
impl<P> ClientSync for P
where
P: Client<Fut = Ready<<Self as Client>::Response>>,
{
fn execute_sync(&self, method: http_types::Request) -> <Self as Client>::Response {
self.execute(method).now_or_never().expect("this is always ready")
}
}
trait ClientResponse {
type Ok;
type Err;
fn as_result(self) -> Result<Self::Ok, Self::Err>;
}
impl<T, E> ClientResponse for Result<T, E> {
type Ok = T;
type Err = E;
fn as_result(self) -> Result<<Self as ClientResponse>::Ok, <Self as ClientResponse>::Err> {
self
}
}
struct AsyncClient;
impl AsyncClient {
/// Create a new account with the given secret key.
pub fn new(secret_key: impl Into<String>) -> StripeClient<Self> {
Self::from_url("https://api.stripe.com/", secret_key)
}
/// Create a new account pointed at a specific URL. This is useful for testing.
pub fn from_url<'a>(
url: impl Into<&'a str>,
secret_key: impl Into<String>,
) -> StripeClient<Self> {
StripeClient::from_url(Self, url, secret_key)
}
}
impl Client for AsyncClient {
type Response = Result<http_types::Response, ()>;
type Fut = impl Future<Output = Self::Response>;
fn execute(&self, method: http_types::Request) -> Self::Fut {
async { Err(()) }
}
}
struct BlockingClient;
impl Client for BlockingClient {
type Response = Result<http_types::Response, ()>;
type Fut = Ready<Self::Response>;
fn execute(&self, request: http_types::Request) -> Self::Fut {
future::ready(Err(()))
}
}
impl BlockingClient {
/// Create a new account with the given secret key.
pub fn new(secret_key: impl Into<String>) -> StripeClient<Self> {
Self::from_url("https://api.stripe.com/", secret_key)
}
/// Create a new account pointed at a specific URL. This is useful for testing.
pub fn from_url<'a>(
url: impl Into<&'a str>,
secret_key: impl Into<String>,
) -> StripeClient<Self> {
StripeClient::from_url(Self, url, secret_key)
}
}
struct StripeClient<T: Client> {
client: T,
secret_key: String,
headers: Headers,
strategy: RequestStrategy,
app_info: Option<AppInfo>,
api_base: Url,
api_root: String,
}
impl<C: Client> StripeClient<C> {
pub fn from_url<'a>(client: C, url: impl Into<&'a str>, secret_key: impl Into<String>) -> Self {
StripeClient {
client,
secret_key: secret_key.into(),
headers: Headers {
stripe_version: VERSION,
user_agent: USER_AGENT.to_string(),
client_id: None,
stripe_account: None,
},
strategy: RequestStrategy::Once,
app_info: None,
api_base: Url::parse(url.into()).expect("invalid url"),
api_root: "v1".to_string(),
}
}
}
impl<C: Client> StripeClient<C>
where
C::Response: ClientResponse<Ok = http_types::Response>,
{
pub async fn get(&self, path: &str) -> () {
let resp = self.client.execute(self.get_req(path)).await.as_result();
}
fn get_req(&self, path: &str) -> Request {
let url = self.api_base.join(&format!("{}/{}", self.api_root, path)).expect("invalid url");
let mut req = Request::new(Method::Get, url);
req.set_body(Body::empty());
req
}
}
impl<C: ClientSync> StripeClient<C>
where
C::Response: ClientResponse<Ok = http_types::Response>,
{
pub fn get_sync(&self, path: &str) -> () {
let resp = self.client.execute_sync(self.get_req(path)).as_result();
}
}
#[cfg(test)]
mod test {
use crate::{AsyncClient, BlockingClient, StripeClient};
#[test]
fn sync_example() {
let client = BlockingClient::new("sk_123");
let data = client.get_sync("/test");
}
#[tokio::test]
async fn async_example() {
let client = AsyncClient::new("sk_123");
let data = client.get("/test").await;
}
}
Types that only implement the sync client can access the async version, but it returns a future so you won't run into footguns. Clients that implement the async interface can only access the async api. All the common code goes in the 'stripe client whose base client is async' such that the sync clients can access it. No macro rubbish or compile flags.
BTW I am very close to saying 'lets merge this into next
and start figuring out miniserde'. No major objections so far.
Although I will say with the above approach, we will need to swap the execution style from Thing::get(client)
to client.call_sync(Thing::get())
unless we want to stamp out sync and async versions of all our structs. There may we some way to make this generic and maybe return a future and maybe not depending on the underlying client but I will need to dig a little more.
Yeah, the more I think about this, the more I think that a sans-io approach makes sense. https://sans-io.readthedocs.io/how-to-sans-io.html Invert the control such that the client is given a request and the types know nothing at all about the client you are using. That way we can provide some clients out of the box but writing your own is trivial (and will reduce another source of recompilation)
I am thinking we merge this as-is in to a next
branch and we can split the workload, I will work on a PR to split out into sans-io-style client packages (and sort out a sync-agnostic solution to a pagination adapter in the process) and you can push ahead with miniserde?
I have fast-forwarded the next branch to current master. I am going to rebase this branch, merge it into there, and open a new PR to merge next
into main which will be the final release branch. In the mean time, we can work on PRs to next
for handling the items identified above:
What else is there to fix on next
outside of "etc"? It also sounds like getting this merged without miniserde would still yield compile time improvements?
This Pr as is will probably break things, as will miniserde, as will restructuring the client. I'd like to do all three at once.
I have fast-forwarded the next branch to current master. I am going to rebase this branch, merge it into there, and open a new PR to merge
next
into main which will be the final release branch. In the mean time, we can work on PRs tonext
for handling the items identified above:
- switching to sans-io style approach and breaking the dep on clients
- miniserde
- etc
Sorry for the long delay in responding here - should have time to work on this again in a week or so! Please let me know if there's anything I can help with for the branching process / getting this merged into next.
cc @arlyon, rebased and CI is green after a few tweaks!
Exciting. I am merging this now.
There is a huge crowd of people excitedly watching from the sidelines 🎉
cc @arlyon, sorry for opening a new PR - finally got back to #314 and rebasing onto current master was quite painful. Also tried to leave a cleaner commit history.
I think this is ready for review now. I'll be working on some more changes along the way, primarily documentation for all these changes. One piece I'm not sure how to handle is updating the release workflow to work for the new workspace structure / what the plan is for something like an early RC branch. I would anticipate making more changes, but having an initial RC release would be great at some point so it's more easily dogfooded with real stripe API usage.
TODOS
closes #448, closes #446, closes #442, closes #437, closes #419, closes #417, closes #396, closes #394, closes #389, closes #384, closes #381, closes #352, closes #287, closes #257, closes #246, closes #218, closes #196, closes #154, closes #137
closes #450, closes #411, closes #409, closes #314, closes #283, closes #197, closes #163, closes #456, closes #462, closes #455, closes #461
Future Work