Client split

[mzeitlin11]

Built on #452, so most of the diff is that. cc @arlyon, I was messing around with implementing the code generation side of your comment #452 (comment) and just wanted to push this up for early conversation and to make sure no work was being duplicated.

Closes #298, closes #380, closes #463, closes #520, most of #535, allows #274 more easily

Details

The main traits are defined here and primarily match #452 (comment), with one exception being defining our own library-agnostic simple RequestBuilder instead of using http-types. This allows removing that dependency for the hyper-based client and removing the need for annoying conversions.

One other cool benefit here is the ability to use .customize on any RequestBuilder, to receive a CustomizableRequestBuilder capable of setting per-request configuration (for example, to solve (#520, #380)).

Beyond this file, most of the work is modifying the code generation to produce this RequestBuilder. Part of making the request structs implement StripeRequest required converting to more of a builder pattern (see the example changes below).

Examples

Previous

let mut payer = PayInvoice::new();
payer.forgive = Some(true);
payer.off_session = Some(true);
let result = payer.send(&client, &id).await.unwrap();

New

let result = PayInvoice::new(&id)
        .forgive(true)
        .off_session(true)
        .send(&client)
        .await
        .unwrap();

Customization Example

let session = RetrieveCheckoutSession::new(&id)
      .customize()
      .request_strategy(...)
      .account_id(...)
      .send(&client)
      .await
      .unwrap();

[arlyon]

This is cool! I will see if I can get some time over the weekend to try the client-oriented approach which may reduce the total amount of code we need to produce. I like the builder API but I'd also like to see what the least amount of code looks like and then work from there. I am sure deriving default probably nukes any gains here but worth a shot.

client.customize().strategy().send(PayInvoice {
  ..Default::default()
});

The StripeRequest trait is v cool.

[mzeitlin11]

I am sure deriving default probably nukes any gains here but worth a shot.

This was on the list of TODO's to add back where possible. Though since the id parameters are now part of the request builder, many fewer request parameters can implement Default since most require at least one id param.

I'm also hopeful that while generating more code is annoying, it won't be much of a maintenance burden since builder methods are straightforward (and does not seem to affect compile time).

[mzeitlin11]

Think this is at least ready for review. There's plenty to bikeshed on naming, other ergonomics changes, etc., but that can happen either in this pr or in future release candidates / PR's on the next branch.

The main TODO left here is improving testing - the hyper and surf clients have their own tests, but the top-level integration tests using stripe-mock should be run with all clients / all hyper client feature combos.

[arlyon]

OK I am going to take some time this weekend to give this some attention. Are the other PRs (#537, #536) ok to go also?

[mzeitlin11]

OK I am going to take some time this weekend to give this some attention. Are the other PRs (#537, #536) ok to go also?

Yep, those are good to go from my end!

[arlyon]

Ok I think we are very close. A few comments:

• I think we should use the 'async-stripe' prefix for everything. It's wordy but I don't think stripe want us to use 'stripe' directly. It was suggested to me in the past to rename this crate to something else altogether such as payments but I am worried about the SEO.
• structure: lets put the clients in their own folder. This leaves a good question of how people actually use this project. Perhaps the async-stripe crate should be just a re-export of async_stripe_tokio_hyper or perhaps we should let people pick which re-export they want using features. This is purely for discoverability. I think async-stripe in the root, two clients in the clients folders (perhaps 3 for blocking?), and some re-export mechanism qualifies as 'done' here.
• examples: missing out on the code docs is unfortunate HOWEVER I don't think we even have a choice any longer. Any meaningful examples need a client and that would create a loop against stripe_shared. What we have now is the best option given this.
• I would like to add simple tests that the example binaries all work but that can be independent of this PR
• Is it possible to avoid clients having to define a 'config' builder? Perhaps we can have one that we give to the client to build it. Maybe add a fn to the Client trait that takes a Config and returns Self? stripe_async_std::config::ClientBuilder feels pretty boilerplatey. Not a huge issue since it is a relatively small amount of code

[arlyon]

Cool! I updated to v1001 and added the new APIs to the mapping. We could maybe do some smart stuff around following dependencies but I will also leave that for another PR. Example:

2024-04-30T11:59:26.633850Z ERROR get_components:infer_all_crate_assignments:ensure_no_missing_crates: stripe_openapi_codegen::crate_inference: Could not infer crate for component confirmation_token. Depended on by components [], crates {}
2024-04-30T11:59:26.633923Z ERROR get_components:infer_all_crate_assignments:ensure_no_missing_crates: stripe_openapi_codegen::crate_inference: Could not infer crate for component confirmation_tokens_resource_mandate_data. Depended on by components [confirmation_token], crates {None}
2024-04-30T11:59:26.633943Z ERROR get_components:infer_all_crate_assignments:ensure_no_missing_crates: stripe_openapi_codegen::crate_inference: Could not infer crate for component confirmation_tokens_resource_mandate_data_resource_customer_acceptance. Depended on by components [confirmation_tokens_resource_mandate_data], crates {None}
2024-04-30T11:59:26.633957Z ERROR get_components:infer_all_crate_assignments:ensure_no_missing_crates: stripe_openapi_codegen::crate_inference: Could not infer crate for component confirmation_tokens_resource_mandate_data_resource_customer_acceptance_resource_online. Depended on by components [confirmation_tokens_resource_mandate_data_resource_customer_acceptance], crates {None}
2024-04-30T11:59:26.633972Z ERROR get_components:infer_all_crate_assignments:ensure_no_missing_crates: stripe_openapi_codegen::crate_inference: Could not infer crate for component confirmation_tokens_resource_payment_method_preview. Depended on by components [confirmation_token], crates {None}
2024-04-30T11:59:26.633987Z ERROR get_components:infer_all_crate_assignments:ensure_no_missing_crates: stripe_openapi_codegen::crate_inference: Could not infer crate for component payment_flows_private_payment_methods_alipay. Depended on by components [confirmation_tokens_resource_payment_method_preview, payment_method], crates {None, Some(CrateInfo { krate: Crate("stripe_payment"), type_defs_are_shared: false })}

In this chain, since payment_flows_private_payment_methods_alipay has a known crate, then surely we can unambiguously follow that dep chain and add all of these to stripe_payment? I will add a task for it. Otherwise, once the above comments are handled I am happy to approve this!

[mzeitlin11]

• I think we should use the 'async-stripe' prefix for everything.

Sounds good, will do. Separate from the crate names, are we allowed to do the lib renaming (e.g. how on current master async-stripe becomes stripe).

• structure: lets put the clients in their own folder. This leaves a good question of how people actually use this project. Perhaps the async-stripe crate should be just a re-export of async_stripe_tokio_hyper or perhaps we should let people pick which re-export they want using features. This is purely for discoverability. I think async-stripe in the root, two clients in the clients folders (perhaps 3 for blocking?), and some re-export mechanism qualifies as 'done' here.

My main thinking here was just that the hyper version would be significantly more popular and didn't want to reexport something like async_stripe_tokio_hyper as a separate crate since then another crate to build/publish/manage. The hyper one is also cool since it gives blocking pretty much for free.

I'll look into allowing a root async-stripe to pick client reexports using features, though I worry a bit about ending back the incompatible feature combo issues this was designed to solve.

• I would like to add simple tests that the example binaries all work but that can be independent of this PR

Right now in the CI there is the step examples, which runs cargo clippy --workspace to check the binaries. Was this what you were imagining, or did you have a different idea?

• Is it possible to avoid clients having to define a 'config' builder? Perhaps we can have one that we give to the client to build it. Maybe add a fn to the Client trait that takes a Config and returns Self? stripe_async_std::config::ClientBuilder feels pretty boilerplatey. Not a huge issue since it is a relatively small amount of code

Config is definitely boilerplatey. My main thoughts around the current iteration of config handling were to keep it more of a simple proof of concept, since I expect it will change and be improved/extended over time. My main thinking in keeping configs separate and not tied to the trait were solely around making the clients as minimally coupled as possible (and ensuring the Client trait does not need to depend on deps like hyper/async-std.

For example, we might want config pieces that are unique to the specific client, e.g.

• timeout for the blocking client
• hyper-specific options for the hyper client
• Or even a way to pass a prebuilt hyper client.

[arlyon]

lib rename

I think this is ok, but only becuase I've not been advised anything else. I think they just don't want the cargo crate name clashing.

structure

I agree that the feature flagging is annoying but if it's simply an optional dependency and a crate re-export we should be ok. I think really this should be a backwards compat / convenience thing mainly. The main thing is that the rest of the library (all the types) are no longer tied to the client type so this feature flag as far less far-reaching implications.

example tests

Compiling is good, running them is better. Not sure what the best option for this is though and I am happy to defer that.

config

Ok, lets leave it.

[mzeitlin11]

lib rename

Done in 4efa17a

feature flagging reexport

Done in c9369a2 (for now also just consolidated the previous async-stripe-async-std-surf crate into the root async-stripe since it's pretty small and hopefully fewer crates makes publishing easier).

Feature flag names + interactions could definitely be debated + improved, but think this is a good starting point since lots of future changes will affect this anyway (e.g. flags for supporting both hyper <1, >= 1, better reexporting of the generated types / requests / etc.)

[seanpianka]

Nice work! Is there a checklist anywhere with what's left (ie remaining work)?

[mzeitlin11]

Nice work! Is there a checklist anywhere with what's left (ie remaining work)?

On my end, main items I expect are

1. Better ergonomics (right now it's annoying to have to depend on generated crates for different requests, would be easier for user and less breaking-change prone to have reexports from a single crate). But this might be tricky with name clashes
2. Continued work on trying to improve compile time
3. Better / more flexible config support + easier usage of custom clients.

FWIW, I've been using the next branch as is to get these improvements earlier :). Though with the caveat that the next branch has significant changes that have not been widely used in the real world, so could cause breakages (so would be careful on usage in production code which is not extensively tested). That being said, the new code includes a bunch more deserialization-related tests, so hopefully any issues that come up can be quickly resolved.

[arlyon]

Hey, @mzeitlin11 I am ready to merge this into next if you are. Just ping me here.

[mzeitlin11]

Hey, @mzeitlin11 I am ready to merge this into next if you are. Just ping me here.

@arlyon, ready to merge!