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

feat: add concrete errors to public API #36

Merged
merged 7 commits into from
Aug 12, 2021
Merged

feat: add concrete errors to public API #36

merged 7 commits into from
Aug 12, 2021

Conversation

enocom
Copy link
Member

@enocom enocom commented Aug 9, 2021

This commit adds three concrete errors to the public API and ensures that
all returned errors are either instances of those concrete types OR are
wrapped by more descriptive errors.

The three error types are:

  1. ConfigError: for all errors that are the result of the client making
    a semantic error in their request

  2. RefreshError: for all the (uncommon) cases where the server returns
    data that is somehow and unexpectedly invalid.

  3. DialError: for any error that occurs when interacting with the SQL
    Admin API or when attempting to connect to a
    particular SQL instance.

By providing concrete errors or wrapping concrete errors, we allow our
clients to uses the errors.As API to possibly react differently to
errors coming out of the dialer.

Fixes #37

This commit adds four concrete errors to the public API and ensures that
all returned errors are either instances of those concrete types OR are
wrapped by more descriptive errors.

The four error types are:

1. ClientError: for all errors that are the result of the client making
   semantic error in their request

2. ServerError: for all the (uncommon) cases where the server returns
   data that is somehow and unexpectedly invalid.

3. APIError: for any errors that occurs when interacting with the SQL
   Admin API. These errors wrap the underlying
   google.golang.org/api/googleapi.Error types.

4. DialError: for any error that occurs when attempting to connect to a
   particular SQL instance.

By providing concrete errors or wrapping concrete errors, we allow our
clients to uses the errors.As API to possibly react differently to
errors coming out of the dialer.
@enocom enocom requested a review from kurtisvg August 9, 2021 14:51
Copy link
Contributor

@kurtisvg kurtisvg left a comment

Choose a reason for hiding this comment

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

High level thoughts:

DialError seems good. Should there be a high level error to capture anything during a refresh (e.g. RefreshError)? It seems like it might be useful between differentiating "the dial didn't' work" and "this refresh operation wasn't successful".

I'm not sure I understand why APIError and ServerError are two different error types. Do I really care if the API returned a non 2xx code vs a bad response (or a response I couldn't parse)? Seems like differentiating isn't useful.

errtypes/errors.go Outdated Show resolved Hide resolved
errtypes/errors.go Outdated Show resolved Hide resolved
Also, use initializers everywhere for errors.
@enocom
Copy link
Member Author

enocom commented Aug 11, 2021

I've consolidated ServerError and APIError into one type. Also, I've renamed ClientError to ConfigError and updated the doc string.

@enocom
Copy link
Member Author

enocom commented Aug 11, 2021

Could ServerError be renamed RefreshError?

@enocom enocom requested a review from kurtisvg August 11, 2021 17:28
@kurtisvg
Copy link
Contributor

RefreshError seems better than ServerError

errtypes/errors.go Outdated Show resolved Hide resolved
internal/cloudsql/refresh.go Outdated Show resolved Hide resolved
enocom and others added 4 commits August 12, 2021 11:27
Co-authored-by: Kurtis Van Gent <31518063+kurtisvg@users.noreply.github.com>
Co-authored-by: Kurtis Van Gent <31518063+kurtisvg@users.noreply.github.com>
@enocom enocom merged commit 7441b71 into main Aug 12, 2021
@enocom enocom deleted the errtypes branch August 12, 2021 18:49
This was referenced Apr 5, 2022
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.

Add concrete error types
2 participants