-
Notifications
You must be signed in to change notification settings - Fork 518
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
refactor: create a generic store and stub out dialect queries #477
Conversation
) | ||
|
||
// SQLDialect abstracts the details of specific SQL dialects | ||
// for goose's few SQL specific statements | ||
type SQLDialect interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To evolve goose
I think we need a more sane way to decouple the dialect queries from the package itself.
We mix raw string queries and db *sql.DB
queries, which leaves it up to the caller to deal with *sql.Rows
directly, that's not great.
var dialect SQLDialect = &PostgresDialect{} | ||
|
||
// GetDialect gets the SQLDialect | ||
func GetDialect() SQLDialect { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the ONLY breaking change in this whole PR. But it's also inconsequential because users can do anything with the SQLDialect
since all the methods are private to this package.
} | ||
|
||
//////////////////////////// | ||
// Postgres |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these queries were moved into the internal/dialect/dialectquery
package and exposed as a single interface:
type Querier interface {
CreateTable() string
InsertVersion() string
DeleteVersion() string
GetMigrationByVersion() string
ListMigrations() string
}
This makes it really trivial to have a Store
interface that calls these queries, which are dialect-specific depending on which querier was initialized:
func (s *store) CreateVersionTable(ctx context.Context, tx *sql.Tx) error {
q := s.querier.CreateTable()
_, err := tx.ExecContext(ctx, q)
return err
}
internal/dialect/dialect.go
Outdated
// The underlying implementation does not modify the error returned by the | ||
// database driver. It is the callers responsibility to assert for the correct | ||
// error, such as sql.ErrNoRows. | ||
type Store interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also gets us one step closer to a fully context-aware package without having to instrument too many internal details.
eadf3bf
to
97560fb
Compare
This PR does a few things, although it's primarily mechanical. At a high level, the goal here is 2-fold:
sql.*
-specific logic into a commonStore
goose
itself never sees or deals with raw queries themselves.TL;DR the
goose
package only has access to theStore
interface, with type-safe methods. AStore
is initialized with a specific database dialect, and the internal implementation ofStore
uses the dialect to implementsql.*
-specific logic. It handles things such as scanning and closing*sql.Rows
, etc.This is what
goose
deals with:But why?
There are places throughout the
goose
package we call things like:It's too easy to mess up the args, and accidentally omit the required placeholders. In this example, one MUST know that the
insertVersionSQL
query requires both a version (int) and a direction (bool).I think it's much safer, and a better separation of concerns, to have a well-defined Store interface with methods like so:
The underlying queries are tucked away in a single place in
dialectquery
package andgoose
package never needs to know them, except for when the Store is initialized with a dialect.The added benefit of the
Store
is that the caller isn't responsible for dealing directly withsql.*
primitives, that's all handled by theStore
. E.g., we do this in a bunch of places, whereas now we do it once: