All enums should have "Other" option.

[kevincox]

Describe the bug

Right now many (all?) enums such as PaymentMethodOptionsKlarnaSetupFutureUsage have a fixed set of options. In this case just None. However this is a loaded gun because Stripe can add new values to these enums.

This can result in errors like the following when trying to parse a webhook:

BadParse(Error("unknown variant off_session, expected none", line: 68, column: 3))

This means that every async-stripe user can start throwing errors at any time because Stripe adds a new value to an enum.

The only way I can see to handle this is add an Other option to every enum. Ideally this Other would also capture the raw value so that it can be logged, and in the worst case introspected. But the bare minimum is that parsing the whole object doesn't fail.

To Reproduce

1. Parse a webhook with a matching API version.

Expected behavior

It successfully parses.

OS

Linux

Rust version

N/A

Library version

0.41.0

API version

2024-06-20

Additional context

I have confirmed with their support engineers that this is expected within an API version:

Ah, I see what you mean. Sorry about the confusion. This is the expected behavior, actually. The enum values in some sense shouldn't be considered static (limited to what API reference lists). We do add new values over time to support new features that are back ported to older API versions so that Stripe users aren't forced to constantly upgrade to the newest version.

Also, in this specific instance, we can't return setup_future_usage as none even on the older API version because that'd be wrong and we also can't omit the field altogether. For example, our first-party SDKs handle this gracefully and expect the values to change dynamically. But yeah, in this case - the rust library would need to be updated to handle this better.

I hope this helps clarify.

[mzeitlin11]

Thanks @kevincox - currently the next branch partially does this on an arbitrary heuristic basis - if the enum has more than 8 variants (iirc, might not be the exact threshold), then an Other(String) variant is added under the basis that it is likely more fields will be added in the future and nobody is matching exhaustively (actual variant is called Unknown currently, but same idea). It definitely could be argued that this should be the case for any enum regardless of number of variants for exactly the reasons you point out, the downsides worth mentioning would be:

• Most enums go from 1 byte -> 24 bytes. Would need to ensure no serious compile regression - also some stripe objects have dozens of fields, so the generated structs are already extremely large (say ~5k bytes), a bunch of enums growing might regress that a good bit
• Most enums currently can be Copy, so a bit more convenient
• Those 2 downsides could be avoided by just having a bare Other variant instead of Other(String), though then you lose the nice debugging benefits
• We try to share types as much as possible between requests and responses - Other can be confusing because it makes sense as a Response variant, but is not a sensible request variant for users to see in docs when trying to figure out what to pass for an API request parameter.
• Less ergonomic pattern matching (not sure how common exhaustive matching is though and not too painful to add unknown arm)

[kevincox]

if the enum has more than 8 variants

It is worth highlighting that in the case we hit the enum only had 1 variant. So this heuristic is (as expected) unreliable and not enough to prevent failure.

It definitely could be argued that this should be the case for any enum

I'd go so far as to argue that any other option is unreasonably dangerous. To be direct if async-stripe concludes against doing this our company will move off. We've already had one outage, and have seen evidence that this is expected to happen again in the future. We can't accept the risk of our payments randomly breaking at any time.

Most enums go from 1 byte -> 24 bytes

This can be mitigated a few ways:

1. Box<str> saves 1 word (just ptr + len, no capacity). This is still very natural to use and is generally a good representation for immutable strings.
2. Box<String> is only 1 word. This isn't the best representation (still has unnecessary capacity word, requires two allocations, two pointer chases to get to the string data).
3. Make a custom string type that stores the length with the data, this is weirder but can be one word, one allocation and one pointer.

If I had to pick Box<String> is what I would choose. This case should be rare. The obvious problem is that once it starts happening it will happen 100% of the time. So there is some risk of a sudden surge in allocations and memory use. However even if one enum changes it is unlikely to be a significant fraction of total allocations and CPU usage so I don't think it is much of a problem.

just having a bare Other variant

I'm also fine with this. It would be nice, even just for debugging to have the data of what was missed. But it is far better to not fail to deserialize than to have an unmitigatable failure. But we log the whole JSON body before deserialization just for this reason, to make sure that we don't lose any information in decoding so that when we debug we can see the source. (Stripe also provides very good logs so having them here is not too critical)

Other can be confusing because it [...] is not a sensible request variant

I don't know if I agree with this. It can make sense if you want to start using a new API before async-stripe updates. I don't think it should be common, and it very obviously introduces risk of typos and other problems. But I think the cost of having it available is minimal and it does provide access to functionality.

Less ergonomic pattern matching ([...] exhaustive matching [...])

Stripe has made it clear that expecting to match all variants is a logic error. The enums should probably be marked as #[non_exhaustive] anyways. Although maybe it is fine to explicitly document that they may break in the future if Stripe adds more values and allowing exhaustive matching can be a way for users to ensure that they handle new variants (basically opt-in to compilation breakage on updates).

[arlyon]

We probably need Other(serde_json::Value) to be completely safe. There is no guarantee that an enum of strings as determined at compile time will continue to be that way. I will see if I can get a PR going. This library needs some love. I have released an alpha build of the next branch but I need to make time to get the auto codegen CI job running again and automatically cutting releases.

Give me a few days I will get a PR with both. Should help resolve in two ways, one we will remain more up-to-date with their spec and two if we do diverge (for oneOf or enum variants) it will not be a catastrophic error.