From 93b044f233bb4cbef259c8b9bb7cbcaa025d893d Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Tue, 3 Oct 2023 15:33:18 -0700 Subject: [PATCH] Various small fixes --- .../doltcore/sqle/database_provider.go | 13 ++++++-- .../doltcore/sqle/dprocedures/dolt_undrop.go | 16 ++++------ .../doltcore/sqle/dropped_databases.go | 22 +++++++------ go/libraries/utils/errors/error_messages.go | 31 +++++++++++++++++++ integration-tests/bats/undrop.bats | 2 +- 5 files changed, 61 insertions(+), 23 deletions(-) create mode 100644 go/libraries/utils/errors/error_messages.go diff --git a/go/libraries/doltcore/sqle/database_provider.go b/go/libraries/doltcore/sqle/database_provider.go index a7678c20b1..ec8862bc73 100644 --- a/go/libraries/doltcore/sqle/database_provider.go +++ b/go/libraries/doltcore/sqle/database_provider.go @@ -624,6 +624,9 @@ func (p DoltDatabaseProvider) DropDatabase(ctx *sql.Context, name string) error } func (p DoltDatabaseProvider) ListUndroppableDatabases(ctx *sql.Context) ([]string, error) { + p.mu.Lock() + defer p.mu.Unlock() + return p.droppedDatabaseManager.ListUndroppableDatabases(ctx) } @@ -644,6 +647,13 @@ func (p DoltDatabaseProvider) UndropDatabase(ctx *sql.Context, name string) (err // function is responsible for instantiating the new Database instance and updating the tracking metadata // in this provider. If any problems are encountered while registering the new database, an error is returned. func (p DoltDatabaseProvider) registerNewDatabase(ctx *sql.Context, name string, newEnv *env.DoltEnv) (err error) { + // This method MUST be called with the provider's mutex locked. TryLock allows us to validate that the + // mutex is locked (without actually locking it and causing a deadlock) and to error out if we detect + // that the mutex is NOT locked. + if p.mu.TryLock() { + return fmt.Errorf("unable to register new database without database provider mutex being locked") + } + // If we're running in a sql-server context, ensure the new database is locked so that it can't // be edited from the CLI. We can't rely on looking for an existing lock file, since this could // be the first db creation if sql-server was started from a bare directory. @@ -679,9 +689,6 @@ func (p DoltDatabaseProvider) registerNewDatabase(ctx *sql.Context, name string, return err } - // TODO: accessing p.databases requires locking!!! - // But right now we're just assuming this function is called from another function - // that has grabbed the right lock, but that's eventually going to cause a problem. formattedName := formatDbMapKeyName(db.Name()) p.databases[formattedName] = db p.dbLocations[formattedName] = newEnv.FS diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_undrop.go b/go/libraries/doltcore/sqle/dprocedures/dolt_undrop.go index e578cd6432..462b00dea7 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_undrop.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_undrop.go @@ -16,11 +16,11 @@ package dprocedures import ( "fmt" - "strings" "github.com/dolthub/go-mysql-server/sql" "github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess" + "github.com/dolthub/dolt/go/libraries/utils/errors" ) // doltClean is the stored procedure version for the CLI command `dolt clean`. @@ -28,20 +28,16 @@ func doltUndrop(ctx *sql.Context, args ...string) (sql.RowIter, error) { doltSession := dsess.DSessFromSess(ctx.Session) provider := doltSession.Provider() + // TODO: What are the right permissions for dolt_undrop? + // the same as drop? or create db? + switch len(args) { case 0: - // TODO: Are there any permission issues for undrop? probably the same as drop? - undroppableDatabases, err := provider.ListUndroppableDatabases(ctx) + availableDatabases, err := provider.ListUndroppableDatabases(ctx) if err != nil { return nil, err } - - extraInformation := "there are no databases that can currently be undropped." - if len(undroppableDatabases) > 0 { - extraInformation = fmt.Sprintf("the following dropped databases are availble to be undropped: %s", - strings.Join(undroppableDatabases, ", ")) - } - return nil, fmt.Errorf("no database name specified. %s", extraInformation) + return nil, fmt.Errorf("no database name specified. %s", errors.CreateUndropErrorMessage(availableDatabases)) case 1: if err := provider.UndropDatabase(ctx, args[0]); err != nil { diff --git a/go/libraries/doltcore/sqle/dropped_databases.go b/go/libraries/doltcore/sqle/dropped_databases.go index 0fca2480b0..3446194e1b 100644 --- a/go/libraries/doltcore/sqle/dropped_databases.go +++ b/go/libraries/doltcore/sqle/dropped_databases.go @@ -23,6 +23,7 @@ import ( "github.com/dolthub/go-mysql-server/sql" "github.com/dolthub/dolt/go/libraries/doltcore/dbfactory" + "github.com/dolthub/dolt/go/libraries/utils/errors" "github.com/dolthub/dolt/go/libraries/utils/filesys" ) @@ -182,16 +183,12 @@ func (dd *droppedDatabases) validateUndropDatabase(ctx *sql.Context, name string } } - // TODO: this error creation information could be extracted to a common function that dolt_undrop function could use if !found { - extraInformation := "there are no databases currently available to be undropped" - if len(availableDatabases) > 0 { - extraInformation = fmt.Sprintf("available databases that can be undropped: %s", strings.Join(availableDatabases, ", ")) - } - return "", "", "", fmt.Errorf("no database named '%s' found to undrop. %s", name, extraInformation) + return "", "", "", fmt.Errorf("no database named '%s' found to undrop. %s", + name, errors.CreateUndropErrorMessage(availableDatabases)) } - // Check to see if the destination directory for restoring the database already exists (case insensitive match) + // Check to see if the destination directory for restoring the database already exists (case-insensitive match) destinationPath, err = dd.fs.Abs(exactCaseName) if err != nil { return "", "", "", err @@ -199,8 +196,15 @@ func (dd *droppedDatabases) validateUndropDatabase(ctx *sql.Context, name string sourcePath = filepath.Join(deletedDatabaseDirectoryName, exactCaseName) - // TODO: is this always a case insensitive check??? It seems like it must be since our test is working? - if exists, _ := dd.fs.Exists(destinationPath); exists { + found = false + dd.fs.Iter(filepath.Dir(destinationPath), false, func(path string, size int64, isDir bool) (stop bool) { + if strings.ToLower(filepath.Base(path)) == strings.ToLower(filepath.Base(destinationPath)) { + found = true + } + return found + }) + + if found { return "", "", "", fmt.Errorf("unable to undrop database '%s'; "+ "another database already exists with the same case-insensitive name", exactCaseName) } diff --git a/go/libraries/utils/errors/error_messages.go b/go/libraries/utils/errors/error_messages.go new file mode 100644 index 0000000000..de41ad310f --- /dev/null +++ b/go/libraries/utils/errors/error_messages.go @@ -0,0 +1,31 @@ +// 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 errors + +import ( + "fmt" + "strings" +) + +// CreateUndropErrorMessage returns a string to be used in errors returned from attempts to undrop a database. +// The error message string either states that no databases are available to be undropped, or it lists +// the names of the databases that are available to be undropped. +func CreateUndropErrorMessage(availableDatabases []string) string { + if len(availableDatabases) == 0 { + return "there are no databases currently available to be undropped" + } else { + return fmt.Sprintf("available databases that can be undropped: %s", strings.Join(availableDatabases, ", ")) + } +} diff --git a/integration-tests/bats/undrop.bats b/integration-tests/bats/undrop.bats index a2ecae66e6..d1db81fdaa 100644 --- a/integration-tests/bats/undrop.bats +++ b/integration-tests/bats/undrop.bats @@ -21,7 +21,7 @@ teardown() { run dolt sql -q "CALL dolt_undrop();" [ $status -eq 1 ] [[ $output =~ "no database name specified." ]] || false - [[ $output =~ "there are no databases that can currently be undropped" ]] || false + [[ $output =~ "there are no databases currently available to be undropped" ]] || false # When called without an invalid database name, dolt_undrop() returns # an error that includes the database names that can be undropped.