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

Support MySQL 8 #72

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions spec/driver_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,25 @@ describe Driver do
it "should connect with credentials" do
with_db do |db|
db.scalar("SELECT DATABASE()").should be_nil
db.scalar("SELECT CURRENT_USER()").should match(/^root@/)
db.scalar("SELECT CURRENT_USER()").should match(/^#{database_user}@/)

# ensure user is deleted
db.exec "GRANT USAGE ON *.* TO crystal_test IDENTIFIED BY 'secret'"
db.exec "DROP USER IF EXISTS crystal_test"
db.exec "CREATE USER crystal_test IDENTIFIED WITH mysql_native_password BY 'Secret123!'"
db.exec "GRANT USAGE ON *.* TO crystal_test"
db.exec "DROP USER crystal_test"
db.exec "DROP DATABASE IF EXISTS crystal_mysql_test"
db.exec "FLUSH PRIVILEGES"

# create test db with user
db.exec "CREATE DATABASE crystal_mysql_test"
db.exec "CREATE USER crystal_test IDENTIFIED BY 'secret'"
db.exec "GRANT ALL PRIVILEGES ON crystal_mysql_test.* TO crystal_test"
db.exec "DROP USER IF EXISTS 'crystal_test'@'#{database_host}'"
db.exec "CREATE USER 'crystal_test'@'#{database_host}' IDENTIFIED WITH mysql_native_password BY 'Secret123!'"
db.exec "GRANT ALL PRIVILEGES ON crystal_mysql_test.* TO 'crystal_test'@'#{database_host}'"
db.exec "FLUSH PRIVILEGES"
end

DB.open "mysql://crystal_test:secret@#{database_host}/crystal_mysql_test" do |db|
DB.open "mysql://crystal_test:Secret123!@#{database_host}/crystal_mysql_test" do |db|
db.scalar("SELECT DATABASE()").should eq("crystal_mysql_test")
db.scalar("SELECT CURRENT_USER()").should match(/^crystal_test@/)
end
Expand Down
10 changes: 9 additions & 1 deletion spec/spec_helper.cr
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,17 @@ require "../src/mysql"
include MySql

def db_url(initial_db = nil)
"mysql://root@#{database_host}/#{initial_db}"
"mysql://#{database_user}:#{database_password}@#{database_host}/#{initial_db}"
end

def database_host
ENV.fetch("DATABASE_HOST", "localhost")
end

def database_user
ENV.fetch("DATABASE_USER", "root")
end

def database_password
ENV.fetch("DATABASE_PASSWORD", "crystal_mysql_spec")
end
2 changes: 0 additions & 2 deletions src/mysql/packets.cr
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,6 @@ module MySql::Protocol
def write(packet : MySql::WritePacket)
caps = CLIENT_PROTOCOL_41 | CLIENT_SECURE_CONNECTION | CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA

caps |= CLIENT_PLUGIN_AUTH if @password
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, thanks for the link, I was looking for something like this hahahha

After some research (aka stack overflow hahaha) I found this PR mysqljs/mysql#1962 that is adding support for MySQL 8, I was reading trying to figure out this auth mechanims, but as this is the very first time I'm dealing with db driver, and using mysql after a few years, I'm having a hard time 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line is the main point of the PR... the other modifications are just some 💅 to fix the specs (like the password stuff)

Copy link
Member

Choose a reason for hiding this comment

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

We are on the same page. This was the first binary protocol I implemented for a db driver. I mostly read the docs of mysql internals and compared other source code.

Is hard to have in mind all the evolution of the protocol and even harders try all the possibilities. But the end result is worth it 🚀.

After adding mysql 8 to travis and whenever you feel the PR ready let us know 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are on the same page. This was the first binary protocol I implemented for a db driver. I mostly read the docs of mysql internals and compared other source code.

awesome!! :D

After adding mysql 8 to travis and whenever you feel the PR ready let us know 😃

Sorry but I think I missed something, what about the password for mysql 5.6 and 5.7? Should we remove both versions from travis, or add password to them? hehehe

Copy link
Member

Choose a reason for hiding this comment

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

None 🙈. I would expect that mysql 8 with a blank root password is added.

I think is common for developers to have root with empty passwords for mysql intallations. I would not drop the support for that.

I've just found a bit more on information regarding the new auth mechanism in mysql 8: caching_sha2_password

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hahahah sure!! that's why I asked, I misunderstood... now I need to figure out how to enable this support in mysql 8

How do you think we should handle this in the design of the driver, leave the current HandshakeResponse41 and use other classes (that inherits from this one), based on protocol_version / version? Because do some "ifs" on the #write method will fastly become a monster hahah

anyway, I'm gonna draft something here when I figure out how to do this and commit here 😉


caps |= CLIENT_CONNECT_WITH_DB if @initial_catalog

packet.write_bytes caps, IO::ByteFormat::LittleEndian
Expand Down