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

Connect to DB whose name consists entirely of numerics #425

Closed

Conversation

BanzaiMan
Copy link
Contributor

If the database name one wishes to connect to has only numeric, then the attempt fails with:

irb(main):008:0> client = Mysql2::Client.new(:host => '127.1.244.2', :database => 12345)                      
TypeError: can't convert Fixnum into String
        from /var/lib/openshift/524c65f6cd178a4609000113/.gem/gems/mysql2-0.3.13/lib/mysql2/client.rb:58:in `connect'
        from /var/lib/openshift/524c65f6cd178a4609000113/.gem/gems/mysql2-0.3.13/lib/mysql2/client.rb:58:in `initialize'
        from (irb):8:in `new'
        from (irb):8
        from /opt/rh/ruby193/root/usr/bin/irb:12:in `<main>'

A similar database works fine with the pg gem (configured via config/database.yml for a Rails app), so it might be a good idea to allow it.

@brianmario
Copy link
Owner

We should just go ahead and do this for pass, host, and socket as well.

@sodabrew
Copy link
Collaborator

sodabrew commented Oct 2, 2013

+1 nice work @BanzaiMan – I was just about to say what @brianmario said – please add .to_s on the other stringy options.

@brianmario
Copy link
Owner

Would you mind adding a test to make sure you can pass integers for these values now?

@BanzaiMan
Copy link
Contributor Author

@brianmario Not at all. I'll work on the tests tonight. Thanks.

@BanzaiMan
Copy link
Contributor Author

Please advise if the spec setup (d8aaac9) can be improved.

@BanzaiMan
Copy link
Contributor Author

By the way, socket can be nil, in which case #to_s will blow up the logic. We can either: a) not send #to_s to it, or b) wrap this in an unless socket.nil? statement. I chose the former strategy here.

@sodabrew
Copy link
Collaborator

sodabrew commented Oct 4, 2013

Oh, that's a really good point. Most of those value could be nil. We sort of want a ".to_s_or_nil" function.

@sodabrew
Copy link
Collaborator

sodabrew commented Oct 4, 2013

This is super ugly, but how about (edited - let's use your latter suggestion instead):

diff --git a/lib/mysql2/client.rb b/lib/mysql2/client.rb
index 5512368..e0a4d7f 100644
--- a/lib/mysql2/client.rb
+++ b/lib/mysql2/client.rb
@@ -56,6 +56,14 @@ module Mysql2
       socket   = opts[:socket] || opts[:sock]
       flags    = opts[:flags] ? opts[:flags] | @query_options[:connect_flags] : @query_options[:connect_flags]

+      # Correct the data types before passing these values down to the C level
+      user = user.to_s unless user.nil?
+      pass = pass.to_s unless pass.nil?
+      host = host.to_s unless host.nil?
+      port = port.to_i unless port.nil?
+      database = database.to_s unless database.nil?
+      socket = socket.to_s unless socket.nil?
+
       connect user, pass, host, port, database, socket, flags
     end

@brianmario
Copy link
Owner

That'll work

On Fri, Oct 4, 2013 at 11:43 AM, Aaron Stone notifications@github.comwrote:

This is super ugly, but how about:

diff --git a/lib/mysql2/client.rb b/lib/mysql2/client.rb
index 5512368..e0a4d7f 100644
--- a/lib/mysql2/client.rb
+++ b/lib/mysql2/client.rb
@@ -56,6 +56,14 @@ module Mysql2
socket = opts[:socket] || opts[:sock]
flags = opts[:flags] ? opts[:flags] | @query_options[:connect_flags] : @query_options[:connect_flags]

  •  # Correct the data types before passing these values down to the C level
    
  •  user = user.nil? ? nil : user.to_s
    
  •  pass = pass.nil? ? nil : pass.to_s
    
  •  host = host.nil? ? nil : host.to_s
    
  •  port = port.nil? ? nil : port.to_i
    
  •  database = database.nil? ? nil : database.to_s
    
  •  socket = socket.nil? ? nil : socket.to_s
    
    • connect user, pass, host, port, database, socket, flags
      end


Reply to this email directly or view it on GitHubhttps://github.com//pull/425#issuecomment-25721394
.

@BanzaiMan
Copy link
Contributor Author

I haven't tested it, so I'll simply ask: can port take a String, rather than a Fixnum?

https://github.com/brianmario/mysql2/blob/926d579/ext/mysql2/client.c#L289 gives me a pause.

@sodabrew
Copy link
Collaborator

sodabrew commented Oct 4, 2013

Nope, port must be an integer: http://dev.mysql.com/doc/refman/5.0/en/mysql-real-connect.html

@BanzaiMan
Copy link
Contributor Author

@sodabrew Oh, I see. You used #to_i. My bad. Carry on.

@sodabrew
Copy link
Collaborator

sodabrew commented Oct 4, 2013

Awesome, I'll close this and merge #427. Thanks for the tests and the initial fix @BanzaiMan!

@sodabrew sodabrew closed this Oct 4, 2013
@BanzaiMan BanzaiMan deleted the connect_numeric_only_database branch October 5, 2013 02:09
@sodabrew
Copy link
Collaborator

sodabrew commented Oct 6, 2013

@BanzaiMan I realized the more obvious solution to the underlying issue a few days late. With YAML, you simply have to quote numbers to prevent it from being cast to a numeric type, e.g.:

production:
  username: "12345"
  password: "67890"

I'm skeptical of whether it is right to support this:
client = Mysql2::Client.new(:host => '127.1.244.2', :database => 12345)

If the code can be simply changed as:
client = Mysql2::Client.new(:host => '127.1.244.2', :database => "12345")

Not sure if I would propose reverting the change; consistency with other db drivers (like pg, as you noted) is somewhat nice.

@BanzaiMan
Copy link
Contributor Author

Indeed, that is a workaround for the issue that led me to this issue in the first place. I opened this issue precisely for the parity with pg that this function will give to mysql2. I won't be too upset if you decide to revert, but do keep in mind that some users may have this expectation.

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.

None yet

3 participants