From d9bb364c423bda204937e19d450f54eb8a6ad1ea Mon Sep 17 00:00:00 2001 From: jennifersp <44716627+jennifersp@users.noreply.github.com> Date: Tue, 31 May 2022 21:51:23 -0700 Subject: [PATCH] case sensitivity for database and branch names --- .../doltcore/sqle/database_provider.go | 42 ++++++++++++------- go/libraries/doltcore/sqle/dsess/session.go | 2 +- .../sqle/enginetest/dolt_engine_test.go | 30 +++++++++++++ integration-tests/bats/sql-server.bats | 21 ++++++---- integration-tests/bats/sql.bats | 10 ++--- 5 files changed, 75 insertions(+), 30 deletions(-) diff --git a/go/libraries/doltcore/sqle/database_provider.go b/go/libraries/doltcore/sqle/database_provider.go index 9e904f3881..b726f5ec4e 100644 --- a/go/libraries/doltcore/sqle/database_provider.go +++ b/go/libraries/doltcore/sqle/database_provider.go @@ -96,10 +96,9 @@ func (p DoltDatabaseProvider) WithDbFactoryUrl(url string) DoltDatabaseProvider } func (p DoltDatabaseProvider) Database(ctx *sql.Context, name string) (db sql.Database, err error) { - name = strings.ToLower(name) var ok bool p.mu.RLock() - db, ok = p.databases[name] + db, ok = p.databases[formatDbMapKeyName(name)] p.mu.RUnlock() if ok { return db, nil @@ -116,8 +115,8 @@ func (p DoltDatabaseProvider) Database(ctx *sql.Context, name string) (db sql.Da p.mu.Lock() defer p.mu.Unlock() - if found, ok := p.databases[name]; !ok { - p.databases[name] = db + if found, ok := p.databases[formatDbMapKeyName(name)]; !ok { + p.databases[formatDbMapKeyName(name)] = db return db, nil } else { return found, nil @@ -184,7 +183,7 @@ func (p DoltDatabaseProvider) CreateDatabase(ctx *sql.Context, name string) erro } db := NewDatabase(name, newEnv.DbData(), opts) - p.databases[strings.ToLower(db.Name())] = db + p.databases[formatDbMapKeyName(db.Name())] = db dbstate, err := GetInitialDBState(ctx, db) if err != nil { @@ -202,18 +201,19 @@ func (p DoltDatabaseProvider) DropDatabase(ctx *sql.Context, name string) error // TODO: there are still cases (not server-first) where we rename databases because the directory name would need // quoting if used as a database name, and that breaks here. We either need the database name to match the directory // name in all cases, or else keep a mapping from database name to directory on disk. - db := p.databases[strings.ToLower(name)] + dbKey := formatDbMapKeyName(name) + db := p.databases[dbKey] // Get the DB's directory exists, isDir := p.fs.Exists(db.Name()) if !exists { // engine should already protect against this - return sql.ErrDatabaseNotFound.New(name) + return sql.ErrDatabaseNotFound.New(db.Name()) } else if !isDir { - return fmt.Errorf("unexpected error: %s exists but is not a directory", name) + return fmt.Errorf("unexpected error: %s exists but is not a directory", dbKey) } - err := p.fs.Delete(name, true) + err := p.fs.Delete(db.Name(), true) if err != nil { return err } @@ -222,21 +222,20 @@ func (p DoltDatabaseProvider) DropDatabase(ctx *sql.Context, name string) error // We not only have to delete this database, but any derivative ones that we've stored as a result of USE or // connection strings - derivativeNamePrefix := strings.ToLower(name) + "/" + derivativeNamePrefix := dbKey + "/" for dbName := range p.databases { - if strings.HasPrefix(strings.ToLower(dbName), derivativeNamePrefix) { - delete(p.databases, strings.ToLower(dbName)) + if strings.HasPrefix(dbName, derivativeNamePrefix) { + delete(p.databases, dbName) } } - delete(p.databases, strings.ToLower(name)) + delete(p.databases, dbKey) return nil } //TODO: databaseForRevision should call checkout on the given branch/commit, returning a non-mutable session // only if a non-branch revspec was indicated. func (p DoltDatabaseProvider) databaseForRevision(ctx *sql.Context, revDB string) (sql.Database, dsess.InitialDbState, bool, error) { - revDB = strings.ToLower(revDB) if !strings.Contains(revDB, dbRevisionDelimiter) { return nil, dsess.InitialDbState{}, false, nil } @@ -245,7 +244,7 @@ func (p DoltDatabaseProvider) databaseForRevision(ctx *sql.Context, revDB string dbName, revSpec := parts[0], parts[1] p.mu.RLock() - candidate, ok := p.databases[dbName] + candidate, ok := p.databases[formatDbMapKeyName(dbName)] p.mu.RUnlock() if !ok { return nil, dsess.InitialDbState{}, false, nil @@ -520,3 +519,16 @@ type staticRepoState struct { func (s staticRepoState) CWBHeadRef() ref.DoltRef { return s.branch } + +// formatDbMapKeyName returns formatted string of database name and/or branch name. Database name is case-insensitive, +// so it's stored in lower case name. Branch name is case-sensitive, so not changed. +func formatDbMapKeyName(name string) string { + if !strings.Contains(name, dbRevisionDelimiter) { + return strings.ToLower(name) + } + + parts := strings.SplitN(name, dbRevisionDelimiter, 2) + dbName, revSpec := parts[0], parts[1] + + return strings.ToLower(dbName) + dbRevisionDelimiter + revSpec +} diff --git a/go/libraries/doltcore/sqle/dsess/session.go b/go/libraries/doltcore/sqle/dsess/session.go index 186d66469f..acd0530e3b 100644 --- a/go/libraries/doltcore/sqle/dsess/session.go +++ b/go/libraries/doltcore/sqle/dsess/session.go @@ -848,7 +848,7 @@ func (sess *Session) AddDB(ctx *sql.Context, dbState InitialDbState) error { sessionState := &DatabaseSessionState{} sess.dbStates[db.Name()] = sessionState - + sessionState.dbName = db.Name() // TODO: get rid of all repo state reader / writer stuff. Until we do, swap out the reader with one of our own, and // the writer with one that errors out sessionState.dbData = dbState.DbData diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go b/go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go index a80da3161a..618298b6a1 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go @@ -396,6 +396,36 @@ func TestCreateDatabase(t *testing.T) { } func TestDropDatabase(t *testing.T) { + enginetest.TestScript(t, newDoltHarness(t), queries.ScriptTest{ + Name: "Drop database engine tests for Dolt only", + SetUpScript: []string{ + "CREATE DATABASE Test1db", + "CREATE DATABASE TEST2db", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "DROP DATABASE TeSt2DB", + Expected: []sql.Row{{sql.OkResult{RowsAffected: 1}}}, + }, + { + Query: "USE test2db", + ExpectedErr: sql.ErrDatabaseNotFound, + }, + { + Query: "USE TEST1DB", + Expected: []sql.Row{}, + }, + { + Query: "DROP DATABASE IF EXISTS test1DB", + Expected: []sql.Row{{sql.OkResult{RowsAffected: 1}}}, + }, + { + Query: "USE Test1db", + ExpectedErr: sql.ErrDatabaseNotFound, + }, + }, + }) + t.Skip("Dolt doesn't yet support dropping the primary database, which these tests do") enginetest.TestDropDatabase(t, newDoltHarness(t)) } diff --git a/integration-tests/bats/sql-server.bats b/integration-tests/bats/sql-server.bats index 6b54913f27..a81fd89600 100644 --- a/integration-tests/bats/sql-server.bats +++ b/integration-tests/bats/sql-server.bats @@ -1131,22 +1131,25 @@ END""") } @test "sql-server: connect to databases case insensitive" { - skip "Database connnection strings are case sensitive and should not be" skiponwindows "Has dependencies that are missing on the Jenkins Windows installation." skip_nbf_dolt_1 - + mkdir no_dolt && cd no_dolt start_sql_server server_query "" 1 "create database Test1" - + server_query "" 1 "show databases" "Database\nTest1\ninformation_schema" - server_query "test1" 1 "create table a(x int)" - server_query "TEST1" 1 "insert into a values (1), (2)" - run server_query "test1" 1 "select dolt_commit('-a', '-m', 'new table a')" - run server_query "test1" 1 "select dolt_checkout('-b', 'newbranch')" - server_query "TEST1/newbranch" 1 "select * from a" "x\n1\n2" - server_query "TEST1/NEWBRANCH" 1 "select * from a" "x\n1\n2" + multi_query "" 1 "use test1; create table a(x int);" + multi_query "" 1 "use TEST1; insert into a values (1), (2);" + run multi_query "" 1 "use test1; select dolt_commit('-a', '-m', 'new table a');" + run multi_query "" 1 "use test1; select dolt_checkout('-b', 'newbranch');" + multi_query "" 1 "use \`TEST1/newbranch\`; select * from a" "x\n1\n2" + multi_query "" 1 "use \`test1/newbranch\`; select * from a" "x\n1\n2" + server_query "" 1 "use \`TEST1/NEWBRANCH\`" "" "database not found: TEST1/NEWBRANCH" + + multi_query "" 1 "create database test2; use test2; select database();" "database()\ntest2" + multi_query "" 1 "use test2; drop database TEST2; select database();" "null" } @test "sql-server: create and drop database with --multi-db-dir" { diff --git a/integration-tests/bats/sql.bats b/integration-tests/bats/sql.bats index f545c463b3..4bf221d08b 100755 --- a/integration-tests/bats/sql.bats +++ b/integration-tests/bats/sql.bats @@ -664,7 +664,7 @@ SQL CREATE DATABASE test1; CREATE DATABASE test2; USE test1; -CALL DOLT_CHECKOUT('-b', 'newbranch'); +CALL DOLT_CHECKOUT('-b', 'newBranch'); USE \`test1/newBranch\`; USE test2; DROP DATABASE test1; @@ -675,16 +675,16 @@ SQL run dolt sql <