feat: Expand Store interface; add GetLatestVersion method in Provider

[Albitko]

First pass at #673

expand Store interface with method GetLatestVersion(ctx context.Context, db DBTxConn) (int64, error)

use it in Provider instead of an inefficient old one

[mfridman]

I don't think this comment is true, nothing returns ErrVersionNotFound from our underlying implementations.

[Albitko]

Really, I'd forgotten about that
3867034

[mfridman]

max functions usually return NULL when nothing is found, which is something we should guard against. I think you'll want something like this.

func (s *store) GetLatestVersion(ctx context.Context, db DBTxConn) (int64, error) {
	q := s.querier.GetLatestVersion(s.tablename)
	var maxVersion sql.NullInt64
	err := db.QueryRowContext(ctx, q).Scan(&maxVersion)
	if err != nil {
		return 0, fmt.Errorf("failed to get last version: %w", err)
	}
	if maxVersion.Valid {
		return maxVersion.Int64, nil
	}
	return -1, ErrVersionNotFound
}

A test like this should help cover most of the edge cases across postgres / sqlite, and we'll need to make sure it works like that across the rest of the databases.

t.Run("GetLatestVersion", func(t *testing.T) {
	dir := t.TempDir()
	db, err := sql.Open("sqlite", filepath.Join(dir, "sql_embed.db"))
	check.NoError(t, err)
	store, err := database.NewStore(database.DialectSQLite3, "foo")
	check.NoError(t, err)
	err = store.CreateVersionTable(context.Background(), db)
	check.NoError(t, err)
	v, err := store.GetLatestVersion(context.Background(), db)
	check.HasError(t, err)
	check.IsError(t, err, database.ErrVersionNotFound)
	check.Number(t, v, -1)
	err = store.Insert(context.Background(), db, database.InsertRequest{Version: 0})
	check.NoError(t, err)
	err = store.Insert(context.Background(), db, database.InsertRequest{Version: 1})
	check.NoError(t, err)
	v, err = store.GetLatestVersion(context.Background(), db)
	check.NoError(t, err)
	check.Number(t, v, 1)
})

go test -count=1 -race -cover ./database -json | tparse
┌────────────────────────────────────────────────────────────────────────────────────────┐
│  STATUS │ ELAPSED │               PACKAGE                │ COVER │ PASS │ FAIL │ SKIP  │
│─────────┼─────────┼──────────────────────────────────────┼───────┼──────┼──────┼───────│
│  PASS   │  4.34s  │ github.com/pressly/goose/v3/database │ 84.3% │  6   │  0   │  0    │
└────────────────────────────────────────────────────────────────────────────────────────┘

[Albitko]

Added tests for different databases
cb36bc6

[Albitko]

@mfridman Is it still actual?

[mfridman]

Sorry for the delay, I'll take another pass to see if we still want to do this.

One thing I wanted to avoid was adding additional database tests to this package, and only sticking to sqlite. It tests the Store interface and the underlying implementation, but not the dialects themselves.

I think those kinds of tests should go in .internal/testing (separate Go module). I know this may seem strange, but I slowly want to reduce the number of dependencies in the core goose library and this was the first step. See #713 for more details.

[mfridman]

Appreciate you taking the time to put together a PR. My main issue with this PR was pulling in more database-specific tests into the store package, I really want to keep that running quickly. The rest of the dialect-specific bits should be handled in the ./internal/testing/integration tests.

Going to close this in favor of #758.

Thanks again though 💯