From dcdfc84f55ea1a7880a30f63b6517745310d24eb Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 15 Nov 2010 14:35:28 -0800 Subject: use quoted id of single AR::Base objects in predicates --- activerecord/lib/active_record/relation/predicate_builder.rb | 2 ++ activerecord/test/cases/relations_test.rb | 6 ++++++ 2 files changed, 8 insertions(+) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb index c5428dccd6..2eda404490 100644 --- a/activerecord/lib/active_record/relation/predicate_builder.rb +++ b/activerecord/lib/active_record/relation/predicate_builder.rb @@ -25,6 +25,8 @@ module ActiveRecord attribute.in(values) when Range, Arel::Relation attribute.in(value) + when ActiveRecord::Base + attribute.eq(value.quoted_id) else attribute.eq(value) end diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index b44c716db8..13c26a0c15 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -426,6 +426,12 @@ class RelationTest < ActiveRecord::TestCase assert_blank authors.all end + def test_where_with_ar_object + author = Author.first + authors = Author.scoped.where(:id => author) + assert_equal 1, authors.all.length + end + def test_exists davids = Author.where(:name => 'David') assert davids.exists? -- cgit v1.2.3 From 7bf9cbb7667d3725535c1410df95892891665a95 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 15 Nov 2010 15:30:05 -0800 Subject: adding more test coverage around finding with active record objects --- activerecord/test/cases/relations_test.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'activerecord') diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 13c26a0c15..24539df6ff 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -432,6 +432,18 @@ class RelationTest < ActiveRecord::TestCase assert_equal 1, authors.all.length end + def test_find_with_list_of_ar + author = Author.first + authors = Author.find([author]) + assert_equal author, authors.first + end + + def test_find_by_id_with_list_of_ar + author = Author.first + authors = Author.find_by_id([author]) + assert_equal author, authors + end + def test_exists davids = Author.where(:name => 'David') assert davids.exists? -- cgit v1.2.3 From ace84a003cb48f60ca478c05c1fd8a57d37663cf Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 15 Nov 2010 20:24:58 -0800 Subject: support finding by a ruby class [#5979 state:resolved] --- activerecord/lib/active_record/relation/predicate_builder.rb | 3 +++ activerecord/test/cases/relations_test.rb | 7 +++++++ 2 files changed, 10 insertions(+) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb index 2eda404490..32c7d08daa 100644 --- a/activerecord/lib/active_record/relation/predicate_builder.rb +++ b/activerecord/lib/active_record/relation/predicate_builder.rb @@ -27,6 +27,9 @@ module ActiveRecord attribute.in(value) when ActiveRecord::Base attribute.eq(value.quoted_id) + when Class + # FIXME: I think we need to deprecate this behavior + attribute.eq(value.name) else attribute.eq(value) end diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 24539df6ff..e39b1f396c 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -438,6 +438,13 @@ class RelationTest < ActiveRecord::TestCase assert_equal author, authors.first end + class Mary < Author; end + + def test_find_by_classname + Author.create!(:name => Mary.name) + assert_equal 1, Author.where(:name => Mary).size + end + def test_find_by_id_with_list_of_ar author = Author.first authors = Author.find_by_id([author]) -- cgit v1.2.3 From 4718d097ffe3af965f3ea7218156050507eabe4f Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Tue, 16 Nov 2010 12:36:47 -0200 Subject: Models should be equals even after destroyed [#5978 state:committed] --- activerecord/lib/active_record/base.rb | 6 +++--- activerecord/test/cases/base_test.rb | 9 +++++++++ activerecord/test/cases/named_scope_test.rb | 2 +- 3 files changed, 13 insertions(+), 4 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index f588475bbf..658c6d7b45 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1585,9 +1585,9 @@ MSG # Returns true if the +comparison_object+ is the same object, or is of the same type and has the same id. def ==(comparison_object) comparison_object.equal?(self) || - persisted? && - (comparison_object.instance_of?(self.class) && - comparison_object.id == id) + comparison_object.instance_of?(self.class) && + id.present? && + comparison_object.id == id end # Delegates to == diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 9f2b0c9c86..73c76606ad 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -397,6 +397,15 @@ class BasicsTest < ActiveRecord::TestCase assert_not_equal Topic.new, Topic.new end + def test_equality_of_destroyed_records + topic_1 = Topic.new(:title => 'test_1') + topic_1.save + topic_2 = Topic.find(topic_1.id) + topic_1.destroy + assert_equal topic_1, topic_2 + assert_equal topic_2, topic_1 + end + def test_hashing assert_equal [ Topic.find(1) ], [ Topic.find(2).topic ] & [ Topic.find(1) ] end diff --git a/activerecord/test/cases/named_scope_test.rb b/activerecord/test/cases/named_scope_test.rb index fb24c65fff..6ac3e3fc56 100644 --- a/activerecord/test/cases/named_scope_test.rb +++ b/activerecord/test/cases/named_scope_test.rb @@ -122,7 +122,7 @@ class NamedScopeTest < ActiveRecord::TestCase :joins => 'JOIN authors ON authors.id = posts.author_id', :conditions => [ 'authors.author_address_id = ?', address.id ] ) - assert_equal posts_with_authors_at_address_titles, Post.with_authors_at_address(address).find(:all, :select => 'title') + assert_equal posts_with_authors_at_address_titles.map(&:title), Post.with_authors_at_address(address).find(:all, :select => 'title').map(&:title) end def test_scope_with_object -- cgit v1.2.3 From a820d0afdd4a09e0902d7b7c4d9724f50089d254 Mon Sep 17 00:00:00 2001 From: Xavier Noria Date: Tue, 16 Nov 2010 15:55:43 +0100 Subject: revises RDoc of AR::Base#== --- activerecord/lib/active_record/base.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 658c6d7b45..b35f59d6df 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1582,7 +1582,15 @@ MSG self.class.columns_hash[name.to_s] end - # Returns true if the +comparison_object+ is the same object, or is of the same type and has the same id. + # Returns true if +comparison_object+ is the same exact object, or +comparison_object+ + # is of the same type and +self+ has an ID and it is equal to +comparison_object.id+. + # + # Note that new records are different from any other record by definition, unless the + # other record is the receiver itself. Besides, if you fetch existing records with + # +select+ and leave the ID out, you're on your own, this predicate will return false. + # + # Note also that destroying a record preserves its ID in the model instance, so deleted + # models are still comparable. def ==(comparison_object) comparison_object.equal?(self) || comparison_object.instance_of?(self.class) && -- cgit v1.2.3 From 437ceab139c9aace851b41ce6103d29302750e0c Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Tue, 16 Nov 2010 17:00:01 +0100 Subject: Create directory before copying migrations if it does not exist --- activerecord/lib/active_record/migration.rb | 2 ++ activerecord/test/cases/migration_test.rb | 15 +++++++++++++++ 2 files changed, 17 insertions(+) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index a4c09b654a..2c306d233a 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -387,6 +387,8 @@ module ActiveRecord def copy(destination, sources, options = {}) copied = [] + FileUtils.mkdir_p(destination) unless File.exists?(destination) + destination_migrations = ActiveRecord::Migrator.migrations(destination) last = destination_migrations.last sources.each do |name, path| diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index e6eef805cf..698075ea0c 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -2024,6 +2024,21 @@ if ActiveRecord::Base.connection.supports_migrations? clear end + def test_copying_migrations_to_non_existing_directory + @migrations_path = MIGRATIONS_ROOT + "/non_existing" + @existing_migrations = [] + + Time.travel_to(created_at = Time.utc(2010, 7, 26, 10, 10, 10)) do + copied = ActiveRecord::Migration.copy(@migrations_path, {:bukkits => MIGRATIONS_ROOT + "/to_copy_with_timestamps"}) + assert File.exists?(@migrations_path + "/20100726101010_people_have_hobbies.rb") + assert File.exists?(@migrations_path + "/20100726101011_people_have_descriptions.rb") + assert_equal 2, copied.length + end + ensure + clear + Dir.delete(@migrations_path) + end + def test_copying_migrations_to_empty_directory @migrations_path = MIGRATIONS_ROOT + "/empty" @existing_migrations = [] -- cgit v1.2.3 From a5cdf0b9eb860c4370ae5fde231e1b61f71b6b65 Mon Sep 17 00:00:00 2001 From: Alexandru Catighera Date: Mon, 15 Nov 2010 21:33:21 -0500 Subject: Fix ActiveRecord calculations when grouped by multiple fields --- .../lib/active_record/relation/calculations.rb | 32 ++++++++++++---------- activerecord/test/cases/calculations_test.rb | 13 +++++++++ 2 files changed, 31 insertions(+), 14 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index 6bf698fe97..fe5c85d24a 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -208,14 +208,19 @@ module ActiveRecord end def execute_grouped_calculation(operation, column_name, distinct) #:nodoc: - group_attr = @group_values.first - association = @klass.reflect_on_association(group_attr.to_sym) - associated = association && association.macro == :belongs_to # only count belongs_to associations - group_field = associated ? association.primary_key_name : group_attr - group_alias = column_alias_for(group_field) - group_column = column_for(group_field) + group_attr = @group_values + association = @klass.reflect_on_association(group_attr.first.to_sym) + associated = group_attr.size == 1 && association && association.macro == :belongs_to # only count belongs_to associations + group_fields = Array(associated ? association.primary_key_name : group_attr) + group_aliases = [] + group_columns = {} + + group_fields.each do |field| + group_aliases << column_alias_for(field) + group_columns[column_alias_for(field)] = column_for(field) + end - group = @klass.connection.adapter_name == 'FrontBase' ? group_alias : group_field + group = @klass.connection.adapter_name == 'FrontBase' ? group_aliases : group_fields if operation == 'count' && column_name == :all aggregate_alias = 'count_all' @@ -223,22 +228,21 @@ module ActiveRecord aggregate_alias = column_alias_for(operation, column_name) end - relation = except(:group).group(group) - relation.select_values = [ - operation_over_aggregate_column(aggregate_column(column_name), operation, distinct).as(aggregate_alias), - "#{group_field} AS #{group_alias}" - ] + relation = except(:group).group(group.join(',')) + relation.select_values = [ operation_over_aggregate_column(aggregate_column(column_name), operation, distinct).as(aggregate_alias) ] + group_fields.each_index{ |i| relation.select_values << "#{group_fields[i]} AS #{group_aliases[i]}" } calculated_data = @klass.connection.select_all(relation.to_sql) if association - key_ids = calculated_data.collect { |row| row[group_alias] } + key_ids = calculated_data.collect { |row| row[group_aliases.first] } key_records = association.klass.base_class.find(key_ids) key_records = Hash[key_records.map { |r| [r.id, r] }] end ActiveSupport::OrderedHash[calculated_data.map do |row| - key = type_cast_calculated_value(row[group_alias], group_column) + key = group_aliases.map{|group_alias| type_cast_calculated_value(row[group_alias], group_columns[group_alias])} + key = key.first if key.size == 1 key = key_records[key] if associated [key, type_cast_calculated_value(row[aggregate_alias], column_for(column_name), operation)] end] diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 61fbf01a50..5cb8485b4b 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -54,6 +54,19 @@ class CalculationsTest < ActiveRecord::TestCase c = Account.sum(:credit_limit, :group => :firm_id) [1,6,2].each { |firm_id| assert c.keys.include?(firm_id) } end + + def test_should_group_by_multiple_fields + c = Account.count(:all, :group => ['firm_id', :credit_limit]) + [ [nil, 50], [1, 50], [6, 50], [6, 55], [9, 53], [2, 60] ].each { |firm_and_limit| assert c.keys.include?(firm_and_limit) } + end + + def test_should_group_by_multiple_fields_having_functions + c = Topic.group(:author_name, 'COALESCE(type, title)').count(:all) + assert_equal 1, c[["Carl", "The Third Topic of the day"]] + assert_equal 1, c[["Mary", "Reply"]] + assert_equal 1, c[["David", "The First Topic"]] + assert_equal 1, c[["Carl", "Reply"]] + end def test_should_group_by_summed_field c = Account.sum(:credit_limit, :group => :firm_id) -- cgit v1.2.3 From 7ebd36d1c4f958ceaf10cf7899936caeb173ac50 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 16 Nov 2010 11:43:11 -0800 Subject: refactor to reduce method calls --- .../lib/active_record/relation/calculations.rb | 29 ++++++++++++++-------- 1 file changed, 19 insertions(+), 10 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index fe5c85d24a..c8adaddfca 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -212,13 +212,10 @@ module ActiveRecord association = @klass.reflect_on_association(group_attr.first.to_sym) associated = group_attr.size == 1 && association && association.macro == :belongs_to # only count belongs_to associations group_fields = Array(associated ? association.primary_key_name : group_attr) - group_aliases = [] - group_columns = {} - - group_fields.each do |field| - group_aliases << column_alias_for(field) - group_columns[column_alias_for(field)] = column_for(field) - end + group_aliases = group_fields.map { |field| column_alias_for(field) } + group_columns = group_aliases.zip(group_fields).map { |aliaz,field| + [aliaz, column_for(field)] + } group = @klass.connection.adapter_name == 'FrontBase' ? group_aliases : group_fields @@ -228,9 +225,19 @@ module ActiveRecord aggregate_alias = column_alias_for(operation, column_name) end + select_values = [ + operation_over_aggregate_column( + aggregate_column(column_name), + operation, + distinct).as(aggregate_alias) + ] + + select_values.concat group_fields.zip(group_aliases).map { |field,aliaz| + "#{field} AS #{aliaz}" + } + relation = except(:group).group(group.join(',')) - relation.select_values = [ operation_over_aggregate_column(aggregate_column(column_name), operation, distinct).as(aggregate_alias) ] - group_fields.each_index{ |i| relation.select_values << "#{group_fields[i]} AS #{group_aliases[i]}" } + relation.select_values = select_values calculated_data = @klass.connection.select_all(relation.to_sql) @@ -241,7 +248,9 @@ module ActiveRecord end ActiveSupport::OrderedHash[calculated_data.map do |row| - key = group_aliases.map{|group_alias| type_cast_calculated_value(row[group_alias], group_columns[group_alias])} + key = group_columns.map { |aliaz, column| + type_cast_calculated_value(row[aliaz], column) + } key = key.first if key.size == 1 key = key_records[key] if associated [key, type_cast_calculated_value(row[aggregate_alias], column_for(column_name), operation)] -- cgit v1.2.3 From 59ba800698ac9dcea1df9e40bb03335ddb4f5156 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 16 Nov 2010 13:34:18 -0800 Subject: refactoring uniq method --- .../lib/active_record/associations/association_collection.rb | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 5b34ac907c..7cdd91e9b2 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -332,13 +332,10 @@ module ActiveRecord end def uniq(collection = self) - seen = Set.new - collection.map do |record| - unless seen.include?(record.id) - seen << record.id - record - end - end.compact + seen = {} + collection.find_all do |record| + seen[record.id] = true unless seen.key?(record.id) + end end # Replace this collection with +other_array+ -- cgit v1.2.3 From c801f233df9d20c59d9756a5279365603dc5cbbd Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 16 Nov 2010 13:43:44 -0800 Subject: reloading an association will properly set attributes of instantiated objects. Thanks Brian Palmer [#5802 state:resolved] --- .../associations/association_collection.rb | 4 +++- .../associations/has_many_associations_test.rb | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 7cdd91e9b2..600f59026e 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -379,7 +379,9 @@ module ActiveRecord if i @target.delete_at(i).tap do |t| keys = ["id"] + t.changes.keys + (f.attribute_names - t.attribute_names) - t.attributes = f.attributes.except(*keys) + f.attributes.except(*keys).each do |k,v| + t.send("#{k}=", v) + end end else f diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index ecfc769f3a..33c53e695b 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -1282,4 +1282,25 @@ class HasManyAssociationsTest < ActiveRecord::TestCase comment = post.comments.build assert post.comments.include?(comment) end + + def test_load_target_respects_protected_attributes + topic = Topic.create! + reply = topic.replies.create(:title => "reply 1") + reply.approved = false + reply.save! + + # Save with a different object instance, so the instance that's still held + # in topic.relies doesn't know about the changed attribute. + reply2 = Reply.find(reply.id) + reply2.approved = true + reply2.save! + + # Force loading the collection from the db. This will merge the existing + # object (reply) with what gets loaded from the db (which includes the + # changed approved attribute). approved is a protected attribute, so if mass + # assignment is used, it won't get updated and will still be false. + first = topic.replies.to_a.first + assert_equal reply.id, first.id + assert_equal true, first.approved? + end end -- cgit v1.2.3 From 5f608fc7c4fa3ac793f1eb1b1f418033dfb13049 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 16 Nov 2010 13:52:57 -0800 Subject: removing space errors --- .../lib/active_record/associations/association_collection.rb | 6 +++--- activerecord/lib/active_record/associations/association_proxy.rb | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 600f59026e..34247deb02 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -182,7 +182,7 @@ module ActiveRecord unless options.blank? raise ArgumentError, "If finder_sql/counter_sql is used then options cannot be passed" end - + @reflection.klass.count_by_sql(custom_counter_sql) else @@ -435,10 +435,10 @@ module ActiveRecord # replace the SELECT clause with COUNT(*), preserving any hints within /* ... */ counter_sql = @reflection.options[:finder_sql].sub(/SELECT\b(\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" } end - + interpolate_sql(counter_sql) end - + def custom_finder_sql interpolate_sql(@reflection.options[:finder_sql]) end diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb index 09bba213ce..7cd04a1ad5 100644 --- a/activerecord/lib/active_record/associations/association_proxy.rb +++ b/activerecord/lib/active_record/associations/association_proxy.rb @@ -211,12 +211,12 @@ module ActiveRecord :create => construct_create_scope } end - + # Implemented by subclasses def construct_find_scope raise NotImplementedError end - + # Implemented by (some) subclasses def construct_create_scope {} -- cgit v1.2.3 From 05320e3788a1be19956e7caa822484b816e11366 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 16 Nov 2010 14:19:14 -0800 Subject: use unless instead of if ! --- activerecord/lib/active_record/associations/association_collection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 34247deb02..98507ad1d1 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -372,7 +372,7 @@ module ActiveRecord def load_target if @owner.persisted? || foreign_key_present begin - if !loaded? + unless loaded? if @target.is_a?(Array) && @target.any? @target = find_target.map do |f| i = @target.index(f) -- cgit v1.2.3 From 08c64bbd390d1ba6b44d29c5892693e219d20c5f Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 16 Nov 2010 14:28:09 -0800 Subject: super automatically passes on the implicit block --- .../lib/active_record/associations/association_collection.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 98507ad1d1..6090376bb8 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -408,11 +408,7 @@ module ActiveRecord end if @target.respond_to?(method) || (!@reflection.klass.respond_to?(method) && Class.respond_to?(method)) - if block_given? - super { |*block_args| yield(*block_args) } - else - super - end + super elsif @reflection.klass.scopes[method] @_named_scopes_cache ||= {} @_named_scopes_cache[method] ||= {} -- cgit v1.2.3 From 2738ec891b6b6584ec7bd79532e5eac71282436e Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 16 Nov 2010 17:06:50 -0800 Subject: removing many unused variables --- .../test/cases/associations/inverse_associations_test.rb | 4 ++-- activerecord/test/cases/associations_test.rb | 2 +- activerecord/test/cases/attribute_methods_test.rb | 2 +- activerecord/test/cases/autosave_association_test.rb | 6 ++---- activerecord/test/cases/base_test.rb | 3 +-- activerecord/test/cases/finder_test.rb | 2 +- activerecord/test/cases/fixtures_test.rb | 6 +++--- activerecord/test/cases/migration_test.rb | 10 +++++----- activerecord/test/cases/persistence_test.rb | 3 +-- activerecord/test/cases/query_cache_test.rb | 1 - activerecord/test/cases/relation_scoping_test.rb | 2 -- activerecord/test/cases/timestamp_test.rb | 2 +- activerecord/test/cases/transactions_test.rb | 2 +- .../test/cases/validations/association_validation_test.rb | 4 ++-- .../test/cases/validations/uniqueness_validation_test.rb | 4 ++-- 15 files changed, 23 insertions(+), 30 deletions(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index fa5c2e49df..081583038f 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -551,8 +551,8 @@ class InverseMultipleHasManyInversesForSameModel < ActiveRecord::TestCase def test_that_we_can_load_associations_that_have_the_same_reciprocal_name_from_different_models assert_nothing_raised(ActiveRecord::AssociationTypeMismatch) do i = Interest.find(:first) - z = i.zine - m = i.man + i.zine + i.man end end diff --git a/activerecord/test/cases/associations_test.rb b/activerecord/test/cases/associations_test.rb index dd8152b219..93a51d3606 100644 --- a/activerecord/test/cases/associations_test.rb +++ b/activerecord/test/cases/associations_test.rb @@ -120,7 +120,7 @@ class AssociationsTest < ActiveRecord::TestCase def test_force_reload_is_uncached firm = Firm.create!("name" => "A New Firm, Inc") - client = Client.create!("name" => "TheClient.com", :firm => firm) + Client.create!("name" => "TheClient.com", :firm => firm) ActiveRecord::Base.cache do firm.clients.each {} assert_queries(0) { assert_not_nil firm.clients.each {} } diff --git a/activerecord/test/cases/attribute_methods_test.rb b/activerecord/test/cases/attribute_methods_test.rb index ab9a65944f..bb0166a60c 100644 --- a/activerecord/test/cases/attribute_methods_test.rb +++ b/activerecord/test/cases/attribute_methods_test.rb @@ -568,7 +568,7 @@ class AttributeMethodsTest < ActiveRecord::TestCase def test_bulk_update_respects_access_control privatize("title=(value)") - assert_raise(ActiveRecord::UnknownAttributeError) { topic = @target.new(:title => "Rants about pants") } + assert_raise(ActiveRecord::UnknownAttributeError) { @target.new(:title => "Rants about pants") } assert_raise(ActiveRecord::UnknownAttributeError) { @target.new.attributes = { :title => "Ants in pants" } } end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 459f9fa55c..b13cb2d7a2 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -355,8 +355,6 @@ class TestDefaultAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCa end def test_invalid_adding_before_save - no_of_firms = Firm.count - no_of_clients = Client.count new_firm = Firm.new("name" => "A New Firm, Inc") new_firm.clients_of_firm.concat([c = Client.new, Client.new("name" => "Apple")]) assert !c.persisted? @@ -461,7 +459,7 @@ class TestDefaultAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCa def test_build_many_before_save company = companies(:first_firm) - new_clients = assert_no_queries { company.clients_of_firm.build([{"name" => "Another Client"}, {"name" => "Another Client II"}]) } + assert_no_queries { company.clients_of_firm.build([{"name" => "Another Client"}, {"name" => "Another Client II"}]) } company.name += '-changed' assert_queries(3) { assert company.save } @@ -481,7 +479,7 @@ class TestDefaultAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCa def test_build_many_via_block_before_save company = companies(:first_firm) - new_clients = assert_no_queries do + assert_no_queries do company.clients_of_firm.build([{"name" => "Another Client"}, {"name" => "Another Client II"}]) do |client| client.name = "changed" end diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 73c76606ad..26f388ca46 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -182,7 +182,7 @@ class BasicsTest < ActiveRecord::TestCase def test_initialize_with_invalid_attribute begin - topic = Topic.new({ "title" => "test", + Topic.new({ "title" => "test", "last_read(1i)" => "2005", "last_read(2i)" => "2", "last_read(3i)" => "31"}) rescue ActiveRecord::MultiparameterAssignmentErrors => ex assert_equal(1, ex.errors.size) @@ -972,7 +972,6 @@ class BasicsTest < ActiveRecord::TestCase end def test_nil_serialized_attribute_with_class_constraint - myobj = MyObject.new('value1', 'value2') topic = Topic.new assert_nil topic.content end diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index 39ce47d9d6..31e4981a1d 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -949,7 +949,7 @@ class FinderTest < ActiveRecord::TestCase # http://dev.rubyonrails.org/ticket/6778 def test_find_ignores_previously_inserted_record - post = Post.create!(:title => 'test', :body => 'it out') + Post.create!(:title => 'test', :body => 'it out') assert_equal [], Post.find_all_by_id(nil) end diff --git a/activerecord/test/cases/fixtures_test.rb b/activerecord/test/cases/fixtures_test.rb index d5ef30e137..9ce163a00f 100644 --- a/activerecord/test/cases/fixtures_test.rb +++ b/activerecord/test/cases/fixtures_test.rb @@ -58,7 +58,7 @@ class FixturesTest < ActiveRecord::TestCase end def test_inserts - topics = create_fixtures("topics") + create_fixtures("topics") first_row = ActiveRecord::Base.connection.select_one("SELECT * FROM topics WHERE author_name = 'David'") assert_equal("The First Topic", first_row["title"]) @@ -114,7 +114,7 @@ class FixturesTest < ActiveRecord::TestCase end def test_insert_with_datetime - topics = create_fixtures("tasks") + create_fixtures("tasks") first = Task.find(1) assert first end @@ -240,7 +240,7 @@ if Account.connection.respond_to?(:reset_pk_sequence!) def test_create_fixtures_resets_sequences_when_not_cached @instances.each do |instance| - max_id = create_fixtures(instance.class.table_name).inject(0) do |_max_id, (name, fixture)| + max_id = create_fixtures(instance.class.table_name).inject(0) do |_max_id, (_, fixture)| fixture_id = fixture['id'].to_i fixture_id > _max_id ? fixture_id : _max_id end diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index 698075ea0c..ab9b35172b 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -1947,7 +1947,7 @@ if ActiveRecord::Base.connection.supports_migrations? @migrations_path = MIGRATIONS_ROOT + "/valid_with_timestamps" @existing_migrations = Dir[@migrations_path + "/*.rb"] - Time.travel_to(created_at = Time.utc(2010, 7, 26, 10, 10, 10)) do + Time.travel_to(Time.utc(2010, 7, 26, 10, 10, 10)) do copied = ActiveRecord::Migration.copy(@migrations_path, {:bukkits => MIGRATIONS_ROOT + "/to_copy_with_timestamps"}) assert File.exists?(@migrations_path + "/20100726101010_people_have_hobbies.rb") assert File.exists?(@migrations_path + "/20100726101011_people_have_descriptions.rb") @@ -1972,7 +1972,7 @@ if ActiveRecord::Base.connection.supports_migrations? sources[:bukkits] = MIGRATIONS_ROOT + "/to_copy_with_timestamps" sources[:omg] = MIGRATIONS_ROOT + "/to_copy_with_timestamps2" - Time.travel_to(created_at = Time.utc(2010, 7, 26, 10, 10, 10)) do + Time.travel_to(Time.utc(2010, 7, 26, 10, 10, 10)) do copied = ActiveRecord::Migration.copy(@migrations_path, sources) assert File.exists?(@migrations_path + "/20100726101010_people_have_hobbies.rb") assert File.exists?(@migrations_path + "/20100726101011_people_have_descriptions.rb") @@ -1992,7 +1992,7 @@ if ActiveRecord::Base.connection.supports_migrations? @migrations_path = MIGRATIONS_ROOT + "/valid_with_timestamps" @existing_migrations = Dir[@migrations_path + "/*.rb"] - Time.travel_to(created_at = Time.utc(2010, 2, 20, 10, 10, 10)) do + Time.travel_to(Time.utc(2010, 2, 20, 10, 10, 10)) do ActiveRecord::Migration.copy(@migrations_path, {:bukkits => MIGRATIONS_ROOT + "/to_copy_with_timestamps"}) assert File.exists?(@migrations_path + "/20100301010102_people_have_hobbies.rb") assert File.exists?(@migrations_path + "/20100301010103_people_have_descriptions.rb") @@ -2028,7 +2028,7 @@ if ActiveRecord::Base.connection.supports_migrations? @migrations_path = MIGRATIONS_ROOT + "/non_existing" @existing_migrations = [] - Time.travel_to(created_at = Time.utc(2010, 7, 26, 10, 10, 10)) do + Time.travel_to(Time.utc(2010, 7, 26, 10, 10, 10)) do copied = ActiveRecord::Migration.copy(@migrations_path, {:bukkits => MIGRATIONS_ROOT + "/to_copy_with_timestamps"}) assert File.exists?(@migrations_path + "/20100726101010_people_have_hobbies.rb") assert File.exists?(@migrations_path + "/20100726101011_people_have_descriptions.rb") @@ -2043,7 +2043,7 @@ if ActiveRecord::Base.connection.supports_migrations? @migrations_path = MIGRATIONS_ROOT + "/empty" @existing_migrations = [] - Time.travel_to(created_at = Time.utc(2010, 7, 26, 10, 10, 10)) do + Time.travel_to(Time.utc(2010, 7, 26, 10, 10, 10)) do copied = ActiveRecord::Migration.copy(@migrations_path, {:bukkits => MIGRATIONS_ROOT + "/to_copy_with_timestamps"}) assert File.exists?(@migrations_path + "/20100726101010_people_have_hobbies.rb") assert File.exists?(@migrations_path + "/20100726101011_people_have_descriptions.rb") diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb index 07262f56be..8ca9d626d1 100644 --- a/activerecord/test/cases/persistence_test.rb +++ b/activerecord/test/cases/persistence_test.rb @@ -192,7 +192,6 @@ class PersistencesTest < ActiveRecord::TestCase topic = Topic.create("title" => "New Topic") do |t| t.author_name = "David" end - topicReloaded = Topic.find(topic.id) assert_equal("New Topic", topic.title) assert_equal("David", topic.author_name) end @@ -270,7 +269,7 @@ class PersistencesTest < ActiveRecord::TestCase end def test_record_not_found_exception - assert_raise(ActiveRecord::RecordNotFound) { topicReloaded = Topic.find(99999) } + assert_raise(ActiveRecord::RecordNotFound) { Topic.find(99999) } end def test_update_all diff --git a/activerecord/test/cases/query_cache_test.rb b/activerecord/test/cases/query_cache_test.rb index 5bb21a54bd..33916c4e46 100644 --- a/activerecord/test/cases/query_cache_test.rb +++ b/activerecord/test/cases/query_cache_test.rb @@ -133,7 +133,6 @@ class QueryCacheExpiryTest < ActiveRecord::TestCase def test_cache_is_expired_by_habtm_delete ActiveRecord::Base.connection.expects(:clear_query_cache).times(2) ActiveRecord::Base.cache do - c = Category.find(1) p = Post.find(1) assert p.categories.any? p.categories.delete_all diff --git a/activerecord/test/cases/relation_scoping_test.rb b/activerecord/test/cases/relation_scoping_test.rb index a27e2e72cd..dae9721a63 100644 --- a/activerecord/test/cases/relation_scoping_test.rb +++ b/activerecord/test/cases/relation_scoping_test.rb @@ -254,13 +254,11 @@ class HasManyScopingTest< ActiveRecord::TestCase end def test_should_maintain_default_scope_on_associations - person = people(:michael) magician = BadReference.find(1) assert_equal [magician], people(:michael).bad_references end def test_should_default_scope_on_associations_is_overriden_by_association_conditions - person = people(:michael) assert_equal [], people(:michael).fixed_bad_references end diff --git a/activerecord/test/cases/timestamp_test.rb b/activerecord/test/cases/timestamp_test.rb index eb93761fb2..70c098bc6d 100644 --- a/activerecord/test/cases/timestamp_test.rb +++ b/activerecord/test/cases/timestamp_test.rb @@ -113,7 +113,7 @@ class TimestampTest < ActiveRecord::TestCase pet = Pet.first owner = pet.owner - owner.update_attribute(:happy_at, (time = 3.days.ago)) + owner.update_attribute(:happy_at, 3.days.ago) previously_owner_updated_at = owner.updated_at pet.name = "I'm a parrot" diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index dd9de3510b..b0ccd71836 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -163,7 +163,7 @@ class TransactionTest < ActiveRecord::TestCase @first.author_name += '_this_should_not_end_up_in_the_db' @first.save! flunk - rescue => e + rescue assert_equal original_author_name, @first.reload.author_name assert_equal nbooks_before_save, Book.count ensure diff --git a/activerecord/test/cases/validations/association_validation_test.rb b/activerecord/test/cases/validations/association_validation_test.rb index 1246dd4276..56e345990f 100644 --- a/activerecord/test/cases/validations/association_validation_test.rb +++ b/activerecord/test/cases/validations/association_validation_test.rb @@ -17,7 +17,7 @@ class AssociationValidationTest < ActiveRecord::TestCase o = Owner.new('name' => 'nopets') assert !o.save assert o.errors[:pets].any? - pet = o.pets.build('name' => 'apet') + o.pets.build('name' => 'apet') assert o.valid? end @@ -27,7 +27,7 @@ class AssociationValidationTest < ActiveRecord::TestCase assert !o.save assert o.errors[:pets].any? - pet = o.pets.build('name' => 'apet') + o.pets.build('name' => 'apet') assert o.valid? 2.times { o.pets.build('name' => 'apet') } diff --git a/activerecord/test/cases/validations/uniqueness_validation_test.rb b/activerecord/test/cases/validations/uniqueness_validation_test.rb index 9a863c25a8..679d67553b 100644 --- a/activerecord/test/cases/validations/uniqueness_validation_test.rb +++ b/activerecord/test/cases/validations/uniqueness_validation_test.rb @@ -60,7 +60,7 @@ class UniquenessValidationTest < ActiveRecord::TestCase def test_validates_uniqueness_with_validates Topic.validates :title, :uniqueness => true - t = Topic.create!('title' => 'abc') + Topic.create!('title' => 'abc') t2 = Topic.new('title' => 'abc') assert !t2.valid? @@ -201,7 +201,7 @@ class UniquenessValidationTest < ActiveRecord::TestCase def test_validate_case_sensitive_uniqueness_with_attribute_passed_as_integer Topic.validates_uniqueness_of(:title, :case_sensitve => true) - t = Topic.create!('title' => 101) + Topic.create!('title' => 101) t2 = Topic.new('title' => 101) assert !t2.valid? -- cgit v1.2.3 From ccd2f3ede585fb2f2d830e661dbb8b8538de2dc5 Mon Sep 17 00:00:00 2001 From: Carl Lerche Date: Tue, 16 Nov 2010 15:11:46 -0800 Subject: Update the version.rb files to include a PRE part --- activerecord/lib/active_record/version.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/version.rb b/activerecord/lib/active_record/version.rb index 89eba15be1..0667be7d23 100644 --- a/activerecord/lib/active_record/version.rb +++ b/activerecord/lib/active_record/version.rb @@ -3,8 +3,8 @@ module ActiveRecord MAJOR = 3 MINOR = 1 TINY = 0 - BUILD = "beta" + PRE = "beta" - STRING = [MAJOR, MINOR, TINY, BUILD].join('.') + STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') end end -- cgit v1.2.3 From 77440ec51ad28a7e63651f0976053584a7f58768 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 13:02:03 -0800 Subject: fixing assertions so error messages will be more helpful --- activerecord/test/cases/migration_test.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index ab9b35172b..f8eabd884a 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -1312,20 +1312,20 @@ if ActiveRecord::Base.connection.supports_migrations? def test_migrator_verbosity ActiveRecord::Migrator.up(MIGRATIONS_ROOT + "/valid", 1) - assert PeopleHaveLastNames.message_count > 0 + assert_operator PeopleHaveLastNames.message_count, :>, 0 PeopleHaveLastNames.message_count = 0 ActiveRecord::Migrator.down(MIGRATIONS_ROOT + "/valid", 0) - assert PeopleHaveLastNames.message_count > 0 + assert_operator PeopleHaveLastNames.message_count, :>, 0 PeopleHaveLastNames.message_count = 0 end def test_migrator_verbosity_off PeopleHaveLastNames.verbose = false ActiveRecord::Migrator.up(MIGRATIONS_ROOT + "/valid", 1) - assert PeopleHaveLastNames.message_count.zero? + assert_equal 0, PeopleHaveLastNames.message_count ActiveRecord::Migrator.down(MIGRATIONS_ROOT + "/valid", 0) - assert PeopleHaveLastNames.message_count.zero? + assert_equal 0, PeopleHaveLastNames.message_count end def test_migrator_going_down_due_to_version_target -- cgit v1.2.3 From 8b2f801ed8690dcbc61d62e6b3518efaac70a4a4 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 12:53:38 -0800 Subject: converted migrations to support instance methods --- activerecord/lib/active_record/migration.rb | 40 ++++++++++++++++++++++++----- activerecord/lib/active_record/schema.rb | 6 ++--- activerecord/test/cases/migration_test.rb | 8 +++--- 3 files changed, 40 insertions(+), 14 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 2c306d233a..4a1a84b74b 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -287,10 +287,30 @@ module ActiveRecord # In application.rb. # class Migration - @@verbose = true - cattr_accessor :verbose - class << self + attr_accessor :verbose + attr_accessor :delegate # :nodoc: + def method_missing(name, *args, &block) # :nodoc: + (delegate || superclass.delegate).send(name, *args, &block) + end + end + self.delegate = new + self.verbose = true + + def name + self.class.name + end + + def up + self.class.delegate = self + self.class.up + end + + def down + self.class.delegate = self + self.class.down + end + def up_with_benchmarks #:nodoc: migrate(:up) end @@ -309,7 +329,7 @@ module ActiveRecord end result = nil - time = Benchmark.measure { result = send("#{direction}_without_benchmarks") } + time = Benchmark.measure { result = send("#{direction}") } case direction when :up then announce "migrated (%.4fs)" % time.real; write @@ -380,10 +400,19 @@ module ActiveRecord unless arguments.empty? || method == :execute arguments[0] = Migrator.proper_table_name(arguments.first) end + return super unless connection.respond_to?(method) connection.send(method, *arguments, &block) end end + def verbose + self.class.verbose + end + + def verbose= verbosity + self.class.verbose = verbosity + end + def copy(destination, sources, options = {}) copied = [] @@ -425,7 +454,6 @@ module ActiveRecord "%.3d" % number end end - end end # MigrationProxy is used to defer loading of the actual migration classes @@ -451,7 +479,7 @@ module ActiveRecord def load_migration require(File.expand_path(filename)) - name.constantize + name.constantize.new end end diff --git a/activerecord/lib/active_record/schema.rb b/activerecord/lib/active_record/schema.rb index c1bc3214ea..a621248cc3 100644 --- a/activerecord/lib/active_record/schema.rb +++ b/activerecord/lib/active_record/schema.rb @@ -30,9 +30,7 @@ module ActiveRecord # ActiveRecord::Schema is only supported by database adapters that also # support migrations, the two features being very similar. class Schema < Migration - private_class_method :new - - def self.migrations_path + def migrations_path ActiveRecord::Migrator.migrations_path end @@ -48,7 +46,7 @@ module ActiveRecord # ... # end def self.define(info={}, &block) - instance_eval(&block) + new.instance_eval(&block) unless info[:version].blank? initialize_schema_migrations_table diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index f8eabd884a..acb63d0031 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -18,10 +18,10 @@ if ActiveRecord::Base.connection.supports_migrations? class ActiveRecord::Migration class < Date: Wed, 17 Nov 2010 13:31:43 -0800 Subject: schema migrations work as instances --- activerecord/lib/active_record/migration.rb | 11 +++++++---- activerecord/lib/active_record/schema.rb | 5 +++-- activerecord/test/cases/migration_test.rb | 1 + 3 files changed, 11 insertions(+), 6 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 4a1a84b74b..390c24c2f2 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -288,12 +288,15 @@ module ActiveRecord # class Migration class << self - attr_accessor :verbose attr_accessor :delegate # :nodoc: - def method_missing(name, *args, &block) # :nodoc: - (delegate || superclass.delegate).send(name, *args, &block) - end end + + def self.method_missing(name, *args, &block) # :nodoc: + (delegate || superclass.delegate).send(name, *args, &block) + end + + cattr_accessor :verbose + self.delegate = new self.verbose = true diff --git a/activerecord/lib/active_record/schema.rb b/activerecord/lib/active_record/schema.rb index a621248cc3..c6bb5c1961 100644 --- a/activerecord/lib/active_record/schema.rb +++ b/activerecord/lib/active_record/schema.rb @@ -46,11 +46,12 @@ module ActiveRecord # ... # end def self.define(info={}, &block) - new.instance_eval(&block) + schema = new + schema.instance_eval(&block) unless info[:version].blank? initialize_schema_migrations_table - assume_migrated_upto_version(info[:version], migrations_path) + assume_migrated_upto_version(info[:version], schema.migrations_path) end end end diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index acb63d0031..5cd6c735af 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -19,6 +19,7 @@ if ActiveRecord::Base.connection.supports_migrations? class < Date: Wed, 17 Nov 2010 13:44:26 -0800 Subject: singleton method added is no longer needed --- activerecord/lib/active_record/migration.rb | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 390c24c2f2..936e9a19a2 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -332,7 +332,7 @@ module ActiveRecord end result = nil - time = Benchmark.measure { result = send("#{direction}") } + time = Benchmark.measure { result = send(direction) } case direction when :up then announce "migrated (%.4fs)" % time.real; write @@ -342,24 +342,6 @@ module ActiveRecord result end - # Because the method added may do an alias_method, it can be invoked - # recursively. We use @ignore_new_methods as a guard to indicate whether - # it is safe for the call to proceed. - def singleton_method_added(sym) #:nodoc: - return if defined?(@ignore_new_methods) && @ignore_new_methods - - begin - @ignore_new_methods = true - - case sym - when :up, :down - singleton_class.send(:alias_method_chain, sym, "benchmarks") - end - ensure - @ignore_new_methods = false - end - end - def write(text="") puts(text) if verbose end -- cgit v1.2.3 From 68b66ef3082b5d6ca47f621ea51cad9321847caf Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 13:55:03 -0800 Subject: testing instance based migrations --- activerecord/lib/active_record/migration.rb | 2 ++ activerecord/test/cases/migration_test.rb | 38 +++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 936e9a19a2..470f63e8f7 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -306,11 +306,13 @@ module ActiveRecord def up self.class.delegate = self + return unless self.class.respond_to?(:up) self.class.up end def down self.class.delegate = self + return unless self.class.respond_to?(:down) self.class.down end diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index 5cd6c735af..3037d73a1b 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -1166,6 +1166,44 @@ if ActiveRecord::Base.connection.supports_migrations? assert_raise(ActiveRecord::StatementInvalid) { Reminder.find(:first) } end + class MockMigration < ActiveRecord::Migration + attr_reader :went_up, :went_down + def initialize + @went_up = false + @went_down = false + end + + def up + @went_up = true + super + end + + def down + @went_down = true + super + end + end + + def test_instance_based_migration_up + migration = MockMigration.new + assert !migration.went_up, 'have not gone up' + assert !migration.went_down, 'have not gone down' + + migration.migrate :up + assert migration.went_up, 'have gone up' + assert !migration.went_down, 'have not gone down' + end + + def test_instance_based_migration_down + migration = MockMigration.new + assert !migration.went_up, 'have not gone up' + assert !migration.went_down, 'have not gone down' + + migration.migrate :down + assert !migration.went_up, 'have gone up' + assert migration.went_down, 'have not gone down' + end + def test_migrator_one_up assert !Person.column_methods_hash.include?(:last_name) assert !Reminder.table_exists? -- cgit v1.2.3 From b0a6f58068394d11841b57d94b3a6ecb42c3b8b0 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 14:10:51 -0800 Subject: do not need these accessors --- activerecord/lib/active_record/migration.rb | 8 -------- 1 file changed, 8 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 470f63e8f7..6acb8382e7 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -392,14 +392,6 @@ module ActiveRecord end end - def verbose - self.class.verbose - end - - def verbose= verbosity - self.class.verbose = verbosity - end - def copy(destination, sources, options = {}) copied = [] -- cgit v1.2.3 From 17c7723451eba00eddbacf65e47d151e3ccafa5b Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 14:26:14 -0800 Subject: updating CHANGELOG --- activerecord/CHANGELOG | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 11eb47917d..7f54c0fa30 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,14 @@ *Rails 3.1.0 (unreleased)* +* Migrations should use instance methods rather than class methods: + class FooMigration < ActiveRecord::Migration + def up + ... + end + end + + [Aaron Patterson] + * has_one maintains the association with separate after_create/after_update instead of a single after_save. [fxn] -- cgit v1.2.3 From 606e41a4dd67fd0af2166b9b74836c2fd11c189e Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 14:48:12 -0800 Subject: these methods are no longer needed --- activerecord/lib/active_record/migration.rb | 8 -------- 1 file changed, 8 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 6acb8382e7..222fb4d83c 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -316,14 +316,6 @@ module ActiveRecord self.class.down end - def up_with_benchmarks #:nodoc: - migrate(:up) - end - - def down_with_benchmarks #:nodoc: - migrate(:down) - end - # Execute this migration in the named direction def migrate(direction) return unless respond_to?(direction) -- cgit v1.2.3 From 7906e08bbacc2f7d96227e8a739c6762cd45a6ca Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 14:49:47 -0800 Subject: fixing indentation since these methods are not class methods --- activerecord/lib/active_record/migration.rb | 166 ++++++++++++++-------------- 1 file changed, 83 insertions(+), 83 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 222fb4d83c..ca3ca54fa0 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -316,115 +316,115 @@ module ActiveRecord self.class.down end - # Execute this migration in the named direction - def migrate(direction) - return unless respond_to?(direction) + # Execute this migration in the named direction + def migrate(direction) + return unless respond_to?(direction) - case direction - when :up then announce "migrating" - when :down then announce "reverting" - end - - result = nil - time = Benchmark.measure { result = send(direction) } + case direction + when :up then announce "migrating" + when :down then announce "reverting" + end - case direction - when :up then announce "migrated (%.4fs)" % time.real; write - when :down then announce "reverted (%.4fs)" % time.real; write - end + result = nil + time = Benchmark.measure { result = send(direction) } - result + case direction + when :up then announce "migrated (%.4fs)" % time.real; write + when :down then announce "reverted (%.4fs)" % time.real; write end - def write(text="") - puts(text) if verbose - end + result + end - def announce(message) - version = defined?(@version) ? @version : nil + def write(text="") + puts(text) if verbose + end - text = "#{version} #{name}: #{message}" - length = [0, 75 - text.length].max - write "== %s %s" % [text, "=" * length] - end + def announce(message) + version = defined?(@version) ? @version : nil - def say(message, subitem=false) - write "#{subitem ? " ->" : "--"} #{message}" - end + text = "#{version} #{name}: #{message}" + length = [0, 75 - text.length].max + write "== %s %s" % [text, "=" * length] + end - def say_with_time(message) - say(message) - result = nil - time = Benchmark.measure { result = yield } - say "%.4fs" % time.real, :subitem - say("#{result} rows", :subitem) if result.is_a?(Integer) - result - end + def say(message, subitem=false) + write "#{subitem ? " ->" : "--"} #{message}" + end - def suppress_messages - save, self.verbose = verbose, false - yield - ensure - self.verbose = save - end + def say_with_time(message) + say(message) + result = nil + time = Benchmark.measure { result = yield } + say "%.4fs" % time.real, :subitem + say("#{result} rows", :subitem) if result.is_a?(Integer) + result + end - def connection - ActiveRecord::Base.connection - end + def suppress_messages + save, self.verbose = verbose, false + yield + ensure + self.verbose = save + end - def method_missing(method, *arguments, &block) - arg_list = arguments.map{ |a| a.inspect } * ', ' + def connection + ActiveRecord::Base.connection + end - say_with_time "#{method}(#{arg_list})" do - unless arguments.empty? || method == :execute - arguments[0] = Migrator.proper_table_name(arguments.first) - end - return super unless connection.respond_to?(method) - connection.send(method, *arguments, &block) + def method_missing(method, *arguments, &block) + arg_list = arguments.map{ |a| a.inspect } * ', ' + + say_with_time "#{method}(#{arg_list})" do + unless arguments.empty? || method == :execute + arguments[0] = Migrator.proper_table_name(arguments.first) end + return super unless connection.respond_to?(method) + connection.send(method, *arguments, &block) end + end - def copy(destination, sources, options = {}) - copied = [] + def copy(destination, sources, options = {}) + copied = [] - FileUtils.mkdir_p(destination) unless File.exists?(destination) + FileUtils.mkdir_p(destination) unless File.exists?(destination) - destination_migrations = ActiveRecord::Migrator.migrations(destination) - last = destination_migrations.last - sources.each do |name, path| - source_migrations = ActiveRecord::Migrator.migrations(path) + destination_migrations = ActiveRecord::Migrator.migrations(destination) + last = destination_migrations.last + sources.each do |name, path| + source_migrations = ActiveRecord::Migrator.migrations(path) - source_migrations.each do |migration| - source = File.read(migration.filename) - source = "# This migration comes from #{name} (originally #{migration.version})\n#{source}" + source_migrations.each do |migration| + source = File.read(migration.filename) + source = "# This migration comes from #{name} (originally #{migration.version})\n#{source}" - if duplicate = destination_migrations.detect { |m| m.name == migration.name } - options[:on_skip].call(name, migration) if File.read(duplicate.filename) != source && options[:on_skip] - next - end + if duplicate = destination_migrations.detect { |m| m.name == migration.name } + options[:on_skip].call(name, migration) if File.read(duplicate.filename) != source && options[:on_skip] + next + end - migration.version = next_migration_number(last ? last.version + 1 : 0).to_i - new_path = File.join(destination, "#{migration.version}_#{migration.name.underscore}.rb") - old_path, migration.filename = migration.filename, new_path - last = migration + migration.version = next_migration_number(last ? last.version + 1 : 0).to_i + new_path = File.join(destination, "#{migration.version}_#{migration.name.underscore}.rb") + old_path, migration.filename = migration.filename, new_path + last = migration - FileUtils.cp(old_path, migration.filename) - copied << migration - options[:on_copy].call(name, migration, old_path) if options[:on_copy] - destination_migrations << migration - end + FileUtils.cp(old_path, migration.filename) + copied << migration + options[:on_copy].call(name, migration, old_path) if options[:on_copy] + destination_migrations << migration end - - copied end - def next_migration_number(number) - if ActiveRecord::Base.timestamped_migrations - [Time.now.utc.strftime("%Y%m%d%H%M%S"), "%.14d" % number].max - else - "%.3d" % number - end + copied + end + + def next_migration_number(number) + if ActiveRecord::Base.timestamped_migrations + [Time.now.utc.strftime("%Y%m%d%H%M%S"), "%.14d" % number].max + else + "%.3d" % number end + end end # MigrationProxy is used to defer loading of the actual migration classes -- cgit v1.2.3 From d1fcba81188fafae2a65cc4c2ebca67df3f36f75 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 14:52:52 -0800 Subject: fixing documentation, removing unused AS files --- activerecord/lib/active_record/migration.rb | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index ca3ca54fa0..80aa123fdd 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -1,6 +1,3 @@ -require 'active_support/core_ext/kernel/singleton_class' -require 'active_support/core_ext/module/aliasing' - module ActiveRecord # Exception that can be raised to stop migrations from going backwards. class IrreversibleMigration < ActiveRecordError @@ -43,11 +40,11 @@ module ActiveRecord # Example of a simple migration: # # class AddSsl < ActiveRecord::Migration - # def self.up + # def up # add_column :accounts, :ssl_enabled, :boolean, :default => 1 # end # - # def self.down + # def down # remove_column :accounts, :ssl_enabled # end # end @@ -63,7 +60,7 @@ module ActiveRecord # Example of a more complex migration that also needs to initialize data: # # class AddSystemSettings < ActiveRecord::Migration - # def self.up + # def up # create_table :system_settings do |t| # t.string :name # t.string :label @@ -77,7 +74,7 @@ module ActiveRecord # :value => 1 # end # - # def self.down + # def down # drop_table :system_settings # end # end @@ -147,11 +144,11 @@ module ActiveRecord # # This will generate the file timestamp_add_fieldname_to_tablename, which will look like this: # class AddFieldnameToTablename < ActiveRecord::Migration - # def self.up + # def up # add_column :tablenames, :fieldname, :string # end # - # def self.down + # def down # remove_column :tablenames, :fieldname # end # end @@ -179,11 +176,11 @@ module ActiveRecord # Not all migrations change the schema. Some just fix the data: # # class RemoveEmptyTags < ActiveRecord::Migration - # def self.up + # def up # Tag.find(:all).each { |tag| tag.destroy if tag.pages.empty? } # end # - # def self.down + # def down # # not much we can do to restore deleted data # raise ActiveRecord::IrreversibleMigration, "Can't recover the deleted tags" # end @@ -192,12 +189,12 @@ module ActiveRecord # Others remove columns when they migrate up instead of down: # # class RemoveUnnecessaryItemAttributes < ActiveRecord::Migration - # def self.up + # def up # remove_column :items, :incomplete_items_count # remove_column :items, :completed_items_count # end # - # def self.down + # def down # add_column :items, :incomplete_items_count # add_column :items, :completed_items_count # end @@ -206,11 +203,11 @@ module ActiveRecord # And sometimes you need to do something in SQL not abstracted directly by migrations: # # class MakeJoinUnique < ActiveRecord::Migration - # def self.up + # def up # execute "ALTER TABLE `pages_linked_pages` ADD UNIQUE `page_id_linked_page_id` (`page_id`,`linked_page_id`)" # end # - # def self.down + # def down # execute "ALTER TABLE `pages_linked_pages` DROP INDEX `page_id_linked_page_id`" # end # end -- cgit v1.2.3 From c1a63c8dba00b9f6ed81b3a46ecf595273cd876d Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 14:54:08 -0800 Subject: fixing more documentation --- activerecord/lib/active_record/migration.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 80aa123fdd..65befa7473 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -135,7 +135,7 @@ module ActiveRecord # in the db/migrate/ directory where timestamp is the # UTC formatted date and time that the migration was generated. # - # You may then edit the self.up and self.down methods of + # You may then edit the up and down methods of # MyNewMigration. # # There is a special syntactic shortcut to generate migrations that add fields to a table. @@ -220,7 +220,7 @@ module ActiveRecord # latest column data from after the new column was added. Example: # # class AddPeopleSalary < ActiveRecord::Migration - # def self.up + # def up # add_column :people, :salary, :integer # Person.reset_column_information # Person.find(:all).each do |p| @@ -240,7 +240,7 @@ module ActiveRecord # You can also insert your own messages and benchmarks by using the +say_with_time+ # method: # - # def self.up + # def up # ... # say_with_time "Updating salaries..." do # Person.find(:all).each do |p| -- cgit v1.2.3 From 4d35f8b6615d31fbed4806ee3c260e24447f435b Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 14:57:46 -0800 Subject: updating generators --- .../rails/generators/active_record/migration/templates/migration.rb | 4 ++-- .../lib/rails/generators/active_record/model/templates/migration.rb | 4 ++-- .../generators/active_record/session_migration/templates/migration.rb | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/rails/generators/active_record/migration/templates/migration.rb b/activerecord/lib/rails/generators/active_record/migration/templates/migration.rb index 8ac21c1410..126b6f434b 100644 --- a/activerecord/lib/rails/generators/active_record/migration/templates/migration.rb +++ b/activerecord/lib/rails/generators/active_record/migration/templates/migration.rb @@ -1,5 +1,5 @@ class <%= migration_class_name %> < ActiveRecord::Migration - def self.up + def up <% attributes.each do |attribute| -%> <%- if migration_action -%> <%= migration_action %>_column :<%= table_name %>, :<%= attribute.name %><% if migration_action == 'add' %>, :<%= attribute.type %><% end %> @@ -7,7 +7,7 @@ class <%= migration_class_name %> < ActiveRecord::Migration <%- end -%> end - def self.down + def down <% attributes.reverse.each do |attribute| -%> <%- if migration_action -%> <%= migration_action == 'add' ? 'remove' : 'add' %>_column :<%= table_name %>, :<%= attribute.name %><% if migration_action == 'remove' %>, :<%= attribute.type %><% end %> diff --git a/activerecord/lib/rails/generators/active_record/model/templates/migration.rb b/activerecord/lib/rails/generators/active_record/model/templates/migration.rb index 1f68487304..70e064be21 100644 --- a/activerecord/lib/rails/generators/active_record/model/templates/migration.rb +++ b/activerecord/lib/rails/generators/active_record/model/templates/migration.rb @@ -1,5 +1,5 @@ class <%= migration_class_name %> < ActiveRecord::Migration - def self.up + def up create_table :<%= table_name %> do |t| <% for attribute in attributes -%> t.<%= attribute.type %> :<%= attribute.name %> @@ -10,7 +10,7 @@ class <%= migration_class_name %> < ActiveRecord::Migration end end - def self.down + def down drop_table :<%= table_name %> end end diff --git a/activerecord/lib/rails/generators/active_record/session_migration/templates/migration.rb b/activerecord/lib/rails/generators/active_record/session_migration/templates/migration.rb index 919822af7b..8f0bf1ef0d 100644 --- a/activerecord/lib/rails/generators/active_record/session_migration/templates/migration.rb +++ b/activerecord/lib/rails/generators/active_record/session_migration/templates/migration.rb @@ -1,5 +1,5 @@ class <%= migration_class_name %> < ActiveRecord::Migration - def self.up + def up create_table :<%= session_table_name %> do |t| t.string :session_id, :null => false t.text :data @@ -10,7 +10,7 @@ class <%= migration_class_name %> < ActiveRecord::Migration add_index :<%= session_table_name %>, :updated_at end - def self.down + def down drop_table :<%= session_table_name %> end end -- cgit v1.2.3 From 43e2e10f4fd1111e485d4d1b1e509c00dc13c58c Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 15:30:01 -0800 Subject: adding an initialize with name and version defaults --- activerecord/lib/active_record/migration.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 65befa7473..0eabc4a4aa 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -297,8 +297,11 @@ module ActiveRecord self.delegate = new self.verbose = true - def name - self.class.name + attr_accessor :name, :version + + def initialize + @name = self.class.name + @version = nil end def up @@ -338,8 +341,6 @@ module ActiveRecord end def announce(message) - version = defined?(@version) ? @version : nil - text = "#{version} #{name}: #{message}" length = [0, 75 - text.length].max write "== %s %s" % [text, "=" * length] -- cgit v1.2.3 From f978c4b2e44401260bbf4b5a954fda0b2bc71781 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Wed, 17 Nov 2010 16:12:21 -0500 Subject: remove the rescue block by returning a not asking Base for lookup_ancestors. It was also marked for later optimization. --- activerecord/lib/active_record/base.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index b35f59d6df..60a9ca7464 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -736,15 +736,12 @@ module ActiveRecord #:nodoc: def lookup_ancestors #:nodoc: klass = self classes = [klass] + return classes if klass == ActiveRecord::Base + while klass != klass.base_class classes << klass = klass.superclass end classes - rescue - # OPTIMIZE this rescue is to fix this test: ./test/cases/reflection_test.rb:56:in `test_human_name_for_column' - # Apparently the method base_class causes some trouble. - # It now works for sure. - [self] end # Set the i18n scope to overwrite ActiveModel. -- cgit v1.2.3 From 9c1993bf6bdd0a3d2c46310c953631c1d5a7e3d1 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Wed, 17 Nov 2010 23:12:22 +0800 Subject: replace and with && as per rails coding convention --- activerecord/lib/active_record/base.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 60a9ca7464..314f676711 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1283,7 +1283,7 @@ MSG # ["name='%s' and group_id='%s'", "foo'bar", 4] returns "name='foo''bar' and group_id='4'" def sanitize_sql_array(ary) statement, *values = ary - if values.first.is_a?(Hash) and statement =~ /:\w+/ + if values.first.is_a?(Hash) && statement =~ /:\w+/ replace_named_bind_variables(statement, values.first) elsif statement.include?('?') replace_bind_variables(statement, values) -- cgit v1.2.3 From fe42c00ac38b834ee9ef34f0707558cf02dbe6c0 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 31 Oct 2010 11:16:16 +0000 Subject: Fix bug with 0bb85ed9ffa9808926b46e8f7e59cab5b85ac19f which missed out a fixtures declaration in cascaded_eager_loading_test.rb --- activerecord/test/cases/associations/cascaded_eager_loading_test.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb index 37c6f354a8..0742e311d9 100644 --- a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb +++ b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb @@ -10,7 +10,8 @@ require 'models/reply' require 'models/person' class CascadedEagerLoadingTest < ActiveRecord::TestCase - fixtures :authors, :mixins, :companies, :posts, :topics, :accounts, :comments, :categorizations, :people + fixtures :authors, :mixins, :companies, :posts, :topics, :accounts, :comments, + :categorizations, :people, :categories def test_eager_association_loading_with_cascaded_two_levels authors = Author.find(:all, :include=>{:posts=>:comments}, :order=>"authors.id") -- cgit v1.2.3 From 56c5820458fd3981161393c285cce67fdf35e60b Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 16:14:17 -0800 Subject: use shorter form for sql literals --- activerecord/lib/active_record/relation/query_methods.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 9c399d3333..514576564c 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -141,7 +141,7 @@ module ActiveRecord "#{@klass.table_name}.#{@klass.primary_key} DESC" : reverse_sql_order(order_clause).join(', ') - except(:order).order(Arel::SqlLiteral.new(order)) + except(:order).order(Arel.sql(order)) end def arel -- cgit v1.2.3 From 00693209ecc222842949d7cab076f89890cbd507 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 17:10:40 -0800 Subject: collapsing same table / column WHERE clauses to be OR [#4598 state:resolved] --- .../lib/active_record/relation/query_methods.rb | 29 +++++++++++++++++++--- 1 file changed, 25 insertions(+), 4 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 514576564c..07ca2e2088 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -174,10 +174,7 @@ module ActiveRecord arel = build_joins(arel, @joins_values) unless @joins_values.empty? - (@where_values - ['']).uniq.each do |where| - where = Arel.sql(where) if String === where - arel = arel.where(Arel::Nodes::Grouping.new(where)) - end + arel = collapse_wheres(arel, (@where_values - ['']).uniq) arel = arel.having(*@having_values.uniq.reject{|h| h.blank?}) unless @having_values.empty? @@ -198,6 +195,30 @@ module ActiveRecord private + def collapse_wheres(arel, wheres) + equalities = wheres.grep(Arel::Nodes::Equality) + + groups = equalities.group_by do |equality| + left = equality.left + # table, column + [left.relation.name, left.name] + end + + groups.each do |_, eqls| + head = eqls.first + test = eqls.inject(head) do |memo, expr| + expr == head ? expr : memo.or(expr) + end + arel = arel.where(test) + end + + (wheres - equalities).each do |where| + where = Arel.sql(where) if String === where + arel = arel.where(Arel::Nodes::Grouping.new(where)) + end + arel + end + def build_where(opts, other = []) case opts when String, Array -- cgit v1.2.3 From c6bfd6802ae4c8e179e875a707ec42bf73d13a20 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Sat, 15 May 2010 17:43:35 -0300 Subject: When use where more than once on the same column, relation doesn't do an 'or' or 'in' with the values --- activerecord/test/cases/relations_test.rb | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'activerecord') diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index e39b1f396c..6649923421 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -451,6 +451,15 @@ class RelationTest < ActiveRecord::TestCase assert_equal author, authors end + def test_find_all_using_where_twice_should_or_the_relation + david = authors(:david) + relation = Author.unscoped + relation = relation.where(:name => david.name) + relation = relation.where(:name => 'Santiago') + relation = relation.where(:id => david.id) + assert_equal [david], relation.all + end + def test_exists davids = Author.where(:name => 'David') assert davids.exists? -- cgit v1.2.3 From 80d9b724c3f2f49ce99f1c41eddbebe7cf16686d Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 17 Nov 2010 17:28:40 -0800 Subject: group can be done by left side only --- activerecord/lib/active_record/relation/query_methods.rb | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 07ca2e2088..9e7503a60d 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -199,15 +199,12 @@ module ActiveRecord equalities = wheres.grep(Arel::Nodes::Equality) groups = equalities.group_by do |equality| - left = equality.left - # table, column - [left.relation.name, left.name] + equality.left end groups.each do |_, eqls| - head = eqls.first - test = eqls.inject(head) do |memo, expr| - expr == head ? expr : memo.or(expr) + test = eqls.inject(eqls.shift) do |memo, expr| + memo.or(expr) end arel = arel.where(test) end -- cgit v1.2.3 From c5a284f8eb6113f06030ea7a18543905146e8768 Mon Sep 17 00:00:00 2001 From: Alex Rothenberg Date: Mon, 8 Nov 2010 15:30:27 -0500 Subject: Adapters can specify maximum number of ids they support in a list of expressions (default is nil meaning unlimited but Oracle imposes a limit of 1000) Limit is used to make multiple queries when preloading associated has_many or habtm records --- .../lib/active_record/association_preload.rb | 26 ++++++++--- .../connection_adapters/abstract_adapter.rb | 5 +++ activerecord/test/cases/associations/eager_test.rb | 52 ++++++++++++++++++++++ 3 files changed, 78 insertions(+), 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index 911a5155fd..a205549d60 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -193,13 +193,17 @@ module ActiveRecord conditions = "t0.#{reflection.primary_key_name} #{in_or_equals_for_ids(ids)}" conditions << append_conditions(reflection, preload_options) - associated_records = reflection.klass.unscoped.where([conditions, ids]). + associated_records_proxy = reflection.klass.unscoped. includes(options[:include]). joins("INNER JOIN #{connection.quote_table_name options[:join_table]} t0 ON #{reflection.klass.quoted_table_name}.#{reflection.klass.primary_key} = t0.#{reflection.association_foreign_key}"). select("#{options[:select] || table_name+'.*'}, t0.#{reflection.primary_key_name} as the_parent_record_id"). - order(options[:order]).to_a + order(options[:order]) - set_association_collection_records(id_to_record_map, reflection.name, associated_records, 'the_parent_record_id') + all_associated_records = associated_records(ids) do |some_ids| + associated_records_proxy.where([conditions, ids]).to_a + end + + set_association_collection_records(id_to_record_map, reflection.name, all_associated_records, 'the_parent_record_id') end def preload_has_one_association(records, reflection, preload_options={}) @@ -358,13 +362,14 @@ module ActiveRecord find_options = { :select => preload_options[:select] || options[:select] || Arel::SqlLiteral.new("#{table_name}.*"), :include => preload_options[:include] || options[:include], - :conditions => [conditions, ids], :joins => options[:joins], :group => preload_options[:group] || options[:group], :order => preload_options[:order] || options[:order] } - reflection.klass.scoped.apply_finder_options(find_options).to_a + associated_records(ids) do |some_ids| + reflection.klass.scoped.apply_finder_options(find_options.merge(:conditions => [conditions, some_ids])).to_a + end end @@ -382,6 +387,17 @@ module ActiveRecord def in_or_equals_for_ids(ids) ids.size > 1 ? "IN (?)" : "= ?" end + + # Some databases impose a limit on the number of ids in a list (in Oracle its 1000) + # Make several smaller queries if necessary or make one query if the adapter supports it + def associated_records(ids) + max_ids_in_a_list = connection.ids_in_list_limit || ids.size + records = [] + ids.each_slice(max_ids_in_a_list) do |some_ids| + records += yield(some_ids) + end + records + end end end end diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index f3fba9a3a9..ada1560ce2 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -91,6 +91,11 @@ module ActiveRecord false end + # Does this adapter restrict the number of ids you can use in a list. Oracle has a limit of 1000. + def ids_in_list_limit + nil + end + # QUOTING ================================================== # Override to return the quoted table name. Defaults to column quoting. diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index 66fb5ac1e1..c532522e76 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -79,6 +79,58 @@ class EagerAssociationTest < ActiveRecord::TestCase end end + def test_preloading_has_many_in_multiple_queries_with_more_ids_than_database_can_handle + Post.connection.expects(:ids_in_list_limit).at_least_once.returns(5) + posts = Post.find(:all, :include=>:comments) + assert_equal 7, posts.size + end + + def test_preloading_has_many_in_one_queries_when_database_has_no_limit_on_ids_it_can_handle + Post.connection.expects(:ids_in_list_limit).at_least_once.returns(nil) + posts = Post.find(:all, :include=>:comments) + assert_equal 7, posts.size + end + + def test_preloading_habtm_in_multiple_queries_with_more_ids_than_database_can_handle + Post.connection.expects(:ids_in_list_limit).at_least_once.returns(5) + posts = Post.find(:all, :include=>:categories) + assert_equal 7, posts.size + end + + def test_preloading_habtm_in_one_queries_when_database_has_no_limit_on_ids_it_can_handle + Post.connection.expects(:ids_in_list_limit).at_least_once.returns(nil) + posts = Post.find(:all, :include=>:categories) + assert_equal 7, posts.size + end + + def test_load_associated_records_in_one_query_when_adapter_has_no_limit + Post.connection.expects(:ids_in_list_limit).at_least_once.returns(nil) + Post.expects(:i_was_called).with([1,2,3,4,5,6,7]).returns([1]) + associated_records = Post.send(:associated_records, [1,2,3,4,5,6,7]) do |some_ids| + Post.i_was_called(some_ids) + end + assert_equal [1], associated_records + end + + def test_load_associated_records_in_several_queries_when_many_ids_passed + Post.connection.expects(:ids_in_list_limit).at_least_once.returns(5) + Post.expects(:i_was_called).with([1,2,3,4,5]).returns([1]) + Post.expects(:i_was_called).with([6,7]).returns([6]) + associated_records = Post.send(:associated_records, [1,2,3,4,5,6,7]) do |some_ids| + Post.i_was_called(some_ids) + end + assert_equal [1,6], associated_records + end + + def test_load_associated_records_in_one_query_when_a_few_ids_passed + Post.connection.expects(:ids_in_list_limit).at_least_once.returns(5) + Post.expects(:i_was_called).with([1,2,3]).returns([1]) + associated_records = Post.send(:associated_records, [1,2,3]) do |some_ids| + Post.i_was_called(some_ids) + end + assert_equal [1], associated_records + end + def test_including_duplicate_objects_from_belongs_to popular_post = Post.create!(:title => 'foo', :body => "I like cars!") comment = popular_post.comments.create!(:body => "lol") -- cgit v1.2.3 From 26923756fb23eb9c2993a365f9819027f20d5e77 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 18 Nov 2010 10:01:21 -0800 Subject: removing space errors --- activerecord/lib/active_record/association_preload.rb | 2 +- activerecord/test/cases/associations/eager_test.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index a205549d60..9743b1b4a7 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -387,7 +387,7 @@ module ActiveRecord def in_or_equals_for_ids(ids) ids.size > 1 ? "IN (?)" : "= ?" end - + # Some databases impose a limit on the number of ids in a list (in Oracle its 1000) # Make several smaller queries if necessary or make one query if the adapter supports it def associated_records(ids) diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index c532522e76..c00b8a1cde 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -102,7 +102,7 @@ class EagerAssociationTest < ActiveRecord::TestCase posts = Post.find(:all, :include=>:categories) assert_equal 7, posts.size end - + def test_load_associated_records_in_one_query_when_adapter_has_no_limit Post.connection.expects(:ids_in_list_limit).at_least_once.returns(nil) Post.expects(:i_was_called).with([1,2,3,4,5,6,7]).returns([1]) -- cgit v1.2.3 From e107dcca6b132d39f578b48fba39891eaf902a0d Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 18 Nov 2010 13:39:21 -0800 Subject: testing multiple ORd queries --- activerecord/test/cases/relations_test.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'activerecord') diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 6649923421..535bcd4396 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -460,6 +460,18 @@ class RelationTest < ActiveRecord::TestCase assert_equal [david], relation.all end + def test_find_all_with_multiple_ors + david = authors(:david) + relation = [ + { :name => david.name }, + { :name => 'Santiago' }, + { :name => 'tenderlove' }, + ].inject(Author.unscoped) do |memo, param| + memo.where(param) + end + assert_equal [david], relation.all + end + def test_exists davids = Author.where(:name => 'David') assert davids.exists? -- cgit v1.2.3 From 07a74f196d6766c09e28ed696debb120a035dde7 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 18 Nov 2010 15:53:59 -0800 Subject: connection is set from the connection pool during migrations --- activerecord/lib/active_record/migration.rb | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 0eabc4a4aa..06a8747fde 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -300,8 +300,9 @@ module ActiveRecord attr_accessor :name, :version def initialize - @name = self.class.name - @version = nil + @name = self.class.name + @version = nil + @connection = nil end def up @@ -326,7 +327,12 @@ module ActiveRecord end result = nil - time = Benchmark.measure { result = send(direction) } + time = nil + ActiveRecord::Base.connection_pool.with_connection do |conn| + @connection = conn + time = Benchmark.measure { result = send(direction) } + @connection = nil + end case direction when :up then announce "migrated (%.4fs)" % time.real; write @@ -367,7 +373,7 @@ module ActiveRecord end def connection - ActiveRecord::Base.connection + @connection || ActiveRecord::Base.connection end def method_missing(method, *arguments, &block) -- cgit v1.2.3 From 9280fbf795d26146fe149514a32e22612b0311ee Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 19 Nov 2010 10:13:56 -0800 Subject: instantiate the delegate object after initialize is defined so that our initialize method actually gets called --- activerecord/lib/active_record/migration.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 06a8747fde..9892c6c338 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -294,9 +294,6 @@ module ActiveRecord cattr_accessor :verbose - self.delegate = new - self.verbose = true - attr_accessor :name, :version def initialize @@ -305,6 +302,10 @@ module ActiveRecord @connection = nil end + # instantiate the delegate object after initialize is defined + self.verbose = true + self.delegate = new + def up self.class.delegate = self return unless self.class.respond_to?(:up) -- cgit v1.2.3 From 24174d1b3aa1b8ac4fec95f82f6204e2d095805d Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 19 Nov 2010 10:23:13 -0800 Subject: this return value is not used, so stop returning it --- activerecord/lib/active_record/migration.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 9892c6c338..b303ff4225 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -327,11 +327,10 @@ module ActiveRecord when :down then announce "reverting" end - result = nil time = nil ActiveRecord::Base.connection_pool.with_connection do |conn| @connection = conn - time = Benchmark.measure { result = send(direction) } + time = Benchmark.measure { send(direction) } @connection = nil end @@ -339,8 +338,6 @@ module ActiveRecord when :up then announce "migrated (%.4fs)" % time.real; write when :down then announce "reverted (%.4fs)" % time.real; write end - - result end def write(text="") -- cgit v1.2.3 From 843e319f78631d056e5018a690d0e16fc4dee619 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 18 Nov 2010 14:59:25 -0800 Subject: partial implementation of the command recorder --- activerecord/lib/active_record/migration.rb | 2 + .../active_record/migration/command_recorder.rb | 48 +++++++++++++++ .../test/cases/migration/command_recorder_test.rb | 72 ++++++++++++++++++++++ 3 files changed, 122 insertions(+) create mode 100644 activerecord/lib/active_record/migration/command_recorder.rb create mode 100644 activerecord/test/cases/migration/command_recorder_test.rb (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index b303ff4225..e7063bca11 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -284,6 +284,8 @@ module ActiveRecord # In application.rb. # class Migration + autoload :CommandRecorder, 'active_record/migration/command_recorder' + class << self attr_accessor :delegate # :nodoc: end diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb new file mode 100644 index 0000000000..25b8649350 --- /dev/null +++ b/activerecord/lib/active_record/migration/command_recorder.rb @@ -0,0 +1,48 @@ +module ActiveRecord + class Migration + # ActiveRecord::Migration::CommandRecorder records commands done during + # a migration and knows how to reverse those commands. + class CommandRecorder + attr_reader :commands + + def initialize + @commands = [] + end + + # record +command+. +command+ should be a method name and arguments. + # For example: + # + # recorder.record(:method_name, [:arg1, arg2]) + def record(*command) + @commands << command + end + + # Returns a list that represents commands that are the inverse of the + # commands stored in +commands+. + def inverse + @commands.map { |name, args| send(:"invert_#{name}", args) } + end + + private + def invert_create_table(args) + [:drop_table, args] + end + + def invert_rename_table(args) + [:rename_table, args.reverse] + end + + def invert_add_column(args) + [:remove_column, args.first(2)] + end + + def invert_rename_index(args) + [:rename_index, args.reverse] + end + + def invert_rename_column(args) + [:rename_column, [args.first] + args.last(2).reverse] + end + end + end +end diff --git a/activerecord/test/cases/migration/command_recorder_test.rb b/activerecord/test/cases/migration/command_recorder_test.rb new file mode 100644 index 0000000000..c33ce7cadf --- /dev/null +++ b/activerecord/test/cases/migration/command_recorder_test.rb @@ -0,0 +1,72 @@ +require "cases/helper" + +module ActiveRecord + class Migration + class CommandRecorderTest < ActiveRecord::TestCase + def setup + @recorder = CommandRecorder.new + end + + def test_record + @recorder.record :create_table, [:system_settings] + assert_equal 1, @recorder.commands.length + end + + def test_inverse + @recorder.record :create_table, [:system_settings] + assert_equal 1, @recorder.inverse.length + + @recorder.record :rename_table, [:old, :new] + assert_equal 2, @recorder.inverse.length + end + + def test_invert_create_table + @recorder.record :create_table, [:system_settings] + drop_table = @recorder.inverse.first + assert_equal [:drop_table, [:system_settings]], drop_table + end + + def test_invert_rename_table + @recorder.record :rename_table, [:old, :new] + rename = @recorder.inverse.first + assert_equal [:rename_table, [:new, :old]], rename + end + + def test_invert_add_column + @recorder.record :add_column, [:table, :column, :type, {}] + remove = @recorder.inverse.first + assert_equal [:remove_column, [:table, :column]], remove + end + + def test_invert_rename_column + @recorder.record :rename_column, [:table, :old, :new] + rename = @recorder.inverse.first + assert_equal [:rename_column, [:table, :new, :old]], rename + end + + def test_invert_add_index + @recorder.record :add_index, [:table, [:one, :two], {:options => true}] + remove = @recorder.inverse.first + assert_equal [:remove_index, [:table, {:column => [:one, :two]}]], remove + end + + def test_invert_rename_index + @recorder.record :rename_index, [:old, :new] + rename = @recorder.inverse.first + assert_equal [:rename_index, [:new, :old]], rename + end + + def test_invert_add_timestamps + @recorder.record :add_timestamps, [:table] + remove = @recorder.inverse.first + assert_equal [:remove_timestamps, [:table]], remove + end + + def test_invert_remove_timestamps + @recorder.record :remove_timestamps, [:table] + add = @recorder.inverse.first + assert_equal [:add_timestamps, [:table]], add + end + end + end +end -- cgit v1.2.3 From 24b637a80f659eb03f8efe459f9d6aae2338e434 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 18 Nov 2010 15:05:29 -0800 Subject: inverting add_index --- activerecord/lib/active_record/migration/command_recorder.rb | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb index 25b8649350..032ce3aad8 100644 --- a/activerecord/lib/active_record/migration/command_recorder.rb +++ b/activerecord/lib/active_record/migration/command_recorder.rb @@ -43,6 +43,11 @@ module ActiveRecord def invert_rename_column(args) [:rename_column, [args.first] + args.last(2).reverse] end + + def invert_add_index(args) + table, columns, _ = *args + [:remove_index, [table, {:column => columns}]] + end end end end -- cgit v1.2.3 From 5d93900dc6a423fe8d7b0c6e330deeaca4b8b72c Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 18 Nov 2010 15:07:25 -0800 Subject: add and remove timestamps can be inverted --- activerecord/lib/active_record/migration/command_recorder.rb | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb index 032ce3aad8..231e981e53 100644 --- a/activerecord/lib/active_record/migration/command_recorder.rb +++ b/activerecord/lib/active_record/migration/command_recorder.rb @@ -48,6 +48,14 @@ module ActiveRecord table, columns, _ = *args [:remove_index, [table, {:column => columns}]] end + + def invert_remove_timestamps(args) + [:add_timestamps, args] + end + + def invert_add_timestamps(args) + [:remove_timestamps, args] + end end end end -- cgit v1.2.3 From b29a24bb6f13b8af9c12b77ee0ddc1f84c79ab55 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 18 Nov 2010 15:09:25 -0800 Subject: commands are reversed --- activerecord/lib/active_record/migration/command_recorder.rb | 2 +- activerecord/test/cases/migration/command_recorder_test.rb | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb index 231e981e53..73450c2390 100644 --- a/activerecord/lib/active_record/migration/command_recorder.rb +++ b/activerecord/lib/active_record/migration/command_recorder.rb @@ -20,7 +20,7 @@ module ActiveRecord # Returns a list that represents commands that are the inverse of the # commands stored in +commands+. def inverse - @commands.map { |name, args| send(:"invert_#{name}", args) } + @commands.reverse.map { |name, args| send(:"invert_#{name}", args) } end private diff --git a/activerecord/test/cases/migration/command_recorder_test.rb b/activerecord/test/cases/migration/command_recorder_test.rb index c33ce7cadf..47c3332078 100644 --- a/activerecord/test/cases/migration/command_recorder_test.rb +++ b/activerecord/test/cases/migration/command_recorder_test.rb @@ -20,6 +20,13 @@ module ActiveRecord assert_equal 2, @recorder.inverse.length end + def test_inverted_commands_are_reveresed + @recorder.record :create_table, [:hello] + @recorder.record :create_table, [:world] + tables = @recorder.inverse.map(&:last) + assert_equal [[:world], [:hello]], tables + end + def test_invert_create_table @recorder.record :create_table, [:system_settings] drop_table = @recorder.inverse.first -- cgit v1.2.3 From 96b50a039276b4391ddf07b0a74850ce7bad6863 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 18 Nov 2010 15:12:09 -0800 Subject: IrreversibleMigration is raised if we cannot invert the command --- activerecord/lib/active_record/migration/command_recorder.rb | 6 +++++- activerecord/test/cases/migration/command_recorder_test.rb | 7 +++++++ 2 files changed, 12 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb index 73450c2390..87ad603c04 100644 --- a/activerecord/lib/active_record/migration/command_recorder.rb +++ b/activerecord/lib/active_record/migration/command_recorder.rb @@ -20,7 +20,11 @@ module ActiveRecord # Returns a list that represents commands that are the inverse of the # commands stored in +commands+. def inverse - @commands.reverse.map { |name, args| send(:"invert_#{name}", args) } + @commands.reverse.map { |name, args| + method = :"invert_#{name}" + raise IrreversibleMigration unless respond_to?(method, true) + send(method, args) + } end private diff --git a/activerecord/test/cases/migration/command_recorder_test.rb b/activerecord/test/cases/migration/command_recorder_test.rb index 47c3332078..50d75e0400 100644 --- a/activerecord/test/cases/migration/command_recorder_test.rb +++ b/activerecord/test/cases/migration/command_recorder_test.rb @@ -7,6 +7,13 @@ module ActiveRecord @recorder = CommandRecorder.new end + def test_unknown_commands_raise_exception + @recorder.record :execute, ['some sql'] + assert_raises(ActiveRecord::IrreversibleMigration) do + @recorder.inverse + end + end + def test_record @recorder.record :create_table, [:system_settings] assert_equal 1, @recorder.commands.length -- cgit v1.2.3 From 0d7410faabaafc6cd64f636b22a450faedc732ca Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 18 Nov 2010 15:15:03 -0800 Subject: updating documentation --- activerecord/lib/active_record/migration/command_recorder.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb index 87ad603c04..8a98a177d5 100644 --- a/activerecord/lib/active_record/migration/command_recorder.rb +++ b/activerecord/lib/active_record/migration/command_recorder.rb @@ -18,7 +18,13 @@ module ActiveRecord end # Returns a list that represents commands that are the inverse of the - # commands stored in +commands+. + # commands stored in +commands+. For example: + # + # recorder.record(:rename_table, [:old, :new]) + # recorder.inverse # => [:rename_table, [:new, :old]] + # + # This method will raise an IrreversibleMigration exception if it cannot + # invert the +commands+. def inverse @commands.reverse.map { |name, args| method = :"invert_#{name}" -- cgit v1.2.3 From 6dbbfae5638a6c847fd63d52a72247e2bb15a320 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 18 Nov 2010 15:52:13 -0800 Subject: adding invertable migration test --- .../test/cases/invertable_migration_test.rb | 42 ++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 activerecord/test/cases/invertable_migration_test.rb (limited to 'activerecord') diff --git a/activerecord/test/cases/invertable_migration_test.rb b/activerecord/test/cases/invertable_migration_test.rb new file mode 100644 index 0000000000..2477048954 --- /dev/null +++ b/activerecord/test/cases/invertable_migration_test.rb @@ -0,0 +1,42 @@ +require "cases/helper" + +module ActiveRecord + class InvertableMigrationTest < ActiveRecord::TestCase + class InvertableMigration < ActiveRecord::Migration + def change + create_table("horses") do |t| + t.column :content, :text + t.column :remind_at, :datetime + end + end + + def write(text = '') + # sssshhhhh!! + end + end + + def treardown + if ActiveRecord::Base.connection.table_exists?("horses") + ActiveRecord::Base.connection.drop_table("horses") + end + end + + def test_invertable? + migration = InvertableMigration.new + assert migration.invertable?, 'should be invertable' + end + + def test_up + migration = InvertableMigration.new + migration.migrate(:up) + assert migration.connection.table_exists?("horses"), "horses should exist" + end + + def test_down + migration = InvertableMigration.new + migration.migrate :up + migration.migrate :down + assert !migration.connection.table_exists?("horses") + end + end +end -- cgit v1.2.3 From 6519df4157a861c9c9d567ee57983ded0e967a10 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 19 Nov 2010 09:26:11 -0800 Subject: command recorder will record commands sent to a delegate object --- .../active_record/migration/command_recorder.rb | 16 +++++++++++++--- .../test/cases/migration/command_recorder_test.rb | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb index 8a98a177d5..fa189b8009 100644 --- a/activerecord/lib/active_record/migration/command_recorder.rb +++ b/activerecord/lib/active_record/migration/command_recorder.rb @@ -3,10 +3,11 @@ module ActiveRecord # ActiveRecord::Migration::CommandRecorder records commands done during # a migration and knows how to reverse those commands. class CommandRecorder - attr_reader :commands + attr_reader :commands, :delegate - def initialize + def initialize(delegate = nil) @commands = [] + @delegate = delegate end # record +command+. +command+ should be a method name and arguments. @@ -29,10 +30,19 @@ module ActiveRecord @commands.reverse.map { |name, args| method = :"invert_#{name}" raise IrreversibleMigration unless respond_to?(method, true) - send(method, args) + __send__(method, args) } end + def respond_to?(*args) # :nodoc: + super || delegate.respond_to?(*args) + end + + def send(method, *args) # :nodoc: + return super unless respond_to?(method) + record(method, args) + end + private def invert_create_table(args) [:drop_table, args] diff --git a/activerecord/test/cases/migration/command_recorder_test.rb b/activerecord/test/cases/migration/command_recorder_test.rb index 50d75e0400..ea2292dda5 100644 --- a/activerecord/test/cases/migration/command_recorder_test.rb +++ b/activerecord/test/cases/migration/command_recorder_test.rb @@ -7,6 +7,28 @@ module ActiveRecord @recorder = CommandRecorder.new end + def test_respond_to_delegates + recorder = CommandRecorder.new(Class.new { + def america; end + }.new) + assert recorder.respond_to?(:america) + end + + def test_send_calls_super + assert_raises(NoMethodError) do + @recorder.send(:create_table, :horses) + end + end + + def test_send_delegates_to_record + recorder = CommandRecorder.new(Class.new { + def create_table(name); end + }.new) + assert recorder.respond_to?(:create_table), 'respond_to? create_table' + recorder.send(:create_table, :horses) + assert_equal [[:create_table, [:horses]]], recorder.commands + end + def test_unknown_commands_raise_exception @recorder.record :execute, ['some sql'] assert_raises(ActiveRecord::IrreversibleMigration) do -- cgit v1.2.3 From 47017bd1697d6b4d6780356a403f91536eacd689 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 19 Nov 2010 10:31:03 -0800 Subject: invertable migrations are working --- activerecord/lib/active_record/migration.rb | 20 +++++++++++++++++++- .../lib/active_record/migration/command_recorder.rb | 2 +- activerecord/test/cases/invertable_migration_test.rb | 5 ----- 3 files changed, 20 insertions(+), 7 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index e7063bca11..99dd50ccb1 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -332,7 +332,25 @@ module ActiveRecord time = nil ActiveRecord::Base.connection_pool.with_connection do |conn| @connection = conn - time = Benchmark.measure { send(direction) } + if respond_to?(:change) + if direction == :down + recorder = CommandRecorder.new(@connection) + suppress_messages do + @connection = recorder + change + end + @connection = conn + time = Benchmark.measure { + recorder.inverse.each do |cmd, args| + send(cmd, *args) + end + } + else + time = Benchmark.measure { change } + end + else + time = Benchmark.measure { send(direction) } + end @connection = nil end diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb index fa189b8009..fc669d2e89 100644 --- a/activerecord/lib/active_record/migration/command_recorder.rb +++ b/activerecord/lib/active_record/migration/command_recorder.rb @@ -3,7 +3,7 @@ module ActiveRecord # ActiveRecord::Migration::CommandRecorder records commands done during # a migration and knows how to reverse those commands. class CommandRecorder - attr_reader :commands, :delegate + attr_accessor :commands, :delegate def initialize(delegate = nil) @commands = [] diff --git a/activerecord/test/cases/invertable_migration_test.rb b/activerecord/test/cases/invertable_migration_test.rb index 2477048954..ab08cf6fe2 100644 --- a/activerecord/test/cases/invertable_migration_test.rb +++ b/activerecord/test/cases/invertable_migration_test.rb @@ -21,11 +21,6 @@ module ActiveRecord end end - def test_invertable? - migration = InvertableMigration.new - assert migration.invertable?, 'should be invertable' - end - def test_up migration = InvertableMigration.new migration.migrate(:up) -- cgit v1.2.3 From 0cc6c46fe9711d2377ff1ae6c55a03b3d1267874 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 19 Nov 2010 10:42:33 -0800 Subject: testing a non-invertible migration case --- .../test/cases/invertable_migration_test.rb | 26 +++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/invertable_migration_test.rb b/activerecord/test/cases/invertable_migration_test.rb index ab08cf6fe2..b4c1dccb22 100644 --- a/activerecord/test/cases/invertable_migration_test.rb +++ b/activerecord/test/cases/invertable_migration_test.rb @@ -2,16 +2,28 @@ require "cases/helper" module ActiveRecord class InvertableMigrationTest < ActiveRecord::TestCase - class InvertableMigration < ActiveRecord::Migration + class SilentMigration < ActiveRecord::Migration + def write(text = '') + # sssshhhhh!! + end + end + + class InvertableMigration < SilentMigration def change create_table("horses") do |t| t.column :content, :text t.column :remind_at, :datetime end end + end - def write(text = '') - # sssshhhhh!! + class NonInvertableMigration < SilentMigration + def change + create_table("horses") do |t| + t.column :content, :text + t.column :remind_at, :datetime + end + remove_column "horses", :content end end @@ -21,6 +33,14 @@ module ActiveRecord end end + def test_no_reverse + migration = NonInvertableMigration.new + migration.migrate(:up) + assert_raises(IrreversibleMigration) do + migration.migrate(:down) + end + end + def test_up migration = InvertableMigration.new migration.migrate(:up) -- cgit v1.2.3 From 87124457e55a207e190fc8dc981c2dc68445c736 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 19 Nov 2010 10:55:57 -0800 Subject: fisting my spelling errors --- .../test/cases/invertable_migration_test.rb | 57 ---------------------- .../test/cases/invertible_migration_test.rb | 57 ++++++++++++++++++++++ 2 files changed, 57 insertions(+), 57 deletions(-) delete mode 100644 activerecord/test/cases/invertable_migration_test.rb create mode 100644 activerecord/test/cases/invertible_migration_test.rb (limited to 'activerecord') diff --git a/activerecord/test/cases/invertable_migration_test.rb b/activerecord/test/cases/invertable_migration_test.rb deleted file mode 100644 index b4c1dccb22..0000000000 --- a/activerecord/test/cases/invertable_migration_test.rb +++ /dev/null @@ -1,57 +0,0 @@ -require "cases/helper" - -module ActiveRecord - class InvertableMigrationTest < ActiveRecord::TestCase - class SilentMigration < ActiveRecord::Migration - def write(text = '') - # sssshhhhh!! - end - end - - class InvertableMigration < SilentMigration - def change - create_table("horses") do |t| - t.column :content, :text - t.column :remind_at, :datetime - end - end - end - - class NonInvertableMigration < SilentMigration - def change - create_table("horses") do |t| - t.column :content, :text - t.column :remind_at, :datetime - end - remove_column "horses", :content - end - end - - def treardown - if ActiveRecord::Base.connection.table_exists?("horses") - ActiveRecord::Base.connection.drop_table("horses") - end - end - - def test_no_reverse - migration = NonInvertableMigration.new - migration.migrate(:up) - assert_raises(IrreversibleMigration) do - migration.migrate(:down) - end - end - - def test_up - migration = InvertableMigration.new - migration.migrate(:up) - assert migration.connection.table_exists?("horses"), "horses should exist" - end - - def test_down - migration = InvertableMigration.new - migration.migrate :up - migration.migrate :down - assert !migration.connection.table_exists?("horses") - end - end -end diff --git a/activerecord/test/cases/invertible_migration_test.rb b/activerecord/test/cases/invertible_migration_test.rb new file mode 100644 index 0000000000..7e61ab0e47 --- /dev/null +++ b/activerecord/test/cases/invertible_migration_test.rb @@ -0,0 +1,57 @@ +require "cases/helper" + +module ActiveRecord + class InvertibleMigrationTest < ActiveRecord::TestCase + class SilentMigration < ActiveRecord::Migration + def write(text = '') + # sssshhhhh!! + end + end + + class InvertibleMigration < SilentMigration + def change + create_table("horses") do |t| + t.column :content, :text + t.column :remind_at, :datetime + end + end + end + + class NonInvertibleMigration < SilentMigration + def change + create_table("horses") do |t| + t.column :content, :text + t.column :remind_at, :datetime + end + remove_column "horses", :content + end + end + + def treardown + if ActiveRecord::Base.connection.table_exists?("horses") + ActiveRecord::Base.connection.drop_table("horses") + end + end + + def test_no_reverse + migration = NonInvertibleMigration.new + migration.migrate(:up) + assert_raises(IrreversibleMigration) do + migration.migrate(:down) + end + end + + def test_up + migration = InvertibleMigration.new + migration.migrate(:up) + assert migration.connection.table_exists?("horses"), "horses should exist" + end + + def test_down + migration = InvertibleMigration.new + migration.migrate :up + migration.migrate :down + assert !migration.connection.table_exists?("horses") + end + end +end -- cgit v1.2.3 From db32b545dadae7808c210cd7ceef949a620490f0 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 19 Nov 2010 11:20:02 -0800 Subject: adding Migration#change to the CHANGELOG --- activerecord/CHANGELOG | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 7f54c0fa30..f1e16ea2c1 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,23 @@ *Rails 3.1.0 (unreleased)* +* Migrations can be defined as reversible, meaning that the migration system +will figure out how to reverse your migration. To use reversible migrations, +just define the "change" method. For example: + + class MyMigration < ActiveRecord::Migration + def change + create_table(:horses) do + t.column :content, :text + t.column :remind_at, :datetime + end + end + end + +Some things cannot be automatically reversed for you. If you know how to +reverse those things, you should define 'up' and 'down' in your migration. If +you define something in `change` that cannot be reversed, an +IrreversibleMigration exception will be raised when going down. + * Migrations should use instance methods rather than class methods: class FooMigration < ActiveRecord::Migration def up -- cgit v1.2.3 From a4d9b1d329ef897f6b23216b01cb510db35a37b5 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 19 Nov 2010 11:34:42 -0800 Subject: adding documentation for reversible migrations --- activerecord/lib/active_record/migration.rb | 32 ++++++++++++++++++++++ .../active_record/migration/command_recorder.rb | 12 +++++++- 2 files changed, 43 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 99dd50ccb1..f6321f1499 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -283,6 +283,38 @@ module ActiveRecord # # In application.rb. # + # == Reversible Migrations + # + # Starting with Rails 3.1, you will be able to define reversible migrations. + # Reversible migrations are migrations that know how to go +down+ for you. + # You simply supply the +up+ logic, and the Migration system will figure out + # how to execute the down commands for you. + # + # To define a reversible migration, define the +change+ method in your + # migration like this: + # + # class TenderloveMigration < ActiveRecord::Migration + # def change + # create_table(:horses) do + # t.column :content, :text + # t.column :remind_at, :datetime + # end + # end + # end + # + # This migration will create the horses table for you on the way up, and + # automatically figure out how to drop the table on the way down. + # + # Some commands like +remove_column+ cannot be reversed. If you care to + # define how to move up and down in these cases, you should define the +up+ + # and +down+ methods as before. + # + # If a command cannot be reversed, an + # ActiveRecord::IrreversibleMigration exception will be raised when + # the migration is moving down. + # + # For a list of commands that are reversible, please see + # ActiveRecord::Migration::CommandRecorder. class Migration autoload :CommandRecorder, 'active_record/migration/command_recorder' diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb index fc669d2e89..d7e481905a 100644 --- a/activerecord/lib/active_record/migration/command_recorder.rb +++ b/activerecord/lib/active_record/migration/command_recorder.rb @@ -1,7 +1,17 @@ module ActiveRecord class Migration # ActiveRecord::Migration::CommandRecorder records commands done during - # a migration and knows how to reverse those commands. + # a migration and knows how to reverse those commands. The CommandRecorder + # knows how to invert the following commands: + # + # * add_column + # * add_index + # * add_timestamp + # * create_table + # * remove_timestamps + # * rename_column + # * rename_index + # * rename_table class CommandRecorder attr_accessor :commands, :delegate -- cgit v1.2.3 From 598fc85f9eff4949264bcec78a0ed9b90c46f616 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 19 Nov 2010 11:42:58 -0800 Subject: fisting typeo, thanks @vinibaggio --- activerecord/test/cases/invertible_migration_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/invertible_migration_test.rb b/activerecord/test/cases/invertible_migration_test.rb index 7e61ab0e47..afec64750e 100644 --- a/activerecord/test/cases/invertible_migration_test.rb +++ b/activerecord/test/cases/invertible_migration_test.rb @@ -27,7 +27,7 @@ module ActiveRecord end end - def treardown + def teardown if ActiveRecord::Base.connection.table_exists?("horses") ActiveRecord::Base.connection.drop_table("horses") end -- cgit v1.2.3 From 6bd93f672ac1c0c4cfe708745c925b4bbe666947 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 19 Nov 2010 11:48:23 -0800 Subject: wtf vim --- activerecord/CHANGELOG | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index f1e16ea2c1..a3e3051b96 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -9,7 +9,7 @@ just define the "change" method. For example: create_table(:horses) do t.column :content, :text t.column :remind_at, :datetime - end + end end end -- cgit v1.2.3 From 938243feb9ea177b84276821818c9eed66064340 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 19 Nov 2010 16:26:09 -0800 Subject: do not require ruby-debug automatically. please require it if you have declared it as a dependency --- activerecord/test/cases/helper.rb | 5 ----- 1 file changed, 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index 52f26b71f5..f9bbc5299b 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -13,11 +13,6 @@ require 'active_record' require 'active_support/dependencies' require 'connection' -begin - require 'ruby-debug' -rescue LoadError -end - # Show backtraces for deprecated behavior for quicker cleanup. ActiveSupport::Deprecation.debug = true -- cgit v1.2.3 From d7db6a88734c3b666f4b85f266d223eff408b294 Mon Sep 17 00:00:00 2001 From: Josh Kalderimis Date: Fri, 19 Nov 2010 18:29:33 +0100 Subject: class inheritable attributes is used no more! all internal use of class inheritable has been changed to class_attribute. class inheritable attributes has been deprecated. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- activerecord/lib/active_record/associations.rb | 13 +++++++------ .../associations/association_collection.rb | 2 +- .../lib/active_record/attribute_methods/dirty.rb | 3 ++- .../attribute_methods/time_zone_conversion.rb | 6 ++++-- activerecord/lib/active_record/base.rb | 20 ++++++++++++-------- activerecord/lib/active_record/named_scope.rb | 10 ++++++---- activerecord/lib/active_record/nested_attributes.rb | 13 +++++++++---- activerecord/lib/active_record/reflection.rb | 20 +++++++++----------- activerecord/lib/active_record/timestamp.rb | 8 +++++--- activerecord/test/cases/associations_test.rb | 8 ++++---- 10 files changed, 59 insertions(+), 44 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 08a4ebfd7e..7da38cd03f 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -4,6 +4,7 @@ require 'active_support/core_ext/module/delegation' require 'active_support/core_ext/object/blank' require 'active_support/core_ext/string/conversions' require 'active_support/core_ext/module/remove_method' +require 'active_support/core_ext/class/attribute' module ActiveRecord class InverseOfAssociationNotFoundError < ActiveRecordError #:nodoc: @@ -1810,12 +1811,12 @@ module ActiveRecord callbacks.each do |callback_name| full_callback_name = "#{callback_name}_for_#{association_name}" defined_callbacks = options[callback_name.to_sym] - if options.has_key?(callback_name.to_sym) - class_inheritable_reader full_callback_name.to_sym - write_inheritable_attribute(full_callback_name.to_sym, [defined_callbacks].flatten) - else - write_inheritable_attribute(full_callback_name.to_sym, []) - end + + full_callback_value = options.has_key?(callback_name.to_sym) ? [defined_callbacks].flatten : [] + + # TODO : why do i need method_defined? I think its because of the inheritance chain + class_attribute full_callback_name.to_sym unless method_defined?(full_callback_name) + self.send("#{full_callback_name}=", full_callback_value) end end diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 6090376bb8..774103342a 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -530,7 +530,7 @@ module ActiveRecord def callbacks_for(callback_name) full_callback_name = "#{callback_name}_for_#{@reflection.name}" - @owner.class.read_inheritable_attribute(full_callback_name.to_sym) || [] + @owner.class.send(full_callback_name.to_sym) || [] end def ensure_owner_is_not_new diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb index 439880c1fa..c19a33faa8 100644 --- a/activerecord/lib/active_record/attribute_methods/dirty.rb +++ b/activerecord/lib/active_record/attribute_methods/dirty.rb @@ -1,3 +1,4 @@ +require 'active_support/core_ext/class/attribute' require 'active_support/core_ext/object/blank' module ActiveRecord @@ -88,7 +89,7 @@ module ActiveRecord end def clone_with_time_zone_conversion_attribute?(attr, old) - old.class.name == "Time" && time_zone_aware_attributes && !skip_time_zone_conversion_for_attributes.include?(attr.to_sym) + old.class.name == "Time" && time_zone_aware_attributes && !self.skip_time_zone_conversion_for_attributes.include?(attr.to_sym) end end end diff --git a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb index d640b26b74..dc2785b6bf 100644 --- a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb +++ b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb @@ -1,3 +1,5 @@ +require 'active_support/core_ext/class/attribute' + module ActiveRecord module AttributeMethods module TimeZoneConversion @@ -7,7 +9,7 @@ module ActiveRecord cattr_accessor :time_zone_aware_attributes, :instance_writer => false self.time_zone_aware_attributes = false - class_inheritable_accessor :skip_time_zone_conversion_for_attributes, :instance_writer => false + class_attribute :skip_time_zone_conversion_for_attributes, :instance_writer => false self.skip_time_zone_conversion_for_attributes = [] end @@ -54,7 +56,7 @@ module ActiveRecord private def create_time_zone_conversion_attribute?(name, column) - time_zone_aware_attributes && !skip_time_zone_conversion_for_attributes.include?(name.to_sym) && [:datetime, :timestamp].include?(column.type) + time_zone_aware_attributes && !self.skip_time_zone_conversion_for_attributes.include?(name.to_sym) && [:datetime, :timestamp].include?(column.type) end end end diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 314f676711..8bde1869c7 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -7,7 +7,7 @@ require 'active_support/time' require 'active_support/core_ext/class/attribute' require 'active_support/core_ext/class/attribute_accessors' require 'active_support/core_ext/class/delegating_attributes' -require 'active_support/core_ext/class/inheritable_attributes' +require 'active_support/core_ext/class/attribute' require 'active_support/core_ext/array/extract_options' require 'active_support/core_ext/hash/deep_merge' require 'active_support/core_ext/hash/indifferent_access' @@ -412,7 +412,7 @@ module ActiveRecord #:nodoc: self.store_full_sti_class = true # Stores the default scope for the class - class_inheritable_accessor :default_scoping, :instance_writer => false + class_attribute :default_scoping, :instance_writer => false self.default_scoping = [] # Returns a hash of all the attributes that have been specified for serialization as @@ -420,6 +420,9 @@ module ActiveRecord #:nodoc: class_attribute :serialized_attributes self.serialized_attributes = {} + class_attribute :_attr_readonly, :instance_writer => false + self._attr_readonly = [] + class << self # Class methods delegate :find, :first, :last, :all, :destroy, :destroy_all, :exists?, :delete, :delete_all, :update, :update_all, :to => :scoped delegate :find_each, :find_in_batches, :to => :scoped @@ -504,12 +507,12 @@ module ActiveRecord #:nodoc: # Attributes listed as readonly will be used to create a new record but update operations will # ignore these fields. def attr_readonly(*attributes) - write_inheritable_attribute(:attr_readonly, Set.new(attributes.map { |a| a.to_s }) + (readonly_attributes || [])) + self._attr_readonly = Set.new(attributes.map { |a| a.to_s }) + (self._attr_readonly || []) end # Returns an array of all the attributes that have been specified as readonly. def readonly_attributes - read_inheritable_attribute(:attr_readonly) || [] + self._attr_readonly end # If you have an attribute that needs to be saved to the database as an object, and retrieved as the same object, @@ -724,8 +727,8 @@ module ActiveRecord #:nodoc: @arel_engine = @relation = @arel_table = nil end - def reset_column_information_and_inheritable_attributes_for_all_subclasses#:nodoc: - descendants.each { |klass| klass.reset_inheritable_attributes; klass.reset_column_information } + def reset_column_information_for_all_subclasses#:nodoc: + descendants.each { |klass| klass.reset_column_information } end def attribute_method?(attribute) @@ -1126,7 +1129,8 @@ MSG # Article.create.published # => true def default_scope(options = {}) reset_scoped_methods - self.default_scoping << construct_finder_arel(options, default_scoping.pop) + default_scoping = self.default_scoping.dup + self.default_scoping = default_scoping << construct_finder_arel(options, default_scoping.pop) end def current_scoped_methods #:nodoc: @@ -1579,7 +1583,7 @@ MSG self.class.columns_hash[name.to_s] end - # Returns true if +comparison_object+ is the same exact object, or +comparison_object+ + # Returns true if +comparison_object+ is the same exact object, or +comparison_object+ # is of the same type and +self+ has an ID and it is equal to +comparison_object.id+. # # Note that new records are different from any other record by definition, unless the diff --git a/activerecord/lib/active_record/named_scope.rb b/activerecord/lib/active_record/named_scope.rb index 0b92ba5caa..0f421560f0 100644 --- a/activerecord/lib/active_record/named_scope.rb +++ b/activerecord/lib/active_record/named_scope.rb @@ -2,12 +2,18 @@ require 'active_support/core_ext/array' require 'active_support/core_ext/hash/except' require 'active_support/core_ext/kernel/singleton_class' require 'active_support/core_ext/object/blank' +require 'active_support/core_ext/class/attribute' module ActiveRecord # = Active Record Named \Scopes module NamedScope extend ActiveSupport::Concern + included do + class_attribute :scopes + self.scopes = {} + end + module ClassMethods # Returns an anonymous \scope. # @@ -33,10 +39,6 @@ module ActiveRecord end end - def scopes - read_inheritable_attribute(:scopes) || write_inheritable_attribute(:scopes, {}) - end - # Adds a class method for retrieving and querying objects. A \scope represents a narrowing of a database query, # such as where(:color => :red).select('shirts.*').includes(:washing_instructions). # diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 0c3392263a..f1d3eaed38 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -2,6 +2,7 @@ require 'active_support/core_ext/hash/except' require 'active_support/core_ext/object/try' require 'active_support/core_ext/object/blank' require 'active_support/core_ext/hash/indifferent_access' +require 'active_support/core_ext/class/attribute' module ActiveRecord module NestedAttributes #:nodoc: @@ -11,7 +12,7 @@ module ActiveRecord extend ActiveSupport::Concern included do - class_inheritable_accessor :nested_attributes_options, :instance_writer => false + class_attribute :nested_attributes_options, :instance_writer => false self.nested_attributes_options = {} end @@ -268,7 +269,11 @@ module ActiveRecord if reflection = reflect_on_association(association_name) reflection.options[:autosave] = true add_autosave_association_callbacks(reflection) + + nested_attributes_options = self.nested_attributes_options.dup nested_attributes_options[association_name.to_sym] = options + self.nested_attributes_options = nested_attributes_options + type = (reflection.collection? ? :collection : :one_to_one) # def pirate_attributes=(attributes) @@ -315,7 +320,7 @@ module ActiveRecord # update_only is true, and a :_destroy key set to a truthy value, # then the existing record will be marked for destruction. def assign_nested_attributes_for_one_to_one_association(association_name, attributes) - options = nested_attributes_options[association_name] + options = self.nested_attributes_options[association_name] attributes = attributes.with_indifferent_access check_existing_record = (options[:update_only] || !attributes['id'].blank?) @@ -364,7 +369,7 @@ module ActiveRecord # { :id => '2', :_destroy => true } # ]) def assign_nested_attributes_for_collection_association(association_name, attributes_collection) - options = nested_attributes_options[association_name] + options = self.nested_attributes_options[association_name] unless attributes_collection.is_a?(Hash) || attributes_collection.is_a?(Array) raise ArgumentError, "Hash or Array expected, got #{attributes_collection.class.name} (#{attributes_collection.inspect})" @@ -433,7 +438,7 @@ module ActiveRecord end def call_reject_if(association_name, attributes) - case callback = nested_attributes_options[association_name][:reject_if] + case callback = self.nested_attributes_options[association_name][:reject_if] when Symbol method(callback).arity == 0 ? send(callback) : send(callback, attributes) when Proc diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index a2260e9a19..a07c321960 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -1,8 +1,15 @@ +require 'active_support/core_ext/class/attribute' + module ActiveRecord # = Active Record Reflection module Reflection # :nodoc: extend ActiveSupport::Concern + included do + class_attribute :reflections + self.reflections = {} + end + # Reflection enables to interrogate Active Record classes and objects # about their associations and aggregations. This information can, # for example, be used in a form builder that takes an Active Record object @@ -20,18 +27,9 @@ module ActiveRecord when :composed_of reflection = AggregateReflection.new(macro, name, options, active_record) end - write_inheritable_hash :reflections, name => reflection - reflection - end - # Returns a hash containing all AssociationReflection objects for the current class. - # Example: - # - # Invoice.reflections - # Account.reflections - # - def reflections - read_inheritable_attribute(:reflections) || write_inheritable_attribute(:reflections, {}) + self.reflections = self.reflections.merge(name => reflection) + reflection end # Returns an array of AggregateReflection objects for all the aggregations in the class. diff --git a/activerecord/lib/active_record/timestamp.rb b/activerecord/lib/active_record/timestamp.rb index 230adf6b2b..2ecbd906bd 100644 --- a/activerecord/lib/active_record/timestamp.rb +++ b/activerecord/lib/active_record/timestamp.rb @@ -1,3 +1,5 @@ +require 'active_support/core_ext/class/attribute' + module ActiveRecord # = Active Record Timestamp # @@ -29,14 +31,14 @@ module ActiveRecord extend ActiveSupport::Concern included do - class_inheritable_accessor :record_timestamps, :instance_writer => false + class_attribute :record_timestamps, :instance_writer => false self.record_timestamps = true end private def create #:nodoc: - if record_timestamps + if self.record_timestamps current_time = current_time_from_proper_timezone all_timestamp_attributes.each do |column| @@ -61,7 +63,7 @@ module ActiveRecord end def should_record_timestamps? - record_timestamps && (!partial_updates? || changed? || (attributes.keys & self.class.serialized_attributes.keys).present?) + self.record_timestamps && (!partial_updates? || changed? || (attributes.keys & self.class.serialized_attributes.keys).present?) end def timestamp_attributes_for_update_in_model diff --git a/activerecord/test/cases/associations_test.rb b/activerecord/test/cases/associations_test.rb index 93a51d3606..83c605d2bb 100644 --- a/activerecord/test/cases/associations_test.rb +++ b/activerecord/test/cases/associations_test.rb @@ -270,17 +270,17 @@ class OverridingAssociationsTest < ActiveRecord::TestCase def test_habtm_association_redefinition_callbacks_should_differ_and_not_inherited # redeclared association on AR descendant should not inherit callbacks from superclass - callbacks = PeopleList.read_inheritable_attribute(:before_add_for_has_and_belongs_to_many) + callbacks = PeopleList.before_add_for_has_and_belongs_to_many assert_equal([:enlist], callbacks) - callbacks = DifferentPeopleList.read_inheritable_attribute(:before_add_for_has_and_belongs_to_many) + callbacks = DifferentPeopleList.before_add_for_has_and_belongs_to_many assert_equal([], callbacks) end def test_has_many_association_redefinition_callbacks_should_differ_and_not_inherited # redeclared association on AR descendant should not inherit callbacks from superclass - callbacks = PeopleList.read_inheritable_attribute(:before_add_for_has_many) + callbacks = PeopleList.before_add_for_has_many assert_equal([:enlist], callbacks) - callbacks = DifferentPeopleList.read_inheritable_attribute(:before_add_for_has_many) + callbacks = DifferentPeopleList.before_add_for_has_many assert_equal([], callbacks) end -- cgit v1.2.3 From 8796f9a31a77bb2ab97ea6c2cf2036fb6e38497e Mon Sep 17 00:00:00 2001 From: Josh Kalderimis Date: Fri, 19 Nov 2010 18:30:48 +0100 Subject: removed an AR method which wasn't used internally, had no tests, and had no docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- activerecord/lib/active_record/base.rb | 4 ---- 1 file changed, 4 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 8bde1869c7..aee6f3a7eb 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -727,10 +727,6 @@ module ActiveRecord #:nodoc: @arel_engine = @relation = @arel_table = nil end - def reset_column_information_for_all_subclasses#:nodoc: - descendants.each { |klass| klass.reset_column_information } - end - def attribute_method?(attribute) super || (table_exists? && column_names.include?(attribute.to_s.sub(/=$/, ''))) end -- cgit v1.2.3 From c8ab3992f85569e43055b9e57c67324f5d5088cd Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Wed, 17 Nov 2010 21:46:55 -0500 Subject: unscoped takes care of named_scopes too --- activerecord/lib/active_record/base.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index aee6f3a7eb..aa028c6f36 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -852,8 +852,7 @@ module ActiveRecord #:nodoc: # limit(10) # Fires "SELECT * FROM posts LIMIT 10" # } # - # It is recommended to use block form of unscoped because chaining unscoped with named_scope - # does not work. Assuming that published is a named_scope following two statements are same. + # Assuming that published is a named_scope following two statements are same. # # Post.unscoped.published # Post.published -- cgit v1.2.3