From 8e9e424fa99769b86c1d3df958ef534868543316 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Sat, 30 Aug 2014 07:59:40 -0400 Subject: Break conditional branches into separate methods This breaks the two branches of the `if reflection.last` and `else` to clearer see where the two methods can be refactored. Eventually we hope to remove the need for these separated methods altogether. Move the first branch outside the loop This code doesn't need to be in the loop because it it always affects the last chain. `get_bind_values` and `add_constraints` must match in this context because `get_bind_values` is the caching of `add_constraints` Use each_cons to remove need for `chain[i + 1]` The `chain[i + 1]` is confusing because it's not immediately obvious what it's trying to achieve. The use of `each_cons` makes it clear we need to get the `next_reflection`. --- .../associations/association_scope.rb | 87 +++++++++++++--------- 1 file changed, 52 insertions(+), 35 deletions(-) (limited to 'activerecord/lib/active_record/associations/association_scope.rb') diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb index 4c47af8cb0..26fed22b4c 100644 --- a/activerecord/lib/active_record/associations/association_scope.rb +++ b/activerecord/lib/active_record/associations/association_scope.rb @@ -45,20 +45,20 @@ module ActiveRecord end def self.get_bind_values(owner, chain) - bvs = [] - chain.each_with_index do |reflection, i| - if reflection == chain.last - bvs << reflection.join_id_for(owner) - if reflection.type - bvs << owner.class.base_class.name - end - else - if reflection.type - bvs << chain[i + 1].klass.base_class.name - end + binds = [] + last_reflection = chain.last + + binds << last_reflection.join_id_for(owner) + if last_reflection.type + binds << owner.class.base_class.name + end + + chain.each_cons(2).each do |reflection, next_reflection| + if reflection.type + binds << next_reflection.klass.base_class.name end end - bvs + binds end private @@ -96,38 +96,55 @@ module ActiveRecord bind_value scope, column, value, tracker end + def last_chain_scope(scope, table, reflection, owner, tracker, assoc_klass) + join_keys = reflection.join_keys(assoc_klass) + key = join_keys.key + foreign_key = join_keys.foreign_key + + bind_val = bind scope, table.table_name, key.to_s, owner[foreign_key], tracker + scope = scope.where(table[key].eq(bind_val)) + + if reflection.type + value = owner.class.base_class.name + bind_val = bind scope, table.table_name, reflection.type, value, tracker + scope = scope.where(table[reflection.type].eq(bind_val)) + else + scope + end + end + + def next_chain_scope(scope, table, reflection, chain, tracker, assoc_klass, foreign_table, next_reflection) + join_keys = reflection.join_keys(assoc_klass) + key = join_keys.key + foreign_key = join_keys.foreign_key + + constraint = table[key].eq(foreign_table[foreign_key]) + + if reflection.type + value = next_reflection.klass.base_class.name + bind_val = bind scope, table.table_name, reflection.type, value, tracker + scope = scope.where(table[reflection.type].eq(bind_val)) + end + + scope = scope.joins(join(foreign_table, constraint)) + end + def add_constraints(scope, owner, assoc_klass, refl, tracker) chain = refl.chain scope_chain = refl.scope_chain tables = construct_tables(chain, assoc_klass, refl, tracker) + reflection = chain.last + table = tables.last + scope = last_chain_scope(scope, table, reflection, owner, tracker, assoc_klass) + chain.each_with_index do |reflection, i| table, foreign_table = tables.shift, tables.first - join_keys = reflection.join_keys(assoc_klass) - key = join_keys.key - foreign_key = join_keys.foreign_key - - if reflection == chain.last - bind_val = bind scope, table.table_name, key.to_s, owner[foreign_key], tracker - scope = scope.where(table[key].eq(bind_val)) - - if reflection.type - value = owner.class.base_class.name - bind_val = bind scope, table.table_name, reflection.type, value, tracker - scope = scope.where(table[reflection.type].eq(bind_val)) - end - else - constraint = table[key].eq(foreign_table[foreign_key]) - - if reflection.type - value = chain[i + 1].klass.base_class.name - bind_val = bind scope, table.table_name, reflection.type, value, tracker - scope = scope.where(table[reflection.type].eq(bind_val)) - end - - scope = scope.joins(join(foreign_table, constraint)) + unless reflection == chain.last + next_reflection = chain[i + 1] + scope = next_chain_scope(scope, table, reflection, chain, tracker, assoc_klass, foreign_table, next_reflection) end is_first_chain = i == 0 -- cgit v1.2.3 From 98318750126b2d1cf6569e7ebfce5d2aee469dcd Mon Sep 17 00:00:00 2001 From: Yves Senn Date: Wed, 3 Sep 2014 18:46:01 +0200 Subject: get rid of shadowing warning when running tests AR and railtie tests. Warning looked like this: ``` /Users/senny/Projects/rails/activerecord/lib/active_record/associations/association_scope.rb:142: warning: shadowing outer local variable - reflection ``` --- activerecord/lib/active_record/associations/association_scope.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord/lib/active_record/associations/association_scope.rb') diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb index 26fed22b4c..b58c902bfc 100644 --- a/activerecord/lib/active_record/associations/association_scope.rb +++ b/activerecord/lib/active_record/associations/association_scope.rb @@ -135,9 +135,9 @@ module ActiveRecord tables = construct_tables(chain, assoc_klass, refl, tracker) - reflection = chain.last + a_reflection = chain.last table = tables.last - scope = last_chain_scope(scope, table, reflection, owner, tracker, assoc_klass) + scope = last_chain_scope(scope, table, a_reflection, owner, tracker, assoc_klass) chain.each_with_index do |reflection, i| table, foreign_table = tables.shift, tables.first -- cgit v1.2.3 From 8dac515688e183d83192249e9552ac5042f9c593 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Wed, 3 Sep 2014 18:55:48 -0400 Subject: Follup to PR #16762 Remove chain from parameters, it's no longer needed since chain and i are being passed via next_reflection Change name of `reflection` to `owner_reflection` because of shadow variable warning. The last reflection will always be the owner. --- activerecord/lib/active_record/associations/association_scope.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'activerecord/lib/active_record/associations/association_scope.rb') diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb index b58c902bfc..3a0ebba3e4 100644 --- a/activerecord/lib/active_record/associations/association_scope.rb +++ b/activerecord/lib/active_record/associations/association_scope.rb @@ -113,7 +113,7 @@ module ActiveRecord end end - def next_chain_scope(scope, table, reflection, chain, tracker, assoc_klass, foreign_table, next_reflection) + def next_chain_scope(scope, table, reflection, tracker, assoc_klass, foreign_table, next_reflection) join_keys = reflection.join_keys(assoc_klass) key = join_keys.key foreign_key = join_keys.foreign_key @@ -135,16 +135,16 @@ module ActiveRecord tables = construct_tables(chain, assoc_klass, refl, tracker) - a_reflection = chain.last + owner_reflection = chain.last table = tables.last - scope = last_chain_scope(scope, table, a_reflection, owner, tracker, assoc_klass) + scope = last_chain_scope(scope, table, owner_reflection, owner, tracker, assoc_klass) chain.each_with_index do |reflection, i| table, foreign_table = tables.shift, tables.first unless reflection == chain.last next_reflection = chain[i + 1] - scope = next_chain_scope(scope, table, reflection, chain, tracker, assoc_klass, foreign_table, next_reflection) + scope = next_chain_scope(scope, table, reflection, tracker, assoc_klass, foreign_table, next_reflection) end is_first_chain = i == 0 -- cgit v1.2.3 From 0b6358beb417f69c47edd93d099906cc818855ee Mon Sep 17 00:00:00 2001 From: eileencodes Date: Tue, 2 Sep 2014 14:13:32 -0400 Subject: Always add lambda to scope chain to eliminate branch in eval_scope We convert all other scopes to lambda's so it makes sense that we should always returns a lambda on a ThroughReflection as well. This eliminates the need to check if the scope is a Relation. --- activerecord/lib/active_record/associations/association_scope.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'activerecord/lib/active_record/associations/association_scope.rb') diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb index b58c902bfc..43e1e79fcf 100644 --- a/activerecord/lib/active_record/associations/association_scope.rb +++ b/activerecord/lib/active_record/associations/association_scope.rb @@ -188,11 +188,7 @@ module ActiveRecord end def eval_scope(klass, scope, owner) - if scope.is_a?(Relation) - scope - else - klass.unscoped.instance_exec(owner, &scope) - end + klass.unscoped.instance_exec(owner, &scope) end end end -- cgit v1.2.3