Repository: supabase/cli
Pull request #1057 (2023-05-03 19:07:10)

fix: skip migrations when backup volume exists
## What kind of change does this PR introduce?

Bug fix https://github.com/supabase/cli/issues/1055

## What is the new behavior?

- introduced a scoped variable for checking if backup exists
- refactors common error fs for testing

## Additional context

Add any other context or screenshots.


coveralls: ## Pull Request Test Coverage Report for [Build 4879686546](https://coveralls.io/builds/59604202)

* **14** of **15**   **(93.33%)**  changed or added relevant lines in **2** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.2%**) to **60.825%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [internal/testing/fstest/open.go](https://coveralls.io/builds/59604202/source?filename=internal%2Ftesting%2Ffstest%2Fopen.go#L20) | 9 | 10 | 90.0%
<!-- | **Total:** | **14** | **15** | **93.33%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/59604202/badge)](https://coveralls.io/builds/59604202) |
| :-- | --: |
| Change from base [Build 4876674330](https://coveralls.io/builds/59595404): |  0.2% |
| Covered Lines: | 4349 |
| Relevant Lines: | 7150 |

---
##### 💛  - [Coveralls](https://coveralls.io)


Patch:
diff --git a/internal/db/start/start.go b/internal/db/start/start.go
index 59e66ce2..a6c5435b 100644
--- a/internal/db/start/start.go
+++ b/internal/db/start/start.go
@@ -72,6 +72,8 @@ EOF
 	return config
 }
 
+var noBackupVolume bool = true
+
 func StartDatabase(ctx context.Context, fsys afero.Fs, w io.Writer, options ...func(*pgx.ConnConfig)) error {
 	config := NewContainerConfig()
 	hostPort := strconv.FormatUint(uint64(utils.Config.Db.Port), 10)
@@ -88,6 +90,7 @@ func StartDatabase(ctx context.Context, fsys afero.Fs, w io.Writer, options ...f
 	fmt.Fprintln(w, "Starting database...")
 	// Creating volume will not override existing volume, so we must inspect explicitly
 	_, err := utils.Docker.VolumeInspect(ctx, utils.DbId)
+	noBackupVolume = client.IsErrNotFound(err)
 	if _, err := utils.DockerStart(ctx, config, hostConfig, utils.DbId); err != nil {
 		return err
 	}
@@ -95,7 +98,7 @@ func StartDatabase(ctx context.Context, fsys afero.Fs, w io.Writer, options ...f
 		fmt.Fprintln(os.Stderr, "Database is not healthy.")
 	}
 	// Initialise if we are on PG14 and there's no existing db volume
-	if client.IsErrNotFound(err) && utils.Config.Db.MajorVersion <= 14 {
+	if noBackupVolume && utils.Config.Db.MajorVersion <= 14 {
 		if err := initDatabase(ctx, w, options...); err != nil {
 			return err
 		}
@@ -141,6 +144,9 @@ func initDatabase(ctx context.Context, w io.Writer, options ...func(*pgx.ConnCon
 }
 
 func SetupDatabase(ctx context.Context, fsys afero.Fs, w io.Writer, options ...func(*pgx.ConnConfig)) error {
+	if !noBackupVolume {
+		return nil
+	}
 	conn, err := utils.ConnectLocalPostgres(ctx, "localhost", utils.Config.Db.Port, "postgres", options...)
 	if err != nil {
 		return err


Test patch:
diff --git a/internal/db/reset/reset_test.go b/internal/db/reset/reset_test.go
index 4b689fd4..d5298081 100644
--- a/internal/db/reset/reset_test.go
+++ b/internal/db/reset/reset_test.go
@@ -92,7 +92,7 @@ func TestResetDatabase(t *testing.T) {
 		// Run test
 		err := resetDatabase(context.Background(), fsys)
 		// Check error
-		assert.ErrorContains(t, err, "invalid port")
+		assert.ErrorContains(t, err, "invalid port (outside range)")
 	})
 
 	t.Run("throws error on duplicate schema", func(t *testing.T) {
diff --git a/internal/db/start/start_test.go b/internal/db/start/start_test.go
index d93cf3b5..eec925bf 100644
--- a/internal/db/start/start_test.go
+++ b/internal/db/start/start_test.go
@@ -17,6 +17,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/supabase/cli/internal/testing/apitest"
+	"github.com/supabase/cli/internal/testing/fstest"
 	"github.com/supabase/cli/internal/testing/pgtest"
 	"github.com/supabase/cli/internal/utils"
 	"gopkg.in/h2non/gock.v1"
@@ -34,18 +35,6 @@ func (m *StatErrorFs) Stat(name string) (fs.FileInfo, error) {
 	return m.MemMapFs.Stat(name)
 }
 
-type OpenErrorFs struct {
-	afero.MemMapFs
-	DenyPath string
-}
-
-func (m *OpenErrorFs) OpenFile(name string, flag int, perm os.FileMode) (afero.File, error) {
-	if strings.HasPrefix(name, m.DenyPath) {
-		return nil, fs.ErrPermission
-	}
-	return m.MemMapFs.OpenFile(name, flag, perm)
-}
-
 func TestInitBranch(t *testing.T) {
 	t.Run("throws error on permission denied", func(t *testing.T) {
 		// Setup in-memory fs
@@ -59,7 +48,6 @@ func TestInitBranch(t *testing.T) {
 	t.Run("throws error on stat failure", func(t *testing.T) {
 		// Setup in-memory fs
 		fsys := &StatErrorFs{DenyPath: utils.CurrBranchPath}
-		// Setup mock docker
 		// Run test
 		err := initCurrentBranch(fsys)
 		// Check error
@@ -68,7 +56,7 @@ func TestInitBranch(t *testing.T) {
 
 	t.Run("throws error on write failure", func(t *testing.T) {
 		// Setup in-memory fs
-		fsys := &OpenErrorFs{DenyPath: utils.CurrBranchPath}
+		fsys := &fstest.OpenErrorFs{DenyPath: utils.CurrBranchPath}
 		// Run test
 		err := initCurrentBranch(fsys)
 		// Check error
@@ -82,7 +70,7 @@ func TestInitDatabase(t *testing.T) {
 		// Run test
 		err := initDatabase(context.Background(), io.Discard)
 		// Check error
-		assert.ErrorContains(t, err, "invalid port")
+		assert.ErrorContains(t, err, "invalid port (outside range)")
 	})
 
 	t.Run("throws error on exec failure", func(t *testing.T) {
@@ -101,12 +89,13 @@ func TestInitDatabase(t *testing.T) {
 }
 
 func TestStartDatabase(t *testing.T) {
-	t.Cleanup(func() {
+	teardown := func() {
 		utils.Containers = []string{}
 		utils.Volumes = []string{}
-	})
+	}
 
 	t.Run("initialise main branch", func(t *testing.T) {
+		defer teardown()
 		utils.DbImage = utils.Pg15Image
 		utils.Config.Db.MajorVersion = 15
 		utils.DbId = "supabase_db_test"
@@ -142,6 +131,7 @@ func TestStartDatabase(t *testing.T) {
 	})
 
 	t.Run("recover from backup volume", func(t *testing.T) {
+		defer teardown()
 		utils.DbImage = utils.Pg15Image
 		utils.Config.Db.MajorVersion = 15
 		utils.DbId = "supabase_db_test"
@@ -176,6 +166,7 @@ func TestStartDatabase(t *testing.T) {
 	})
 
 	t.Run("throws error on start failure", func(t *testing.T) {
+		defer teardown()
 		utils.DbImage = utils.Pg15Image
 		utils.Config.Db.MajorVersion = 15
 		utils.DbId = "supabase_db_test"
@@ -284,3 +275,68 @@ func TestStartCommand(t *testing.T) {
 		assert.Empty(t, apitest.ListUnmatchedRequests())
 	})
 }
+
+func TestSetupDatabase(t *testing.T) {
+	t.Run("skips when backup exists", func(t *testing.T) {
+		noBackupVolume = false
+		// Run test
+		err := SetupDatabase(context.Background(), nil, io.Discard)
+		// Check error
+		assert.NoError(t, err)
+		// Reset variable
+		noBackupVolume = true
+	})
+
+	t.Run("initialises database", func(t *testing.T) {
+		utils.Config.Db.Port = 5432
+		// Setup in-memory fs
+		fsys := afero.NewMemMapFs()
+		// Setup mock postgres
+		conn := pgtest.NewConn()
+		defer conn.Close(t)
+		// Run test
+		err := SetupDatabase(context.Background(), fsys, io.Discard, conn.Intercept)
+		// Check error
+		assert.NoError(t, err)
+	})
+
+	t.Run("throws error on connect failure", func(t *testing.T) {
+		utils.Config.Db.Port = 0
+		// Setup in-memory fs
+		fsys := &fstest.OpenErrorFs{DenyPath: utils.CustomRolesPath}
+		// Run test
+		err := SetupDatabase(context.Background(), fsys, io.Discard)
+		// Check error
+		assert.ErrorContains(t, err, "invalid port (outside range)")
+	})
+
+	t.Run("throws error on read failure", func(t *testing.T) {
+		utils.Config.Db.Port = 5432
+		// Setup in-memory fs
+		fsys := &fstest.OpenErrorFs{DenyPath: utils.CustomRolesPath}
+		// Setup mock postgres
+		conn := pgtest.NewConn()
+		defer conn.Close(t)
+		// Run test
+		err := SetupDatabase(context.Background(), fsys, io.Discard, conn.Intercept)
+		// Check error
+		assert.ErrorIs(t, err, os.ErrPermission)
+	})
+
+	t.Run("throws error on exec failure", func(t *testing.T) {
+		utils.Config.Db.Port = 5432
+		// Setup in-memory fs
+		fsys := afero.NewMemMapFs()
+		sql := "create role postgres"
+		require.NoError(t, afero.WriteFile(fsys, utils.CustomRolesPath, []byte(sql), 0644))
+		// Setup mock postgres
+		conn := pgtest.NewConn()
+		defer conn.Close(t)
+		conn.Query(sql).
+			ReplyError(pgerrcode.DuplicateObject, `role "postgres" already exists`)
+		// Run test
+		err := SetupDatabase(context.Background(), fsys, io.Discard, conn.Intercept)
+		// Check error
+		assert.ErrorContains(t, err, `ERROR: role "postgres" already exists (SQLSTATE 42710)`)
+	})
+}
diff --git a/internal/link/link_test.go b/internal/link/link_test.go
index c07348f6..ab5ba3c8 100644
--- a/internal/link/link_test.go
+++ b/internal/link/link_test.go
@@ -33,11 +33,12 @@ var dbConfig = pgconn.Config{
 func TestPreRun(t *testing.T) {
 	// Reset global variable
 	copy := utils.Config
-	t.Cleanup(func() {
+	teardown := func() {
 		utils.Config = copy
-	})
+	}
 
 	t.Run("passes sanity check", func(t *testing.T) {
+		defer teardown()
 		project := apitest.RandomProjectRef()
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
@@ -49,6 +50,7 @@ func TestPreRun(t *testing.T) {
 	})
 
 	t.Run("throws error on invalid project ref", func(t *testing.T) {
+		defer teardown()
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
 		// Run test
@@ -58,6 +60,7 @@ func TestPreRun(t *testing.T) {
 	})
 
 	t.Run("throws error on missing config", func(t *testing.T) {
+		defer teardown()
 		project := apitest.RandomProjectRef()
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
@@ -68,8 +71,16 @@ func TestPreRun(t *testing.T) {
 	})
 }
 
+// Reset global variable
+func teardown() {
+	for k := range updatedConfig {
+		delete(updatedConfig, k)
+	}
+}
+
 func TestPostRun(t *testing.T) {
 	t.Run("prints completion message", func(t *testing.T) {
+		defer teardown()
 		project := "test-project"
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
@@ -82,6 +93,7 @@ func TestPostRun(t *testing.T) {
 	})
 
 	t.Run("prints changed config", func(t *testing.T) {
+		defer teardown()
 		project := "test-project"
 		updatedConfig["api"] = "test"
 		// Setup in-memory fs
@@ -103,14 +115,8 @@ func TestLinkCommand(t *testing.T) {
 	// Mock credentials store
 	keyring.MockInit()
 
-	// Reset global variable
-	t.Cleanup(func() {
-		for k := range updatedConfig {
-			delete(updatedConfig, k)
-		}
-	})
-
 	t.Run("link valid project", func(t *testing.T) {
+		defer teardown()
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
 		// Setup mock postgres
@@ -152,6 +158,7 @@ func TestLinkCommand(t *testing.T) {
 	})
 
 	t.Run("throws error on connect failure", func(t *testing.T) {
+		defer teardown()
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
 		// Flush pending mocks after test execution
@@ -171,6 +178,7 @@ func TestLinkCommand(t *testing.T) {
 	})
 
 	t.Run("throws error on write failure", func(t *testing.T) {
+		defer teardown()
 		// Setup in-memory fs
 		fsys := afero.NewReadOnlyFs(afero.NewMemMapFs())
 		// Flush pending mocks after test execution
@@ -197,14 +205,8 @@ func TestLinkPostgrest(t *testing.T) {
 	token := apitest.RandomAccessToken(t)
 	t.Setenv("SUPABASE_ACCESS_TOKEN", string(token))
 
-	// Reset global variable
-	t.Cleanup(func() {
-		for k := range updatedConfig {
-			delete(updatedConfig, k)
-		}
-	})
-
 	t.Run("ignores matching config", func(t *testing.T) {
+		defer teardown()
 		// Flush pending mocks after test execution
 		defer gock.OffAll()
 		gock.New(utils.DefaultApiHost).
@@ -220,6 +222,7 @@ func TestLinkPostgrest(t *testing.T) {
 	})
 
 	t.Run("updates api on newer config", func(t *testing.T) {
+		defer teardown()
 		// Flush pending mocks after test execution
 		defer gock.OffAll()
 		gock.New(utils.DefaultApiHost).
@@ -244,6 +247,7 @@ func TestLinkPostgrest(t *testing.T) {
 	})
 
 	t.Run("throws error on network failure", func(t *testing.T) {
+		defer teardown()
 		// Flush pending mocks after test execution
 		defer gock.OffAll()
 		gock.New(utils.DefaultApiHost).
@@ -257,6 +261,7 @@ func TestLinkPostgrest(t *testing.T) {
 	})
 
 	t.Run("throws error on server unavailable", func(t *testing.T) {
+		defer teardown()
 		// Flush pending mocks after test execution
 		defer gock.OffAll()
 		gock.New(utils.DefaultApiHost).
@@ -276,14 +281,8 @@ func TestSliceEqual(t *testing.T) {
 }
 
 func TestLinkDatabase(t *testing.T) {
-	// Reset global variable
-	t.Cleanup(func() {
-		for k := range updatedConfig {
-			delete(updatedConfig, k)
-		}
-	})
-
 	t.Run("throws error on connect failure", func(t *testing.T) {
+		defer teardown()
 		// Run test
 		err := linkDatabase(context.Background(), pgconn.Config{})
 		// Check error
@@ -292,6 +291,7 @@ func TestLinkDatabase(t *testing.T) {
 	})
 
 	t.Run("ignores missing server version", func(t *testing.T) {
+		defer teardown()
 		// Setup mock postgres
 		conn := pgtest.NewWithStatus(map[string]string{
 			"standard_conforming_strings": "on",
@@ -309,6 +309,7 @@ func TestLinkDatabase(t *testing.T) {
 	})
 
 	t.Run("updates config to newer db version", func(t *testing.T) {
+		defer teardown()
 		utils.Config.Db.MajorVersion = 14
 		// Setup mock postgres
 		conn := pgtest.NewWithStatus(map[string]string{
@@ -331,6 +332,7 @@ func TestLinkDatabase(t *testing.T) {
 	})
 
 	t.Run("throws error on query failure", func(t *testing.T) {
+		defer teardown()
 		utils.Config.Db.MajorVersion = 14
 		// Setup mock postgres
 		conn := pgtest.NewConn()
diff --git a/internal/migration/apply/apply_test.go b/internal/migration/apply/apply_test.go
index e71b3b72..c405ae18 100644
--- a/internal/migration/apply/apply_test.go
+++ b/internal/migration/apply/apply_test.go
@@ -3,7 +3,6 @@ package apply
 import (
 	"bufio"
 	"context"
-	"io/fs"
 	"os"
 	"path/filepath"
 	"strings"
@@ -15,6 +14,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/supabase/cli/internal/migration/repair"
+	"github.com/supabase/cli/internal/testing/fstest"
 	"github.com/supabase/cli/internal/testing/pgtest"
 	"github.com/supabase/cli/internal/utils"
 	"github.com/supabase/cli/internal/utils/parser"
@@ -73,28 +73,16 @@ func TestMigrateDatabase(t *testing.T) {
 	t.Run("throws error on open failure", func(t *testing.T) {
 		path := filepath.Join(utils.MigrationsDir, "20220727064247_create_table.sql")
 		// Setup in-memory fs
-		fsys := &OpenErrorFs{DenyPath: path}
-		query := "create table test"
-		require.NoError(t, afero.WriteFile(fsys, path, []byte(query), 0644))
+		fsys := &fstest.OpenErrorFs{DenyPath: path}
+		_, err := fsys.MemMapFs.Create(path)
+		require.NoError(t, err)
 		// Run test
-		err := MigrateDatabase(context.Background(), nil, fsys)
+		err = MigrateDatabase(context.Background(), nil, fsys)
 		// Check error
 		assert.ErrorIs(t, err, os.ErrPermission)
 	})
 }
 
-type OpenErrorFs struct {
-	afero.MemMapFs
-	DenyPath string
-}
-
-func (m *OpenErrorFs) Open(name string) (afero.File, error) {
-	if strings.HasPrefix(name, m.DenyPath) {
-		return nil, fs.ErrPermission
-	}
-	return m.MemMapFs.Open(name)
-}
-
 func TestMigrationFile(t *testing.T) {
 	t.Run("new from file sets max token", func(t *testing.T) {
 		viper.Reset()
diff --git a/internal/start/start_test.go b/internal/start/start_test.go
index a5fc633c..b9dfd8c4 100644
--- a/internal/start/start_test.go
+++ b/internal/start/start_test.go
@@ -247,14 +247,11 @@ func TestDatabaseStart(t *testing.T) {
 					Health:  &types.Health{Status: "healthy"},
 				},
 			}})
-		// Setup mock postgres
-		conn := pgtest.NewConn()
-		defer conn.Close(t)
 		// Run test
 		exclude := ExcludableContainers()
 		exclude = append(exclude, "invalid", exclude[0])
 		err := utils.RunProgram(context.Background(), func(p utils.Program, ctx context.Context) error {
-			return run(p, context.Background(), fsys, exclude, conn.Intercept)
+			return run(p, context.Background(), fsys, exclude)
 		})
 		// Check error
 		assert.NoError(t, err)
diff --git a/internal/testing/fstest/open.go b/internal/testing/fstest/open.go
new file mode 100644
index 00000000..bc756e3c
--- /dev/null
+++ b/internal/testing/fstest/open.go
@@ -0,0 +1,28 @@
+package fstest
+
+import (
+	"io/fs"
+	"os"
+	"strings"
+
+	"github.com/spf13/afero"
+)
+
+type OpenErrorFs struct {
+	afero.MemMapFs
+	DenyPath string
+}
+
+func (m *OpenErrorFs) OpenFile(name string, flag int, perm os.FileMode) (afero.File, error) {
+	if strings.HasPrefix(name, m.DenyPath) {
+		return nil, fs.ErrPermission
+	}
+	return m.MemMapFs.OpenFile(name, flag, perm)
+}
+
+func (m *OpenErrorFs) Open(name string) (afero.File, error) {
+	if strings.HasPrefix(name, m.DenyPath) {
+		return nil, fs.ErrPermission
+	}
+	return m.MemMapFs.Open(name)
+}
diff --git a/internal/utils/access_token_test.go b/internal/utils/access_token_test.go
index 609c7ff0..4a7049bd 100644
--- a/internal/utils/access_token_test.go
+++ b/internal/utils/access_token_test.go
@@ -1,15 +1,14 @@
 package utils
 
 import (
-	"io/fs"
 	"os"
-	"strings"
 	"testing"
 
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/supabase/cli/internal/testing/apitest"
+	"github.com/supabase/cli/internal/testing/fstest"
 	"github.com/zalando/go-keyring"
 )
 
@@ -80,7 +79,7 @@ func TestLoadTokenFallback(t *testing.T) {
 		path, err := getAccessTokenPath()
 		assert.NoError(t, err)
 		// Setup in-memory fs
-		fsys := &OpenErrorFs{DenyPath: path}
+		fsys := &fstest.OpenErrorFs{DenyPath: path}
 		// Run test
 		token, err := fallbackLoadToken(fsys)
 		// Check error
@@ -154,29 +153,10 @@ func TestSaveTokenFallback(t *testing.T) {
 		home, err := os.UserHomeDir()
 		assert.NoError(t, err)
 		// Setup in-memory fs
-		fsys := &OpenErrorFs{DenyPath: home}
+		fsys := &fstest.OpenErrorFs{DenyPath: home}
 		// Run test
 		err = fallbackSaveToken(token, fsys)
 		// Check error
 		assert.ErrorContains(t, err, "permission denied")
 	})
 }
-
-type OpenErrorFs struct {
-	afero.MemMapFs
-	DenyPath string
-}
-
-func (m *OpenErrorFs) OpenFile(name string, flag int, perm os.FileMode) (afero.File, error) {
-	if strings.HasPrefix(name, m.DenyPath) {
-		return nil, fs.ErrPermission
-	}
-	return m.MemMapFs.OpenFile(name, flag, perm)
-}
-
-func (m *OpenErrorFs) Open(name string) (afero.File, error) {
-	if strings.HasPrefix(name, m.DenyPath) {
-		return nil, fs.ErrPermission
-	}
-	return m.MemMapFs.Open(name)
-}
diff --git a/internal/utils/config_test.go b/internal/utils/config_test.go
index a5188a48..24515471 100644
--- a/internal/utils/config_test.go
+++ b/internal/utils/config_test.go
@@ -19,17 +19,19 @@ var (
 func TestConfigParsing(t *testing.T) {
 	// Reset global variable
 	copy := initConfigTemplate
-	t.Cleanup(func() {
+	teardown := func() {
 		initConfigTemplate = copy
-	})
+	}
 
 	t.Run("classic config file", func(t *testing.T) {
+		defer teardown()
 		fsys := afero.NewMemMapFs()
 		assert.NoError(t, WriteConfig(fsys, false))
 		assert.NoError(t, LoadConfigFS(fsys))
 	})
 
 	t.Run("config file with environment variables", func(t *testing.T) {
+		defer teardown()
 		initConfigTemplate = testInitConfigTemplate
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
@@ -44,6 +46,7 @@ func TestConfigParsing(t *testing.T) {
 	})
 
 	t.Run("config file with environment variables fails when unset", func(t *testing.T) {
+		defer teardown()
 		initConfigTemplate = testInitConfigTemplate
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()