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
Making GoogleAuthException public #1122
Comments
Hey @TimurSadykov thanks for assigning yourself! let me know if Im missing anything |
Hi, thanks for raising the issue! Exceptions are package private intentionally. We want to have flexibility to change them in the future without breaking anyone's code. At the same time, we understand that there could be valid use cases for public exceptions. Could you please describe scenarios, that you think you can handle if Exceptions are public? One more thing to keep in mind is that this library has internal retry mechanism, so if Exception is retryable - it was already retried 3 times with appropriate backoff. A proposal: rely on interfaces. It is way easier to maintain public interfaces. You can already catch GoogleAuthException by checking Retryable interface. How about we do the same for OAuthException? Let's define the OAuthError interface in the credentials package. |
Thanks for the response Timur! We cant really catch I like the proposal if it means to extend My intention was to convince you of making the exceptions public but I wasnt sure myself tbh, and Im probably not smart enough to make the point anyways, so I asked chat GPT and the results were interesting: Ok cool, so it depends, but what about this specific case? |
@julianSelser Yes, more specifically I meant catching IOExceptions and checking for instances of Retryable or other interfaces. This makes more sense also because IOException is already a historical de-facto standard exception type and if you want to catch all auth exceptions, you have to catch IOExceptions anyways. The name OAuthException works better than OAuth2Error, but there is already an exception called OAuthException and java does not allow interface and exception with the same name. So we need something else, ideas are welcome :) |
@TimurSadykov I'd rather have the exceptions public instead of forcing users to do an Is it ok if I redo my PR to accomodate the interface? Im wondering if we can have some |
@julianSelser Ideally, we want to follow the OAuth2 spec for errors. It has a field called errorDescription which is similar to what you are describing? I would suggest following the OAuthException implementation as a base for you interface. It should have what you need. Then we can make the existing OAuthException to extend the new interface. It already extends the Retryable via GoogleAuthException. Then we can throw the updated exception from our credential classes. Let me know if any questions. |
I'm using the google ads java lib which has this project as a dependency.
its
GoogleAdsClient
take these project'sGoogleCredentials
and when trying to authenticate, it may throwcom.google.auth.oauth2.GoogleAuthException
(orOAuthException
)If so, we would love to catch it like this:
But we cant because
GoogleAuthException
is package private, would be nice to have it aspublic
(along with the other exceptions that may bubble outside the package)The text was updated successfully, but these errors were encountered: