Initialize console if -U is specified #121

apoorvdeshmukh · 2022-08-19T11:55:43Z

In absence of console or when console reference line is set
to nil, noPwd is set to true which disables password prompt
and when console is not initialized, password cannot be read
from user. This is why when -i is specified, console is not
initialized and we do not see password prompt.
This commit initializes console even if -U is specified so
that password can be read interactively from user.
Resolves #115

Validation:

go-sqlcmd>.\sqlcmd.exe -i test.sql -U sa
Password:



------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Microsoft SQL Server 2012 (SP4-GDR) (KB4583465) - 11.0.7507.2 (X64)
        Nov  1 2020 00:48:37
        Copyright (c) Microsoft Corporation
        Enterprise Edition: Core-based Licensing (64-bit) on Windows NT 6.3 <X64> (Build 9600: ) (Hypervisor)


(1 row affected)

In absence of console or when console reference line is set to nil, noPwd is set to true which disables password prompt and when console is not initialized, password cannot be read from user. This is why when -i is specified, console is not initialized and we do not see password prompt. This commit initializes console even if -U is specified so that password can be read interactively from user.

shueybubbles · 2022-08-19T12:36:05Z

pls add a test that fails without the fix.

cmd/sqlcmd/main.go

shueybubbles · 2022-08-19T19:19:01Z

cmd/sqlcmd/main_test.go

@@ -162,8 +162,8 @@ func TestUnicodeOutput(t *testing.T) {
 	}
 	vars := sqlcmd.InitializeVariables(!args.DisableCmdAndWarn)
 	setVars(vars, &args)
-
-	exitCode, err := run(vars, &args)
+	sqlcmdConsole := getSqlcmdConsole(args)


let's go with a model that doesn't require so many changes.
Just define a type like

type allocConsole func(historyFile string) sqlcmd.Console

And create a field whose default assignment is console.NewConsole. The test can override it by assigning it a new func.

The unit test now no more relies on the console allocator function hence have removed it.
The unit test will validate console initialization and conditions for password prompt based on the parameters supplied.

cmd/sqlcmd/main.go

cmd/sqlcmd/main_test.go

shueybubbles · 2022-08-22T15:56:06Z

setConnect(&s.Connect, args, vars)

maybe the uactive assignment should move below this line , and use change s.Connect.requiresPassword to RequiresPassword and call it here

Refers to: cmd/sqlcmd/main.go:224 in 73fc6e1. [](commit_id = 73fc6e1, deletion_comment = False)

shueybubbles · 2022-08-25T14:45:27Z

cmd/sqlcmd/main.go

 	var line sqlcmd.Console = nil
