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.