PR feedback

Author: stuartpa

NOTICE.md

@@ -41,7 +41,6 @@ func main() {
 			ErrorHandler:   checkErr,
 			HintHandler:    displayHints})}
 	rootCmd = cmdparser.New[*Root](dependencies)
-
 	if isFirstArgModernCliSubCommand() {
 		cmdparser.Initialize(initializeCallback)
 		rootCmd.Execute()
@@ -75,11 +74,6 @@ func initializeCallback() {
 			OutputType:     rootCmd.outputType,
 			LoggingLevel:   verbosity.Level(rootCmd.loggingLevel),
 		})
-	rootCmd.SetCrossCuttingConcerns(
-		dependency.Options{
-			EndOfLine: sqlcmd.SqlcmdEol,
-			Output:    outputter,
-		})
 	internal.Initialize(
 		internal.InitializeOptions{
 			ErrorHandler: checkErr,
@@ -115,5 +109,6 @@ func displayHints(hints []string) {
 		for i, hint := range hints {
 			outputter.Infof("  %d. %v", i+1, hint)
 		}
+		outputter.Infof("")
 	}
 }

cmd/modern/main.go

@@ -33,7 +33,7 @@ func TestDisplayHints(t *testing.T) {
 	assert.Equal(t, pal.LineBreak()+
 		"HINT:"+
 		pal.LineBreak()+
-		"  1. This is a hint"+pal.LineBreak(), buf.String())
+		"  1. This is a hint"+pal.LineBreak()+pal.LineBreak(), buf.String())
 }
 
 func TestCheckErr(t *testing.T) {

cmd/modern/main_test.go

@@ -87,13 +87,16 @@ func (c *Root) addGlobalFlags() {
 		Usage:         "Configuration file",
 	})
 