-	if iactive {
-		line = console.NewConsole("")
+	if connectConfig.RequiresPassword() {


don't you still need the iactive check? How are you testing this before pushing?

My bad. I missed that locally it is only using SQL authentication.
Adding testcases for AAD modes

Added the iactive check.

shueybubbles · 2022-08-25T14:48:44Z

setConnect(&s.Connect, args, vars)

i think this can change to copy connectConfig to s.Connect instead

Refers to: cmd/sqlcmd/main.go:235 in 99140d4. [](commit_id = 99140d4, deletion_comment = False)

shueybubbles · 2022-09-06T16:29:45Z

cmd/sqlcmd/main.go

@@ -201,22 +201,28 @@ func setConnect(connect *sqlcmd.ConnectSettings, args *SQLCmdArguments, vars *sq
 	connect.ErrorSeverityLevel = args.ErrorSeverityLevel
 }

+func IsConsoleInitializationRequired(connect *sqlcmd.ConnectSettings, args *SQLCmdArguments) bool {


IsCons

isConsoleInitializationRequired
no need for this to be public

shueybubbles · 2022-09-06T16:39:22Z

cmd/sqlcmd/main_test.go

+	vars := sqlcmd.InitializeVariables(!args.DisableCmdAndWarn)
+	setVars(vars, &args)
+	var connectConfig sqlcmd.ConnectSettings
+	setConnect(&connectConfig, &args, vars)


Let's not use setVars and setConnect in this test. Their behavior depends on the values of environment variables set in the pipeline.
Just set the specific combinations of authentication type and user name/password in each struct explicitly and validate the right values for isConsoleInitializationRequired.

setConnect only seems to set password from env variable and that too if DisableCmdAndWarn is false.
servername and usernamre is fetched from vars.get() vars is obtained from InitializeVariables() which has an argument to indicate if values should be fetched from end. In this case DisableCmdAndWarn is set to true and value of DisableCmdAndWarn is passed to ensure that we don't fetch anything from env.
setVars() doesn't seems to fetch any anything from env.
I will, however, parameterize the test to add more scenarios.

shueybubbles · 2022-09-06T16:41:53Z

pkg/sqlcmd/commands_test.go

@@ -167,7 +167,7 @@ func TestConnectCommand(t *testing.T) {
 	err := connectCommand(s, []string{"someserver -U someuser"}, 1)
 	assert.NoError(t, err, "connectCommand with valid arguments doesn't return an error on connect failure")
 	assert.True(t, prompted, "connectCommand with user name and no password should prompt for password")
-	assert.NotEqual(t, "someserver", s.Connect.ServerName, "On error, sqlCmd.Connect does not copy inputs")
+	assert.Equal(t, "someserver", s.Connect.ServerName, "servername should match with the input parameter")



why is this changing? s.Connect.ServerName being changed implies that the connection attempt succeeded when it should have failed.

Thanks for adding the context. This was a side effect of changing type of connect from ConnectSettings to a pointer to ConnectSettings in sqlcmd struct.
In connectCommand() we copy s.Connect to local connect object to establish connection.
I will fix this in my next commit.

if the message in the assert didn't provide sufficient context, please update the assert message to be clearer.

shueybubbles · 2022-09-07T13:56:33Z

cmd/sqlcmd/main_test.go

+		connectConfig.AuthenticationMethod = testcase.authenticationMethod
+		connectConfig.Password = testcase.pwd
+		assert.Equal(t, testcase.expectedResult, isConsoleInitializationRequired(&connectConfig, &args), "Unexpected test result encountered for console initialization")
+		assert.Equal(t, testcase.expectedResult, connectConfig.RequiresPassword() && connectConfig.Password == "", "Unexpected test result encountered for password prompt conditions")


sse

this assert is odd. Shouldn't you assert that if RequiresPassword is true then Password == ""?

Yes. This was to avoid the conditional check and to assert the same condition for password prompt that we use in connectDb()

shueybubbles

apoorvdeshmukh requested review from shueybubbles and stuartpa August 19, 2022 11:55

shueybubbles reviewed Aug 19, 2022

View reviewed changes

cmd/sqlcmd/main.go Outdated Show resolved Hide resolved

shueybubbles requested changes Aug 19, 2022

View reviewed changes

apoorvdeshmukh force-pushed the 115 branch from b694b18 to 04a49dd Compare August 19, 2022 22:11

apoorvdeshmukh added the Not to be merged PR not to be merged. POC/Demo/Repro label Aug 21, 2022

shueybubbles reviewed Aug 22, 2022

View reviewed changes

cmd/sqlcmd/main.go Outdated Show resolved Hide resolved

apoorvdeshmukh force-pushed the 115 branch from 04a49dd to 14106b1 Compare August 22, 2022 13:07

Add the reworked testcase

8834ae9

apoorvdeshmukh force-pushed the 115 branch from 14106b1 to 8834ae9 Compare August 22, 2022 15:03

Reproduce login failed error

73fc6e1

shueybubbles reviewed Aug 22, 2022

View reviewed changes

cmd/sqlcmd/main_test.go Show resolved Hide resolved

Reproduce pipeline test failure

d9d6821

shueybubbles reviewed Aug 22, 2022

View reviewed changes

cmd/sqlcmd/main_test.go Outdated Show resolved Hide resolved

Initialize console based on password prompt requirement

99140d4

shueybubbles reviewed Aug 25, 2022

View reviewed changes

apoorvdeshmukh force-pushed the 115 branch 3 times, most recently from cad1251 to c634d1f Compare September 6, 2022 06:42

Add minimal unit testcase

444287d

apoorvdeshmukh force-pushed the 115 branch from c634d1f to 444287d Compare September 6, 2022 07:28

apoorvdeshmukh removed the Not to be merged PR not to be merged. POC/Demo/Repro label Sep 6, 2022

shueybubbles reviewed Sep 6, 2022

View reviewed changes

Add parameterized test and fix connectCommand testcase

9eeac74

Merge branch 'main' into 115

1837590

shueybubbles reviewed Sep 7, 2022

View reviewed changes

shueybubbles approved these changes Sep 7, 2022

View reviewed changes

apoorvdeshmukh merged commit fe6b80a into main Sep 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize console if -U is specified #121

Initialize console if -U is specified #121

apoorvdeshmukh commented Aug 19, 2022 •

edited

Loading

shueybubbles commented Aug 19, 2022

shueybubbles Aug 19, 2022

apoorvdeshmukh Sep 6, 2022

shueybubbles commented Aug 22, 2022

shueybubbles Aug 25, 2022

apoorvdeshmukh Aug 25, 2022

apoorvdeshmukh Sep 6, 2022

shueybubbles commented Aug 25, 2022

shueybubbles Sep 6, 2022

shueybubbles Sep 6, 2022

apoorvdeshmukh Sep 7, 2022

shueybubbles Sep 6, 2022 •

edited

Loading

apoorvdeshmukh Sep 7, 2022

shueybubbles Sep 7, 2022

shueybubbles Sep 7, 2022

apoorvdeshmukh Sep 7, 2022

shueybubbles left a comment

Initialize console if -U is specified #121

Initialize console if -U is specified #121

Conversation

apoorvdeshmukh commented Aug 19, 2022 • edited Loading

shueybubbles commented Aug 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shueybubbles commented Aug 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shueybubbles commented Aug 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shueybubbles Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shueybubbles left a comment

Choose a reason for hiding this comment

apoorvdeshmukh commented Aug 19, 2022 •

edited

Loading

shueybubbles Sep 6, 2022 •

edited

Loading