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.

Sign up for GitHub

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

Jump to bottom

Added a method to create a mysql database from a connection object #583

Merged
merged 4 commits into from
Jul 6, 2021
Merged

Added a method to create a mysql database from a connection object #583

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f98ad3a
Added a method to create a mysql database from a connection object
Jun 17, 2021
394279a
Calling WithConnection from WithInstance to de-duplicate code
Jun 29, 2021
73081e2
Adding context and ping to mysql.WithConnection
Jul 6, 2021
4bfe192
Interface type check at compile time
Jul 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions database/mysql/mysql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,20 +54,22 @@ type Mysql struct {
config *Config
}

// instance must have `multiStatements` set to true
func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) {
// connection instance must have `multiStatements` set to true
func WithConnection(conn *sql.Conn, config *Config) (*Mysql, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on making this context aware? Eventually, we want most DB driver methods to be context aware. I'm thinking that new methods should be context aware.

Also, unfortunately, these functions that create DB drivers currently return interfaces instead of concrete times. I'm thinking returning a concrete type as you're doing now is a cleaner pattern. Can you add a type check? e.g. var _ database.Driver = (*Mysql)(nil) // explicit compile time type check

Copy link
Contributor Author

@Seb-C Seb-C Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If it's consistent across all drivers, then it would be a good thing. But it seems out of scope for this PR.

Can you add a type check?

Sorry, I don't understand what you are asking. Where do you want this check?

Copy link
Member

Choose a reason for hiding this comment

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

If it's consistent across all drivers, then it would be a good thing. But it seems out of scope for this PR.

Oh yeah, making the db drivers consistent is not in the scope of the PR. But new code should fit within the future plans/vision if possible. In this case, we should add a ctx context.Context parameter to WithConnection()

Can you add a type check?

Sorry, I don't understand what you are asking. Where do you want this check?

Add var _ database.Driver = (*Mysql)(nil) // explicit compile time type check as a package level ignored variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I understood your point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (both of it).

if config == nil {
return nil, ErrNilConfig
}

if err := instance.Ping(); err != nil {
return nil, err
mx := &Mysql{
conn: conn,
db: nil,
config: config,
}

if config.DatabaseName == "" {
query := `SELECT DATABASE()`
var databaseName sql.NullString
if err := instance.QueryRow(query).Scan(&databaseName); err != nil {
if err := conn.QueryRowContext(context.Background(), query).Scan(&databaseName); err != nil {
return nil, &database.Error{OrigErr: err, Query: []byte(query)}
}

Expand All @@ -82,21 +84,31 @@ func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) {
config.MigrationsTable = DefaultMigrationsTable
}

conn, err := instance.Conn(context.Background())
if err != nil {
if err := mx.ensureVersionTable(); err != nil {
return nil, err
}

mx := &Mysql{
conn: conn,
db: instance,
config: config,
return mx, nil
}

// instance must have `multiStatements` set to true
func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) {
if err := instance.Ping(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Move this to WithConnection() and use conn.PingContext(). DB.Ping() will create a connection to ping anyways

Copy link
Contributor Author

@Seb-C Seb-C Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The problem I am trying to solve is that this class does unexpected stuff to my connection pool.
If I inject an active pool with n idle connections to this class, do my stuff and close it, I expect my pool to be returned to it's initial state, which means an active pool with n idle connections.
But currently I can only choose between ending-up with a closed pool (because Close not only returns the connection to the pool, but also closes the pool itself), or (if I don't call Close) a pool with n-1 idle connections and 1 allocated connection that will be stuck forever

So the last thing I want is for WithConnection to create new connections in a pool that I did not even inject in the first place.

If the connection I inject is not working, I expect the process to fail without side-effects on my pool.

If the connection is not working, doing conn.PingContext().DB.Ping() would create a new connection in the pool and ping the new connection, which does not test or fix the injected one anyway.

Copy link
Member

@dhui dhui Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My understanding is that connections are pooled at the sql.DB instance level so conn.PingContext() shouldn't create a new connection e.g. connections won't create new connections

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I was misunderstanding your question.
I added the context to WithConnection, as well as calling PingContext.
I cannot add it as a parameter in WithInstance without breaking change, so it should be done along with #14.

return nil, err
}

if err := mx.ensureVersionTable(); err != nil {
conn, err := instance.Conn(context.Background())
if err != nil {
return nil, err
}

mx, err := WithConnection(conn, config)
if err != nil {
return nil, err
}

mx.db = instance

return mx, nil
}

Expand Down Expand Up @@ -243,7 +255,11 @@ func (m *Mysql) Open(url string) (database.Driver, error) {

func (m *Mysql) Close() error {
connErr := m.conn.Close()
dbErr := m.db.Close()
var dbErr error
if m.db != nil {
dbErr = m.db.Close()
}

if connErr != nil || dbErr != nil {
return fmt.Errorf("conn: %v, db: %v", connErr, dbErr)
}
Expand Down