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

[Questions] On ApiException design #654

Closed
andreaTP opened this issue Sep 14, 2023 · 3 comments · Fixed by #675
Closed

[Questions] On ApiException design #654

andreaTP opened this issue Sep 14, 2023 · 3 comments · Fixed by #675
Labels

Comments

@andreaTP
Copy link
Contributor

Hi there 👋 !

I'm digging down a bit in the codebase and I have a couple of questions regarding the design of ApiException and how you envision its interaction with Authentication mechanisms:

  1. requestStatusCode is a public mutable field, which seems odd, once the Exception is thrown we want the statusCode to be a stable number that cannot be easily changed in user code.
  2. ApiException is a "checked exception" which means that it should be explicitly written in all the method signatures using it. There are quite some discussions around checked exceptions in Java, but, in this context, how do I throw an ApiException(401) from an AuthenticationProvider so that it's indistinguishable from a business logic 401 handled at the API level?
@baywet
Copy link
Member

baywet commented Sep 15, 2023

The reason the status code is public is because it needs to be set by the http request adapter, which might be in this package namespace or in a completely different one.

Adding the exceptions to the method signature is something we briefly mentioned here. Right now, because we're relying on completable futures, the exception will be wrapped in and execution exception anyway.

Api Exception is really meant to materialize a "problem" in the API response, authentication providers should throw their own exception types so users can handle precisely the error based on the type.

@andreaTP
Copy link
Contributor Author

Thanks for the feedback @baywet , this seems reasonable to me.

I think I will review the code for setting the field, maybe we can improve using a builder pattern or something similar.

@baywet
Copy link
Member

baywet commented Sep 15, 2023

Here are a couple of reasons with we didn't do that so far:

  • we need to make sure derived types can still be instantiated trivially (no private constructor)
  • derived instances need to be able to set that field
  • the only time this specific type is instantiated is when there's no matching error code registration (single place in the request adapter)

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 a pull request may close this issue.

2 participants