PR Feedback: normalizing physical directory names when moving them to the dropped dbs directory, so that they will always match the logical name of the database when they are restored.

go/libraries/doltcore/dbfactory/dirtodbname.go

@@ -0,0 +1,41 @@
+// Copyright 2023 Dolthub, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package dbfactory
+
+import (
+	"strings"
+	"unicode"
+)
+
+// DirToDBName takes the physical directory name, |dirName|, and replaces any unsupported characters to create a
+// valid logical database name. For example, hyphens and spaces are replaced with underscores.
+func DirToDBName(dirName string) string {
+	dbName := strings.TrimSpace(dirName)
+	dbName = strings.Map(func(r rune) rune {
+		if unicode.IsSpace(r) || r == '-' {
+			return '_'
+		}
+		return r
+	}, dbName)
+
+	newDBName := strings.ReplaceAll(dbName, "__", "_")
+
+	for dbName != newDBName {
+		dbName = newDBName
+		newDBName = strings.ReplaceAll(dbName, "__", "_")
+	}
+
+	return dbName
+}

go/libraries/doltcore/env/multi_repo_env.go

@@ -21,7 +21,6 @@ import (
 	"path/filepath"
 	"sort"
 	"strings"
-	"unicode"
 
 	"github.com/sirupsen/logrus"
 	"gopkg.in/src-d/go-errors.v1"
@@ -87,7 +86,7 @@ func MultiEnvForDirectory(
 			return nil, err
 		}
 		envName := getRepoRootDir(path, string(os.PathSeparator))
-		dbName = dirToDBName(envName)
+		dbName = dbfactory.DirToDBName(envName)
 
 		newDEnv = Load(ctx, GetCurrentUserHomeDir, dataDirFS, doltdb.LocalDirDoltDB, version)
 	}
@@ -126,7 +125,7 @@ func MultiEnvForDirectory(
 
 		newEnv := Load(ctx, GetCurrentUserHomeDir, newFs, doltdb.LocalDirDoltDB, version)
 		if newEnv.Valid() {
-			envSet[dirToDBName(dir)] = newEnv
+			envSet[dbfactory.DirToDBName(dir)] = newEnv
 		}
 		return false
 	})
@@ -403,22 +402,3 @@ func enforceSingleFormat(envSet map[string]*DoltEnv) {
 		}
 	}
 }
-
-func dirToDBName(dirName string) string {
-	dbName := strings.TrimSpace(dirName)
-	dbName = strings.Map(func(r rune) rune {
-		if unicode.IsSpace(r) || r == '-' {
-			return '_'
-		}
-		return r
-	}, dbName)
-
-	newDBName := strings.ReplaceAll(dbName, "__", "_")
-
-	for dbName != newDBName {
-		dbName = newDBName
-		newDBName = strings.ReplaceAll(dbName, "__", "_")
-	}
-
-	return dbName
-}

go/libraries/doltcore/env/multi_repo_env_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/dolthub/dolt/go/libraries/doltcore/dbfactory"
 	"github.com/dolthub/dolt/go/libraries/utils/config"
 	"github.com/dolthub/dolt/go/libraries/utils/earl"
 	"github.com/dolthub/dolt/go/libraries/utils/filesys"
@@ -38,7 +39,7 @@ func TestDirToDBName(t *testing.T) {
 	}
 
 	for dirName, expected := range tests {
-		actual := dirToDBName(dirName)
+		actual := dbfactory.DirToDBName(dirName)
 		assert.Equal(t, expected, actual)
 	}
 }

go/libraries/doltcore/sqle/dropped_databases.go

@@ -86,11 +86,11 @@ func (dd *droppedDatabaseManager) DropDatabase(ctx *sql.Context, name string, dr
 		destinationDirectory = filepath.Join(droppedDatabaseDirectoryName, file)
 	}
 
-	// Add the final directory segment and convert all hyphens to underscores in the database directory name
-	dir, file := filepath.Split(destinationDirectory)
-	if strings.Contains(file, "-") {
-		destinationDirectory = filepath.Join(dir, strings.ReplaceAll(file, "-", "_"))
-	}
+	// Add the final directory segment and convert any invalid chars so that the physical directory
+	// name matches the current logical/SQL name of the database.
+	dir, base := filepath.Split(destinationDirectory)
+	base = dbfactory.DirToDBName(file)
+	destinationDirectory = filepath.Join(dir, base)
 
 	if err := dd.prepareToMoveDroppedDatabase(ctx, destinationDirectory); err != nil {
 		return err
@@ -174,9 +174,9 @@ func (dd *droppedDatabaseManager) ListDroppedDatabases(_ *sql.Context) ([]string
 
 	databaseNames := make([]string, 0, 5)
 	callback := func(path string, size int64, isDir bool) (stop bool) {
-		_, lastPathSegment := filepath.Split(path)
-		lastPathSegment = strings.ReplaceAll(lastPathSegment, "-", "_")
-		databaseNames = append(databaseNames, lastPathSegment)
+		// When we move a database to the dropped database directory, we normalize the physical directory
+		// name to be the same as the logical SQL name, so there's no need to do any name mapping here.
+		databaseNames = append(databaseNames, filepath.Base(path))
 		return false
 	}
 

integration-tests/bats/undrop.bats

@@ -6,10 +6,11 @@ setup() {
   dolt init
 
   # NOTE: Instead of running setup_common, we embed the same commands here so that we can set up our test data
-  #       to work with with the remote-engine test variant. To test database directory names that contain a hyphen
-  #       (which is converted to an underscore when accessed through a SQL interface), we need to create the
-  #       directory on disk *before* the remote-engine sql-server starts.
-  mkdir drop-me-2 && cd drop-me-2
+  #       to work with with the remote-engine test variant. To test database directory names that need to be
+  #       mapped from their physical, directory name to a valid logical/SQL database name, we create a directory
+  #       that contains hyphens and whitespace. We need to create the directory on disk *before* the remote-engine
+  #       sql-server starts.
+  mkdir ' drop- me-2 ' && cd ' drop- me-2 '
   dolt init && cd ..
   setup_remote_server
 }
@@ -60,7 +61,7 @@ teardown() {
 @test "undrop: undrop root database" {
   # Create a new Dolt database directory to use as a root database
   # NOTE: We use hyphens here to test how db dirs are renamed.
-  mkdir test-db-1 && cd test-db-1
+  mkdir ' test- db-1 ' && cd ' test- db-1 '
   dolt init
 
   # Create some data and a commit in the database