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

Adding context to database provider #1118

Open
wants to merge 23 commits into
base: development
Choose a base branch
from

Conversation

mfreeman451
Copy link
Contributor

@mfreeman451 mfreeman451 commented Oct 17, 2024

Pull Request Template

Description:

  • Added the ability to pass a context down through the DB provider to the Connect() call
  • Connect() now returns an error.
  • Currently the Connect() interface doesn't accept a context, invalid connections to databases will not be known.

Breaking Changes (if applicable):

  • Connect() calls in the DB provider will now return an error and require a context to be passed.
  • This breaks the ability to daisy chain method calls.

Additional Information:

Checklist:

  • I have formatted my code using goimport and golangci-lint.
  • All new code is covered by unit tests.
  • This PR does not decrease the overall code coverage.
  • I have reviewed the code comments and documentation for clarity.

Thank you for your contribution!

@mfreeman451 mfreeman451 marked this pull request as draft October 17, 2024 00:58
@mfreeman451 mfreeman451 marked this pull request as ready for review October 17, 2024 01:01
@vipul-rawat
Copy link
Member

Hey @mfreeman451, the mocks have not been updated, but before that can we discuss some approaches and implementations?

I feel like AddMongo, AddClickhouse, etc... should not return any error but log the error, as we do with the SQL, Redis, etc...

One other train of thought would be to FATAL log the errors, as it would panic the code even if it starts as the handler might expect the databases to be connected.

Comment on lines +6 to +16
// ErrInvalidURI is returned when the MongoDB URI is invalid or cannot be parsed.
ErrInvalidURI = errors.New("invalid MongoDB URI")

// ErrAuthentication is returned when authentication fails.
ErrAuthentication = errors.New("authentication failed")

// ErrDatabaseConnection is returned when the client fails to connect to the specified database.
ErrDatabaseConnection = errors.New("failed to connect to database")

// ErrGenericConnection is returned for general connection issues.
ErrGenericConnection = errors.New("MongoDB connection error")
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a suggestion, feel free to consider it or ignore it

Suggested change
// ErrInvalidURI is returned when the MongoDB URI is invalid or cannot be parsed.
ErrInvalidURI = errors.New("invalid MongoDB URI")
// ErrAuthentication is returned when authentication fails.
ErrAuthentication = errors.New("authentication failed")
// ErrDatabaseConnection is returned when the client fails to connect to the specified database.
ErrDatabaseConnection = errors.New("failed to connect to database")
// ErrGenericConnection is returned for general connection issues.
ErrGenericConnection = errors.New("MongoDB connection error")
// ErrInvalidURI is returned when the MongoDB URI is invalid or cannot be parsed.
ErrInvalidURI = fmt.Errorf("%w: invalid MongoDB URI", ErrConnection)
// ErrAuthentication is returned when authentication fails.
ErrAuthentication = fmt.Errorf("%w: authentication failed", ErrConnection)
// ErrDatabaseConnection is returned when the client fails to connect to the specified database.
ErrDatabaseConnection = fmt.Errorf("%w: failed to connect to database", ErrConnection)
// ErrGenericConnection is returned for general connection issues.
ErrConnection = errors.New("MongoDB connection error")

This way the caller can catch all mongodb error with an errors.Is(err, ErrConnection)

Comment on lines +134 to +141
func (*Client) isTimeoutError(err error) bool {
return strings.Contains(err.Error(), "connection timeout") || mongo.IsTimeout(err)
}

return
func (*Client) isAuthenticationError(err error) bool {
return strings.Contains(err.Error(), "authentication failed") ||
strings.Contains(err.Error(), "AuthenticationFailed")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a big problem with this

Validating errors with what they contains is a bad idea

If you have no other way to do that, then you have to add a clear comment about it

Copy link
Contributor

Choose a reason for hiding this comment

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

}

if errors.Is(err, mongo.ErrClientDisconnected) {
return fmt.Errorf("%w: client disconnected", ErrGenericConnection)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("%w: client disconnected", ErrGenericConnection)
return fmt.Errorf("%w: client disconnected: %w", ErrGenericConnection, err)


func (c *Client) handlePingError(err error) error {
if mongo.IsTimeout(err) {
return fmt.Errorf("%w: connection timeout", ErrGenericConnection)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("%w: connection timeout", ErrGenericConnection)
return fmt.Errorf("%w: connection timeout: %w", ErrGenericConnection, err)

You should never hide low level error with yours. You should wrap them with yours

Comment on lines +134 to +135
func (*Client) isTimeoutError(err error) bool {
return strings.Contains(err.Error(), "connection timeout") || mongo.IsTimeout(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to do not rely only on mongo.IsTimeOut ?

It seems strange to check if error contains a text

If you have to, something I'm not sure, you should add a comment explaining it.

@aryanmehrotra
Copy link
Member

@mfreeman451 can you fix the reveiw comments?
We would not want to keep a PR open for more than 2 weeks - for better maintainability.

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.

mongo driver doesnt support contexts
5 participants