dolthub/dolt#10136: Fix dolt_backup to work in non-Dolt directories

go/cmd/dolt/commands/backup.go

@@ -29,6 +29,8 @@ import (
 	eventsapi "github.com/dolthub/eventsapi_schema/dolt/services/eventsapi/v1alpha1"
 )
 
+const DoltBackupCommandName = "backup"
+
 var backupDocs = cli.CommandDocumentationContent{
 	ShortDesc: "Manage database backups, including creation, sync, and restore.",
 	LongDesc: `

go/cmd/dolt/dolt.go

@@ -680,8 +680,14 @@ If you're interested in running this command against a remote host, hit us up on
 	//
 	// This is also allowed when --help is passed. So we defer the error
 	// until the caller tries to use the cli.LateBindQueryist.
-	isValidRepositoryRequired := subcommandName != "init" && subcommandName != "sql" && subcommandName != "sql-server" && subcommandName != "sql-client"
-
+	commandsNotRequiringRepo := map[string]bool{
+		"init":                         true,
+		"sql":                          true,
+		"sql-server":                   true,
+		"sql-client":                   true,
+		commands.DoltBackupCommandName: true,
+	}
+	isValidRepositoryRequired := !commandsNotRequiringRepo[subcommandName]
 	if noValidRepository && isValidRepositoryRequired {
 		return func(ctx context.Context, opts ...cli.LateBindQueryistOption) (res cli.LateBindQueryistResult, err error) {
 			err = errors.New("The current directory is not a valid dolt repository.")

go/libraries/doltcore/sqle/dprocedures/dolt_backup.go

@@ -31,6 +31,7 @@ import (
 	"github.com/dolthub/dolt/go/libraries/doltcore/sqlserver"
 	"github.com/dolthub/dolt/go/libraries/utils/argparser"
 	"github.com/dolthub/dolt/go/store/datas/pull"
+	"github.com/dolthub/dolt/go/store/types"
 )
 
 const (
@@ -55,17 +56,25 @@ var awsParamsUsage = []string{
 // based on the first argument. The procedure requires superuser privileges and write access to the current database.
 // Supported operations are: add, remove/rm, sync, sync-url, and restore.
 func doltBackup(ctx *sql.Context, args ...string) (sql.RowIter, error) {
-	dbName := ctx.GetCurrentDatabase()
-	if dbName == "" {
-		return nil, fmt.Errorf("empty database name")
-	}
-
-	err := branch_control.CheckAccess(ctx, branch_control.Permissions_Write)
+	apr, err := cli.CreateBackupArgParser().Parse(args)
 	if err != nil {
 		return nil, err
 	}
 
-	apr, err := cli.CreateBackupArgParser().Parse(args)
+	if apr.NArg() == 0 || (apr.NArg() == 1 && apr.Contains(cli.VerboseFlag)) {
+		return nil, fmt.Errorf("use '%s' table to list backups", doltdb.BackupsTableName)
+	}
+
+	var dbName string
+	funcParam := apr.Arg(0)
+	if funcParam != DoltBackupParamRestore {
+		dbName = ctx.GetCurrentDatabase()
+		if dbName == "" {
+			return nil, fmt.Errorf("empty database name")
+		}
+	}
+
+	err = branch_control.CheckAccess(ctx, branch_control.Permissions_Write)
 	if err != nil {
 		return nil, err
 	}
@@ -82,17 +91,12 @@ func doltBackup(ctx *sql.Context, args ...string) (sql.RowIter, error) {
 		}
 	}
 
-	if apr.NArg() == 0 || (apr.NArg() == 1 && apr.Contains(cli.VerboseFlag)) {
-		return nil, fmt.Errorf("use '%s' table to list backups", doltdb.BackupsTableName)
-	}
-
 	doltSess := dsess.DSessFromSess(ctx.Session)
 	dbData, ok := doltSess.GetDbData(ctx, dbName)
-	if !ok {
+	if !ok && funcParam != DoltBackupParamRestore {
 		return nil, sql.ErrDatabaseNotFound.New(dbName)
 	}
 
-	funcParam := apr.Arg(0)
 	switch funcParam {
 	case DoltBackupParamAdd:
 		if apr.NArg() != 3 {
@@ -225,7 +229,14 @@ func doltBackupRestore(ctx *sql.Context, dbData env.DbData[*sql.Context], dsess
 	}
 
 	remote := env.NewRemote(DoltBackupParamRestore, remoteUrl, remoteParams)
-	remoteDb, err := dsess.Provider().GetRemoteDB(ctx, dbData.Ddb.Format(), remote, true)
+
+	// Use default format if no database context is available (e.g., when run from invalid directory).
+	format := types.Format_Default
+	if dbData.Ddb != nil {
+		format = dbData.Ddb.Format()
+	}
+
+	remoteDb, err := dsess.Provider().GetRemoteDB(ctx, format, remote, true)
 	if err != nil {
 		return err
 	}
@@ -247,7 +258,7 @@ func doltBackupRestore(ctx *sql.Context, dbData env.DbData[*sql.Context], dsess
 		}
 	}
 
-	if lookupDbInFileSys {
+	if lookupDbInFileSys && !hasLookupDb {
 		err = fileSys.Delete(lookupDbName, forceRestore)
 		if err != nil {
 			return err

integration-tests/bats/backup.bats

@@ -1,4 +1,10 @@
 #!/usr/bin/env bats
+# Since the dolt backup command uses dolt_backup procedure internally, `sql-backup.bats` tests should generally apply to
+# `backup.bats` as well, removing the need to duplicate tests. As a result, prefer writing backup tests in
+# `sql-backup.bats`. Exceptions to this rule is testing external factors related to commands (i.e. invalid repo checks
+#  before calling dolt_backup) or command specific functionality (i.e. the command version can list current database
+#  backup table).
+
 load $BATS_TEST_DIRNAME/helper/common.bash
 load $BATS_TEST_DIRNAME/helper/remotesrv-common.bash
 load $BATS_TEST_DIRNAME/helper/query-server-common.bash
@@ -337,7 +343,7 @@ teardown() {
 # No HTTPS test for sync-url; `dolt/go/utils/remotesrv` does not expose TLS configuration flags.
 @test "backup: sync-url and restore HTTP" {
     start_remotesrv
-    
+
     cd repo1
     dolt sql -q "CREATE TABLE t2 (b int)"
     dolt add .
@@ -378,3 +384,21 @@ teardown() {
 
     stop_sql_server
 }
+
+@test "backup: restore works from non-dolt directory" {
+    cd repo1
+    backupFileUrl="file://$BATS_TEST_TMPDIR/backup"
+    dolt sql<<EOF
+create table t (i int);
+insert into t values (1), (2);
+call dolt_backup('sync-url', '$backupFileUrl');
+EOF
+
+    invalidRepo="$BATS_TEST_TMPDIR/invalidRepo"
+    mkdir "$invalidRepo" && cd "$invalidRepo"
+
+    run dolt backup restore "$backupFileUrl" new_db
+    [ "$status" -eq 0 ]
+    run dolt sql -r csv -q "select * from t"
+    [[ "$output" =~ i.*2.*1 ]] || false
+}

integration-tests/bats/helper/local-remote.bash

@@ -68,7 +68,6 @@ SKIP_SERVER_TESTS=$(cat <<-EOM
 ~arg-parsing.bats~
 ~dump.bats~
 ~rename-tables.bats~
-~sql-backup.bats~
 ~drop-create.bats~
 ~constraint-violations.bats~
 ~branch-control.bats~

integration-tests/bats/sql-backup.bats

@@ -7,15 +7,16 @@ setup() {
 }
 
 teardown() {
-    stop_sql_server
     teardown_common
 }
 
 @test "sql-backup: dolt_backup no argument" {
     run dolt sql -q "call dolt_backup()"
     [ "$status" -ne 0 ]
+    [[ "$output" =~ "use 'dolt_backups' table to list backups" ]] || false
     run dolt sql -q "CALL dolt_backup()"
     [ "$status" -ne 0 ]
+    [[ "$output" =~ "use 'dolt_backups' table to list backups" ]] || false
 }
 
 @test "sql-backup: dolt_backup add" {
@@ -28,16 +29,16 @@ teardown() {
 }
 
 @test "sql-backup: dolt_backup add cannot add remote with address of existing backup" {
-    mkdir bac1
-    dolt sql -q "call dolt_backup('add','bac1','file://./bac1')"
-    run dolt sql -q "call dolt_backup('add','rem1','file://./bac1')"
+    backupFileUrl="file://$BATS_TEST_TMPDIR/backup"
+    dolt sql -q "call dolt_backup('add','bac1', '$backupFileUrl')"
+    run dolt sql -q "call dolt_backup('add','rem1', '$backupFileUrl')"
     [ "$status" -eq 1 ]
     [[ "$output" =~ "address conflict with a remote: 'bac1'" ]] || false
 }
 
 @test "sql-backup: dolt_backup remove" {
-    mkdir bac1
-    dolt sql -q "call dolt_backup('add', 'bac1', 'file://./bac1')"
+    backupFileUrl="file://$BATS_TEST_TMPDIR/backup"
+    dolt sql -q "call dolt_backup('add', 'bac1', '$backupFileUrl')"
     run dolt backup -v
     [ "$status" -eq 0 ]
     [ "${#lines[@]}" -eq 1 ]
@@ -61,8 +62,8 @@ teardown() {
 }
 
 @test "sql-backup: dolt_backup rm" {
-    mkdir bac1
-    dolt sql -q "call dolt_backup('add', 'bac1', 'file://./bac1')"
+    backupFileUrl="files://$BATS_TEST_TMPDIR/backup"
+    dolt sql -q "call dolt_backup('add', 'bac1', '$backupFileUrl')"
     run dolt backup -v
     [ "$status" -eq 0 ]
     [ "${#lines[@]}" -eq 1 ]
@@ -93,18 +94,18 @@ teardown() {
 }
 
 @test "sql-backup: dolt_backup restore" {
-    backupsDir="$PWD/backups"
+    backupFileUrl="file://$BATS_TEST_TMPDIR/backups"
     mkdir backupsDir
 
     # Created a nested database, back it up, drop it, then restore it with a new name
     dolt sql -q "create database db1;"
     cd db1
     dolt sql -q "create table t1 (pk int primary key); insert into t1 values (42); call dolt_commit('-Am', 'creating table t1');"
-    dolt sql -q "call dolt_backup('add', 'backups', 'file://$backupsDir');"
+    dolt sql -q "call dolt_backup('add', 'backups', '$backupFileUrl');"
     dolt sql -q "call dolt_backup('sync', 'backups');"
     cd ..
     dolt sql -q "drop database db1;"
-    dolt sql -q "call dolt_backup('restore', 'file://$backupsDir', 'db2');"
+    dolt sql -q "call dolt_backup('restore', '$backupFileUrl', 'db2');"
 
     # Assert that db2 is present, and db1 is not
     run dolt sql -q "show databases;"
@@ -124,33 +125,53 @@ teardown() {
     [ "${#lines[@]}" -eq 0 ]
 }
 
-@test "sql-backup: dolt_backup restore --force" {
-    backupsDir="$PWD/backups"
-    mkdir backupsDir
+@test "sql-backup: dolt_backup restore --force on current database" {
+    backupFileUrl="file://$BATS_TEST_TMPDIR/backups"
 
     # Created a nested database, and back it up
     dolt sql -q "create database db1;"
-    cd db1
-    dolt sql -q "create table t1 (pk int primary key); insert into t1 values (42); call dolt_commit('-Am', 'creating table t1');"
-    dolt sql -q "call dolt_backup('add', 'backups', 'file://$backupsDir');"
-    dolt sql -q "call dolt_backup('sync', 'backups');"
+    # We could cd into db1 but Windows does not like us touching its CWD when we drop the database when restoring.
+    dolt sql -q "use db1; create table t1 (pk int primary key); insert into t1 values (42); call dolt_commit('-Am', 'creating table t1');"
+    dolt sql -q "use db1; call dolt_backup('add', 'backups', '$backupFileUrl');"
+    dolt sql -q "use db1; call dolt_backup('sync', 'backups');"
 
     # Make a new commit in db1, but don't push it to the backup
-    dolt sql -q "update t1 set pk=100; call dolt_commit('-Am', 'updating table t1');"
+    dolt sql -q "use db1; update t1 set pk=100; call dolt_commit('-Am', 'updating table t1');"
 
     # Assert that without --force, we can't update an existing db from a backup
-    run dolt sql -q "call dolt_backup('restore', 'file://$backupsDir', 'db1');"
+    run dolt sql -q "use db1; call dolt_backup('restore', '$backupFileUrl', 'db1');"
     [ "$status" -eq 1 ]
     [[ "$output" =~ "database 'db1' already exists, use '--force' to overwrite" ]] || false
 
     # Use --force to overwrite the existing database and sanity check the data
-    run dolt sql -q "call dolt_backup('restore', '--force', 'file://$backupsDir', 'db1');"
+    run dolt sql -q "use db1; call dolt_backup('restore', '--force', '$backupFileUrl', 'db1');"
     [ "$status" -eq 0 ]
     run dolt sql -q "use db1; select * from t1;"
     [ "$status" -eq 0 ]
     [[ "$output" =~ "42" ]] || false
 }
 
+@test "sql-backup: dolt_backup restore --force on current database and as cwd" {
+    skiponwindows "Windows storage system locks the terminal cwd when trying to drop database in restore procedure; this includes mounted storage in WSL"
+    backupFileUrl="file://$BATS_TEST_TMPDIR/backup"
+    dolt sql -q "create database db1;"
+    cd db1
+    dolt sql <<EOF
+create table t (i int);
+insert into t values (3), (4);
+call dolt_backup('sync-url', '$backupFileUrl');
+EOF
+
+    run dolt sql -q "call dolt_backup('restore', '$backupFileUrl', 'db1');"
+    [ "$status" -eq 1 ]
+    [[ "$output" =~ "database 'db1' already exists, use '--force' to overwrite" ]] || false
+
+    run dolt sql -q "call dolt_backup('restore', '--force', '$backupFileUrl', 'db1');"
+    [ "$status" -eq 0 ]
+    run dolt sql -q "select * from t;"
+    [[ "$output" =~ i.*3.*4 ]] || false
+}
+
 @test "sql-backup: dolt_backup unrecognized" {
     run dolt sql -q "call dolt_backup('unregonized', 'hostedapidb-0', 'file:///some_directory')"
     [ "$status" -ne 0 ]
@@ -177,37 +198,31 @@ teardown() {
 }
 
 @test "sql-backup: dolt_backup sync to a backup" {
-    mkdir the_backup
-    dolt backup add hostedapidb-0 file://./the_backup
+    backupFileUrl="file://$BATS_TEST_TMPDIR/the_backup"
+    dolt backup add hostedapidb-0 "$backupFileUrl"
     dolt backup -v
     dolt sql -q "call dolt_backup('sync', 'hostedapidb-0')"
     # Initial backup works.
-    dolt backup restore file://./the_backup the_restore
+    dolt backup restore "$backupFileUrl" the_restore
     (cd the_restore && dolt status)
     # Backup with nothing to push works.
     dolt sql -q "call dolt_backup('sync', 'hostedapidb-0')"
 
-    rm -rf the_backup the_restore
-
-    mkdir the_backup
     dolt sql -q "CALL dolt_backup('sync', 'hostedapidb-0')"
-    dolt backup restore file://./the_backup the_restore
+    dolt backup restore "$backupFileUrl" the_restore --force
     (cd the_restore && dolt status)
     dolt sql -q "CALL dolt_backup('sync', 'hostedapidb-0')"
 }
 
 @test "sql-backup: dolt_backup sync-url" {
-    mkdir the_backup
-    dolt sql -q "call dolt_backup('sync-url', 'file://./the_backup')"
+    backupFileUrl="file://$BATS_TEST_TMPDIR/the_backup"
+    dolt sql -q "call dolt_backup('sync-url', '$backupFileUrl')"
     # Initial backup works.
-    dolt backup restore file://./the_backup the_restore
+    dolt backup restore "$backupFileUrl" the_restore
     (cd the_restore && dolt status)
 
-    rm -rf the_backup the_restore
-
-    mkdir the_backup
-    dolt sql -q "CALL dolt_backup('sync-url', 'file://./the_backup')"
-    dolt backup restore file://./the_backup the_restore
+    dolt sql -q "CALL dolt_backup('sync-url', '$backupFileUrl')"
+    dolt backup restore "$backupFileUrl" the_restore --force
     (cd the_restore && dolt status)
 }
 
@@ -219,6 +234,7 @@ teardown() {
 }
 
 @test "sql-backup: dolt_backup rejects AWS parameters fails in sql-server" {
+    skip_if_remote
     start_sql_server
 
     run dolt sql -q "call dolt_backup('add', 'backup1', 'aws://[table:bucket]/db', '--aws-region=us-east-1')"
@@ -236,4 +252,27 @@ teardown() {
     run dolt sql -q "call dolt_backup('add', 'backup4', 'aws://[table:bucket]/db', '--aws-creds-profile=profile')"
     [ "$status" -eq 1 ]
     [[ "$output" =~ "AWS parameters are unavailable when running in server mode" ]] || false
+
+    stop_sql_server 1
+}
+
+@test "sql-backup: dolt_backup works in invalid dolt repository" {
+    backupFileUrl="file://$BATS_TEST_TMPDIR/t_backup"
+    run dolt sql -q "create table t (i int);"
+    [ "$status" -eq 0 ]
+    run dolt sql -q "insert into t values (4), (3);"
+    [ "$status" -eq 0 ]
+    run dolt sql -q "call dolt_backup('sync-url', '$backupFileUrl')"
+    [ "$status" -eq 0 ]
+
+    invalidRepoDir="$BATS_TEST_TMPDIR/invalid_repo"
+    mkdir -p "$invalidRepoDir"
+    cd $invalidRepoDir
+    dolt sql -q "call dolt_backup('restore', '$backupFileUrl', 't_db')"
+    [ "$status" -eq 0 ]
+
+    cd t_db
+    run dolt sql --result-format csv -q "select * from t"
+    [ "$status" -eq 0 ]
+    [[ "$output" =~ i.*3.*4 ]] || false
 }