From 79e951ca9bc875da9e4a19adeff3634f0b5b7b76 Mon Sep 17 00:00:00 2001 From: Emilio Tagua Date: Tue, 18 Aug 2009 07:50:11 -0300 Subject: [PATCH] Use finder options as relation method names to provide more familiar naming. Use bang methods convention in methods that alter the relation. --- .../lib/active_record/associations.rb | 34 +++++++++---------- activerecord/lib/active_record/base.rb | 31 ++++++++--------- .../lib/active_record/calculations.rb | 18 +++++----- activerecord/lib/active_record/relation.rb | 32 ++++++++++++----- .../test/cases/associations/eager_test.rb | 10 +++--- activerecord/test/cases/base_test.rb | 2 +- activerecord/test/cases/relations_test.rb | 22 ++++++------ 7 files changed, 81 insertions(+), 68 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 406f08e247..d0322280d9 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1681,15 +1681,15 @@ def construct_finder_arel_with_included_associations(options, join_dependency) for association in join_dependency.join_associations relation = association.join_relation(relation) end - relation.join(construct_join(options[:joins], scope)) - relation.where(construct_conditions(options[:conditions], scope)) - relation.where(construct_arel_limited_ids_condition(options, join_dependency)) if !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit]) + relation.joins!(construct_join(options[:joins], scope)). + select!(column_aliases(join_dependency)). + group!(construct_group(options[:group], options[:having], scope)). + order!(construct_order(options[:order], scope)). + conditions!(construct_conditions(options[:conditions], scope)) - relation.project(column_aliases(join_dependency)) - relation.group(construct_group(options[:group], options[:having], scope)) - relation.order(construct_order(options[:order], scope)) - relation.take(construct_limit(options[:limit], scope)) if using_limitable_reflections?(join_dependency.reflections) + relation.conditions!(construct_arel_limited_ids_condition(options, join_dependency)) if !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit]) + relation.limit!(construct_limit(options[:limit], scope)) if using_limitable_reflections?(join_dependency.reflections) relation end @@ -1724,15 +1724,15 @@ def construct_finder_sql_for_association_limiting(options, join_dependency) for association in join_dependency.join_associations relation = association.join_relation(relation) end - relation.join(construct_join(options[:joins], scope)) - relation.where(construct_conditions(options[:conditions], scope)) - relation.project(connection.distinct("#{connection.quote_table_name table_name}.#{primary_key}", construct_order(options[:order], scope(:find)).join(","))) + relation.joins!(construct_join(options[:joins], scope)). + conditions!(construct_conditions(options[:conditions], scope)). + group!(construct_group(options[:group], options[:having], scope)). + order!(construct_order(options[:order], scope)). + limit!(construct_limit(options[:limit], scope)). + offset!(construct_limit(options[:offset], scope)) - relation.group(construct_group(options[:group], options[:having], scope)) - relation.order(construct_order(options[:order], scope)) - relation.take(construct_limit(options[:limit], scope)) - relation.skip(construct_limit(options[:offset], scope)) + relation.select!(connection.distinct("#{connection.quote_table_name table_name}.#{primary_key}", construct_order(options[:order], scope(:find)).join(","))) sanitize_sql(relation.to_sql) end @@ -2222,10 +2222,10 @@ def join_type def join_relation(joining_relation, join = nil) if relation.is_a?(Array) - joining_relation.join(relation.first, join_type).on(association_join.first) - joining_relation.join(relation.last, join_type).on(association_join.last) + joining_relation.joins!(relation.first, join_type).on!(association_join.first) + joining_relation.joins!(relation.last, join_type).on!(association_join.last) else - joining_relation.join(relation, join_type).on(association_join) + joining_relation.joins!(relation, join_type).on!(association_join) end joining_relation end diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index a6832b47ef..2b79788b89 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -866,18 +866,18 @@ def destroy(id) def update_all(updates, conditions = nil, options = {}) scope = scope(:find) - relation = arel_table.relation + relation = arel_table if conditions = construct_conditions(conditions, scope) - relation = relation.where(Arel::SqlLiteral.new(conditions)) + relation.conditions!(Arel::SqlLiteral.new(conditions)) end if options.has_key?(:limit) || (scope && scope[:limit]) # Only take order from scope if limit is also provided by scope, this # is useful for updating a has_many association with a limit. - relation = relation.order(construct_order(options[:order], scope)).take(construct_limit(options[:limit], scope)) + relation.order!(construct_order(options[:order], scope)).limit!(construct_limit(options[:limit], scope)) else - relation = relation.order(construct_order(options[:order], nil)) + relation.order!(options[:order]) end relation.update(sanitize_sql_for_assignment(updates)) @@ -932,7 +932,7 @@ def destroy_all(conditions = nil) # associations or call your before_* or +after_destroy+ callbacks, use the +destroy_all+ method instead. def delete_all(conditions = nil) if conditions - arel_table.where(Arel::SqlLiteral.new(construct_conditions(conditions, scope(:find)))).delete + arel_table.conditions!(Arel::SqlLiteral.new(construct_conditions(conditions, scope(:find)))).delete else arel_table.delete end @@ -1719,15 +1719,14 @@ def default_select(qualified) def construct_finder_arel(options = {}, scope = scope(:find)) # TODO add lock to Arel - arel_table(options[:from]). - join(construct_join(options[:joins], scope)). - where(construct_conditions(options[:conditions], scope)). - project(options[:select] || (scope && scope[:select]) || default_select(options[:joins] || (scope && scope[:joins]))). - group(construct_group(options[:group], options[:having], scope)). - order(construct_order(options[:order], scope)). - take(construct_limit(options[:limit], scope)). - skip(construct_offset(options[:offset], scope) - ) + relation = arel_table(options[:from]). + joins!(construct_join(options[:joins], scope)). + conditions!(construct_conditions(options[:conditions], scope)). + select!(options[:select] || (scope && scope[:select]) || default_select(options[:joins] || (scope && scope[:joins]))). + group!(construct_group(options[:group], options[:having], scope)). + order!(construct_order(options[:order], scope)). + limit!(construct_limit(options[:limit], scope)). + offset!(construct_offset(options[:offset], scope)) end def construct_finder_sql(options, scope = scope(:find)) @@ -2570,7 +2569,7 @@ def delete # be made (since they can't be persisted). def destroy unless new_record? - arel_table(true).where(arel_table[self.class.primary_key].eq(id)).delete + arel_table(true).conditions!(arel_table[self.class.primary_key].eq(id)).delete end @destroyed = true @@ -2865,7 +2864,7 @@ def create_or_update def update(attribute_names = @attributes.keys) attributes_with_values = arel_attributes_values(false, false, attribute_names) return 0 if attributes_with_values.empty? - arel_table(true).where(arel_table[self.class.primary_key].eq(id)).update(attributes_with_values) + arel_table(true).conditions!(arel_table[self.class.primary_key].eq(id)).update(attributes_with_values) end # Creates a record with values matching those of the instance attributes diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb index 82a171c6ad..cd2fbe9b79 100644 --- a/activerecord/lib/active_record/calculations.rb +++ b/activerecord/lib/active_record/calculations.rb @@ -146,12 +146,12 @@ def calculate(operation, column_name, options = {}) join_dependency = ActiveRecord::Associations::ClassMethods::JoinDependency.new(self, merged_includes, construct_join(options[:joins], scope)) construct_finder_arel_with_included_associations(options, join_dependency) else - arel_table(options[:from]). - join(construct_join(options[:joins], scope)). - where(construct_conditions(options[:conditions], scope)). - order(options[:order]). - take(options[:limit]). - skip(options[:offset]) + relation = arel_table(options[:from]). + joins!(construct_join(options[:joins], scope)). + conditions!(construct_conditions(options[:conditions], scope)). + order!(options[:order]). + limit!(options[:limit]). + offset!(options[:offset]) end if options[:group] return execute_grouped_calculation(operation, column_name, options, relation) @@ -171,7 +171,7 @@ def execute_simple_calculation(operation, column_name, options, relation) #:nodo (column_name == :all ? "*" : column_name.to_s)) end - relation.project(operation == 'count' ? column.count(options[:distinct]) : column.send(operation)) + relation.select!(operation == 'count' ? column.count(options[:distinct]) : column.send(operation)) type_cast_calculated_value(connection.select_value(relation.to_sql), column_for(column_name), operation) end @@ -194,8 +194,8 @@ def execute_grouped_calculation(operation, column_name, options, relation) #:nod options[:select] << ", #{group_field} AS #{group_alias}" - relation.project(options[:select]) - relation.group(construct_group(options[:group], options[:having], nil)) + relation.select!(options[:select]) + relation.group!(construct_group(options[:group], options[:having], nil)) calculated_data = connection.select_all(relation.to_sql) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 456de73250..bbbb1da210 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -1,7 +1,7 @@ module ActiveRecord class Relation delegate :delete, :to_sql, :to => :relation - CLAUSES_METHODS = ["project", "group", "order", "take", "skip", "on"].freeze + CLAUSES_METHODS = ["project", "where", "group", "order", "take", "skip", "on"].freeze attr_reader :relation, :klass def initialize(klass, table = nil) @@ -24,25 +24,41 @@ def first for clause in CLAUSES_METHODS class_eval %{ - def #{clause}(_#{clause}) + def #{clause}!(_#{clause}) @relation = @relation.#{clause}(_#{clause}) if _#{clause} self end } end - def join(joins, join_type = nil) - if !joins.blank? - if [String, Hash, Array, Symbol].include?(joins.class) - @relation = @relation.join(@klass.send(:construct_join, joins, nil)) + + def select!(selection) + @relation = @relation.project(selection) if selection + self + end + + def limit!(limit) + @relation = @relation.take(limit) if limit + self + end + + def offset!(offset) + @relation = @relation.skip(offset) if offset + self + end + + def joins!(join, join_type = nil) + if !join.blank? + if [String, Hash, Array, Symbol].include?(join.class) + @relation = @relation.join(@klass.send(:construct_join, join, nil)) else - @relation = @relation.join(joins, join_type) + @relation = @relation.join(join, join_type) end end self end - def where(conditions) + def conditions!(conditions) if !conditions.blank? conditions = @klass.send(:merge_conditions, conditions) if [String, Hash, Array].include?(conditions.class) @relation = @relation.where(conditions) diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index 811ebfbe3f..d5a4d9007b 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -301,13 +301,13 @@ def test_eager_load_has_many_with_string_keys subscriber =Subscriber.find(subscribers(:second).id, :include => :subscriptions) assert_equal subscriptions, subscriber.subscriptions.sort_by(&:id) end - + def test_eager_load_has_many_through_with_string_keys books = books(:awdr, :rfr) subscriber = Subscriber.find(subscribers(:second).id, :include => :books) assert_equal books, subscriber.books.sort_by(&:id) end - + def test_eager_load_belongs_to_with_string_keys subscriber = subscribers(:second) subscription = Subscription.find(subscriptions(:webster_awdr).id, :include => :subscriber) @@ -434,7 +434,7 @@ def test_eager_count_performed_on_a_has_many_association_with_multi_table_condit author_posts_without_comments = author.posts.select { |post| post.comments.blank? } assert_equal author_posts_without_comments.size, author.posts.count(:all, :include => :comments, :conditions => 'comments.id is null') end - + def test_eager_count_performed_on_a_has_many_through_association_with_multi_table_conditional person = people(:michael) person_posts_without_comments = person.posts.select { |post| post.comments.blank? } @@ -823,7 +823,7 @@ def test_include_has_many_using_primary_key assert_equal expected, firm.clients_using_primary_key end end - + def test_preload_has_one_using_primary_key expected = Firm.find(:first).account_using_primary_key firm = Firm.find :first, :include => :account_using_primary_key @@ -839,5 +839,5 @@ def test_include_has_one_using_primary_key assert_equal expected, firm.account_using_primary_key end end - + end diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index df15c1a797..b4f29f939a 100755 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1893,7 +1893,7 @@ def test_last end def test_all_with_conditions - assert_equal Developer.find(:all, :order => 'id desc'), Developer.all.order('id desc').to_a + assert_equal Developer.find(:all, :order => 'id desc'), Developer.all.order!('id desc').to_a end def test_find_ordered_last diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index ac3a80afb7..663f54b5a1 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -11,49 +11,47 @@ class RealtionTest < ActiveRecord::TestCase fixtures :authors, :topics, :entrants, :developers, :companies, :developers_projects, :accounts, :categories, :categorizations, :posts def test_finding_with_conditions - authors = Author.all.where("name = 'David'") - - assert_equal Author.find(:all, :conditions => "name = 'David'"), authors.to_a + assert_equal Author.find(:all, :conditions => "name = 'David'"), Author.all.conditions!("name = 'David'").to_a end def test_finding_with_order - topics = Topic.all.order('id') + topics = Topic.all.order!('id') assert_equal 4, topics.size assert_equal topics(:first).title, topics.first.title end def test_finding_with_order_and_take - entrants = Entrant.all.order("id ASC").take(2).to_a + entrants = Entrant.all.order!("id ASC").limit!(2).to_a assert_equal(2, entrants.size) assert_equal(entrants(:first).name, entrants.first.name) end def test_finding_with_order_limit_and_offset - entrants = Entrant.all.order("id ASC").take(2).skip(1) + entrants = Entrant.all.order!("id ASC").limit!(2).offset!(1) assert_equal(2, entrants.size) assert_equal(entrants(:second).name, entrants.first.name) - entrants = Entrant.all.order("id ASC").take(2).skip(2) + entrants = Entrant.all.order!("id ASC").limit!(2).offset!(2) assert_equal(1, entrants.size) assert_equal(entrants(:third).name, entrants.first.name) end def test_finding_with_group - developers = Developer.all.group("salary").project("salary").to_a + developers = Developer.all.group!("salary").select!("salary").to_a assert_equal 4, developers.size assert_equal 4, developers.map(&:salary).uniq.size end def test_finding_with_hash_conditions_on_joined_table - firms = DependentFirm.all.join(:account).where({:name => 'RailsCore', :accounts => { :credit_limit => 55..60 }}).to_a + firms = DependentFirm.all.joins!(:account).conditions!({:name => 'RailsCore', :accounts => { :credit_limit => 55..60 }}).to_a assert_equal 1, firms.size assert_equal companies(:rails_core), firms.first end def test_find_all_with_join - developers_on_project_one = Developer.all.join('LEFT JOIN developers_projects ON developers.id = developers_projects.developer_id').where('project_id=1').to_a + developers_on_project_one = Developer.all.joins!('LEFT JOIN developers_projects ON developers.id = developers_projects.developer_id').conditions!('project_id=1').to_a assert_equal 3, developers_on_project_one.length developer_names = developers_on_project_one.map { |d| d.name } @@ -62,11 +60,11 @@ def test_find_all_with_join end def test_find_on_hash_conditions - assert_equal Topic.find(:all, :conditions => {:approved => false}), Topic.all.where({ :approved => false }).to_a + assert_equal Topic.find(:all, :conditions => {:approved => false}), Topic.all.conditions!({ :approved => false }).to_a end def test_joins_with_string_array - person_with_reader_and_post = Post.all.join([ + person_with_reader_and_post = Post.all.joins!([ "INNER JOIN categorizations ON categorizations.post_id = posts.id", "INNER JOIN categories ON categories.id = categorizations.category_id AND categories.type = 'SpecialCategory'" ] -- GitLab