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

`inspect` for AR model classes does not initiate a new connection. #11014

Merged

Conversation

@senny
Copy link
Member

@senny senny commented Jun 19, 2013

Closes #10936

The behavior of Model.inspect with a missing database connection was different for each adapter:

sqlite3

irb(main):001:0> User.inspect
=> "User(Table doesn't exist)"

postgresql

irb(main):001:0> User.inspect
PG::Error: FATAL:  database "i_do_not_exist" does not exist

mysql2

irb(main):001:0> User.inspect
Mysql2::Error: Access denied for user 'myuser'@'localhost' (using password: YES)

This patch changes the implementation of inspect to not initiate a connection and simply return "not connected" if no connection is established. This prevents the exceptions on inspect and should still be expressive enough.

@senny
Copy link
Member Author

@senny senny commented Jun 19, 2013

@rafaelfranca @neerajdotname can you take a look?

@egilburg
egilburg reviewed Jun 19, 2013
View changes
activerecord/CHANGELOG.md Outdated

Example:

Author.inspect # => "Author(not connected)""

This comment has been minimized.

@egilburg

egilburg Jun 19, 2013
Contributor

extra " here

@egilburg
egilburg reviewed Jun 19, 2013
View changes
activerecord/lib/active_record/core.rb Outdated
@@ -123,6 +123,8 @@ def inspect
super
elsif abstract_class?
"#{super}(abstract)"
elsif !connected?
"#{super}(not connected)"

This comment has been minimized.

@egilburg

egilburg Jun 19, 2013
Contributor

I think "#{super} (no database connection)" or "#{super} (not connected to database)" will explain it better.

@drogus
Copy link
Member

@drogus drogus commented Jun 21, 2013

Looks good to me, thanks!

drogus added a commit that referenced this pull request Jun 21, 2013
…hout_connection

`inspect` for AR model classes does not initiate a new connection.
@drogus drogus merged commit 5ac2298 into rails:master Jun 21, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@qqshfox
Copy link
Contributor

@qqshfox qqshfox commented Oct 12, 2013

I try to get the User.inspect after I called establish_connection, and it still showed no db connection.

ActiveRecord::Base.establish_connection :development # `adapter: mysql2`
User.inspect #=> User(no database connection)

There was no connection made actually

User.connected? #=> false
ActiveRecord::Base.connected? #=> false

After triggering the connection initialization manually

ActiveRecord::Base.connection
ActiveRecord::Base.connected? #=> true
User.connected? #=> true

or

User.first
User.connected? #=> true

then succeeded to get the User scheme

User.inspect #=> User(id: integer)

It's just a little bit inconvenient to call ActiveRecord::Base.connection to init the connection if I want to get the scheme.

I think it's got nothing to do with Base.inspect but how the connection is made. So I try to patch the ConnectionHandling.establish_connection

def establish_connection(spec = ENV["DATABASE_URL"])
  resolver = ConnectionAdapters::ConnectionSpecification::Resolver.new spec, configurations
  spec = resolver.spec

  unless respond_to?(spec.adapter_method)
    raise AdapterNotFound, "database configuration specifies nonexistent #{spec.config[:adapter]} adapter"
  end

  remove_connection
  connection_handler.establish_connection(self, spec).tap { retrieve_connection } # origin: connection_handler.establish_connection self, spec
end

But I don't think it's a good thing. Any suggestion here?

Thanks,
Hanfei

@robin850
Copy link
Member

@robin850 robin850 commented Oct 12, 2013

I can confirm this behavior with the following gist. I don't know ActiveRecord so much but as far as I can see, ActiveRecord is not establishing the connection to the database until you really need it. This leads to an unexpected behavior so I think the fix should be at the ActiveRecord::Base#connected? level ; it should check whether a connection could be triggered so it establishes it and returns the right value Could you give a try to such a patch ?

@qqshfox
Copy link
Contributor

@qqshfox qqshfox commented Oct 12, 2013

Before patching ConnectionHandling.establish_connection

ActiveRecord: 4.0.0
Run options: --seed 53183

# Running tests:

F

Finished tests in 0.008572s, 116.6589 tests/s, 233.3178 assertions/s.

  1) Failure:
HasManyDeleteIfTest#test_validations_for_aliased_columns [t.rb:33]:
Failed assertion, no message given.

1 tests, 2 assertions, 1 failures, 0 errors, 0 skips

After patching, it passed.

ActiveRecord: 4.0.0
Run options: --seed 34196

# Running tests:

-- create_table(:users)
D, [2013-10-12T19:01:35.729567 #27174] DEBUG -- :    (0.6ms)  CREATE TABLE "users" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL)
   -> 0.0041s
.

Finished tests in 0.014912s, 67.0601 tests/s, 201.1803 assertions/s.

1 tests, 3 assertions, 0 failures, 0 errors, 0 skips
@robin850
Copy link
Member

@robin850 robin850 commented Oct 12, 2013

I agree but what your patch is doing is that it retrieve the connection in any case. It means that when you call establish_connection, you are connected to the database even if you do not need it and I think that this is unexpected for performances issues but I will wait for feedback before saying more bullshit if I'm wrong. 😄

@qqshfox
Copy link
Contributor

@qqshfox qqshfox commented Oct 12, 2013

I agree with you. Not only am I not a ActiveRecord guy, I don't think it's a good approach either. It was just a bad example. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.