+	/* BUG:(stuartpa) - At the moment this is a top level flag, but it doesn't
+	work with all sub-commands (e.g. query), so removing for now.
 	c.AddFlag(cmdparser.FlagOptions{
 		String:        &c.outputType,
-		DefaultString: "yaml",
+		DefaultString: "json",
 		Name:          "output",
 		Shorthand:     "o",
 		Usage:         "output type (yaml, json or xml)",
 	})
+	*/
 
 	c.AddFlag(cmdparser.FlagOptions{
 		Int:        (*int)(&c.loggingLevel),

cmd/modern/root.go

@@ -21,6 +21,7 @@ func (c *Config) DefineCommand(...cmdparser.CommandOptions) {
 		Short:       `Modify sqlconfig files using subcommands like "sqlcmd config use-context mssql"`,
 		SubCommands: c.SubCommands(),
 	}
+
 	c.Cmd.DefineCommand(options)
 }
 

cmd/modern/root/config.go

@@ -49,7 +49,7 @@ type MssqlBase struct {
 
 	port int
 
-	attachDatabaseUrl string
+	usingDatabaseUrl string
 
 	unitTesting bool
 
@@ -187,7 +187,7 @@ func (c *MssqlBase) AddFlags(
 	})
 
 	addFlag(cmdparser.FlagOptions{
-		String:        &c.attachDatabaseUrl,
+		String:        &c.usingDatabaseUrl,
 		DefaultString: "",
 		Name:          "using",
 		Usage:         "Download (into container) and attach database (.bak) from URL",
@@ -210,7 +210,7 @@ func (c *MssqlBase) Run() {
 	if !c.acceptEula && viper.GetString("ACCEPT_EULA") == "" {
 		output.FatalWithHints(
 			[]string{"Either, add the --accept-eula flag to the command-line",
-				"Or, set the environment variable SQLCMD_ACCEPT_EULA=YES "},
+				fmt.Sprintf("Or, set the environment variable i.e. %s SQLCMD_ACCEPT_EULA=YES ", pal.CreateEnvVarKeyword())},
 			"EULA not accepted")
 	}
 
@@ -247,20 +247,15 @@ func (c *MssqlBase) createContainer(imageName string, contextName string) {
 		c.port = config.FindFreePortForTds()
 	}
 
-	if c.attachDatabaseUrl != "" {
+	// Do an early exit if url doesn't exist
+	if c.usingDatabaseUrl != "" {
+		c.validateUsingUrlExists()
+	}
 
-		// At the moment we only support attaching .bak files, but we should
-		// support .bacpacs and .mdfs in the future
-		if _, file := filepath.Split(c.attachDatabaseUrl); filepath.Ext(file) != ".bak" {
-			output.FatalfWithHints(
-				[]string{
-					"File must be a .bak file",
-				},
-				"Invalid file type")
+	if c.defaultDatabase != "" {
+		if !c.validateDbName(c.defaultDatabase) {
+			output.Fatalf("--user-database %q contains non-ASCII chars and/or quotes", c.defaultDatabase)
 		}
-
-		// Verify the url actually exists, and early exit if it doesn't
-		urlExists(c.attachDatabaseUrl, output)
 	}
 
 	controller := container.NewController()
@@ -320,24 +315,22 @@ func (c *MssqlBase) createContainer(imageName string, contextName string) {
 	if c.errorLogEntryToWaitFor == "Hello from Docker!" {
 		c.sql = sql.New(sql.SqlOptions{UnitTesting: true})
 	} else {
-		c.sql = sql.New(sql.SqlOptions{})
+		c.sql = sql.New(sql.SqlOptions{UnitTesting: false})
 	}
 
-	c.sql.Connect(
-		endpoint,
-		&sqlconfig.User{
-			AuthenticationType: "basic",
-			BasicAuth: &sqlconfig.BasicAuthDetails{
-				Username:          "sa",
-				PasswordEncrypted: c.encryptPassword,
-				Password:          secret.Encode(saPassword, c.encryptPassword),
-			},
-			Name: "sa",
-		}, sql.ConnectOptions{Interactive: false})
+	saUser := &sqlconfig.User{
+		AuthenticationType: "basic",
+		BasicAuth: &sqlconfig.BasicAuthDetails{
+			Username:          "sa",
+			PasswordEncrypted: c.encryptPassword,
+			Password:          secret.Encode(saPassword, c.encryptPassword)},
+		Name: "sa"}
+
+	c.sql.Connect(endpoint, saUser, sql.ConnectOptions{Interactive: false})
 
 	// Download and restore DB if asked
 	defaultDbAlreadyCreated := false
-	if c.attachDatabaseUrl != "" {
+	if c.usingDatabaseUrl != "" {
 		defaultDbAlreadyCreated = c.downloadAndRestoreDb(
 			controller,
 			containerId,
@@ -371,6 +364,33 @@ func (c *MssqlBase) createContainer(imageName string, contextName string) {
 	)
 }
 
+func (c *MssqlBase) validateUsingUrlExists() {
+	output := c.Cmd.Output()
+	u, err := url.Parse(c.usingDatabaseUrl)
+	c.CheckErr(err)
+
+	if u.Scheme != "http" && u.Scheme != "https" {
+		output.FatalfWithHints(
+			[]string{
+				"--using URL must be http or https",
+			},
+			"%q is not a valid URL for --using flag", c.usingDatabaseUrl)
+	}
+
+	// At the moment we only support attaching .bak files, but we should
+	// support .bacpacs and .mdfs in the future
+	if _, file := filepath.Split(c.usingDatabaseUrl); filepath.Ext(file) != ".bak" {
+		output.FatalfWithHints(
+			[]string{
+				"--using file URL must be a .bak file",
+			},
+			"Invalid --using file type")
+	}
+
+	// Verify the url actually exists, and early exit if it doesn't
+	urlExists(c.usingDatabaseUrl, output)
+}
+
 func (c *MssqlBase) query(commandText string) {
 	c.sql.Query(commandText)
 }
@@ -428,18 +448,20 @@ func (c *MssqlBase) downloadAndRestoreDb(
 ) (defaultDatabaseAlreadyCreated bool) {
 	output := c.Cmd.Output()
 
-	u, err := url.Parse(c.attachDatabaseUrl)
+	u, err := url.Parse(c.usingDatabaseUrl)
 	c.CheckErr(err)
-	_, file := filepath.Split(c.attachDatabaseUrl)
+	_, file := filepath.Split(c.usingDatabaseUrl)
 	fileNameWithNoExt := strings.TrimSuffix(file, filepath.Ext(file))
 
 	// Download file from URL into container
 	output.Infof("Downloading %s from %s", file, u.Hostname())
 
+	temporaryFolder := "/tmp"
+
 	controller.DownloadFile(
 		containerId,
-		c.attachDatabaseUrl,
-		"/var/opt/sql/backup",
+		c.usingDatabaseUrl,
+		temporaryFolder,
 	)
 
 	// Restore database from file
@@ -478,15 +500,15 @@ DECLARE @fileListTable TABLE (
 )
 
 INSERT INTO @fileListTable
-EXEC('RESTORE FILELISTONLY FROM DISK = ''/var/opt/sql/backup/%s''')
-SET @sql = 'RESTORE DATABASE [%s] FROM DISK = ''/var/opt/sql/backup/%s'' WITH '
+EXEC('RESTORE FILELISTONLY FROM DISK = ''%s/%s''')
+SET @sql = 'RESTORE DATABASE [%s] FROM DISK = ''%s/%s'' WITH '
 SELECT @sql = @sql + char(13) + ' MOVE ''' + LogicalName + ''' TO ''/var/opt/sql/' + LogicalName + '.' + RIGHT(PhysicalName,CHARINDEX('\',PhysicalName)) + ''','
 FROM @fileListTable
 WHERE IsPresent = 1
 SET @sql = SUBSTRING(@sql, 1, LEN(@sql)-1)
 EXEC(@sql)`
 
-	c.query(fmt.Sprintf(text, file, fileNameWithNoExt, file))
+	c.query(fmt.Sprintf(text, temporaryFolder, file, fileNameWithNoExt, temporaryFolder, file))
 
 	if c.defaultDatabase == "" {
 		c.defaultDatabase = fileNameWithNoExt
@@ -536,3 +558,21 @@ func (c *MssqlBase) generatePassword() (password string) {
 
 	return
 }
+
+// validateDbName checks if the database name is something that is likely
+// to work seamlessly through all tools, connection strings etc.
+//
+// TODO: Right now this is any ASCII char except for quotes,
+// but this needs to be opened up for Kanji characters etc. with a full test suite
+// to ensure the database name is valid in all the places it is passed to.
+func (c *MssqlBase) validateDbName(s string) bool {
+	for _, b := range []byte(s) {
+		if b > 127 {
+			return false
+		}
+	}
+	if strings.ContainsAny(s, "'\"`'") {
+		return false
+	}
+	return true
+}

cmd/modern/root/install/mssql-base.go

@@ -96,7 +96,7 @@ func (c *Ads) launchAds(host string, port int, username string) {
 	}
 
 	tool := tools.NewTool("ads")
-	if tool.IsInstalled() {
+	if !tool.IsInstalled() {
 		output.Fatalf(tool.HowToInstall())
 	} else {
 		_, err := tool.Run(args)

cmd/modern/root/open/ads.go

@@ -28,5 +28,7 @@ func TestAds(t *testing.T) {
 		Name: "context",
 	})
 	config.SetCurrentContextName("context")
-	cmdparser.TestCmd[*Ads]()
+
+	// TODO: Need to test this without launching the ADS UI itself
+	// cmdparser.TestCmd[*Ads]()
 }

cmd/modern/root/open/ads_test.go

@@ -7,6 +7,7 @@ import (
 	"bufio"
 	"bytes"
 	"context"
+	"fmt"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/filters"
@@ -224,39 +225,46 @@ func (c Controller) ContainerFiles(id string, filespec string) (files []string)
 	return strings.Split(string(stdout), "\n")
 }
 
-func (c Controller) DownloadFile(id string, src string, dest string) {
+func (c Controller) DownloadFile(id string, src string, destFolder string) {
 	if id == "" {
 		panic("Must pass in non-empty id")
 	}
 	if src == "" {
 		panic("Must pass in non-empty src")
 	}
-	if dest == "" {
-		panic("Must pass in non-empty dest")
+	if destFolder == "" {
+		panic("Must pass in non-empty destFolder")
 	}
 
-	cmd := []string{"mkdir", "/var/opt/sql/backup"}
-	c.runCmdInContainer(id, cmd)
+	cmd := []string{"mkdir", "--parents", destFolder}
+	_, stderr := c.runCmdInContainer(id, cmd)
+	if len(stderr) > 0 {
+		trace("Debugging info, running `du /var/opt`:")
+		c.runCmdInContainer(id, []string{"du", "/var/opt"})
+		checkErr(fmt.Errorf("Error creating backup directory: %s", stderr))
+	}
 
 	_, file := filepath.Split(src)
 
 	// Wget the .bak file from the http src, and place it in /var/opt/sql/backup
 	cmd = []string{
 		"wget",
 		"-O",
-		"/var/opt/sql/backup/" + file, // not using filepath.Join here, this is in the *nix container. always /
+		destFolder + "/" + file, // not using filepath.Join here, this is in the *nix container. always /
 		src,
 	}
 
 	c.runCmdInContainer(id, cmd)
 }
 
-func (c Controller) runCmdInContainer(id string, cmd []string) []byte {
+func (c Controller) runCmdInContainer(id string, cmd []string) ([]byte, []byte) {
+	trace("Running command in container: " + strings.Join(cmd, " "))
+
 	response, err := c.cli.ContainerExecCreate(
 		context.Background(),
 		id,
 		types.ExecConfig{
-			AttachStderr: false,
+			AttachStderr: true,
 			AttachStdout: true,
 			Cmd:          cmd,
 		},
@@ -285,7 +293,13 @@ func (c Controller) runCmdInContainer(id string, cmd []string) []byte {
 	checkErr(err)
 	stdout, err := io.ReadAll(&outBuf)
 	checkErr(err)
-	return stdout
+	stderr, err := io.ReadAll(&errBuf)
+	checkErr(err)
+
+	trace("Stdout: " + string(stdout))
+	trace("Stderr: " + string(stderr))
+
+	return stdout, stderr
 }
 
 // ContainerRunning returns true if the container with the given ID is running.

internal/container/controller.go

@@ -34,7 +34,7 @@ func UserName() (userName string) {
 func CmdLineWithEnvVars(vars []string, cmd string) string {
 	var sb strings.Builder
 	for _, v := range vars {
-		sb.WriteString(envVarCommand())
+		sb.WriteString(CreateEnvVarKeyword())
 		sb.WriteString(cliQuoteIdentifier() + v + cliQuoteIdentifier())
 	}
 	sb.WriteString(cliCommandSeparator())