feat(experimental): replace dialect with a state.Storage interface

[oliverpool]

Hi, thanks for taking into consideration my suggestions in #379

I had an other idea regarding the dialect, that I want to share with you. It is not fully thought through yet, but you will probably have some ideas/suggestions, that I would be glad to read!

Currently in goose.NewProvider, the first argument is dialect Dialect, with type Dialect string. I think this presents some shortcomings, mainly it lacks of type-safety (I can write NewProvider("mysqli", nil, nil): it looks fine and compiles, but fails upon execution).

I would suggest creating a type Factory func(tableName string) state.Storage (state.Storage is under internal for now, maybe exported in the future to allow 3rd party implementations)

I put the Factory definition in the storage package to ease discoverability of the options (the storage package exports only Factory and the actual implementations, like Sqlite3).

The caller can now enjoy typing autocompletion and type safety (modulo nil-checking).

package main

import (
	"github.com/pressly/goose/v3"
	"github.com/pressly/goose/v3/storage"
)
func main(){
	// ...
	_, err := goose.NewProvider(storage.Sqlite3, db, fsys)
}

If you think that this idea is sensible, I can port the other dialects to this new architecture.

[oliverpool]

I have just converted the global store and tableName to use this new architecture and it looks like this change can be made without breaking existing code.

If you like the approach, I can finish this PR:

• port the remaining dialects
• remove the internal/dialect and internal/sqladapter packages

[mfridman]

Haven't had a chance to look at this, but can you rebase master? I moved the provider to an internal package (for now).

As we're iterating I'd like to keep it unexported.

[oliverpool]

I have rebased my branch on top of master

[mfridman]

Alright, re. autocompletion and safety, you kind of get it with the string type:

If you pass a string directly, there's still a check to make sure it resolves to an internal known dialect.

From a UX perspective, I think this is what most users would like. The factory seems a bit overwhelming, but this could also be a preference thing.

[mfridman]

Also, note this comment. #520 (comment)

I'd like to make sure we're able to somehow expose knobs to users that want an implementation different than the default ones the goose package supports, such as #530 vs #520

[oliverpool]

I'd like to make sure we're able to somehow expose knobs to users that want an implementation different than the default ones the goose package supports, such as #530 vs #520

I adapted my PR to support this usecase.

The caller can choose to implement the (now exposed) state.Storage interface:

type Storage interface {
	// CreateVersionTable creates the version table.
	// This table is used to store goose migrations.
	CreateVersionTable(ctx context.Context, db DB) error

	// InsertVersion inserts a version id into the version table.
	InsertVersion(ctx context.Context, db DB, version int64) error

	// DeleteVersion deletes a version id from the version table.
	DeleteVersion(ctx context.Context, db DB, version int64) error

	// GetMigrationRow retrieves a single migration by version id.
	//
	// Returns the raw sql error if the query fails. It is the callers responsibility
	// to assert for the correct error, such as sql.ErrNoRows.
	GetMigration(ctx context.Context, db DB, version int64) (*GetMigrationResult, error)

	// ListMigrations retrieves all migrations sorted in descending order by id.
	//
	// If there are no migrations, an empty slice is returned with no error.
	ListMigrations(ctx context.Context, db DB) ([]*ListMigrationsResult, error)
}

Since the tableName is only relevant for the given storage, I added it as an argument to the chosen storage (and completely dropped the Factory, which was to cumbersome, as you rightly pointed).

p, err := provider.NewProvider(storage.Sqlite3(""), db, fsys) // the "" triggers the use of the default table name

The empty string argument is maybe not the nicest, but after some thoughts, I think it is the right place to customize the table name (since it is not used anywhere else).

