Repository: supabase/cli
Pull request #696 (2022-12-14 16:14:48)

feat: implement db dump command
## What kind of change does this PR introduce?

feature

## What is the current behavior?

difficult to dump the remote project schema with pg_dump as non-superuser

## What is the new behavior?

- Adds `db dump` command, defaults to stdout
- Adds `db dump -f schema.sql` flag to exporting to path
- For baselining, chain with pipe `supabase db dump | supabase migration new baseline`

## Additional context

Add any other context or screenshots.


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

* **31** of **39**   **(79.49%)**  changed or added relevant lines in **3** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.1%**) to **57.206%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [cmd/migration.go](https://coveralls.io/builds/55161325/source?filename=cmd%2Fmigration.go#L74) | 7 | 15 | 46.67%
<!-- | **Total:** | **31** | **39** | **79.49%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/55161325/badge)](https://coveralls.io/builds/55161325) |
| :-- | --: |
| Change from base [Build 3695720979](https://coveralls.io/builds/55157047): |  0.1% |
| Covered Lines: | 3521 |
| Relevant Lines: | 6155 |

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

soedirgo: > supabase migration pull
supabase migration repair --status applied <version>

I would argue this is even more confusing because this is a potential footgun:
- user runs `supabase migration pull`
- user forgets to run `supabase migration repair`
- user runs `supabase migration push`

Let's just rename `db remote commit` but keep the old behavior.
sweatybridge: > potential footgun

Can you think of case where a non-idempotent migration will succeed `db push` and cause harm?
soedirgo: Something like creating indexes and sequences - you'd end up having duplicates.

We should also ask the flip side of this: is there a case where we want `migration pull` to *not* insert a migration version? And is that use case common enough that we want that to be the default? IMO `migration repair` should only be run if something is wrong with the migration history, whereas in the common case of `migration pull` things are working normally.
sweatybridge: > creating indexes and sequences - you'd end up having duplicates.

I don't think that's the case for create sequence statements generated by pg_dump/migra? Each sequence should have a unique name https://www.postgresql.org/docs/current/sql-createsequence.html

> is there a case where we want migration pull to not insert a migration version? And is that use case common enough that we want that to be the default?

I believe a pull operation should not change the remote state. This is what I find most confusing about remote commit as we often have to explain to users that it is safe.

There are many reasons a user may choose to dump the remote schemas repeatedly. For example, backups, integration with 3rd party services, moving to/from self-hosted, checking schemas are correct, etc. I think it's nice to make all our fixes in dump_initial_migration.sh available to users, especially since we are removing superuser access. Having a more general purpose pull command helps with these use cases.
soedirgo: > I don't think that's the case for create sequence statements generated by pg_dump/migra?

Maybe not, but users may edit the generated migration. But generally I think asserting there's no migration that's non-idempotent and doesn't fail to execute on subsequent runs is a big bet against unknown unknowns.

> This is what I find most confusing about remote commit as we often have to explain to users that it is safe.

But that's why it's called `db remote commit` in the first place instead of `db pull`: it *does* change state on the remote database - it "commits" the changes you made on the remote database as a migration.

> There are many reasons a user may choose to dump the remote schemas repeatedly

OK I didn't notice what this command was doing at first - I thought it was doing schema diffing. Then it doesn't look like it involves migrations at all? Shouldn't this be called `db dump` instead, and print to stdout instead of adding a migration? I can't think of a case where you already have some migrations, and then want to run this command, which saves as a migration some DDLs, some of which are already included in previous migrations.
soedirgo: The "safe" alternative to `db remote commit` would be `db remote changes` (now `db diff --linked`) - that seems to capture some of the use cases there, like checking schemas are consistent.

So basically where I'm coming from is: I find that it's very common to be in a situation where you made some schema changes directly to the remote db and want to save it as a migration. I think there should be a single command for that, and it shouldn't be gated by scary-sounding commands like `repair`, because it's not in a bad state per se, we just need a way to get back to a state where you can start making schema changes locally.
sweatybridge: For now I will remove the deprecation notice.

The pull/dump command is still useful for squashing migrations. By dumping into a new migration file, users can delete previous migration files which might be old and irrelevant. This process is also called baselining: https://flywaydb.org/documentation/command/baseline

I will also add a flag to support dumping schema to a user defined path. This will unlock many use cases for 3rd party tools. 

Patch:
diff --git a/cmd/db.go b/cmd/db.go
index eb9ba220..7f5685f0 100644
--- a/cmd/db.go
+++ b/cmd/db.go
@@ -12,6 +12,7 @@ import (
 	"github.com/supabase/cli/internal/db/branch/list"
 	"github.com/supabase/cli/internal/db/branch/switch_"
 	"github.com/supabase/cli/internal/db/diff"
+	"github.com/supabase/cli/internal/db/dump"
 	"github.com/supabase/cli/internal/db/lint"
 	"github.com/supabase/cli/internal/db/push"
 	"github.com/supabase/cli/internal/db/remote/changes"
@@ -94,6 +95,20 @@ var (
 		},
 	}
 
+	dbDumpCmd = &cobra.Command{
+		Use:   "dump",
+		Short: "Dumps schemas from the remote database",
+		RunE: func(cmd *cobra.Command, args []string) error {
+			fsys := afero.NewOsFs()
+			if err := loadLinkedProject(fsys); err != nil {
+				return err
+			}
+			host := utils.GetSupabaseDbHost(projectRef)
+			ctx, _ := signal.NotifyContext(cmd.Context(), os.Interrupt)
+			return dump.Run(ctx, file, username, dbPassword, database, host, fsys)
+		},
+	}
+
 	dryRun bool
 
 	dbPushCmd = &cobra.Command{
@@ -197,9 +212,15 @@ func init() {
 	diffFlags := dbDiffCmd.Flags()
 	diffFlags.BoolVar(&useMigra, "use-migra", false, "Use migra to generate schema diff.")
 	diffFlags.BoolVar(&linked, "linked", false, "Diffs local schema against linked project.")
-	diffFlags.StringVarP(&file, "file", "f", "", "Saves schema diff to a file.")
+	diffFlags.StringVarP(&file, "file", "f", "", "Saves schema diff to a new migration file.")
 	diffFlags.StringSliceVarP(&schema, "schema", "s", []string{"public"}, "List of schema to include.")
 	dbCmd.AddCommand(dbDiffCmd)
+	// Build dump command
+	dumpFlags := dbDumpCmd.Flags()
+	dumpFlags.StringVarP(&file, "file", "f", "", "File path to save the dumped schema.")
+	dumpFlags.StringVarP(&dbPassword, "password", "p", "", "Password to your remote Postgres database.")
+	cobra.CheckErr(viper.BindPFlag("DB_PASSWORD", dumpFlags.Lookup("password")))
+	dbCmd.AddCommand(dbDumpCmd)
 	// Build push command
 	pushFlags := dbPushCmd.Flags()
 	pushFlags.BoolVar(&dryRun, "dry-run", false, "Print the migrations that would be applied, but don't actually apply them.")
diff --git a/cmd/migration.go b/cmd/migration.go
index f997e994..04ac4312 100644
--- a/cmd/migration.go
+++ b/cmd/migration.go
@@ -78,6 +78,8 @@ func init() {
 	repairFlags := migrationRepairCmd.Flags()
 	repairFlags.Var(&targetStatus, "status", "Version status to update.")
 	cobra.CheckErr(migrationRepairCmd.MarkFlagRequired("status"))
+	repairFlags.StringVarP(&dbPassword, "password", "p", "", "Password to your remote Postgres database.")
+	cobra.CheckErr(viper.BindPFlag("DB_PASSWORD", repairFlags.Lookup("password")))
 	migrationCmd.AddCommand(migrationRepairCmd)
 	// Build new command
 	migrationCmd.AddCommand(migrationNewCmd)
diff --git a/internal/db/dump/dump.go b/internal/db/dump/dump.go
new file mode 100644
index 00000000..919a98f8
--- /dev/null
+++ b/internal/db/dump/dump.go
@@ -0,0 +1,41 @@
+package dump
+
+import (
+	"context"
+	_ "embed"
+	"errors"
+	"fmt"
+	"os"
+	"strings"
+
+	"github.com/spf13/afero"
+	"github.com/supabase/cli/internal/utils"
+)
+
+//go:embed templates/dump_initial_migration.sh
+var dumpInitialMigrationScript string
+
+func Run(ctx context.Context, path, username, password, database, host string, fsys afero.Fs) error {
+	fmt.Fprintln(os.Stderr, "Dumping schemas from remote database...")
+	out, err := utils.DockerRunOnce(ctx, utils.Pg15Image, []string{
+		"PGHOST=" + host,
+		"PGUSER=" + username,
+		"PGPASSWORD=" + password,
+		"EXCLUDED_SCHEMAS=" + strings.Join(utils.InternalSchemas, "|"),
+		"DB_URL=" + database,
+	}, []string{"bash", "-c", dumpInitialMigrationScript})
+	if err != nil {
+		return errors.New("Error running pg_dump on remote database: " + err.Error())
+	}
+
+	if len(path) > 0 {
+		if err := afero.WriteFile(fsys, path, []byte(out), 0644); err != nil {
+			return err
+		}
+		fmt.Fprintln(os.Stderr, "Dumped schema to "+utils.Bold(path)+".")
+	} else {
+		fmt.Println(out)
+	}
+
+	return nil
+}
diff --git a/internal/db/dump/templates/dump_initial_migration.sh b/internal/db/dump/templates/dump_initial_migration.sh
new file mode 100755
index 00000000..0bcc42a5
--- /dev/null
+++ b/internal/db/dump/templates/dump_initial_migration.sh
@@ -0,0 +1,23 @@
+#!/usr/bin/env bash
+set -euo pipefail
+
+# Explanation of special flags:
+#
+#   --schema-only     omit data like migration history, pgsodium key, etc.
+#   --exclude-schema  omit internal schemas as they are maintained by platform
+#   --no-comments     only object owner can set comment, omit to allow restore by non-superuser
+#   --extension '*'   prevents event triggers from being dumped, bash escaped with single quote
+pg_dump \
+    --schema-only \
+    --quote-all-identifier \
+    --exclude-schema "$EXCLUDED_SCHEMAS" \
+    --extension '*' \
+    --no-comments \
+    --dbname "$DB_URL" \
+| sed 's/ALTER DEFAULT PRIVILEGES FOR ROLE "supabase_admin"/-- ALTER DEFAULT PRIVILEGES FOR ROLE "supabase_admin"/' \
+| sed 's/GRANT ALL ON FUNCTION "graphql_public"/-- GRANT ALL ON FUNCTION "graphql_public"/' \
+| sed 's/GRANT ALL ON FUNCTION "graphql"/-- GRANT ALL ON FUNCTION "graphql"/' \
+| sed 's/CREATE POLICY "cron_job_/-- CREATE POLICY "cron_job_/'
+
+# Reset session config generated by pg_dump
+echo "RESET ALL;"
diff --git a/internal/migration/new/new.go b/internal/migration/new/new.go
index 32f2b713..97f85581 100644
--- a/internal/migration/new/new.go
+++ b/internal/migration/new/new.go
@@ -11,11 +11,10 @@ import (
 )
 
 func Run(migrationName string, stdin afero.File, fsys afero.Fs) error {
-	if err := utils.MkdirIfNotExistFS(fsys, utils.MigrationsDir); err != nil {
+	path := GetMigrationPath(migrationName)
+	if err := utils.MkdirIfNotExistFS(fsys, filepath.Dir(path)); err != nil {
 		return err
 	}
-
-	path := GetMigrationPath(migrationName)
 	f, err := fsys.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644)
 	if err != nil {
 		return err


Test patch:
diff --git a/internal/db/dump/dump_test.go b/internal/db/dump/dump_test.go
new file mode 100644
index 00000000..7729784b
--- /dev/null
+++ b/internal/db/dump/dump_test.go
@@ -0,0 +1,91 @@
+package dump
+
+import (
+	"context"
+	"net/http"
+	"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/utils"
+	"gopkg.in/h2non/gock.v1"
+)
+
+const (
+	username = "admin"
+	password = "password"
+	database = "postgres"
+	host     = "localhost"
+)
+
+func TestPullCommand(t *testing.T) {
+	imageUrl := utils.GetRegistryImageUrl(utils.Pg15Image)
+	const containerId = "test-container"
+
+	t.Run("pulls from remote", func(t *testing.T) {
+		// Setup in-memory fs
+		fsys := afero.NewMemMapFs()
+		// Setup mock docker
+		require.NoError(t, apitest.MockDocker(utils.Docker))
+		defer gock.OffAll()
+		apitest.MockDockerStart(utils.Docker, imageUrl, containerId)
+		require.NoError(t, apitest.MockDockerLogs(utils.Docker, containerId, "hello world"))
+		// Run test
+		err := Run(context.Background(), "schema.sql", username, password, database, host, fsys)
+		// Check error
+		assert.NoError(t, err)
+		assert.Empty(t, apitest.ListUnmatchedRequests())
+		// Validate migration
+		contents, err := afero.ReadFile(fsys, "schema.sql")
+		assert.NoError(t, err)
+		assert.Equal(t, []byte("hello world"), contents)
+	})
+
+	t.Run("writes to stdout", func(t *testing.T) {
+		// Setup in-memory fs
+		fsys := afero.NewMemMapFs()
+		// Setup mock docker
+		require.NoError(t, apitest.MockDocker(utils.Docker))
+		defer gock.OffAll()
+		apitest.MockDockerStart(utils.Docker, imageUrl, containerId)
+		require.NoError(t, apitest.MockDockerLogs(utils.Docker, containerId, "hello world"))
+		// Run test
+		err := Run(context.Background(), "", username, password, database, host, fsys)
+		// Check error
+		assert.NoError(t, err)
+		assert.Empty(t, apitest.ListUnmatchedRequests())
+	})
+
+	t.Run("throws error on missing docker", func(t *testing.T) {
+		// Setup in-memory fs
+		fsys := afero.NewMemMapFs()
+		// Setup mock docker
+		require.NoError(t, apitest.MockDocker(utils.Docker))
+		defer gock.OffAll()
+		gock.New(utils.Docker.DaemonHost()).
+			Get("/v" + utils.Docker.ClientVersion() + "/images").
+			Reply(http.StatusServiceUnavailable)
+		// Run test
+		err := Run(context.Background(), "", username, password, database, host, fsys)
+		// Check error
+		assert.ErrorContains(t, err, "Error running pg_dump on remote database: request returned Service Unavailable")
+		assert.Empty(t, apitest.ListUnmatchedRequests())
+	})
+
+	t.Run("throws error on permission denied", func(t *testing.T) {
+		// Setup in-memory fs
+		fsys := afero.NewReadOnlyFs(afero.NewMemMapFs())
+		// Setup mock docker
+		require.NoError(t, apitest.MockDocker(utils.Docker))
+		defer gock.OffAll()
+		apitest.MockDockerStart(utils.Docker, imageUrl, containerId)
+		require.NoError(t, apitest.MockDockerLogs(utils.Docker, containerId, "hello world"))
+		// Run test
+		err := Run(context.Background(), "schema.sql", username, password, database, host, fsys)
+		// Check error
+		assert.ErrorContains(t, err, "operation not permitted")
+		assert.Empty(t, apitest.ListUnmatchedRequests())
+	})
+}
diff --git a/internal/migration/new/new_test.go b/internal/migration/new/new_test.go
index 2e13b524..39e2fe11 100644
--- a/internal/migration/new/new_test.go
+++ b/internal/migration/new/new_test.go
@@ -3,7 +3,6 @@ package new
 import (
 	"os"
 	"path/filepath"
-	"regexp"
 	"testing"
 
 	"github.com/spf13/afero"
@@ -24,9 +23,8 @@ func TestNewCommand(t *testing.T) {
 		// Validate output
 		files, err := afero.ReadDir(fsys, utils.MigrationsDir)
 		assert.NoError(t, err)
-		match, err := regexp.MatchString(`([0-9]{14})_test_migrate\.sql`, files[0].Name())
-		assert.NoError(t, err)
-		assert.True(t, match)
+		assert.Equal(t, 1, len(files))
+		assert.Regexp(t, `([0-9]{14})_test_migrate\.sql`, files[0].Name())
 	})
 
 	t.Run("streams content from pipe", func(t *testing.T) {
@@ -44,6 +42,7 @@ func TestNewCommand(t *testing.T) {
 		// Validate output
 		files, err := afero.ReadDir(fsys, utils.MigrationsDir)
 		assert.NoError(t, err)
+		assert.Equal(t, 1, len(files))
 		path := filepath.Join(utils.MigrationsDir, files[0].Name())
 		contents, err := afero.ReadFile(fsys, path)
 		assert.NoError(t, err)