Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Rename Reflection#through_reflection_chain and #through_options to Re…

…flection#chain and Reflection#options as they now no longer relate solely to through associations.
  • Loading branch information...
commit 2d3d9e3531d0d49a94ded10b993640053bd76c03 1 parent 6490d65
@jonleighton jonleighton authored
View
12 activerecord/lib/active_record/associations/association_scope.rb
@@ -4,7 +4,7 @@ class AssociationScope #:nodoc:
attr_reader :association, :alias_tracker
delegate :klass, :owner, :reflection, :interpolate, :to => :association
- delegate :through_reflection_chain, :through_conditions, :options, :source_options, :to => :reflection
+ delegate :chain, :conditions, :options, :source_options, :to => :reflection
def initialize(association)
@association = association
@@ -50,7 +50,7 @@ def select_value
def add_constraints(scope)
tables = construct_tables
- through_reflection_chain.each_with_index do |reflection, i|
+ chain.each_with_index do |reflection, i|
table, foreign_table = tables.shift, tables.first
if reflection.source_macro == :has_and_belongs_to_many
@@ -73,10 +73,10 @@ def add_constraints(scope)
foreign_key = reflection.active_record_primary_key
end
- if reflection == through_reflection_chain.last
+ if reflection == chain.last
scope = scope.where(table[key].eq(owner[foreign_key]))
- through_conditions[i].each do |condition|
+ conditions[i].each do |condition|
if options[:through] && condition.is_a?(Hash)
condition = { table.name => condition }
end
@@ -86,7 +86,7 @@ def add_constraints(scope)
else
constraint = table[key].eq foreign_table[foreign_key]
- join = inner_join(foreign_table, reflection, constraint, *through_conditions[i])
+ join = inner_join(foreign_table, reflection, constraint, *conditions[i])
scope = scope.joins(join)
end
end
@@ -96,7 +96,7 @@ def add_constraints(scope)
def construct_tables
tables = []
- through_reflection_chain.each do |reflection|
+ chain.each do |reflection|
tables << alias_tracker.aliased_table_for(
table_name_for(reflection),
table_alias_for(reflection, reflection != self.reflection)
View
16 activerecord/lib/active_record/associations/join_dependency/join_association.rb
@@ -22,7 +22,7 @@ class JoinAssociation < JoinPart # :nodoc:
attr_reader :tables
- delegate :options, :through_reflection, :source_reflection, :through_reflection_chain, :to => :reflection
+ delegate :options, :through_reflection, :source_reflection, :chain, :to => :reflection
delegate :table, :table_name, :to => :parent, :prefix => :parent
delegate :alias_tracker, :to => :join_dependency
@@ -57,14 +57,12 @@ def find_parent_in(other_join_dependency)
end
def join_to(relation)
- # The chain starts with the target table, but we want to end with it here (makes
- # more sense in this context)
- chain = through_reflection_chain.reverse
-
foreign_table = parent_table
index = 0
- chain.each do |reflection|
+ # The chain starts with the target table, but we want to end with it here (makes
+ # more sense in this context), so we reverse
+ chain.reverse.each do |reflection|
table = tables[index]
conditions = []
@@ -178,7 +176,7 @@ def table_alias_for(reflection, join = false)
# later generate joins for. We must do this in advance in order to correctly allocate
# the proper alias.
def setup_tables
- @tables = through_reflection_chain.map do |reflection|
+ @tables = chain.map do |reflection|
table = alias_tracker.aliased_table_for(
reflection.table_name,
table_alias_for(reflection, reflection != self.reflection)
@@ -200,7 +198,7 @@ def setup_tables
end
end
- # The joins are generated from the through_reflection_chain in reverse order, so
+ # The joins are generated from the chain in reverse order, so
# reverse the tables too (but it's important to generate the aliases in the 'forward'
# order, which is why we only do the reversal now.
@tables.reverse!
@@ -219,7 +217,7 @@ def sanitize_sql(condition, table_name)
end
def reflection_conditions(index, table)
- reflection.through_conditions.reverse[index].map do |condition|
+ reflection.conditions.reverse[index].map do |condition|
process_conditions(condition, table.table_alias || table.name)
end
end
View
5 activerecord/lib/active_record/associations/through_association.rb
@@ -3,8 +3,7 @@ module ActiveRecord
module Associations
module ThroughAssociation #:nodoc:
- delegate :source_options, :through_options, :source_reflection, :through_reflection,
- :through_reflection_chain, :through_conditions, :to => :reflection
+ delegate :source_reflection, :through_reflection, :chain, :to => :reflection
protected
@@ -18,7 +17,7 @@ module ThroughAssociation #:nodoc:
# itself.
def target_scope
scope = super
- through_reflection_chain[1..-1].each do |reflection|
+ chain[1..-1].each do |reflection|
scope = scope.merge(reflection.klass.scoped)
end
scope
View
60 activerecord/lib/active_record/reflection.rb
@@ -262,23 +262,28 @@ def check_validity_of_inverse!
end
def through_reflection
- false
+ nil
+ end
+
+ def source_reflection
+ nil
end
- def through_reflection_chain
+ # A chain of reflections from this one back to the owner. For more see the explanation in
+ # ThroughReflection.
+ def chain
[self]
end
- def through_conditions
+ # An array of arrays of conditions. Each item in the outside array corresponds to a reflection
+ # in the #chain. The inside arrays are simply conditions (and each condition may itself be
+ # a hash, array, arel predicate, etc...)
+ def conditions
conditions = [options[:conditions]].compact
conditions << { type => active_record.base_class.name } if options[:as]
[conditions]
end
- def source_reflection
- nil
- end
-
alias :source_macro :macro
def has_inverse?
@@ -401,33 +406,34 @@ def through_reflection
@through_reflection ||= active_record.reflect_on_association(options[:through])
end
- # Returns an array of AssociationReflection objects which are involved in this through
- # association. Each item in the array corresponds to a table which will be part of the
- # query for this association.
+ # Returns an array of reflections which are involved in this association. Each item in the
+ # array corresponds to a table which will be part of the query for this association.
#
# If the source reflection is itself a ThroughReflection, then we don't include self in
# the chain, but just defer to the source reflection.
#
- # The chain is built by recursively calling through_reflection_chain on the source
- # reflection and the through reflection. The base case for the recursion is a normal
- # association, which just returns [self] for its through_reflection_chain.
- def through_reflection_chain
- @through_reflection_chain ||= begin
+ # The chain is built by recursively calling #chain on the source reflection and the through
+ # reflection. The base case for the recursion is a normal association, which just returns
+ # [self] as its #chain.
+ def chain
+ @chain ||= begin
if source_reflection.source_reflection
# If the source reflection has its own source reflection, then the chain must start
# by getting us to that source reflection.
- chain = source_reflection.through_reflection_chain
+ chain = source_reflection.chain
else
# If the source reflection does not go through another reflection, then we can get
# to this reflection directly, and so start the chain here
#
# It is important to use self, rather than the source_reflection, because self
# may has a :source_type option which needs to be used.
+ #
+ # FIXME: Not sure this is correct justification now that we have #conditions
chain = [self]
end
# Recursively build the rest of the chain
- chain += through_reflection.through_reflection_chain
+ chain += through_reflection.chain
# Finally return the completed chain
chain
@@ -451,18 +457,18 @@ def through_reflection_chain
# end
#
# There may be conditions on Person.comment_tags, Article.comment_tags and/or Comment.tags,
- # but only Comment.tags will be represented in the through_reflection_chain. So this method
- # creates an array of conditions corresponding to the through_reflection_chain. Each item in
- # the through_conditions array corresponds to an item in the through_reflection_chain, and is
- # itself an array of conditions from an arbitrary number of relevant reflections.
- def through_conditions
- @through_conditions ||= begin
- conditions = source_reflection.through_conditions
+ # but only Comment.tags will be represented in the #chain. So this method creates an array
+ # of conditions corresponding to the chain. Each item in the #conditions array corresponds
+ # to an item in the #chain, and is itself an array of conditions from an arbitrary number
+ # of relevant reflections, plus any :source_type or polymorphic :as constraints.
+ def conditions
+ @conditions ||= begin
+ conditions = source_reflection.conditions
# Add to it the conditions from this reflection if necessary.
conditions.first << options[:conditions] if options[:conditions]
- through_conditions = through_reflection.through_conditions
+ through_conditions = through_reflection.conditions
if options[:source_type]
through_conditions.first << { foreign_type => options[:source_type] }
@@ -476,14 +482,14 @@ def through_conditions
end
end
+ # The macro used by the source association
def source_macro
source_reflection.source_macro
end
# A through association is nested iff there would be more than one join table
@alloy
alloy added a note

While you’re at it, there's an incorrect ‘if’ in that comment :)

@jonleighton Collaborator

I was just being over-precise ;) http://en.wikipedia.org/wiki/If_and_only_if

@alloy
alloy added a note

Oh I see, interesting. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
def nested?
- through_reflection_chain.length > 2 ||
- through_reflection.macro == :has_and_belongs_to_many
+ chain.length > 2 || through_reflection.macro == :has_and_belongs_to_many
end
# We want to use the klass from this reflection, rather than just delegate straight to
View
10 activerecord/test/cases/reflection_test.rb
@@ -202,24 +202,24 @@ def test_has_many_through_reflection
assert_kind_of ThroughReflection, Subscriber.reflect_on_association(:books)
end
- def test_through_reflection_chain
+ def test_chain
expected = [
Author.reflect_on_association(:essay_categories),
Author.reflect_on_association(:essays),
Organization.reflect_on_association(:authors)
]
- actual = Organization.reflect_on_association(:author_essay_categories).through_reflection_chain
+ actual = Organization.reflect_on_association(:author_essay_categories).chain
assert_equal expected, actual
end
- def test_through_conditions
+ def test_conditions
expected = [
["tags.name = 'Blue'"],
["taggings.comment = 'first'", {"taggable_type"=>"Post"}],
["posts.title LIKE 'misc post%'"]
]
- actual = Author.reflect_on_association(:misc_post_first_blue_tags).through_conditions
+ actual = Author.reflect_on_association(:misc_post_first_blue_tags).conditions
assert_equal expected, actual
expected = [
@@ -227,7 +227,7 @@ def test_through_conditions
[{"taggable_type"=>"Post"}],
[]
]
- actual = Author.reflect_on_association(:misc_post_first_blue_tags_2).through_conditions
+ actual = Author.reflect_on_association(:misc_post_first_blue_tags_2).conditions
assert_equal expected, actual
end

4 comments on commit 2d3d9e3

@josevalim
Owner

This required a deprecation warning. Most of the methods in the reflection are public. :(

@jonleighton
Collaborator

The through_reflection_chain stuff (and friends) is all new in 3.1 so it doesn't matter that I changed the name.

Potentially we should worry about the return value of the conditions method changing though. I kind of took the view that although the method itself is public, it is :nodoc:ed and therefore we have no obligation to support it across a major upgrade. Do you agree/disagree? If we were to support this with a deprecation or whatever, we could keep and old conditions method with a deprecation warning and then have an appropriately named new_conditions method for the new stuff. But then we would want to rename new_conditions to conditions in 3.2 (causing further breakage for 3rd parties) so I kinda think it's better not to bother.

@josevalim
Owner

It is tricky because most of Rails has no formal description of what is public or private. The thing is that reflect_on_association is widely used in plugins and it returns the Reflection object (making almost all of it public). I found this commit when I was searching a breakage that I thought it was on the reflections, but happened to be completely another case. Then I was all like "why people are changing methods", hahahaha.

About conditions, what are you returning now? A scope? And previously it was a hash?

@alloy

While you’re at it, there's an incorrect ‘if’ in that comment :)

@jonleighton
Collaborator
@alloy

Oh I see, interesting. Thanks!

Please sign in to comment.
Something went wrong with that request. Please try again.