To drop it, one could change the definition to func Sqlite3(tableName ...string) state.Storage { and then fail if multiple tableNames are given. What do you think?

[oliverpool]

Your example from #520 (comment) would become:

store := yourpkg.NewStore(tableName, otherOptions...)

goose.NewProvider(store, db, fsys)

When the struct of @arunk-s implements the interface defined here: https://github.com/pressly/goose/blob/fbebaf85221d7879930286b9a1eab9b809992868/state/storage.go

[oliverpool]

@mfridman to get rid of the empty string argument, I propose to split the functions with a storage.*WithTableName:

storage.Sqlite3() // default table name
storage.Sqlite3WithTableName("foo") // custom table name

What do you think?

[oliverpool]

@mfridman I just discoverd #623 and commented there: #623 (comment)

[mfridman]

Let's continue the conversation here, from #623 (comment)

Honestly as a user, I don't see much difference for simple usecases between the current master:

db, err := sql.Open("sqlite", ":memory:")
if err != nil {
	return err
}
p, err := provider.NewProvider(database.DialectSQLite3, db, os.DirFS("path/to/migrations"))
if err != nil {
	return err
}

And what I proposed in #608:

db, err := sql.Open("sqlite", ":memory:")
if err != nil {
	return err
}
p, err := provider.NewProvider(storage.Sqlite3(), db, os.DirFS("path/to/migrations"))
if err != nil {
	return err
}

---

However I think the state.Storage interface provides a lot of advantages for advanced usage:

• using a custom state.Storage is very easy (just provide it as first argument), compared to having to use 2 arguments: database.DialectCustom and WithCustomStore
• the provider API gets harder to misuse: if you provide something that does not implement the interface, the compiler will yell at you (whereas currently if you give "non-existing" as first parameter, the code compiles but fails to run)
• table name customization is directly managed by the state.Storage implementations (where it actually belongs)
• no need for dispatch/lookup code (like in database/dialect.go)

I can understand that you don't want to consider my proposal further. If this is the case, please close my PR #608.

---

Most of the points you make are valid. I'll try to address them one by one.

• using a custom state.Storage is very easy (just provide it as first argument), compared to having to use 2 arguments: database.DialectCustom and WithCustomStore

The goal was to make the most common use case easy, i.e., the user doesn't have to think about "storage" or "store" and simply match (one of the supported) dialect to their *sql.DB driver. We're now exposing an internal implementation, but it still feels like most users shouldn't have to think about this layer unless doing something custom.

My main goal is to make the list of arguments short and the types simple.

• the provider API gets harder to misuse: if you provide something that does not implement the interface, the compiler will yell at you (whereas currently if you give "non-existing" as first parameter, the code compiles but fails to run)

Indeed, a compile-time error is always better. I suspect most users would use one of the const and not a raw string, but your point stands.

• table name customization is directly managed by the state.Storage implementations (where it actually belongs)

I didn't like having 2 constructors for each "dialect". But yes, there's probably room to lift the name from the provider to the store/storage instead. Indeed this is where it belongs.

Given we've settled on functional options, I suspect there could be yet another set of options here.

• no need for dispatch/lookup code (like in database/dialect.go)

In the same way you get type hints via the storage interface you also get them for the dialect

Having said all that, maybe there's a common ground? By default goose (already today in the old API) defaults to postgres, so can remove the first argument entirely and expose goose.WithStore option.

func NewProvider(db *sql.DB, fsys fs.FS, opts ...ProviderOption) (*Provider, error) {

It's sort of a middle ground, albeit slightly less clear. A part of me liked the fist 2 args being explictly and the constructor comment reading:

// The caller is responsible for matching the database dialect with the database/sql driver. For
// example, if the database dialect is "postgres", the database/sql driver could be
// github.com/lib/pq or github.com/jackc/pgx.

[oliverpool]

A part of me liked the fist 2 args being explictly

I agree with this part of you. Having to explicitly provide a sql dialect/storage is the best way to ensure the users make the right choice. Defaulting to Postgres seems wrong (I exclusively used goose with sqlite until now).

Regarding #624, I am not sure why the storage would need to provide a Tablename() method: it seems to only be useful on the initialization. So it is probably better for this parameter to be checked by the storage itself - especially if some extra constraints should apply (like the lack of white-space or special characters, depending on the SQL engine).

I like the InsertRequest struct. Maybe it could be merged with GetMigrationResult and ListMigrationsResult.

Thinking about the Storage interface, it could be further simplified:

type Storage interface {
	// CommittedMigrations retrieves all migrations sorted in descending order by id.
	// It should create the migration table if needed.
	//
	// If there are no migrations, an empty slice is returned with no error.
	CommittedMigrations(ctx context.Context, db DB) ([]Migration, error)

	// CommitMigration is called after a migration has been applied
	CommitMigration(context.Context, DB, Migration) error

	// RollbackMigration should mark the specified migration as uncommited
	RollbackMigration(ctx context.Context, db DB, version int64) error
}

type Migration struct {
	ID int64
	Timestamp time.Time
	Checksum []byte  // for #288 (can be ignored by goose if nil)
}

Having a well-defined storage interface will ensure that most of the logic ends up in the storage implementation (for #461 for instance).

It drops:

• CreateVersionTable (which should be handled automatically/internally by the storage on the first call to CommittedMigrations)
• GetMigration (just call CommittedMigrations and perform the search manually)

[oliverpool]

@mfridman since #635 has been merged, shall I consider that my suggestions above has been declined?

If this is the case, please close this PR.

[mfridman]

Thanks for your suggestions and working through the design. I was a bit hesitant delaying this work and hopefully the /v3 implementation can inform what we like/dislike about the current API.

I'm going to be writing a set of blog posts on the learnings/pros/cons of the new provider API, and I can mention the alternatives we toyed around with.

It may not be clear but I've had additional features and wanted something that felt easy but extensible. I want to believe we arrived at some least worst option.

[mfridman]

Thanks for your ideas and fruitful discussions @oliverpool. I still have a few ideas lined up for more detailed posts, but made sure to give you a shoutout in the acknowledgments section:

https://pressly.github.io/goose/blog/2023/goose-provider/#acknowledgments