cli: add missing username in SQL prompt for connection URL by a6802739 · Pull Request #12408 · cockroachdb/cockroach

a6802739 · 2016-12-15T15:51:21Z

This change is

knz · 2016-12-15T16:50:36Z

Please change the title of the commit message as well.

asubiotto

Thank you for your PR. However, I think it would be better to implement a built-in function through which the client could query the server and avoid discrepancies between the user shown in the prompt and the current session user.

asubiotto · 2016-12-16T16:24:24Z

+			if err == nil {
+				username = u.Username
+			} else {
+				username, _ = envutil.EnvString("COCKROACH_USER", 2)


I think it would be wrong to do this because we would not be using COCKROACH_USER to connect to the server (since we prefer url parameters over connection flags/environment variables) so there would be a disconnect between the user shown in the prompt and the actual session user. I would just remove this.

@asubiotto , Thanks for your review. done.

knz · 2016-12-16T16:40:22Z

@a6802739 this is similar to the work already performed in the shell to determine the current txn state, albeit easier. This is done as follows:

add a new entry in varGen in show.go that displays the current user
extend the check for the transaction state in the SQL shell to also query the current user, and change the prompt accordingly.

a6802739 · 2016-12-20T14:51:34Z

@asubiotto, @knz, Thanks for your review. I have extend it with a internal query now.

knz · 2016-12-20T15:47:15Z

Thank you for working on this. It looks much better now! Just minor stuff left.

Reviewed 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

pkg/cli/sql.go, line 418 at r2 (raw file):

			username = parsedURL.User.Username()
		} else {
			query := makeQuery(`SHOW CURRENT USER`)

Please extract this else block into its own method getCurrentUser().

pkg/sql/show.go, line 37 at r2 (raw file):

var varGen = map[string]func(p *planner) string{
	`CURRENT USER`:                  func(p *planner) string { return p.session.User },

Name this CURRENT_USER for compatibility with postgres.
Also since you are already looking at the part of this code, please add an alias SESSION_USER which does the same thing, for compatibility too. You do not need it in this PR but client applications would be surprised to see one and not the other.

pkg/sql/parser/sql.y, line 1532 at r2 (raw file):

    $$.val = &ShowConstraints{Table: $4.normalizableTableName()}
  }
| SHOW CURRENT USER

If you rename the variable to CURRENT_USER this new rule should not be necessary any more.

Comments from Reviewable

a6802739 · 2016-12-21T11:31:24Z

@knz, Thanks for your review.

And I also add the alias session_user in this PR.

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

pkg/cli/sql.go, line 418 at r2 (raw file):

Previously, knz (kena) wrote…

Please extract this else block into its own method getCurrentUser().

Done.

pkg/sql/show.go, line 37 at r2 (raw file):

Previously, knz (kena) wrote…

Name this CURRENT_USER for compatibility with postgres.
Also since you are already looking at the part of this code, please add an alias SESSION_USER which does the same thing, for compatibility too. You do not need it in this PR but client applications would be surprised to see one and not the other.

Ok, I will make another PR for SESSION_USER.

pkg/sql/parser/sql.y, line 1532 at r2 (raw file):

Previously, knz (kena) wrote…

If you rename the variable to CURRENT_USER this new rule should not be necessary any more.

Done.

Comments from Reviewable

knz · 2017-01-05T20:23:20Z

Reviewed 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

pkg/cli/sql.go, line 407 at r3 (raw file):

}

func (c *cliState) getCurrentUser() string {

@pmamatsis want to comment on how to better structure this code based on your previous experience?

pkg/sql/parser/sql.y, line 1545 at r3 (raw file):

    $$.val = &Show{Name: "CURRENT_USER"}
  }
| SHOW SESSION_USER

if you intend to support SESSION_USER in a separate PR please remove this rule then.

Comments from Reviewable

knz · 2017-01-05T20:28:50Z

Also please rebase the PR using the latest master branch.

pmamatsis · 2017-01-05T21:10:31Z

Hi @knz,
following up on your REALLY HUGE help with my first commit 👍 for another issue related with the cli, the function that you are mentioning above named getCurrentUser must be refactored in order to pass as arguments the nextState and also all the appropriate prompt strings.... The whole idea is that it's based on FSM design in which one piece of function does it's job and then then chains down to the next one until the state has finished and then continue with the next state. My repo it's missing this function....please let me update it....

