-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cli: fix username semantics for userfile #56046
cli: fix username semantics for userfile #56046
Conversation
dfb13a4
to
707a8b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz)
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but please revert the now-unnecessary changes
Reviewed 2 of 3 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru)
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had misread. This is fine as is. Go ahead.
Reviewed 1 of 3 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru)
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