From 0e1bbfc9f0c074c2ece97d9aa31df2f502cd3287 Mon Sep 17 00:00:00 2001
From: Aaron Son <aaron@dolthub.com>
Date: Wed, 6 Dec 2023 09:03:32 -0800
Subject: [PATCH] go/libraries/doltcore/sqle: DatabaseProvider: Fix a bug where
 a database created with call dolt_clone() would not be dropped until the
 server was restarted.

Fixes #7106.
---
 .../doltcore/sqle/database_provider.go        | 42 ++++---------------
 integration-tests/bats/clone-drop.bats        | 31 ++++++++++++++
 .../bats/helper/local-remote.bash             |  1 +
 3 files changed, 40 insertions(+), 34 deletions(-)
 create mode 100644 integration-tests/bats/clone-drop.bats

diff --git a/go/libraries/doltcore/sqle/database_provider.go b/go/libraries/doltcore/sqle/database_provider.go
index 41135786009..3a83c4ba4b7 100644
--- a/go/libraries/doltcore/sqle/database_provider.go
+++ b/go/libraries/doltcore/sqle/database_provider.go
@@ -485,7 +485,7 @@ func (p *DoltDatabaseProvider) CloneDatabaseFromRemote(
 		return fmt.Errorf("cannot create DB, file exists at %s", dbName)
 	}
 
-	dEnv, err := p.cloneDatabaseFromRemote(ctx, dbName, remoteName, branch, remoteUrl, remoteParams)
+	err := p.cloneDatabaseFromRemote(ctx, dbName, remoteName, branch, remoteUrl, remoteParams)
 	if err != nil {
 		// Make a best effort to clean up any artifacts on disk from a failed clone
 		// before we return the error
@@ -499,7 +499,7 @@ func (p *DoltDatabaseProvider) CloneDatabaseFromRemote(
 		return err
 	}
 
-	return ConfigureReplicationDatabaseHook(ctx, p, dbName, dEnv)
+	return nil
 }
 
 // cloneDatabaseFromRemote encapsulates the inner logic for cloning a database so that if any error
@@ -510,26 +510,26 @@ func (p *DoltDatabaseProvider) cloneDatabaseFromRemote(
 	ctx *sql.Context,
 	dbName, remoteName, branch, remoteUrl string,
 	remoteParams map[string]string,
-) (*env.DoltEnv, error) {
+) error {
 	if p.remoteDialer == nil {
-		return nil, fmt.Errorf("unable to clone remote database; no remote dialer configured")
+		return fmt.Errorf("unable to clone remote database; no remote dialer configured")
 	}
 
 	// TODO: params for AWS, others that need them
 	r := env.NewRemote(remoteName, remoteUrl, nil)
 	srcDB, err := r.GetRemoteDB(ctx, types.Format_Default, p.remoteDialer)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	dEnv, err := actions.EnvForClone(ctx, srcDB.ValueReadWriter().Format(), r, dbName, p.fs, "VERSION", env.GetCurrentUserHomeDir)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	err = actions.CloneRemote(ctx, srcDB, remoteName, branch, dEnv)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	err = dEnv.RepoStateWriter().UpdateBranch(dEnv.RepoState.CWBHeadRef().GetPath(), env.BranchConfig{
@@ -537,33 +537,7 @@ func (p *DoltDatabaseProvider) cloneDatabaseFromRemote(
 		Remote: remoteName,
 	})
 
-	fkChecks, err := ctx.GetSessionVariable(ctx, "foreign_key_checks")
-	if err != nil {
-		return nil, err
-	}
-
-	opts := editor.Options{
-		Deaf: dEnv.DbEaFactory(),
-		// TODO: this doesn't seem right, why is this getting set in the constructor to the DB
-		ForeignKeyChecksDisabled: fkChecks.(int8) == 0,
-	}
-
-	db, err := NewDatabase(ctx, dbName, dEnv.DbData(), opts)
-	if err != nil {
-		return nil, err
-	}
-
-	// If we have an initialization hook, invoke it.  By default, this will
-	// be ConfigureReplicationDatabaseHook, which will setup replication
-	// for the new database if a remote url template is set.
-	err = p.InitDatabaseHook(ctx, p, dbName, dEnv)
-	if err != nil {
-		return nil, err
-	}
-
-	p.databases[formatDbMapKeyName(db.Name())] = db
-
-	return dEnv, nil
+	return p.registerNewDatabase(ctx, dbName, dEnv)
 }
 
 // DropDatabase implements the sql.MutableDatabaseProvider interface
diff --git a/integration-tests/bats/clone-drop.bats b/integration-tests/bats/clone-drop.bats
new file mode 100644
index 00000000000..7e4785b7a34
--- /dev/null
+++ b/integration-tests/bats/clone-drop.bats
@@ -0,0 +1,31 @@
+#!/usr/bin/env bats
+load $BATS_TEST_DIRNAME/helper/common.bash
+
+setup() {
+  setup_no_dolt_init
+}
+
+teardown() {
+  stop_sql_server
+  assert_feature_version
+  teardown_common
+}
+
+@test "clone-drop: clone a database and then drop it" {
+  mkdir repo
+  cd repo
+  dolt init
+  dolt remote add pushed 'file://../pushed'
+  dolt push pushed main:main
+  dolt sql -q 'call dolt_clone("file://../pushed", "cloned"); drop database cloned;'
+}
+
+@test "clone-drop: sql-server: clone a database and then drop it" {
+  mkdir repo
+  cd repo
+  dolt init
+  dolt remote add pushed 'file://../pushed'
+  dolt push pushed main:main
+  start_sql_server
+  dolt sql -q 'call dolt_clone("file://../pushed", "cloned"); drop database cloned;'
+}
diff --git a/integration-tests/bats/helper/local-remote.bash b/integration-tests/bats/helper/local-remote.bash
index f478ffa6b93..7b8c428ff88 100644
--- a/integration-tests/bats/helper/local-remote.bash
+++ b/integration-tests/bats/helper/local-remote.bash
@@ -22,6 +22,7 @@ SKIP_SERVER_TESTS=$(cat <<-EOM
 ~1pk5col-strings.bats~
 ~sql-tags.bats~
 ~empty-repo.bats~
+~clone-drops.bats~
 ~verify-constraints.bats~
 ~db-revision-specifiers.bats~
 ~ignore.bats~