Skip to content

Commit

Permalink
deprecate, join, preload, eager load of instance dependent associations.
Browse files Browse the repository at this point in the history
Closes #15024.

These operations happen before instances are created.
The current behavior is misleading and can result in broken behavior.
  • Loading branch information
senny committed May 10, 2014
1 parent f55f5e2 commit ed56e59
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 4 deletions.
9 changes: 9 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
* Deprecate joining, eager loading and preloading of instance dependent
associations without replacement. These operations happen before instances
are created. The current behavior is unexpected and can result in broken
behavior.

Fixes #15024.

*Yves Senn*

* Fixed HABTM's CollectionAssociation size calculation.

HABTM should fall back to using the normal CollectionAssociation's size
Expand Down
4 changes: 4 additions & 0 deletions activerecord/lib/active_record/associations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,10 @@ def association_instance_set(name, association)
# has_many :birthday_events, ->(user) { where starts_on: user.birthday }, class_name: 'Event'
# end
#
# Note: Joining, eager loading and preloading of these associations is not fully possibly.

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov May 10, 2014

Contributor

Possibly a typo?

... fully possibly. -> ... fully possible.

This comment has been minimized.

Copy link
@senny

senny May 10, 2014

Author Member

yea... fixed with adcba34

This comment has been minimized.

Copy link
@senny

senny May 10, 2014

Author Member

@gsamokovarov thanks 💛

# These operations happen before instance creation. The scope will be called with a +nil+ argument.
# This can lead to unexpected behavior and is deprecated.
#
# == Association callbacks
#
# Similar to the normal callbacks that hook into the life cycle of an Active Record object,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ def build(associations, base_klass)
associations.map do |name, right|
reflection = find_reflection base_klass, name
reflection.check_validity!
reflection.check_eager_loadable!

if reflection.options[:polymorphic]
raise EagerLoadPolymorphicError.new(reflection)
Expand Down
1 change: 1 addition & 0 deletions activerecord/lib/active_record/associations/preloader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ def preloader_for(reflection, owners, rhs_klass)
if owners.first.association(reflection.name).loaded?
return AlreadyLoaded
end
reflection.check_preloadable!

case reflection.macro
when :has_many
Expand Down
14 changes: 14 additions & 0 deletions activerecord/lib/active_record/reflection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,20 @@ def check_validity_of_inverse!
end
end

def check_preloadable!
return unless scope

if scope.arity > 0
ActiveSupport::Deprecation.warn <<-WARNING
The association scope '#{name}' is instance dependent (the scope block takes an argument).
Preloading happens before the individual instances are created. This means that there is no instance
being passed to the association scope. This will most likely result in broken or incorrect behavior.
Joining, Preloading and eager loading of these associations is deprecated and will be removed in the future.
WARNING
end
end
alias :check_eager_loadable! :check_preloadable!

def join_id_for(owner) #:nodoc:
key = (source_macro == :belongs_to) ? foreign_key : active_record_primary_key
owner[key]
Expand Down
31 changes: 27 additions & 4 deletions activerecord/test/cases/associations/eager_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -826,11 +826,15 @@ def test_limited_eager_with_numeric_in_association
end

def test_preload_with_interpolation
post = Post.includes(:comments_with_interpolated_conditions).find(posts(:welcome).id)
assert_equal [comments(:greetings)], post.comments_with_interpolated_conditions
assert_deprecated do
post = Post.includes(:comments_with_interpolated_conditions).find(posts(:welcome).id)
assert_equal [comments(:greetings)], post.comments_with_interpolated_conditions
end

post = Post.joins(:comments_with_interpolated_conditions).find(posts(:welcome).id)
assert_equal [comments(:greetings)], post.comments_with_interpolated_conditions
assert_deprecated do
post = Post.joins(:comments_with_interpolated_conditions).find(posts(:welcome).id)
assert_equal [comments(:greetings)], post.comments_with_interpolated_conditions
end
end

def test_polymorphic_type_condition
Expand Down Expand Up @@ -1232,4 +1236,23 @@ def test_deep_including_through_habtm
assert_equal 2, author.posts.size
}
end

test "include instance dependent associations is deprecated" do
message = "association scope 'posts_with_signature' is"
assert_deprecated message do
begin
Author.includes(:posts_with_signature).to_a
rescue NoMethodError
# it's expected that preloading of this association fails
end
end

assert_deprecated message do
Author.preload(:posts_with_signature).to_a rescue NoMethodError
end

assert_deprecated message do
Author.eager_load(:posts_with_signature).to_a
end
end
end

5 comments on commit ed56e59

@senny
Copy link
Member Author

@senny senny commented on ed56e59 May 10, 2014

Choose a reason for hiding this comment

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

/cc @matthewd

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

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

Seems good

@arthurnn
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

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

@senny when removing this deprecation should we raise an exception when arity is greater than 0?

@senny
Copy link
Member Author

@senny senny commented on ed56e59 Jan 5, 2015

Choose a reason for hiding this comment

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

Please sign in to comment.