Implement miniserde-based deserialization

[mzeitlin11]

Built on #452, only the last commit is new. Very much a first pass, still some questions to answer. Initial results look promising and hopeful that profiling will reveal some other low-hanging fruit to further improve compile time.

For some quite unscientific timings, a clean release build of examples/endpoints goes from ~4m to 50s with min-ser enabled. More importantly though, the actual time to build just the binary goes from 75s to 7s, so incremental builds for code depending on async-stripe should be much faster.

Stripped binary size of the examples/endpoints binary also went from ~70MB to ~20MB. There's likely room for further binary size improvement since a fat LTO build shrunk this further to ~13 MB.

Feature Flags

The most important question to answer is how to expose the option of using miniserde. This PR takes the cautious approach of adding a min-ser feature which skips implementing serde::Deserialize and instead always implements miniserde::Deserialize where necessary for requests. The advantage with this approach is that min-ser could left as an "experimental" feature flag, making an initial release less breaking by requiring explicitly opting into min-ser. There are some annoyances with this approach, though

• Cargo features are additive, but min-ser is not additive - it removes functionality, so using it can be unintuitive (and might lead to similar feature incompatibility pains as the current runtime* features.
• Additional code complexity - see for example the added StripeDeserialize trait, added to abstract over the fact that a request might need either serde or miniserde to deserialize. Also, some types become feature flag dependent, e.g. a field might be miniserde::Value or serde_json::Value, which can't be shown easily in docs. (And we have to add a bunch of cfg blocks)

The other alternative would be to always use miniserde for library functionality (deserializing requests, webhooks, etc.). Then an additional additive serde-deserialize flag could be added to ask for serde::Deserialize to be derived on all types (useful for testing purposes). This would solve the complexity issues above at the cost of making miniserde impossible to opt out of.

Implementation Details

As seen by the diff, this adds a bunch of generated code :) . This new code is essentially using our codegen mechanism to mimic what the miniserde derive is already doing to allow deserialization cases that miniserde cannot support:

Expandable<T>:

• We manually derive Deserialize for T so that we can publicly expose the underlying Builder type so that Expandable<T> can use the same underlying implementation as T.

"Union of objects" types where the type is determined with the "object" field.

• This is implemented similarly to how serde(tag = "x") works - the data gets deserialized into an untyped JSON representation, which can then be converted to the correct variant using the "object" key. miniserde provides a convenient miniserde::Value for this purpose, but we then need to also generate impls for Value -> T for each T that we need to deserialize.

"Deleted or not" types where the type is determined by whether there is a deleted: true field.

• This is implemented pretty much identically to the "object" case above, just discriminating between variants using the "deleted" boolean instead.

[codecov]

Codecov Report

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

Project coverage is 7.38%. Comparing base (7686a0b) to head (7f9d426).

❗ Current head 7f9d426 differs from pull request most recent head f568feb. Consider uploading reports for the commit f568feb to get more accurate results

Files	Patch %	Lines
...d/stripe_checkout/src/checkout_session/requests.rs	0.00%	410 Missing ⚠️
generated/stripe_billing/src/invoice/requests.rs	0.00%	353 Missing ⚠️
...erated/stripe_billing/src/subscription/requests.rs	0.00%	283 Missing ⚠️
...ated/stripe_checkout/src/checkout_session/types.rs	63.09%	131 Missing ⚠️
...stripe_billing/src/billing_portal_session/types.rs	0.00%	90 Missing ⚠️
...heckout/src/checkout_acss_debit_mandate_options.rs	0.00%	90 Missing ⚠️
..._billing/src/billing_portal_configuration/types.rs	0.00%	88 Missing ⚠️
.../src/checkout_acss_debit_payment_method_options.rs	0.00%	87 Missing ⚠️
...out/src/checkout_session_payment_method_options.rs	50.32%	77 Missing ⚠️
...rc/payment_pages_checkout_session_custom_fields.rs	0.00%	77 Missing ⚠️
... and 102 more

Additional details and impacted files

@@           Coverage Diff            @@
##            next    #523      +/-   ##
========================================
+ Coverage   5.55%   7.38%   +1.82%     
========================================
  Files        932     934       +2     
  Lines      38837   97550   +58713     
========================================
+ Hits        2159    7205    +5046     
- Misses     36678   90345   +53667     

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

[arlyon]

OK. Flagging is a hard one.

I can see a world where people would want to store the stripe structs verbatim so having serde would be useful but I am also tempted to say lets just fire it off without serde for now and wait for someone to complain. I think as far as the lib is concerned we should only speak miniserde, and then, as you said, you can add a flag to get all that compile time hell back if you'd like it. It wouldn't be too hard to have the clients speak both miniserde and serde so I think we could end up at some point just letting the user pick one or both (or neither) but lets aim for miniserde for this change.

The motivation here is that I'd like to see if I can do some graph analysis and split these crates up also. If we can do individual clients and split the apis into a few crates that would be lovely since you could then opt-in to serde for a subset of the API which may be even more helpful still. We just need to be careful here so I will call that a stretch goal. The compile time improvements we have seen so far are more more more than enough. 13MB is 'large' but not obscene. It is very common for web servers to hit that just with tokio and a framework so I don't think it is an issue.

[arlyon]

Implementation details all look solid to me.

[arlyon]

BTW I sent you a maintainer invite. Master needs a PR but you can push / merge freely to next

[mzeitlin11]

BTW I sent you a maintainer invite. Master needs a PR but you can push / merge freely to next

Thanks, have accepted!

[mzeitlin11]

I can see a world where people would want to store the stripe structs verbatim so having serde would be useful but I am also tempted to say lets just fire it off without serde for now and wait for someone to complain. I think as far as the lib is concerned we should only speak miniserde, and then, as you said, you can add a flag to get all that compile time hell back if you'd like it.

I like this approach! I think in that case this would benefit from a bunch more testing to minimize the chance these miniserde changes cause regressions (which are particularly annoying because miniserde purposefully reports no errors. But if we generate requests properly, this should not be an issue!). It would be great to find a way to automate some deserialization tests (probably using the Stripe OpenAPI fixtures, but needs more investigation).

[mzeitlin11]

The motivation here is that I'd like to see if I can do some graph analysis and split these crates up also. If we can do individual clients and split the apis into a few crates that would be lovely since you could then opt-in to serde for a subset of the API which may be even more helpful still.

That would be wonderful - there's some basic graph analysis implemented to help infer crates, but could certainly be fleshed out. The main annoyance I saw was that there's one huge connected component (which right now gets shoved into stripe_shared to avoid circular dependencies, but is the limiting factor on deserialization-related compile time)

[mzeitlin11]

Rebased and implemented the alternative mentioned above about speaking miniserde and a feature flag for enabling full serde de/serialization

[mzeitlin11]

Added some basic testing using the OpenAPI provided fixtures in the last commit. There are still some tweaks I'd like to make (for example, see the ugly addition of single variant enums to ensure the "object" key is properly serialized, only necessary so we better match stripe when serializing for testing purposes), but that don't need to be part of this pr.

[mzeitlin11]

After using this, thought it might be better to split the serde feature into serialize and deserialize (done in latest commit). Think it makes sense because these features are mostly useful for testing contexts - deserialize is the big compile time offender, and avoidable by using miniserde in tests instead. So the flexibility of just enabling the lighter weight serialize on its own seems useful.

[arlyon]

Happy with this now!