Codegen revamp

[mzeitlin11]

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

• Figure out release handling

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

• Better ergonomics around naming (user feedback will be helpful here)

[codecov]

Codecov Report

Attention: Patch coverage is 6.16875% with 3681 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (next@ee601fd). Click here to learn what that means.

Files	Patch %	Lines
...es_checkout_session_shipping_address_collection.rs	0.00%	504 Missing ⚠️
...d/stripe_billing/src/subscription_item/requests.rs	0.00%	396 Missing ⚠️
...ated/stripe_checkout/src/checkout_session/types.rs	11.27%	362 Missing ⚠️
...ckout/src/payment_pages_checkout_session_tax_id.rs	0.00%	163 Missing ⚠️
...stripe_billing/src/billing_portal_session/types.rs	0.00%	125 Missing ⚠️
generated/stripe_billing/src/plan/requests.rs	8.73%	115 Missing ⚠️
.../src/checkout_acss_debit_payment_method_options.rs	0.00%	107 Missing ⚠️
...heckout/src/checkout_acss_debit_mandate_options.rs	0.00%	103 Missing ⚠️
...er_balance_bank_transfer_payment_method_options.rs	0.00%	81 Missing ⚠️
...d/stripe_billing/src/portal_subscription_update.rs	0.00%	71 Missing ⚠️
... and 96 more

Additional details and impacted files

@@          Coverage Diff           @@
##             next    #452   +/-   ##
======================================
  Coverage        ?   5.71%           
======================================
  Files           ?     932           
  Lines           ?   38941           
  Branches        ?       0           
======================================
  Hits            ?    2227           
  Misses          ?   36714           
  Partials        ?       0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[arlyon]

Damn! Exciting. I will probably tackle this in a few rounds, starting now!

[arlyon]

Flushing a few comments. Went for the low handing fruit to get it out of the way. Will come back to the codegen code soon.

[arlyon]

Not against structuring like this but what is the motivation here over individual items? Reducing boilerplate?

[mzeitlin11]

Yeah don't love it but was for boilerplate / feature annoyance. e.g. the Cargo.toml for this example has

[features]
async = []

runtime-tokio-hyper = ["async-stripe/runtime-tokio-hyper", "async"]
runtime-tokio-hyper-rustls = ["async-stripe/runtime-tokio-hyper-rustls", "async"]
runtime-tokio-hyper-rustls-webpki = ["async-stripe/runtime-tokio-hyper-rustls-webpki", "async"]
runtime-async-std-surf = ["async-stripe/runtime-async-std-surf", "async"]

There are probably cleverer solutions (or could just have the example define a specific runtime), but hoping lots of this can be cleaned up in future when changing client structure allows cleaning up the features.

[arlyon]

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.

[mzeitlin11]

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.

[mzeitlin11]

Last 2 commits make some changes worth nothing:

1. Integration of search request pagination (Added Search API functionality #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!

2. 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:

   • No need for network call on usual development loop, so faster local dev + one fewer flaky CI error possibility
   • The doc URL list is small enough that keeping manually updated should be reasonable (since new objects are not added that frequently), and no more worries about Stripe changing the format
   • I left the script which generated the file in case useful - I basically queried a Wayback Machine URL and wrote the relevant data we need to a file.

[mzeitlin11]

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.

[mzeitlin11]

Also 2 more breaking changes I'm curious if any thoughts on:

1. Right now we use 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: d8a2dec)
   • More consistent with how we handle other types like 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 that
2. Would also like to remove AsRef<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.

[arlyon]

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 :)

[mzeitlin11]

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

[arlyon]

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.

[arlyon]

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.

[arlyon]

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)

[arlyon]

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?

[arlyon]

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:

• switching to sans-io style approach and breaking the dep on clients
• miniserde
• etc

[seanpianka]

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?

[arlyon]

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.

[mzeitlin11]

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:

• 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.

[mzeitlin11]

cc @arlyon, rebased and CI is green after a few tweaks!

[arlyon]

Exciting. I am merging this now.

[seanpianka]

There is a huge crowd of people excitedly watching from the sidelines 🎉