In the mean time i am checking the Reviewable site in order to comment.

Best regards,
Panos.

pmamatsis · 2017-01-05T21:53:35Z

Hi @knz,

based on my previous experience i would have done the following:

In the function named getCurrentUser since we know beforehand that is going to retrieve only one piece of information the user name i would change the val := make([]driver.Value, len(rows.Columns())) to [1]driver.Value as stated by @andreimatei
Assert that len(rows.Columns) == 0 and also that rows.Next() returns io.EOF when called a second time as stated by @andreimatei (as a helpful "guide" kindly try to understand how the existing function named doRefreshPrompts is working...you will see that if an error happens just breaks to the next chained function call).
For the function call username = formatVal(val[0], false, false) the false arguments must be commented inline.
Now for the most difficult part to explain (because i am now getting the grasp of it thanks to @knz)... the whole function must be refactored so that it confronts with the design pattern called "continuation passing". This means that the function must be chained with a cascade call to the preparePrompts function as following
1. Make the function getCurrentUser take as argument the following dbURL
2. In the function named doStart replace the call to preparePrompts with the call to getCurrentUser function.
3. In the preparePrompts change it to accept one more argument named dbUser
4. In the function getCurrentUser replace the current return value with return c.preparePrompts
  and passing the extra argument as gathered from the previous function. The final step is to check into the preparePrompts if the user from the command line URL argument is empty so provide the one as received from your function.

@knz how do you thing? Did i put it properly ?

Best regards,
Panos.

knz · 2017-01-05T22:03:22Z

Yes this sounds fine. @a6802739 what do you think? Are you still planning/willing to work on this? (of course it's ok if you're not!)

pmamatsis · 2017-01-05T22:05:46Z

If not then i can do it @knz ! Really thanks and greetings to all from cold Greece!!! I will be waiting for @a6802739 reply on this so i can start working asap if it's negative...

Best regards,
Panos.

a6802739 · 2017-01-08T08:07:17Z

@pmamatsis, please go ahead.
@knz, So I should delete the PR?

pmamatsis · 2017-01-31T17:50:41Z

Hi @knz,
prior to start working on this issue do i need to rebase my PR ?

Best regards,
Panos.

knz · 2017-02-02T19:03:11Z

Please wait for #13379 to merge then rebase this one on top of it.
Thanks

pmamatsis · 2017-02-19T09:44:59Z

Good morning @knz,
about this issue i wanted to ask you if there is any hard deadline. I have started to working on it since the previous week but since i am still working on it i am wondering if i am delaying the project. My delay is mostly in trying to understand the rest of the structure behind the sql package in order not to ask stupid questions...

Best regards and thank you in advance,
Panos.

knz · 2017-02-19T12:26:08Z

There is no deadline.

knz · 2017-02-19T12:27:02Z

Note however that since the last time it is now possible to ask SHOW USER to the server and this will report the current user. I think you will need this.

(You'll need to rebase to pick up this change).

pmamatsis · 2017-02-19T14:23:27Z

Good evening @knz,
i have done the rebase and rebuilt the main executable file but when i am trying to perform the SHOW USER statement through the sql console i am getting an error message that this statement does not exist. Can you please advice?

Best regards,
Panos.

knz · 2017-02-20T11:32:45Z

Oh, that's a bug. Thanks for finding it.
(The logic for SHOW is already implemented; you can see the information in the output of SHOW ALL. The variable is session_user btw not user, but this should not happen. The problem is that SHOW SESSION_USER is not currently recognized by the parser, as I thought it would. Will fix.)

pmamatsis · 2017-02-20T11:54:34Z

Good afternoon @knz,
i am thinking...do you believe that (with your guidance of course) i can help you in fixing this issue?

Best regards,
Panos.

knz · 2017-02-20T12:11:04Z

I'm already nearly done with it. There were also other changes needed around it. :)

knz · 2017-02-20T13:33:49Z

ok you'll need the changes in #13678

pmamatsis · 2017-03-11T16:45:14Z

Hi @knz,
sorry for my so late reply but i was in (a much needed) vacation away from the office! I have just performed the latest rebase of my repo against the main repo but after i build the project and run the sql console i still cannot get the SHOW USER to work. Can you please let me know if this has update the main source of the project?

Best regards and thank you in advance,
Panos.

knz · 2017-03-13T20:09:37Z

try SHOW "SESSION_USER"

pmamatsis · 2017-03-18T14:38:50Z

Hi @knz,
thank you so much for your quick response. It worked as you suggested. I will start working now on the issue.

Best regards,
Panos.

pmamatsis · 2017-03-18T15:07:13Z

Hi @knz,
before i will actually touch this i wanted your opinion about my thinking...I understood that the function named preparePrompts is the one that does the parsing of the command line URL so in the beginning of it after it parses the URL as following check:

                if parsedURL.User != nil {
			username = parsedURL.User.Username()
		}

To add an else part to it and then query about the CURRENT_USER. How do you think about it?

Best regards and thank you in advance,
Panos.

knz · 2017-03-18T17:34:07Z

I would recommend replacing this check entirely by a query to the server. This way if the server decides to do something else than the connection string, it will be reflected in the client.

knz · 2017-04-12T10:21:28Z

This feature now works properly. Closing this. Thank you for starting this work!

a6802739 assigned knz Dec 15, 2016

a6802739 changed the title ~~li: missing username in connection URL prevents username from appeari…~~ cli: add missing username in SQL prombt for connection URL Dec 15, 2016

a6802739 changed the title ~~cli: add missing username in SQL prombt for connection URL~~ cli: add missing username in SQL prompt for connection URL Dec 15, 2016

knz requested a review from asubiotto December 15, 2016 16:51

asubiotto reviewed Dec 16, 2016

View reviewed changes

cli: add missing username in SQL prompt for connection URL

bd71510

knz mentioned this pull request Jan 5, 2017

cli: missing username in connection URL prevents username from appearing in prompt #10863

Closed

pmamatsis mentioned this pull request Jan 29, 2017

cli: show the current database name in the sql prompt. #12747

Merged

pmamatsis mentioned this pull request Feb 1, 2017

Show current database in sql prompt #12038

Closed

pmamatsis mentioned this pull request Feb 7, 2017

cli: show the current database name in the prompt. #13379

Merged

knz mentioned this pull request Feb 20, 2017

sql: make SHOW and SET more consistent. #13678

Merged

knz closed this Apr 12, 2017

Conversation

a6802739 commented Dec 15, 2016 • edited by rjnn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knz commented Dec 15, 2016

Uh oh!

asubiotto left a comment

Choose a reason for hiding this comment

Uh oh!

asubiotto Dec 16, 2016

Choose a reason for hiding this comment

Uh oh!

a6802739 Dec 20, 2016

Choose a reason for hiding this comment

Uh oh!

knz commented Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a6802739 commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knz commented Dec 20, 2016

Uh oh!

a6802739 commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knz commented Jan 5, 2017

Uh oh!

knz commented Jan 5, 2017

Uh oh!

pmamatsis commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmamatsis commented Jan 5, 2017 • edited by knz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knz commented Jan 5, 2017

Uh oh!

pmamatsis commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a6802739 commented Jan 8, 2017

Uh oh!

pmamatsis commented Jan 31, 2017

Uh oh!

knz commented Feb 2, 2017

Uh oh!

pmamatsis commented Feb 19, 2017

Uh oh!

knz commented Feb 19, 2017

Uh oh!

knz commented Feb 19, 2017

Uh oh!

pmamatsis commented Feb 19, 2017

Uh oh!

knz commented Feb 20, 2017

Uh oh!

pmamatsis commented Feb 20, 2017

Uh oh!

knz commented Feb 20, 2017

Uh oh!

knz commented Feb 20, 2017

Uh oh!

pmamatsis commented Mar 11, 2017

Uh oh!

knz commented Mar 13, 2017

Uh oh!

pmamatsis commented Mar 18, 2017

Uh oh!

pmamatsis commented Mar 18, 2017

Uh oh!

knz commented Mar 18, 2017

Uh oh!

knz commented Apr 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

a6802739 commented Dec 15, 2016 •

edited by rjnn

Loading

knz commented Dec 16, 2016 •

edited

Loading

a6802739 commented Dec 20, 2016 •

edited

Loading

a6802739 commented Dec 21, 2016 •

edited

Loading

pmamatsis commented Jan 5, 2017 •

edited

Loading

pmamatsis commented Jan 5, 2017 •

edited by knz

Loading

pmamatsis commented Jan 5, 2017 •

edited

Loading