Skip to content
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

Added proper syntax parsing for account names, along with DROP USER #97

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

Hydrocharged
Copy link

@Hydrocharged Hydrocharged commented Dec 6, 2021

This PR primarily implements the changes necessary for account names, which required edits in the parser itself. DROP USER was implemented to test the name variations, so refer to the tests in parse_test.go for how account names work (and to see why the changes were necessary). All parser tests were validated against a MySQL 8.0 instance.

In a nutshell, account names take the form user@host, and both user and host must be separate strings. The parser treated @ as a standard letter, and we used regex matching in GMS and Dolt to catch "invalid" names (@@system_var is valid but what@@ is invalid). My first pass was to treat the entire account name as a string and split it using a regex, however I could not get it to pass all of the parser tests. This was the only way I could make it work with all tests passing.

Notes:

  • The other statements will come in a different PR, which will be straightforward in comparison.
  • DROP USER ''; looks like weird syntax, but an empty name means that all names match. In this case, you can assign some dbs/tables to be available to everyone.
  • Following the above, DROP USER ``; is valid, but failed as we returned an error on empty quoted identifiers. MySQL allows empty quoted identifiers to parse, but rejects them as invalid names depending on the query. This has been fixed.
  • Added ast_permissions.go to put all of the statements dealing with users, grants, etc. For me, ast.go operates very slowly and occasionally hangs for a few minutes, so I'm putting my new statements in a new file for now. I doubt you'll have pushback, but if so I can merge the file back into ast.go when I'm done.

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, just one comment to think about going forward

go/vt/sqlparser/token.go Show resolved Hide resolved
@Hydrocharged Hydrocharged merged commit a909c06 into main Dec 8, 2021
@Hydrocharged Hydrocharged deleted the daylon/users-1 branch December 8, 2021 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants