Closed mzeitlin11 closed 5 months ago
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
).:exclamation: Current head 7f9d426 differs from pull request most recent head f568feb. Consider uploading reports for the commit f568feb to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
Implementation details all look solid to me.
BTW I sent you a maintainer invite. Master needs a PR but you can push / merge freely to next
BTW I sent you a maintainer invite. Master needs a PR but you can push / merge freely to next
Thanks, have accepted!
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 withoutserde
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).
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)
Rebased and implemented the alternative mentioned above about speaking miniserde
and a feature flag for enabling full serde
de/serialization
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.
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.
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 withmin-ser
enabled. More importantly though, the actual time to build just the binary goes from 75s to 7s, so incremental builds for code depending onasync-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 amin-ser
feature which skips implementingserde::Deserialize
and instead always implementsminiserde::Deserialize
where necessary for requests. The advantage with this approach is thatmin-ser
could left as an "experimental" feature flag, making an initial release less breaking by requiring explicitly opting intomin-ser
. There are some annoyances with this approach, thoughmin-ser
is not additive - it removes functionality, so using it can be unintuitive (and might lead to similar feature incompatibility pains as the currentruntime*
features.StripeDeserialize
trait, added to abstract over the fact that a request might need eitherserde
orminiserde
to deserialize. Also, some types become feature flag dependent, e.g. a field might beminiserde::Value
orserde_json::Value
, which can't be shown easily in docs. (And we have to add a bunch ofcfg
blocks)The other alternative would be to always use
miniserde
for library functionality (deserializing requests, webhooks, etc.). Then an additional additiveserde-deserialize
flag could be added to ask forserde::Deserialize
to be derived on all types (useful for testing purposes). This would solve the complexity issues above at the cost of makingminiserde
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 thatminiserde
cannot support:Expandable<T>
:Deserialize
forT
so that we can publicly expose the underlyingBuilder
type so thatExpandable<T>
can use the same underlying implementation asT
."Union of objects" types where the type is determined with the
"object"
field.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 convenientminiserde::Value
for this purpose, but we then need to also generate impls forValue -> T
for eachT
that we need to deserialize."Deleted or not" types where the type is determined by whether there is a
deleted: true
field."object"
case above, just discriminating between variants using the"deleted"
boolean instead.