Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Make subscription_details.metadata optional #443

Closed

Conversation

benbrandt
Copy link

@benbrandt benbrandt commented Sep 27, 2023

Summary

Looking at the OpenAPI spec, this field is nullable, and therefore should be an Option. All of our invoice webhook events are failing because the webhook events has

{ metadata: null }

On both the latest and last API version of stripe.

Checklist

It appears that metadata can be null from the stripe api
@benbrandt benbrandt force-pushed the subscription-details-metadata branch from a4a8c1d to 563e39c Compare September 27, 2023 10:40
@timsueberkrueb
Copy link
Contributor

@arlyon can we get this merged timely and can we have a release containing this fix on crates.io? Thanks!

@arlyon
Copy link
Owner

arlyon commented Sep 27, 2023

CI running now I'll check back in 30 mins

@benbrandt
Copy link
Author

@arlyon it seems this will fail because the generated code doesn't match. I assumed this might be the case, but I'm not sure where the code would be to make this generate an Option based on the nullable field

@arlyon
Copy link
Owner

arlyon commented Sep 27, 2023

I am going to sort this out in the codegen for you I have a free 30 mins now

@@ -1234,7 +1234,11 @@ pub fn gen_field_rust_type<T: Borrow<Schema>>(
}
if field_name == "metadata" {
state.use_params.insert("Metadata");
return "Metadata".into();
return if !required || is_nullable {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arlyon after doing a quick search, this seemed the likely culprit, but feel free to do something else if this isn't the way to fix it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a PR that also applies to EventData also, closing in favour of that one :)

@arlyon
Copy link
Owner

arlyon commented Sep 27, 2023

Closing for #444

@arlyon arlyon closed this Sep 27, 2023
@arlyon
Copy link
Owner

arlyon commented Sep 27, 2023

Released as 0.25.1

Technically a breaking change but this library has pretty lax opinions on breaking changes that come as a result of openapi changes, since a breaking change in stripe is a breaking change for code that relies on stripe.

Now to investigate why releases / changelogs are broken ... always something broken 😂

@timsueberkrueb
Copy link
Contributor

Thanks a lot for the timely fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants