Merge cockroachdb#45575

45575: cli: reveal SQL error details/hints in CLI commands r=ajwerner a=knz

Found while investigating cockroachdb#43114. First commit from that PR.

Previously, when a CLI command that uses SQL under the hood
encountered an error, only the main error message was displayed.

This patch changes it to reveal the other additional user-visible
fields.

Release note (cli change): Certain CLI command now provide more
details and/or a hint when they encounter an error.

Co-authored-by: Raphael 'kena' Poss <[email protected]>

pkg/cli/cli_test.go

@@ -1339,7 +1339,9 @@ func Example_cert() {
 	// cert create-client Ομηρος
 	// cert create-client 0foo
 	// cert create-client ,foo
-	// failed to generate client certificate and key: username ",foo" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters
+	// ERROR: failed to generate client certificate and key: username ",foo" invalid
+	// SQLSTATE: 42602
+	// HINT: Usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, periods, or underscores, and must not exceed 63 characters.
 }
 
 // TestFlagUsage is a basic test to make sure the fragile

pkg/cli/error.go

@@ -182,13 +182,23 @@ var reGRPCConnFailed = regexp.MustCompile(`desc = (transport is closing|all SubC
 func MaybeDecorateGRPCError(
 	wrapped func(*cobra.Command, []string) error,
 ) func(*cobra.Command, []string) error {
-	return func(cmd *cobra.Command, args []string) error {
-		err := wrapped(cmd, args)
+	return func(cmd *cobra.Command, args []string) (err error) {
+		err = wrapped(cmd, args)
 
 		if err == nil {
 			return nil
 		}
 
+		defer func() {
+			// We want to flatten the error to reveal the hints, details etc.
+			// However we can't do it twice, so we need to detect first if
+			// some code already added the formattedError{} wrapper.
+			var f *formattedError
+			if !errors.As(err, &f) {
+				err = &formattedError{err: err, showSeverity: true}
+			}
+		}()
+
 		extraInsecureHint := func() string {
 			extra := ""
 			if baseCfg.Insecure {
@@ -256,8 +266,9 @@ func MaybeDecorateGRPCError(
 			if wErr.Code == pgcode.ProtocolViolation {
 				return connSecurityHint()
 			}
-			// Otherwise, there was a regular SQL error. Just report that.
-			return wErr
+			// Otherwise, there was a regular SQL error. Just report
+			// that.
+			return err
 		}
 
 		if wErr := (*net.OpError)(nil); errors.As(err, &wErr) {

pkg/sql/create_user.go

@@ -196,10 +196,10 @@ func (*CreateUserNode) Close(context.Context) {}
 // FastPathResults implements the planNodeFastPath interface.
 func (n *CreateUserNode) FastPathResults() (int, bool) { return n.run.rowsAffected, true }
 
-const usernameHelp = "usernames are case insensitive, must start with a letter, " +
-	"digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters"
+const usernameHelp = "Usernames are case insensitive, must start with a letter, " +
+	"digit or underscore, may contain letters, digits, dashes, periods, or underscores, and must not exceed 63 characters."
 
-var usernameRE = regexp.MustCompile(`^[\p{Ll}0-9_][\p{Ll}0-9_-]{0,62}$`)
+var usernameRE = regexp.MustCompile(`^[\p{Ll}0-9_][---\p{Ll}0-9_.]+$`)
 
 var blacklistedUsernames = map[string]struct{}{
 	security.NodeUser: {},
@@ -214,7 +214,7 @@ func NormalizeAndValidateUsername(username string) (string, error) {
 		return "", err
 	}
 	if _, ok := blacklistedUsernames[username]; ok {
-		return "", errors.Errorf("username %q reserved", username)
+		return "", pgerror.Newf(pgcode.ReservedName, "username %q reserved", username)
 	}
 	return username, nil
 }
@@ -224,7 +224,10 @@ func NormalizeAndValidateUsername(username string) (string, error) {
 func NormalizeAndValidateUsernameNoBlacklist(username string) (string, error) {
 	username = tree.Name(username).Normalize()
 	if !usernameRE.MatchString(username) {
-		return "", errors.Errorf("username %q invalid; %s", username, usernameHelp)
+		return "", errors.WithHint(pgerror.Newf(pgcode.InvalidName, "username %q invalid", username), usernameHelp)
+	}
+	if len(username) > 63 {
+		return "", errors.WithHint(pgerror.Newf(pgcode.NameTooLong, "username %q is too long", username), usernameHelp)
 	}
 	return username, nil
 }

pkg/sql/create_user_test.go

@@ -0,0 +1,63 @@
+// Copyright 2020 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package sql_test
+
+import (
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/sql"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+)
+
+func TestUserName(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	testCases := []struct {
+		username   string
+		normalized string
+		err        string
+		sqlstate   string
+	}{
+		{"Abc123", "abc123", "", ""},
+		{"0123121132", "0123121132", "", ""},
+		{"HeLlO", "hello", "", ""},
+		{"Ομηρος", "ομηρος", "", ""},
+		{"_HeLlO", "_hello", "", ""},
+		{"a-BC-d", "a-bc-d", "", ""},
+		{"A.Bcd", "a.bcd", "", ""},
+		{"WWW.BIGSITE.NET", "www.bigsite.net", "", ""},
+		{"", "", `username "" invalid`, pgcode.InvalidName},
+		{"-ABC", "", `username "-abc" invalid`, pgcode.InvalidName},
+		{".ABC", "", `username ".abc" invalid`, pgcode.InvalidName},
+		{"*.wildcard", "", `username "\*.wildcard" invalid`, pgcode.InvalidName},
+		{"foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoof", "", `username "foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoof" is too long`, pgcode.NameTooLong},
+	}
+
+	for _, tc := range testCases {
+		normalized, err := sql.NormalizeAndValidateUsername(tc.username)
+		if !testutils.IsError(err, tc.err) {
+			t.Errorf("%q: expected %q, got %v", tc.username, tc.err, err)
+			continue
+		}
+		if err != nil {
+			if pgcode := pgerror.GetPGCode(err); pgcode != tc.sqlstate {
+				t.Errorf("%q: expected SQLSTATE %s, got %s", tc.username, tc.sqlstate, pgcode)
+				continue
+			}
+		}
+		if normalized != tc.normalized {
+			t.Errorf("%q: expected %q, got %q", tc.username, tc.normalized, normalized)
+		}
+	}
+}

pkg/sql/logictest/testdata/logic_test/drop_user

@@ -54,7 +54,7 @@ DROP USER IF EXISTS user1
 statement error username "node" reserved
 DROP USER node
 
-statement error pq: username "foo☂" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters
+statement error pq: username "foo☂" invalid
 DROP USER foo☂
 
 statement ok

pkg/sql/logictest/testdata/logic_test/user

@@ -48,10 +48,10 @@ CREATE USER uSEr2 WITH PASSWORD 'cockroach'
 statement ok
 CREATE USER user3 WITH PASSWORD '蟑螂'
 
-statement error pq: username "foo☂" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters
+statement error pq: username "foo☂" invalid
 CREATE USER foo☂
 
-statement error pq: username "-foo" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters
+statement error pq: username "-foo" invalid
 CREATE USER "-foo"
 
 statement error at or near "-": syntax error