cli: fix username semantics for userfile#56046
Conversation
|
This change is |
dfb13a4 to
707a8b5
Compare
knz
left a comment
There was a problem hiding this comment.
Very nice! 💯
Reviewed 5 of 5 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @adityamaru)
pkg/cli/userfile.go, line 182 at r1 (raw file):
// - Support for all current and future valid usernames. func getDefaultQualifiedTableName(user security.SQLUsername) string { if !lexbase.IsRestrictedSQLIdentQuotedOnEncode(user.Normalized()) {
-
to ease readability, what about a
username := user.Normalized()at the beginning then reuse that. -
can you explain why you care whether the username is a reserved keyword? When concatenated with
username_, its reserved nature disappears because of the prefix. What do you think?
707a8b5 to
e988766
Compare
adityamaru
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cli/userfile.go, line 182 at r1 (raw file):
Previously, knz (kena) wrote…
to ease readability, what about a
username := user.Normalized()at the beginning then reuse that.can you explain why you care whether the username is a reserved keyword? When concatenated with
username_, its reserved nature disappears because of the prefix. What do you think?
good point, i didn't think of that. Switched to just checking whether it is a bare identifier or not.
knz
left a comment
There was a problem hiding this comment.
Reviewed 2 of 3 files at r2.
Reviewable status:
pkg/sql/lexbase/encode.go, line 54 at r2 (raw file):
// keyword. func EncodeRestrictedSQLIdent(buf *bytes.Buffer, s string, flags EncodeFlags) { if flags.HasFlags(EncBareIdentifiers) || (!isReservedKeyword(s) && IsBareIdentifier(s)) {
you can revert the changes to this file now
pkg/sql/lexbase/predicates.go, line 72 at r2 (raw file):
// IsBareIdentifier returns true if the input string is a permissible bare SQL // identifier. func IsBareIdentifier(s string) bool {
ditto
|
I had to export IsBareIdentifier() so that I could use it in the userfile logic. The changes to those files are all related to that. Am I misunderstanding something? |
knz
left a comment
There was a problem hiding this comment.
I had misread. This is fine as is. Go ahead.
Reviewed 1 of 3 files at r2.
Reviewable status:
Previously, userfile would break if a user had a username with special characters, which is otherwise supported by CRDB. This is because, unless specified, userfile uses the username to generate the underlying storage table names. This change introduces a new name generation scheme which accounts for all usernames supported by the database. The details are explained in: cockroachdb#55389 (comment) Fixes: cockroachdb#55389 Release note: None
e988766 to
e2130f7
Compare
|
TFTR! bors r=knz |
|
Build succeeded: |
Previously, userfile would break if a user had a username with special
characters, which is otherwise supported by CRDB.
This is because, unless specified, userfile uses the username to
generate the underlying storage table names.
This change introduces a new name generation scheme which accounts for
all usernames supported by the database. The details are explained in:
#55389 (comment)
Fixes: #55389
Release note: None