Access an instance's connection via its class, rather than via #connection #9371

Merged
merged 1 commit into from Mar 9, 2013

6 participants

@benmoss

Had a problem today where we had a belongs_to :connection on our model. Hadn't occurred to me that this might be a problem, but when we ran our tests we were getting an obscure undefined method substitute_at for nil:NilClass exception whenever an instance of this class was being destroyed.

Discovered the root cause was this method, which was attempting to call connection on the instance, which in turn delegates to the class. This seems like a pretty safe change to make, I can't think of any possible repercussions. I wasn't sure if this warranted a regression test, but I'd be happy to provide one if it's required.

@senny
Ruby on Rails member

not sure about the test either but we need a CHANGELOG entry for sure. I'm leaning towards creating a test.

@benmoss

Added a test and a CHANGELOG entry

@senny senny commented on an outdated diff Feb 24, 2013
activerecord/test/cases/persistence_test.rb
@@ -315,6 +315,11 @@ def test_destroy
assert_raise(ActiveRecord::RecordNotFound) { Topic.find(topic.id) }
end
+ def test_destroy_doesnt_rely_on_connection_instance_method
+ warehouse = WarehouseThing.find(1)
+ assert_nothing_raised(NoMethodError) { warehouse.destroy }
@senny
Ruby on Rails member
senny added a line comment Feb 24, 2013

maybe we should assert here that #connection returns "a connection" and supply a description in the assertion message why this is relevant. Otherwise the connection method could be easily removed from the model and we could run back into regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@benmoss

I experimented with removing the connection instance method from core and these were the only other places in the codebase that seemed to rely on it:

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb#L22
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb#L29
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb#L44
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/locking/optimistic.rb#L89
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/locking/optimistic.rb#L120

It seems like it only makes sense to try to make the earlier change if I change all these others as well, otherwise it'll only be a matter of time until they likely bite me or someone else. Do you think this is worthwhile? It still seems like it's going to be hard to protect against others introducing new code that relies on it, the only sure-fire way to remove it as a 'reserved method' would be to rename it or remove it entirely, but both of those seem like too big of changes to the AR interface.

@rafaelfranca
Ruby on Rails member

I think you can change all these places, but we can't protect for introducing new code without removing connection from the instance. We can't do this without a deprecation cycle.

@jonleighton @tenderlove WDYT about removing connection method from instance?

@jonleighton
Ruby on Rails member

I am in favour. I've encountered this problem myself.

@rafaelfranca
Ruby on Rails member

@benmoss mind to update your pull request deprecating connection at instance level and removing all the use in the Rails codebase?

@benmoss

Sure, awesome!

@steveklabnik
Ruby on Rails member

Neat.

@benmoss

Any word on this?

@jonleighton
Ruby on Rails member

@benmoss I'll merge this, but it needs a rebase. Please comment when you have rebased as github doesn't send a notification when there's a code update.

@benmoss

Rebased!

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Mar 9, 2013
activerecord/CHANGELOG.md
@@ -1717,5 +1717,8 @@
*Aaron Patterson*
+* `connection` is deprecated as an instance method.
+
+ *Ben Moss*
@carlosantoniodasilva
Ruby on Rails member
carlosantoniodasilva added a line comment Mar 9, 2013

Can you please move this to the top of the changelog, right after the "unreleased" title?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Mar 9, 2013
activerecord/test/cases/fixtures_test.rb
@@ -477,11 +477,6 @@ class CustomConnectionFixturesTest < ActiveRecord::TestCase
fixtures :courses
self.use_transactional_fixtures = false
- def test_connection
- assert_kind_of Course, courses(:ruby)
- assert_equal Course.connection, courses(:ruby).connection
- end
@carlosantoniodasilva
Ruby on Rails member
carlosantoniodasilva added a line comment Mar 9, 2013

It'd be better to change the test to ensure it's deprecated instead:

def test_connection_instance_method_deprecation
  assert_deprecated { courses(:ruby).connection }
end

Or something like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@benmoss

Fixed! Sorry about that.

@carlosantoniodasilva
Ruby on Rails member

@benmoss no problem, looks good to me now. Thanks.

@jonleighton all yours :)

@jonleighton
Ruby on Rails member

@benmoss could you add the reasoning for the change to your commit message / the changelog? thanks

@benmoss

Done

@benmoss benmoss Deprecate #connection in favour of accessing it via the class
This allows end-users to have a `connection` method on their models
without clashing with ActiveRecord internals.
992d87d
@jonleighton jonleighton merged commit 15970ef into rails:master Mar 9, 2013
@yahonda yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Mar 10, 2013
@yahonda yahonda Deprecate #connection in favour of accessing it via the class 8fed415
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment