-
Notifications
You must be signed in to change notification settings - Fork 500
fix(cli): repair remote db pull with pg-delta and per-connection role step-down #5895
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
base: develop
Are you sure you want to change the base?
Changes from all commits
6ffa721
4407659
70f1717
797c43b
555063e
d72d355
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,7 @@ func Run(ctx context.Context, schema []string, file string, config pgconn.Config | |
| out := result.SQL | ||
| branch := utils.GetGitBranch(fsys) | ||
| fmt.Fprintln(os.Stderr, "Finished "+utils.Aqua("supabase db diff")+" on branch "+utils.Aqua(branch)+".\n") | ||
| if err := SaveDiff(out, file, fsys); err != nil { | ||
| if err := SaveDiff(result, file, fsys); err != nil { | ||
| return err | ||
| } | ||
| drops := findDropStatements(out) | ||
|
|
@@ -225,22 +225,31 @@ func DiffDatabase(ctx context.Context, schema []string, config pgconn.Config, w | |
| } else { | ||
| fmt.Fprintln(w, "Diffing schemas...") | ||
| } | ||
| if IsPgDeltaDebugEnabled() && usePgDelta { | ||
| // Capture the shadow baseline catalog and edge-runtime stderr so an | ||
| // empty diff can be inspected later. DiffPgDeltaRefDetailed mirrors the | ||
| // pg-delta differ but additionally surfaces stderr, which differ() drops. | ||
| debugCapture := &PgDeltaDebugCapture{} | ||
| if snapshot, exportErr := exportCatalogPgDelta(ctx, utils.ToPostgresURL(shadowConfig), "postgres", options...); exportErr == nil { | ||
| debugCapture.SourceCatalog = snapshot | ||
| } else { | ||
| fmt.Fprintf(w, "Warning: failed to export shadow pg-delta catalog: %v\n", exportErr) | ||
| if usePgDelta { | ||
| // pg-delta always goes through the diffPgDeltaRefDetailed seam so callers get | ||
| // the execution-aware per-unit files (db pull writes one migration file each); | ||
| // db diff/declarative flatten them back via SQL. This mirrors the config-based | ||
| // differ (DiffPgDelta) exactly, so it is safe to bypass the injected differ() | ||
| // here — differ() remains the migra engine path below. | ||
| var debugCapture *PgDeltaDebugCapture | ||
| if IsPgDeltaDebugEnabled() { | ||
| // Capture the shadow baseline catalog and edge-runtime stderr so an | ||
| // empty diff can be inspected later. | ||
| debugCapture = &PgDeltaDebugCapture{} | ||
| if snapshot, exportErr := exportCatalogPgDelta(ctx, utils.ToPostgresURL(shadowConfig), "postgres", options...); exportErr == nil { | ||
| debugCapture.SourceCatalog = snapshot | ||
| } else { | ||
| fmt.Fprintf(w, "Warning: failed to export shadow pg-delta catalog: %v\n", exportErr) | ||
| } | ||
| } | ||
| result, err := DiffPgDeltaRefDetailed(ctx, utils.ToPostgresURL(shadowConfig), utils.ToPostgresURL(config), schema, pgDeltaFormatOptions(), options...) | ||
| result, err := diffPgDeltaRefDetailed(ctx, utils.ToPostgresURL(shadowConfig), utils.ToPostgresURL(config), schema, pgDeltaFormatOptions(), options...) | ||
| if err != nil { | ||
| return DatabaseDiff{}, err | ||
| } | ||
| debugCapture.Stderr = result.Stderr | ||
| return DatabaseDiff{SQL: result.SQL, Debug: debugCapture}, nil | ||
| if debugCapture != nil { | ||
| debugCapture.Stderr = result.Stderr | ||
| } | ||
| return DatabaseDiff{SQL: joinPgDeltaFiles(result.Files), Files: result.Files, Debug: debugCapture}, nil | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When pg-delta produces multiple execution units, such as Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 555063e: |
||
| } | ||
| output, err := differ(ctx, shadowConfig, config, schema, options...) | ||
| if err != nil { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| package diff | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/spf13/afero" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| "github.com/supabase/cli/internal/utils" | ||
| ) | ||
|
|
||
| func TestSaveDiff(t *testing.T) { | ||
| t.Run("reports no changes on empty diff", func(t *testing.T) { | ||
| fsys := afero.NewMemMapFs() | ||
| require.NoError(t, SaveDiff(DatabaseDiff{SQL: ""}, "my_diff", fsys)) | ||
| // Nothing written when there are no schema changes. | ||
| entries, err := afero.ReadDir(fsys, utils.MigrationsDir) | ||
| assert.Error(t, err) | ||
| assert.Empty(t, entries) | ||
| }) | ||
|
|
||
| t.Run("writes a single migration file for a single-unit plan", func(t *testing.T) { | ||
| fsys := afero.NewMemMapFs() | ||
| files := []PgDeltaPlanFile{ | ||
| {Order: 1, Name: "schema_changes", TransactionMode: "transactional", SQL: "create table a ();"}, | ||
| } | ||
| result := DatabaseDiff{SQL: joinPgDeltaFiles(files), Files: files} | ||
| require.NoError(t, SaveDiff(result, "my_diff", fsys)) | ||
| entries, err := afero.ReadDir(fsys, utils.MigrationsDir) | ||
| require.NoError(t, err) | ||
| require.Len(t, entries, 1) | ||
| // A single-unit plan keeps the plain `<ts>_<name>.sql` name and the exact | ||
| // diff SQL, byte-identical to the pre-multi-file behavior (no trailing newline | ||
| // added, no unit-name suffix). | ||
| assert.Regexp(t, `^\d{14}_my_diff\.sql$`, entries[0].Name()) | ||
| contents, err := afero.ReadFile(fsys, utils.MigrationsDir+"/"+entries[0].Name()) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "create table a ();", string(contents)) | ||
| }) | ||
|
|
||
| t.Run("writes one migration file per unit for a multi-unit plan", func(t *testing.T) { | ||
| fsys := afero.NewMemMapFs() | ||
| files := []PgDeltaPlanFile{ | ||
| {Order: 1, Name: "schema_changes", TransactionMode: "transactional", SQL: "alter type mood add value 'ok';"}, | ||
| {Order: 2, Name: "after_enum_values", TransactionMode: "transactional", SQL: "insert into t values ('ok');"}, | ||
| } | ||
| result := DatabaseDiff{SQL: joinPgDeltaFiles(files), Files: files} | ||
| require.NoError(t, SaveDiff(result, "my_diff", fsys)) | ||
| entries, err := afero.ReadDir(fsys, utils.MigrationsDir) | ||
| require.NoError(t, err) | ||
| require.Len(t, entries, 2) | ||
| // Multi-unit plans split into one ordered file per unit, each suffixed with the | ||
| // unit name, so `db push`/`reset` applies each unit as its own transaction. | ||
| assert.Regexp(t, `^\d{14}_my_diff_schema_changes\.sql$`, entries[0].Name()) | ||
| assert.Regexp(t, `^\d{14}_my_diff_after_enum_values\.sql$`, entries[1].Name()) | ||
| }) | ||
|
|
||
| t.Run("prints diff to stdout when no file is given", func(t *testing.T) { | ||
| fsys := afero.NewMemMapFs() | ||
| require.NoError(t, SaveDiff(DatabaseDiff{SQL: "create table a ();"}, "", fsys)) | ||
| entries, _ := afero.ReadDir(fsys, utils.MigrationsDir) | ||
| assert.Empty(t, entries) | ||
| }) | ||
|
|
||
| t.Run("creates nested parent directories for a nested single-unit name", func(t *testing.T) { | ||
| fsys := afero.NewMemMapFs() | ||
| require.NoError(t, SaveDiff(DatabaseDiff{SQL: "create table a ();"}, "snapshots/remote", fsys)) | ||
| matches, err := afero.Glob(fsys, utils.MigrationsDir+"/*_snapshots/remote.sql") | ||
| require.NoError(t, err) | ||
| require.Len(t, matches, 1) | ||
| contents, err := afero.ReadFile(fsys, matches[0]) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "create table a ();", string(contents)) | ||
| }) | ||
|
|
||
| t.Run("creates nested parent directories for a nested multi-unit name", func(t *testing.T) { | ||
| fsys := afero.NewMemMapFs() | ||
| files := []PgDeltaPlanFile{ | ||
| {Order: 1, Name: "schema_changes", TransactionMode: "transactional", SQL: "alter type mood add value 'ok';"}, | ||
| {Order: 2, Name: "after_enum_values", TransactionMode: "transactional", SQL: "insert into t values ('ok');"}, | ||
| } | ||
| result := DatabaseDiff{SQL: joinPgDeltaFiles(files), Files: files} | ||
| require.NoError(t, SaveDiff(result, "snapshots/remote", fsys)) | ||
| matches, err := afero.Glob(fsys, utils.MigrationsDir+"/*_snapshots/remote_*.sql") | ||
| require.NoError(t, err) | ||
| require.Len(t, matches, 2) | ||
| }) | ||
| } |
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.
When a declarative delta needs multiple pg-delta units, such as adding an enum value and then using it,
DiffDeclarativeToMigrationshas already flattened those units intoDiffSQL, and constructingDatabaseDiffwith onlySQLhere preventsSaveDifffrom using the new per-unit writer. The active smart-sync paths have the same loss (cmd/db_schema_declarative.gowritesresult.DiffSQL, while the TS sync handler writesresult.diffSQL), sodb schema declarative syncstill creates and optionally applies one transactionally wrapped migration that can fail at the required commit boundary. Carry the plan files through the sync result and write/apply them as separate migrations.Useful? React with 👍 / 👎.
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 one is deliberately out of scope for this PR, as noted in the earlier replies on the flatten/units threads: declarative sync is experimental surface and carrying the plan units through
DiffDeclarativeToMigrations/SyncResult(both CLIs, plus the apply step) is a self-contained change we're tracking as a follow-up rather than growing this PR further. The per-unit writer landed here (diff.WritePgDeltaMigrations) is what that follow-up will reuse.