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

Add public models for errors #678

Merged
merged 1 commit into from Feb 16, 2019
Merged

Add public models for errors #678

merged 1 commit into from Feb 16, 2019

Conversation

ob-stripe
Copy link
Contributor

@ob-stripe ob-stripe commented Feb 14, 2019

r? @mickjermsurawong-stripe @remi-stripe
cc @scherr-stripe

This PR adds two new publicly exposed models:

  • com.stripe.model.StripeError for regular API errors
  • com.stripe.model.oauth.OAuthError for OAuth errors

StripeException now has a getError() accessors that returns the StripeError instance, and similarly OAuthException has a getOauthError() that returns the OAuthError instance (overriding getError() isn't possible because StripeError and OAuthError aren't covariant).

Users can now also access the StripeResponse via getLastResponse() on the StripeError / OAuthError instance.

(I've also removed the manually defined getters in exception classes in favor of @Getter.)

@remi-stripe
Copy link
Contributor

decide if we want to get rid of PaymentIntentLastPaymentError and use StripeError instead (this would be a breaking change)

I don't think it's worth doing this now. We should do it in 8.X which @mickjermsurawong-stripe has already planned per the code here

@mickjermsurawong-stripe
Copy link
Contributor

mickjermsurawong-stripe commented Feb 14, 2019

ah thank you @ob-stripe for taking the DX-3354!

The 8.0 will have this new structure that has StripeError as you described here, but it's not attached to our StripeException yet. It will have the following structure, with payment intent using the exact error content of our API error wrapper previously StripeErrorContainer. We will have:

// replacing StripeErrorContainer
public class StripeErrorResponse extends StripeObject {
  @SerializedName("error")
  StripeError error;
}
// actual error content
public class StripeError {
  String docUrl;
  ...
}

public class PaymentIntent extends ApiResource implements HasId, MetadataStore<PaymentIntent> {
  /** The payment error encountered in the previous PaymentIntent confirmation. */
  @SerializedName("last_payment_error")
  StripeError lastPaymentError;
}

And stripe error now has doc_url and other fields according to OpenAPI error

This is the impact of the proposed change

Please let me know if you have feedback on the new structure!
If this is urgent fix maybe it would be great if we can decide on a common structure that is compatible with 8.0 too. Otherwise I agree with @remi-stripe

Copy link
Contributor

@remi-stripe remi-stripe left a comment

Choose a reason for hiding this comment

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

I don't fully understand this change and why it's not a breaking change since you seemed to move things around like declineCode and param disappearing.

Will defer to @mickjermsurawong-stripe who will likely better understand this.

src/main/java/com/stripe/exception/StripeException.java Outdated Show resolved Hide resolved
@ob-stripe ob-stripe changed the title [WIP] Add public models for errors Add public models for errors Feb 14, 2019
@ob-stripe
Copy link
Contributor Author

you seemed to move things around like declineCode and param disappearing.

I just opportunistically removed the manually defined getters in favor of Lombok's @Getter annotation. The getters still exist, they're just automatically defined by Lombok now.

@ob-stripe
Copy link
Contributor Author

@mickjermsurawong-stripe I think this should largely be compatible with the 8.0 branch. Unless I missed something, the only notable difference is that I'm not exposing a StripeErrorResponse class for the top-level error key, and instead directly deserialize the nested hash into StripeError.

@mickjermsurawong-stripe
Copy link
Contributor

ack @ob-stripe will give a thorough review again!

@mickjermsurawong-stripe
Copy link
Contributor

mickjermsurawong-stripe commented Feb 14, 2019

I'm wondering if we should rename error to underlyingError or originalError in StripeException? My small worry is that user might get confused between the error values that we specifically pick up to different kinds of exception (eg. CardException, RateLimitException).
A more explicit naming could also help to indicate that it is for more "advanced" usage if one wants to dig into the "underlying/source/original". And it can indicate to users that, this StripeError with full range of fields isn't the first thing they have to think about.
ptal @ob-stripe

@ob-stripe
Copy link
Contributor Author

@mickjermsurawong-stripe What if I renamed error to stripeError, and added a Javadoc comment to emphasize that this is the model of the exact JSON error returned by Stripe's API?

@mickjermsurawong-stripe
Copy link
Contributor

mickjermsurawong-stripe commented Feb 15, 2019

Ah yup, explicit documentation would address the concern I had too. Given that OAuthException also extends StripeException, explicit stripeError would also be helpful to distinguish that from its sibling property oAuthError.

Other than that, the code this looks great to me. As you mentioned, this is indeed compatible with 8.0, except for StripeErrorResponse that will be public then (the error schema as the default response for all endpoints in OpenAPI).

Thank you @ob-stripe for the fix! Please feel free to self-approve this after the changes! (I'm afraid another round of time-zone waiting for my stamp will block you unnecessarily)

ptal @ob-stripe

@ob-stripe
Copy link
Contributor Author

Released as 7.22.0.

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

Successfully merging this pull request may close these issues.

None yet

3 participants