From 57d750edf7c71e001ac314fa188aa1fc6292f8ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 30 Jun 2010 19:38:20 +0200 Subject: Make relation a private method. --- activerecord/lib/active_record/base.rb | 11 ++++++----- activerecord/lib/active_record/named_scope.rb | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index c8795e4496..c868ff3ae8 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -896,11 +896,6 @@ module ActiveRecord #:nodoc: store_full_sti_class ? name : name.demodulize end - def relation - @relation ||= Relation.new(self, arel_table) - finder_needs_type_condition? ? @relation.where(type_condition) : @relation - end - def arel_table @arel_table ||= Arel::Table.new(table_name, :engine => arel_engine) end @@ -941,6 +936,12 @@ module ActiveRecord #:nodoc: end private + + def relation #:nodoc: + @relation ||= Relation.new(self, arel_table) + finder_needs_type_condition? ? @relation.where(type_condition) : @relation + end + # Finder methods must instantiate through this method to work with the # single-table inheritance model that makes it possible to create # objects of different types from the same table. diff --git a/activerecord/lib/active_record/named_scope.rb b/activerecord/lib/active_record/named_scope.rb index c010dac64e..849ec9c884 100644 --- a/activerecord/lib/active_record/named_scope.rb +++ b/activerecord/lib/active_record/named_scope.rb @@ -29,7 +29,7 @@ module ActiveRecord if options.present? scoped.apply_finder_options(options) else - current_scoped_methods ? unscoped.merge(current_scoped_methods) : unscoped.clone + current_scoped_methods ? relation.merge(current_scoped_methods) : relation.clone end end -- cgit v1.2.3 From f3fedd7f84c25d1d99a70af1e21e20abb48f100f Mon Sep 17 00:00:00 2001 From: James Le Cuirot Date: Wed, 30 Jun 2010 23:22:13 +0100 Subject: Don't remove scheduled destroys when loading an association. [#4642 state:resolved] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- .../lib/active_record/associations/association_collection.rb | 7 ++++++- activerecord/test/cases/nested_attributes_test.rb | 6 ++++++ 2 files changed, 12 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 ddf4ce4058..a4e08c7d41 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -393,7 +393,12 @@ module ActiveRecord @target = find_target.map do |f| i = @target.index(f) t = @target.delete_at(i) if i - (t && t.changed?) ? t : f + if t && t.changed? + t + else + f.mark_for_destruction if t && t.marked_for_destruction? + f + end end + @target else @target = find_target diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index 3c797076e0..62237f955b 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -489,6 +489,12 @@ module NestedAttributesOnACollectionAssociationTests assert_equal 'Polly', @pirate.send(@association_name).send(:load_target).last.name end + def test_should_not_remove_scheduled_destroys_when_loading_association + @pirate.reload + @pirate.send(association_setter, [{ :id => @child_1.id, :_destroy => '1' }]) + assert @pirate.send(@association_name).send(:load_target).find { |r| r.id == @child_1.id }.marked_for_destruction? + end + def test_should_take_a_hash_with_composite_id_keys_and_assign_the_attributes_to_the_associated_models @child_1.stubs(:id).returns('ABC1X') @child_2.stubs(:id).returns('ABC2X') -- cgit v1.2.3 From 53b34e84762b7f2d6b641f99dadbb1eab42907ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 1 Jul 2010 17:07:48 +0200 Subject: Avoid calls to Rails::Application since this is not the official API. Your application should *always* reference your application const (as Blog::Application) and Rails.application should be used just internally. --- activerecord/lib/active_record/railties/databases.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/railties/databases.rake b/activerecord/lib/active_record/railties/databases.rake index 7882f05d07..5024787c3c 100644 --- a/activerecord/lib/active_record/railties/databases.rake +++ b/activerecord/lib/active_record/railties/databases.rake @@ -1,7 +1,7 @@ namespace :db do task :load_config => :rails_env do require 'active_record' - ActiveRecord::Base.configurations = Rails::Application.config.database_configuration + ActiveRecord::Base.configurations = Rails.application.config.database_configuration end namespace :create do -- cgit v1.2.3 From d10aaefcfd4141144eaf971c0560da5631e3dff5 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Thu, 1 Jul 2010 23:06:58 -0400 Subject: clarifying the comments regarding base_class declaration --- activerecord/lib/active_record/base.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index c8795e4496..9860c3c725 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -869,6 +869,9 @@ module ActiveRecord #:nodoc: # Returns the base AR subclass that this class descends from. If A # extends AR::Base, A.base_class will return A. If B descends from A # through some arbitrarily deep hierarchy, B.base_class will return A. + # + # If B < A and C < B and if A is an abstract_class then both B.base_class + # and C.base_class would return B as the answer since A is an abstract_class. def base_class class_of_active_record_descendant(self) end @@ -876,8 +879,7 @@ module ActiveRecord #:nodoc: # Set this to true if this is an abstract class (see abstract_class?). attr_accessor :abstract_class - # Returns whether this class is a base AR class. If A is a base class and - # B descends from A, then B.base_class will return B. + # Returns whether this class is an abstract class or not. def abstract_class? defined?(@abstract_class) && @abstract_class == true end -- cgit v1.2.3 From a5dda97602f2188a13cbcab5c7e9a5ba84ba876b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 5 Jul 2010 12:50:08 +0200 Subject: Define a convention for descendants and subclasses. The former should be symmetric with ancestors and include all children. However, it should not include self since ancestors + descendants should not have duplicated. The latter is symmetric to superclass in the sense it only includes direct children. By adopting a convention, we expect to have less conflict with other frameworks, as Datamapper. For this moment, to ensure ActiveModel::Validations can be used with Datamapper, we should always call ActiveSupport::DescendantsTracker.descendants(self) internally instead of self.descendants avoiding conflicts. --- activerecord/lib/active_record/observer.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/observer.rb b/activerecord/lib/active_record/observer.rb index 5f80bd86df..d2ed643f35 100644 --- a/activerecord/lib/active_record/observer.rb +++ b/activerecord/lib/active_record/observer.rb @@ -94,7 +94,7 @@ module ActiveRecord def initialize super - observed_subclasses.each { |klass| add_observer!(klass) } + observed_descendants.each { |klass| add_observer!(klass) } end def self.method_added(method) @@ -108,8 +108,8 @@ module ActiveRecord protected - def observed_subclasses - observed_classes.sum([]) { |klass| klass.send(:descendants) } + def observed_descendants + observed_classes.sum([]) { |klass| klass.descendants } end def observe_callbacks? -- cgit v1.2.3 From c1fc59c7ac1726f54d54b2896004997892e15ec9 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Tue, 6 Jul 2010 10:09:54 -0400 Subject: added more info about << operation in associations --- activerecord/lib/active_record/associations.rb | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 4caa434fc0..49da8595b2 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -763,6 +763,8 @@ module ActiveRecord # An empty array is returned if none are found. # [collection<<(object, ...)] # Adds one or more objects to the collection by setting their foreign keys to the collection's primary key. + # Note that this operation instantly fires update sql without waiting for the save or update call on the + # parent object. # [collection.delete(object, ...)] # Removes one or more objects from the collection by setting their foreign keys to +NULL+. # Objects will be in addition destroyed if they're associated with :dependent => :destroy, @@ -1186,6 +1188,8 @@ module ActiveRecord # [collection<<(object, ...)] # Adds one or more objects to the collection by creating associations in the join table # (collection.push and collection.concat are aliases to this method). + # Note that this operation instantly fires update sql without waiting for the save or update call on the + # parent object. # [collection.delete(object, ...)] # Removes one or more objects from the collection by removing their associations from the join table. # This does not destroy the objects. -- cgit v1.2.3 From 92ff71bb14b1b589a3d5c04d120e4b9210b243b1 Mon Sep 17 00:00:00 2001 From: Xavier Noria Date: Tue, 6 Jul 2010 17:29:19 +0200 Subject: documents automatic management of join models in hmt associations, in particular the gotcha that deletion is direct --- activerecord/lib/active_record/associations.rb | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 4caa434fc0..eebbd17f43 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -768,15 +768,20 @@ module ActiveRecord # Objects will be in addition destroyed if they're associated with :dependent => :destroy, # and deleted if they're associated with :dependent => :delete_all. # [collection=objects] - # Replaces the collections content by deleting and adding objects as appropriate. + # Replaces the collections content by deleting and adding objects as appropriate. If the :through + # option is true callbacks in the join models are triggered except destroy callbacks, since deletion is + # direct. # [collection_singular_ids] # Returns an array of the associated objects' ids # [collection_singular_ids=ids] - # Replace the collection with the objects identified by the primary keys in +ids+ + # Replace the collection with the objects identified by the primary keys in +ids+. This + # method loads the models and calls collection=. See above. # [collection.clear] # Removes every object from the collection. This destroys the associated objects if they # are associated with :dependent => :destroy, deletes them directly from the # database if :dependent => :delete_all, otherwise sets their foreign keys to +NULL+. + # If the :through option is true no destroy callbacks are invoked on the join models. + # Join models are directly deleted. # [collection.empty?] # Returns +true+ if there are no associated objects. # [collection.size] @@ -869,9 +874,11 @@ module ActiveRecord # [:as] # Specifies a polymorphic interface (See belongs_to). # [:through] - # Specifies a Join Model through which to perform the query. Options for :class_name and :foreign_key + # Specifies a join model through which to perform the query. Options for :class_name and :foreign_key # are ignored, as the association uses the source reflection. You can only use a :through query through a belongs_to - # has_one or has_many association on the join model. + # has_one or has_many association on the join model. The collection of join models can be managed via the collection + # API. For example, new join models are created for newly associated objects, and if some are gone their rows are deleted (directly, + # no destroy callbacks are triggered). # [:source] # Specifies the source association name used by has_many :through queries. Only use it if the name cannot be # inferred from the association. has_many :subscribers, :through => :subscriptions will look for either :subscribers or -- cgit v1.2.3 From c58e7a71b52a38885352768c74b7563395566bb6 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 7 Jul 2010 14:23:16 -0700 Subject: adding some behavioral tests for the sqlite adapter. [#5065 state:resolved] Signed-off-by: wycats --- .../cases/adapters/sqlite/sqlite_adapter_test.rb | 100 +++++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 activerecord/test/cases/adapters/sqlite/sqlite_adapter_test.rb (limited to 'activerecord') diff --git a/activerecord/test/cases/adapters/sqlite/sqlite_adapter_test.rb b/activerecord/test/cases/adapters/sqlite/sqlite_adapter_test.rb new file mode 100644 index 0000000000..69cfb00faf --- /dev/null +++ b/activerecord/test/cases/adapters/sqlite/sqlite_adapter_test.rb @@ -0,0 +1,100 @@ +require "cases/helper" + +module ActiveRecord + module ConnectionAdapters + class SQLiteAdapterTest < ActiveRecord::TestCase + def setup + @ctx = Base.sqlite3_connection :database => ':memory:', + :adapter => 'sqlite3', + :timeout => nil + @ctx.execute <<-eosql + CREATE TABLE items ( + id integer PRIMARY KEY AUTOINCREMENT, + number integer + ) + eosql + end + + def test_execute + @ctx.execute "INSERT INTO items (number) VALUES (10)" + records = @ctx.execute "SELECT * FROM items" + assert_equal 1, records.length + + record = records.first + assert_equal 10, record['number'] + assert_equal 1, record['id'] + end + + def test_quote_string + assert_equal "''", @ctx.quote_string("'") + end + + def test_insert_sql + 2.times do |i| + rv = @ctx.insert_sql "INSERT INTO items (number) VALUES (#{i})" + assert_equal(i + 1, rv) + end + + records = @ctx.execute "SELECT * FROM items" + assert_equal 2, records.length + end + + def test_insert_sql_logged + sql = "INSERT INTO items (number) VALUES (10)" + name = "foo" + + assert_logged([[sql, name]]) do + @ctx.insert_sql sql, name + end + end + + def test_insert_id_value_returned + sql = "INSERT INTO items (number) VALUES (10)" + idval = 'vuvuzela' + id = @ctx.insert_sql sql, nil, nil, idval + assert_equal idval, id + end + + def test_select_rows + 2.times do |i| + @ctx.create "INSERT INTO items (number) VALUES (#{i})" + end + rows = @ctx.select_rows 'select number, id from items' + assert_equal [[0, 1], [1, 2]], rows + end + + def test_select_rows_logged + sql = "select * from items" + name = "foo" + + assert_logged([[sql, name]]) do + @ctx.select_rows sql, name + end + end + + def test_transaction + count_sql = 'select count(*) from items' + + @ctx.begin_db_transaction + @ctx.create "INSERT INTO items (number) VALUES (10)" + + assert_equal 1, @ctx.select_rows(count_sql).first.first + @ctx.rollback_db_transaction + assert_equal 0, @ctx.select_rows(count_sql).first.first + end + + def assert_logged logs + @ctx.extend(Module.new { + attr_reader :logged + def log sql, name + @logged ||= [] + @logged << [sql, name] + yield + end + }) + yield + assert_equal logs, @ctx.logged + end + end + end +end -- cgit v1.2.3 From de51cbccf8c9d4e59a128ca8dca8c42d8d7c4dc9 Mon Sep 17 00:00:00 2001 From: Ben Somers Date: Wed, 7 Jul 2010 10:05:58 -0700 Subject: Fixed gruoped_by_title spelling [#5063 state:committed] Signed-off-by: Xavier Noria --- .../cases/associations/has_and_belongs_to_many_associations_test.rb | 4 ++-- activerecord/test/models/category.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb index 004d0156e1..b11969a841 100644 --- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb @@ -684,8 +684,8 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase end def test_find_scoped_grouped - assert_equal 4, categories(:general).posts_gruoped_by_title.size - assert_equal 1, categories(:technology).posts_gruoped_by_title.size + assert_equal 4, categories(:general).posts_grouped_by_title.size + assert_equal 1, categories(:technology).posts_grouped_by_title.size end def test_find_scoped_grouped_having diff --git a/activerecord/test/models/category.rb b/activerecord/test/models/category.rb index 5efce6aaa6..48415846dd 100644 --- a/activerecord/test/models/category.rb +++ b/activerecord/test/models/category.rb @@ -15,7 +15,7 @@ class Category < ActiveRecord::Base :conditions => { :title => 'Yet Another Testing Title' } has_and_belongs_to_many :popular_grouped_posts, :class_name => "Post", :group => "posts.type", :having => "sum(comments.post_id) > 2", :include => :comments - has_and_belongs_to_many :posts_gruoped_by_title, :class_name => "Post", :group => "title", :select => "title" + has_and_belongs_to_many :posts_grouped_by_title, :class_name => "Post", :group => "title", :select => "title" def self.what_are_you 'a category...' -- cgit v1.2.3 From f6fa6cf6117e691899c821b8999bd7dfb3f48b38 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Thu, 8 Jul 2010 10:09:20 -0400 Subject: clarifying how to create non standard primary key --- .../connection_adapters/abstract/schema_statements.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index 76b65bf219..555a611e68 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -106,7 +106,13 @@ module ActiveRecord # Join tables for +has_and_belongs_to_many+ should set :id => false. # [:primary_key] # The name of the primary key, if one is to be added automatically. - # Defaults to +id+. + # Defaults to +id+. You must NOT pass :id => false otherwise :primary_key option + # will have no effect. + # + # Also note that this just sets the primary_key in the table. You still need to + # add :set_primary_key => '' in the model to tell model what column is the + # primary_key. Models do NOT auto-detect the primary_key from table defintion. + # # [:options] # Any extra options you want appended to the table definition. # [:temporary] -- cgit v1.2.3 From a9587935dec6b5de01d51553ecc6d4157a8ec173 Mon Sep 17 00:00:00 2001 From: Xavier Noria Date: Thu, 8 Jul 2010 16:53:37 +0200 Subject: copy-edits some docs --- .../connection_adapters/abstract/schema_statements.rb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index 555a611e68..ffc3847a31 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -103,15 +103,14 @@ module ActiveRecord # The +options+ hash can include the following keys: # [:id] # Whether to automatically add a primary key column. Defaults to true. - # Join tables for +has_and_belongs_to_many+ should set :id => false. + # Join tables for +has_and_belongs_to_many+ should set it to false. # [:primary_key] # The name of the primary key, if one is to be added automatically. - # Defaults to +id+. You must NOT pass :id => false otherwise :primary_key option - # will have no effect. + # Defaults to +id+. If :id is false this option is ignored. # - # Also note that this just sets the primary_key in the table. You still need to - # add :set_primary_key => '' in the model to tell model what column is the - # primary_key. Models do NOT auto-detect the primary_key from table defintion. + # Also note that this just sets the primary key in the table. You additionally + # need to configure the primary key in the model via the +set_primary_key+ macro. + # Models do NOT auto-detect the primary key from their table definition. # # [:options] # Any extra options you want appended to the table definition. -- cgit v1.2.3 From 723a0bbe3a8737a099cd995a397b919b1957413d Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Thu, 8 Jul 2010 09:29:38 -0400 Subject: This test never runs and it has never run. Since the day this file was created this test has name not beginning with test_. Also this test is trying to use has_many on another has_many which is not supported. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- .../test/cases/associations/has_one_through_associations_test.rb | 4 ---- 1 file changed, 4 deletions(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/associations/has_one_through_associations_test.rb b/activerecord/test/cases/associations/has_one_through_associations_test.rb index 9aef3eb374..178c57435b 100644 --- a/activerecord/test/cases/associations/has_one_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_through_associations_test.rb @@ -65,10 +65,6 @@ class HasOneThroughAssociationsTest < ActiveRecord::TestCase assert_equal clubs(:moustache_club), @member.sponsor_club end - def has_one_through_to_has_many - assert_equal 2, @member.fellow_members.size - end - def test_has_one_through_eager_loading members = assert_queries(3) do #base table, through table, clubs table Member.find(:all, :include => :club, :conditions => ["name = ?", "Groucho Marx"]) -- cgit v1.2.3 From 606088df3f10dd8daec8ccc97d8279c119a503b5 Mon Sep 17 00:00:00 2001 From: Eric Chapweske Date: Fri, 29 Jan 2010 17:02:12 -0800 Subject: Mass assignment security refactoring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- activerecord/lib/active_record.rb | 1 + activerecord/lib/active_record/base.rb | 144 ++----------------- .../lib/active_record/mass_assignment_security.rb | 160 +++++++++++++++++++++ .../mass_assignment_security/permission_set.rb | 44 ++++++ .../mass_assignment_security/sanitizer.rb | 27 ++++ activerecord/test/cases/base_test.rb | 26 ++-- .../mass_assignment_security/black_list_test.rb | 28 ++++ .../permission_set_test.rb | 30 ++++ .../mass_assignment_security/sanitizer_test.rb | 36 +++++ .../mass_assignment_security/white_list_test.rb | 28 ++++ 10 files changed, 378 insertions(+), 146 deletions(-) create mode 100644 activerecord/lib/active_record/mass_assignment_security.rb create mode 100644 activerecord/lib/active_record/mass_assignment_security/permission_set.rb create mode 100644 activerecord/lib/active_record/mass_assignment_security/sanitizer.rb create mode 100644 activerecord/test/cases/mass_assignment_security/black_list_test.rb create mode 100644 activerecord/test/cases/mass_assignment_security/permission_set_test.rb create mode 100644 activerecord/test/cases/mass_assignment_security/sanitizer_test.rb create mode 100644 activerecord/test/cases/mass_assignment_security/white_list_test.rb (limited to 'activerecord') diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb index e2f2508ae8..9ab05a0548 100644 --- a/activerecord/lib/active_record.rb +++ b/activerecord/lib/active_record.rb @@ -64,6 +64,7 @@ module ActiveRecord autoload :CounterCache autoload :DynamicFinderMatch autoload :DynamicScopeMatch + autoload :MassAssignmentSecurity autoload :Migration autoload :Migrator, 'active_record/migration' autoload :NamedScope diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 3f1015dd4b..affb1fee3c 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -24,7 +24,7 @@ require 'active_record/errors' require 'active_record/log_subscriber' module ActiveRecord #:nodoc: - # = Active Record + # = Active Record # # Active Record objects don't specify their attributes directly, but rather infer them from the table definition with # which they're linked. Adding, removing, and changing attributes and their type is done directly in the database. Any change @@ -476,112 +476,16 @@ module ActiveRecord #:nodoc: connection.select_value(sql, "#{name} Count").to_i end - # Attributes named in this macro are protected from mass-assignment, - # such as new(attributes), - # update_attributes(attributes), or - # attributes=(attributes). - # - # Mass-assignment to these attributes will simply be ignored, to assign - # to them you can use direct writer methods. This is meant to protect - # sensitive attributes from being overwritten by malicious users - # tampering with URLs or forms. - # - # class Customer < ActiveRecord::Base - # attr_protected :credit_rating - # end - # - # customer = Customer.new("name" => David, "credit_rating" => "Excellent") - # customer.credit_rating # => nil - # customer.attributes = { "description" => "Jolly fellow", "credit_rating" => "Superb" } - # customer.credit_rating # => nil - # - # customer.credit_rating = "Average" - # customer.credit_rating # => "Average" - # - # To start from an all-closed default and enable attributes as needed, - # have a look at +attr_accessible+. - # - # If the access logic of your application is richer you can use Hash#except - # or Hash#slice to sanitize the hash of parameters before they are - # passed to Active Record. - # - # For example, it could be the case that the list of protected attributes - # for a given model depends on the role of the user: - # - # # Assumes plan_id is not protected because it depends on the role. - # params[:account] = params[:account].except(:plan_id) unless admin? - # @account.update_attributes(params[:account]) - # - # Note that +attr_protected+ is still applied to the received hash. Thus, - # with this technique you can at most _extend_ the list of protected - # attributes for a particular mass-assignment call. - def attr_protected(*attributes) - write_inheritable_attribute(:attr_protected, Set.new(attributes.map {|a| a.to_s}) + (protected_attributes || [])) - end - - # Returns an array of all the attributes that have been protected from mass-assignment. - def protected_attributes # :nodoc: - read_inheritable_attribute(:attr_protected) - end - - # Specifies a white list of model attributes that can be set via - # mass-assignment, such as new(attributes), - # update_attributes(attributes), or - # attributes=(attributes) - # - # This is the opposite of the +attr_protected+ macro: Mass-assignment - # will only set attributes in this list, to assign to the rest of - # attributes you can use direct writer methods. This is meant to protect - # sensitive attributes from being overwritten by malicious users - # tampering with URLs or forms. If you'd rather start from an all-open - # default and restrict attributes as needed, have a look at - # +attr_protected+. - # - # class Customer < ActiveRecord::Base - # attr_accessible :name, :nickname - # end - # - # customer = Customer.new(:name => "David", :nickname => "Dave", :credit_rating => "Excellent") - # customer.credit_rating # => nil - # customer.attributes = { :name => "Jolly fellow", :credit_rating => "Superb" } - # customer.credit_rating # => nil - # - # customer.credit_rating = "Average" - # customer.credit_rating # => "Average" - # - # If the access logic of your application is richer you can use Hash#except - # or Hash#slice to sanitize the hash of parameters before they are - # passed to Active Record. - # - # For example, it could be the case that the list of accessible attributes - # for a given model depends on the role of the user: - # - # # Assumes plan_id is accessible because it depends on the role. - # params[:account] = params[:account].except(:plan_id) unless admin? - # @account.update_attributes(params[:account]) - # - # Note that +attr_accessible+ is still applied to the received hash. Thus, - # with this technique you can at most _narrow_ the list of accessible - # attributes for a particular mass-assignment call. - def attr_accessible(*attributes) - write_inheritable_attribute(:attr_accessible, Set.new(attributes.map(&:to_s)) + (accessible_attributes || [])) + # Attributes listed as readonly can be set for a new record, but will be ignored in database updates afterwards. + def attr_readonly(*attributes) + write_inheritable_attribute(:attr_readonly, Set.new(attributes.map(&:to_s)) + (readonly_attributes || [])) end - # Returns an array of all the attributes that have been made accessible to mass-assignment. - def accessible_attributes # :nodoc: - read_inheritable_attribute(:attr_accessible) + # Returns an array of all the attributes that have been specified as readonly. + def readonly_attributes + read_inheritable_attribute(:attr_readonly) || [] end - # Attributes listed as readonly can be set for a new record, but will be ignored in database updates afterwards. - def attr_readonly(*attributes) - write_inheritable_attribute(:attr_readonly, Set.new(attributes.map(&:to_s)) + (readonly_attributes || [])) - end - - # Returns an array of all the attributes that have been specified as readonly. - def readonly_attributes - read_inheritable_attribute(: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, # then specify the name of that attribute using this method and it will be handled automatically. # The serialization is done through YAML. If +class_name+ is specified, the serialized object must be of that @@ -1716,27 +1620,6 @@ MSG end end - def remove_attributes_protected_from_mass_assignment(attributes) - safe_attributes = - if self.class.accessible_attributes.nil? && self.class.protected_attributes.nil? - attributes.reject { |key, value| attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) } - elsif self.class.protected_attributes.nil? - attributes.reject { |key, value| !self.class.accessible_attributes.include?(key.gsub(/\(.+/, "")) || attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) } - elsif self.class.accessible_attributes.nil? - attributes.reject { |key, value| self.class.protected_attributes.include?(key.gsub(/\(.+/,"")) || attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) } - else - raise "Declare either attr_protected or attr_accessible for #{self.class}, but not both." - end - - removed_attributes = attributes.keys - safe_attributes.keys - - if removed_attributes.any? - log_protected_attribute_removal(removed_attributes) - end - - safe_attributes - end - # Removes attributes which have been marked as readonly. def remove_readonly_attributes(attributes) unless self.class.readonly_attributes.nil? @@ -1746,16 +1629,10 @@ MSG end end - def log_protected_attribute_removal(*attributes) - if logger - logger.debug "WARNING: Can't mass-assign these protected attributes: #{attributes.join(', ')}" - end - end - # The primary key and inheritance column can never be set by mass-assignment for security reasons. - def attributes_protected_by_default - default = [ self.class.primary_key, self.class.inheritance_column ] - default << 'id' unless self.class.primary_key.eql? 'id' + def self.attributes_protected_by_default + default = [ primary_key, inheritance_column ] + default << 'id' unless primary_key.eql? 'id' default end @@ -1920,6 +1797,7 @@ MSG include AttributeMethods::PrimaryKey include AttributeMethods::TimeZoneConversion include AttributeMethods::Dirty + extend MassAssignmentSecurity include Callbacks, ActiveModel::Observing, Timestamp include Associations, AssociationPreload, NamedScope diff --git a/activerecord/lib/active_record/mass_assignment_security.rb b/activerecord/lib/active_record/mass_assignment_security.rb new file mode 100644 index 0000000000..07beb6405e --- /dev/null +++ b/activerecord/lib/active_record/mass_assignment_security.rb @@ -0,0 +1,160 @@ +require 'active_record/mass_assignment_security/permission_set' + +module ActiveRecord + module MassAssignmentSecurity + # Mass assignment security provides an interface for protecting attributes + # from end-user assignment. For more complex permissions, mass assignment security + # may be handled outside the model by extending a non-ActiveRecord class, + # such as a controller, with this behavior. + # + # For example, a logged in user may need to assign additional attributes depending + # on their role: + # + # class AccountsController < ApplicationController + # extend ActiveRecord::MassAssignmentSecurity + # + # attr_accessible :first_name, :last_name + # + # def self.admin_accessible_attributes + # accessible_attributes + [ :plan_id ] + # end + # + # def update + # ... + # @account.update_attributes(account_params) + # ... + # end + # + # protected + # + # def account_params + # remove_attributes_protected_from_mass_assignment(params[:account]) + # end + # + # def mass_assignment_authorizer + # admin ? admin_accessible_attributes : super + # end + # + # end + # + def self.extended(base) + base.send(:include, InstanceMethods) + end + + module InstanceMethods + + protected + + def remove_attributes_protected_from_mass_assignment(attributes) + mass_assignment_authorizer.sanitize(attributes) + end + + def mass_assignment_authorizer + self.class.mass_assignment_authorizer + end + + end + + # Attributes named in this macro are protected from mass-assignment, + # such as new(attributes), + # update_attributes(attributes), or + # attributes=(attributes). + # + # Mass-assignment to these attributes will simply be ignored, to assign + # to them you can use direct writer methods. This is meant to protect + # sensitive attributes from being overwritten by malicious users + # tampering with URLs or forms. + # + # class Customer < ActiveRecord::Base + # attr_protected :credit_rating + # end + # + # customer = Customer.new("name" => David, "credit_rating" => "Excellent") + # customer.credit_rating # => nil + # customer.attributes = { "description" => "Jolly fellow", "credit_rating" => "Superb" } + # customer.credit_rating # => nil + # + # customer.credit_rating = "Average" + # customer.credit_rating # => "Average" + # + # To start from an all-closed default and enable attributes as needed, + # have a look at +attr_accessible+. + # + # Note that using Hash#except or Hash#slice in place of +attr_protected+ + # to sanitize attributes won't provide sufficient protection. + def attr_protected(*keys) + use_authorizer(:protected_attributes) + protected_attributes.merge(keys) + end + + # Specifies a white list of model attributes that can be set via + # mass-assignment, such as new(attributes), + # update_attributes(attributes), or + # attributes=(attributes) + # + # This is the opposite of the +attr_protected+ macro: Mass-assignment + # will only set attributes in this list, to assign to the rest of + # attributes you can use direct writer methods. This is meant to protect + # sensitive attributes from being overwritten by malicious users + # tampering with URLs or forms. If you'd rather start from an all-open + # default and restrict attributes as needed, have a look at + # +attr_protected+. + # + # class Customer < ActiveRecord::Base + # attr_accessible :name, :nickname + # end + # + # customer = Customer.new(:name => "David", :nickname => "Dave", :credit_rating => "Excellent") + # customer.credit_rating # => nil + # customer.attributes = { :name => "Jolly fellow", :credit_rating => "Superb" } + # customer.credit_rating # => nil + # + # customer.credit_rating = "Average" + # customer.credit_rating # => "Average" + # + # Note that using Hash#except or Hash#slice in place of +attr_accessible+ + # to sanitize attributes won't provide sufficient protection. + def attr_accessible(*keys) + use_authorizer(:accessible_attributes) + accessible_attributes.merge(keys) + end + + # Returns an array of all the attributes that have been protected from mass-assignment. + def protected_attributes + read_inheritable_attribute(:protected_attributes) || begin + authorizer = BlackList.new + authorizer += attributes_protected_by_default + authorizer.logger = logger + write_inheritable_attribute(:protected_attributes, authorizer) + end + end + + # Returns an array of all the attributes that have been made accessible to mass-assignment. + def accessible_attributes + read_inheritable_attribute(:accessible_attributes) || begin + authorizer = WhiteList.new + authorizer.logger = logger + write_inheritable_attribute(:accessible_attributes, authorizer) + end + end + + def mass_assignment_authorizer + protected_attributes + end + + private + + # Sets the active authorizer, (attr_protected or attr_accessible). Subsequent calls + # will raise an exception when using a different authorizer_id. + def use_authorizer(authorizer_id) # :nodoc: + if active_authorizer_id = read_inheritable_attribute(:active_authorizer_id) + unless authorizer_id == active_authorizer_id + raise("Already using #{active_authorizer_id}, cannot use #{authorizer_id}") + end + else + write_inheritable_attribute(:active_authorizer_id, authorizer_id) + end + end + + end +end diff --git a/activerecord/lib/active_record/mass_assignment_security/permission_set.rb b/activerecord/lib/active_record/mass_assignment_security/permission_set.rb new file mode 100644 index 0000000000..1d34dce02e --- /dev/null +++ b/activerecord/lib/active_record/mass_assignment_security/permission_set.rb @@ -0,0 +1,44 @@ +require 'active_record/mass_assignment_security/sanitizer' + +module ActiveRecord + module MassAssignmentSecurity + class PermissionSet < Set + + attr_accessor :logger + + def merge(values) + super(values.map(&:to_s)) + end + + def include?(key) + super(remove_multiparameter_id(key)) + end + + protected + + def remove_multiparameter_id(key) + key.gsub(/\(.+/, '') + end + + end + + class WhiteList < PermissionSet + include Sanitizer + + def deny?(key) + !include?(key) + end + + end + + class BlackList < PermissionSet + include Sanitizer + + def deny?(key) + include?(key) + end + + end + + end +end \ No newline at end of file diff --git a/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb b/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb new file mode 100644 index 0000000000..4a099a147c --- /dev/null +++ b/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb @@ -0,0 +1,27 @@ +module ActiveRecord + module MassAssignmentSecurity + module Sanitizer + + # Returns all attributes not denied by the authorizer. + def sanitize(attributes) + sanitized_attributes = attributes.reject { |key, value| deny?(key) } + debug_protected_attribute_removal(attributes, sanitized_attributes) if debug? + sanitized_attributes + end + + protected + + def debug_protected_attribute_removal(attributes, sanitized_attributes) + removed_keys = attributes.keys - sanitized_attributes.keys + if removed_keys.any? + logger.debug "WARNING: Can't mass-assign protected attributes: #{removed_keys.join(', ')}" + end + end + + def debug? + logger.present? + end + + end + end +end diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index ba7db838ca..dff39cf54f 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -71,9 +71,8 @@ class Task < ActiveRecord::Base attr_protected :starting end -class TopicWithProtectedContentAndAccessibleAuthorName < ActiveRecord::Base +class TopicWithProtectedContent < ActiveRecord::Base self.table_name = 'topics' - attr_accessible :author_name attr_protected :content end @@ -956,9 +955,9 @@ class BasicsTest < ActiveRecord::TestCase end def test_mass_assignment_should_raise_exception_if_accessible_and_protected_attribute_writers_are_both_used - topic = TopicWithProtectedContentAndAccessibleAuthorName.new - assert_raise(RuntimeError) { topic.attributes = { "author_name" => "me" } } - assert_raise(RuntimeError) { topic.attributes = { "content" => "stuff" } } + assert_raise(RuntimeError) do + TopicWithProtectedContent.attr_accessible :author_name + end end def test_mass_assignment_protection @@ -1021,19 +1020,20 @@ class BasicsTest < ActiveRecord::TestCase end def test_mass_assignment_protection_inheritance - assert_nil LoosePerson.accessible_attributes - assert_equal Set.new([ 'credit_rating', 'administrator' ]), LoosePerson.protected_attributes + assert LoosePerson.accessible_attributes.blank? + assert_equal Set.new([ 'credit_rating', 'administrator', *LoosePerson.attributes_protected_by_default ]), LoosePerson.protected_attributes - assert_nil LooseDescendant.accessible_attributes - assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number' ]), LooseDescendant.protected_attributes + assert LooseDescendant.accessible_attributes.blank? + assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', *LoosePerson.attributes_protected_by_default ]), LooseDescendant.protected_attributes - assert_nil LooseDescendantSecond.accessible_attributes - assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', 'name' ]), LooseDescendantSecond.protected_attributes, 'Running attr_protected twice in one class should merge the protections' + assert LooseDescendantSecond.accessible_attributes.blank? + assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', 'name', *LoosePerson.attributes_protected_by_default ]), LooseDescendantSecond.protected_attributes, + 'Running attr_protected twice in one class should merge the protections' - assert_nil TightPerson.protected_attributes + assert (TightPerson.protected_attributes - TightPerson.attributes_protected_by_default).blank? assert_equal Set.new([ 'name', 'address' ]), TightPerson.accessible_attributes - assert_nil TightDescendant.protected_attributes + assert (TightDescendant.protected_attributes - TightDescendant.attributes_protected_by_default).blank? assert_equal Set.new([ 'name', 'address', 'phone_number' ]), TightDescendant.accessible_attributes end diff --git a/activerecord/test/cases/mass_assignment_security/black_list_test.rb b/activerecord/test/cases/mass_assignment_security/black_list_test.rb new file mode 100644 index 0000000000..8b7f48a5f6 --- /dev/null +++ b/activerecord/test/cases/mass_assignment_security/black_list_test.rb @@ -0,0 +1,28 @@ +require "cases/helper" + +class BlackListTest < ActiveRecord::TestCase + + def setup + @black_list = ActiveRecord::MassAssignmentSecurity::BlackList.new + @included_key = 'admin' + @black_list += [ @included_key ] + end + + test "deny? is true for included items" do + assert_equal true, @black_list.deny?(@included_key) + end + + test "deny? is false for non-included items" do + assert_equal false, @black_list.deny?('first_name') + end + + test "sanitize attributes" do + original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied', 'admin(1)' => 'denied' } + attributes = @black_list.sanitize(original_attributes) + + assert attributes.key?('first_name'), "Allowed key shouldn't be rejected" + assert !attributes.key?('admin'), "Denied key should be rejected" + assert !attributes.key?('admin(1)'), "Multi-parameter key should be detected" + end + +end diff --git a/activerecord/test/cases/mass_assignment_security/permission_set_test.rb b/activerecord/test/cases/mass_assignment_security/permission_set_test.rb new file mode 100644 index 0000000000..de7f3982a2 --- /dev/null +++ b/activerecord/test/cases/mass_assignment_security/permission_set_test.rb @@ -0,0 +1,30 @@ +require "cases/helper" + +class PermissionSetTest < ActiveRecord::TestCase + + def setup + @permission_list = ActiveRecord::MassAssignmentSecurity::PermissionSet.new + end + + test "+ stringifies added collection values" do + symbol_collection = [ :admin ] + @permission_list += symbol_collection + + assert @permission_list.include?('admin'), "did not add collection to #{@permission_list.inspect}}" + end + + test "include? normalizes multi-parameter keys" do + multi_param_key = 'admin(1)' + @permission_list += [ 'admin' ] + + assert_equal true, @permission_list.include?(multi_param_key), "#{multi_param_key} not found in #{@permission_list.inspect}" + end + + test "include? normal keys" do + normal_key = 'admin' + @permission_list += [ normal_key ] + + assert_equal true, @permission_list.include?(normal_key), "#{normal_key} not found in #{@permission_list.inspect}" + end + +end diff --git a/activerecord/test/cases/mass_assignment_security/sanitizer_test.rb b/activerecord/test/cases/mass_assignment_security/sanitizer_test.rb new file mode 100644 index 0000000000..122bc7e114 --- /dev/null +++ b/activerecord/test/cases/mass_assignment_security/sanitizer_test.rb @@ -0,0 +1,36 @@ +require "cases/helper" + +class SanitizerTest < ActiveRecord::TestCase + + class SanitizingAuthorizer + include ActiveRecord::MassAssignmentSecurity::Sanitizer + + attr_accessor :logger + + def deny?(key) + [ 'admin' ].include?(key) + end + + end + + def setup + @sanitizer = SanitizingAuthorizer.new + end + + test "sanitize attributes" do + original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied' } + attributes = @sanitizer.sanitize(original_attributes) + + assert attributes.key?('first_name'), "Allowed key shouldn't be rejected" + assert !attributes.key?('admin'), "Denied key should be rejected" + end + + test "debug mass assignment removal" do + original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied' } + log = StringIO.new + @sanitizer.logger = Logger.new(log) + @sanitizer.sanitize(original_attributes) + assert (log.string =~ /admin/), "Should log removed attributes: #{log.string}" + end + +end diff --git a/activerecord/test/cases/mass_assignment_security/white_list_test.rb b/activerecord/test/cases/mass_assignment_security/white_list_test.rb new file mode 100644 index 0000000000..4601263437 --- /dev/null +++ b/activerecord/test/cases/mass_assignment_security/white_list_test.rb @@ -0,0 +1,28 @@ +require "cases/helper" + +class WhiteListTest < ActiveRecord::TestCase + + def setup + @white_list = ActiveRecord::MassAssignmentSecurity::WhiteList.new + @included_key = 'first_name' + @white_list += [ @included_key ] + end + + test "deny? is false for included items" do + assert_equal false, @white_list.deny?(@included_key) + end + + test "deny? is true for non-included items" do + assert_equal true, @white_list.deny?('admin') + end + + test "sanitize attributes" do + original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied', 'admin(1)' => 'denied' } + attributes = @white_list.sanitize(original_attributes) + + assert attributes.key?('first_name'), "Allowed key shouldn't be rejected" + assert !attributes.key?('admin'), "Denied key should be rejected" + assert !attributes.key?('admin(1)'), "Multi-parameter key should be detected" + end + +end -- cgit v1.2.3 From 7c86e8e21ba6a1f88226ddd0cf012a563f234d06 Mon Sep 17 00:00:00 2001 From: Josh Kalderimis Date: Wed, 7 Jul 2010 17:05:42 +0200 Subject: minor changes to mass assignment security patch to bring it in line with rails standards 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 +- .../lib/active_record/mass_assignment_security.rb | 198 ++++++++++----------- .../mass_assignment_security/permission_set.rb | 7 +- .../mass_assignment_security/sanitizer.rb | 8 +- activerecord/test/cases/base_test.rb | 126 +------------ .../permission_set_test.rb | 12 +- .../test/cases/mass_assignment_security_test.rb | 96 ++++++++++ .../test/models/mass_assignment_specific.rb | 32 ++++ 8 files changed, 234 insertions(+), 249 deletions(-) create mode 100644 activerecord/test/cases/mass_assignment_security_test.rb create mode 100644 activerecord/test/models/mass_assignment_specific.rb (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index affb1fee3c..56b4ba8260 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1480,7 +1480,7 @@ MSG attributes = new_attributes.stringify_keys multi_parameter_attributes = [] - attributes = remove_attributes_protected_from_mass_assignment(attributes) if guard_protected_attributes + attributes = sanitize_for_mass_assignment(attributes) if guard_protected_attributes attributes.each do |k, v| if k.include?("(") @@ -1797,7 +1797,7 @@ MSG include AttributeMethods::PrimaryKey include AttributeMethods::TimeZoneConversion include AttributeMethods::Dirty - extend MassAssignmentSecurity + include MassAssignmentSecurity include Callbacks, ActiveModel::Observing, Timestamp include Associations, AssociationPreload, NamedScope diff --git a/activerecord/lib/active_record/mass_assignment_security.rb b/activerecord/lib/active_record/mass_assignment_security.rb index 07beb6405e..8f4d6e1c74 100644 --- a/activerecord/lib/active_record/mass_assignment_security.rb +++ b/activerecord/lib/active_record/mass_assignment_security.rb @@ -1,7 +1,16 @@ require 'active_record/mass_assignment_security/permission_set' module ActiveRecord + # = Active Record Mass-Assignment Security module MassAssignmentSecurity + extend ActiveSupport::Concern + + included do + class_attribute :_accessible_attributes + class_attribute :_protected_attributes + class_attribute :_active_authorizer + end + # Mass assignment security provides an interface for protecting attributes # from end-user assignment. For more complex permissions, mass assignment security # may be handled outside the model by extending a non-ActiveRecord class, @@ -11,7 +20,7 @@ module ActiveRecord # on their role: # # class AccountsController < ApplicationController - # extend ActiveRecord::MassAssignmentSecurity + # include ActiveRecord::MassAssignmentSecurity # # attr_accessible :first_name, :last_name # @@ -28,7 +37,7 @@ module ActiveRecord # protected # # def account_params - # remove_attributes_protected_from_mass_assignment(params[:account]) + # sanitize_for_mass_assignment(params[:account]) # end # # def mass_assignment_authorizer @@ -37,123 +46,96 @@ module ActiveRecord # # end # - def self.extended(base) - base.send(:include, InstanceMethods) - end - - module InstanceMethods - - protected - - def remove_attributes_protected_from_mass_assignment(attributes) - mass_assignment_authorizer.sanitize(attributes) - end - - def mass_assignment_authorizer - self.class.mass_assignment_authorizer - end + module ClassMethods + # Attributes named in this macro are protected from mass-assignment, + # such as new(attributes), + # update_attributes(attributes), or + # attributes=(attributes). + # + # Mass-assignment to these attributes will simply be ignored, to assign + # to them you can use direct writer methods. This is meant to protect + # sensitive attributes from being overwritten by malicious users + # tampering with URLs or forms. + # + # class Customer < ActiveRecord::Base + # attr_protected :credit_rating + # end + # + # customer = Customer.new("name" => David, "credit_rating" => "Excellent") + # customer.credit_rating # => nil + # customer.attributes = { "description" => "Jolly fellow", "credit_rating" => "Superb" } + # customer.credit_rating # => nil + # + # customer.credit_rating = "Average" + # customer.credit_rating # => "Average" + # + # To start from an all-closed default and enable attributes as needed, + # have a look at +attr_accessible+. + # + # Note that using Hash#except or Hash#slice in place of +attr_protected+ + # to sanitize attributes won't provide sufficient protection. + def attr_protected(*names) + self._protected_attributes = self.protected_attributes + names + self._active_authorizer = self._protected_attributes + end - end + # Specifies a white list of model attributes that can be set via + # mass-assignment, such as new(attributes), + # update_attributes(attributes), or + # attributes=(attributes) + # + # This is the opposite of the +attr_protected+ macro: Mass-assignment + # will only set attributes in this list, to assign to the rest of + # attributes you can use direct writer methods. This is meant to protect + # sensitive attributes from being overwritten by malicious users + # tampering with URLs or forms. If you'd rather start from an all-open + # default and restrict attributes as needed, have a look at + # +attr_protected+. + # + # class Customer < ActiveRecord::Base + # attr_accessible :name, :nickname + # end + # + # customer = Customer.new(:name => "David", :nickname => "Dave", :credit_rating => "Excellent") + # customer.credit_rating # => nil + # customer.attributes = { :name => "Jolly fellow", :credit_rating => "Superb" } + # customer.credit_rating # => nil + # + # customer.credit_rating = "Average" + # customer.credit_rating # => "Average" + # + # Note that using Hash#except or Hash#slice in place of +attr_accessible+ + # to sanitize attributes won't provide sufficient protection. + def attr_accessible(*names) + self._accessible_attributes = self.accessible_attributes + names + self._active_authorizer = self._accessible_attributes + end - # Attributes named in this macro are protected from mass-assignment, - # such as new(attributes), - # update_attributes(attributes), or - # attributes=(attributes). - # - # Mass-assignment to these attributes will simply be ignored, to assign - # to them you can use direct writer methods. This is meant to protect - # sensitive attributes from being overwritten by malicious users - # tampering with URLs or forms. - # - # class Customer < ActiveRecord::Base - # attr_protected :credit_rating - # end - # - # customer = Customer.new("name" => David, "credit_rating" => "Excellent") - # customer.credit_rating # => nil - # customer.attributes = { "description" => "Jolly fellow", "credit_rating" => "Superb" } - # customer.credit_rating # => nil - # - # customer.credit_rating = "Average" - # customer.credit_rating # => "Average" - # - # To start from an all-closed default and enable attributes as needed, - # have a look at +attr_accessible+. - # - # Note that using Hash#except or Hash#slice in place of +attr_protected+ - # to sanitize attributes won't provide sufficient protection. - def attr_protected(*keys) - use_authorizer(:protected_attributes) - protected_attributes.merge(keys) - end + def protected_attributes + self._protected_attributes ||= BlackList.new(attributes_protected_by_default).tap { |w| w.logger = logger } + end - # Specifies a white list of model attributes that can be set via - # mass-assignment, such as new(attributes), - # update_attributes(attributes), or - # attributes=(attributes) - # - # This is the opposite of the +attr_protected+ macro: Mass-assignment - # will only set attributes in this list, to assign to the rest of - # attributes you can use direct writer methods. This is meant to protect - # sensitive attributes from being overwritten by malicious users - # tampering with URLs or forms. If you'd rather start from an all-open - # default and restrict attributes as needed, have a look at - # +attr_protected+. - # - # class Customer < ActiveRecord::Base - # attr_accessible :name, :nickname - # end - # - # customer = Customer.new(:name => "David", :nickname => "Dave", :credit_rating => "Excellent") - # customer.credit_rating # => nil - # customer.attributes = { :name => "Jolly fellow", :credit_rating => "Superb" } - # customer.credit_rating # => nil - # - # customer.credit_rating = "Average" - # customer.credit_rating # => "Average" - # - # Note that using Hash#except or Hash#slice in place of +attr_accessible+ - # to sanitize attributes won't provide sufficient protection. - def attr_accessible(*keys) - use_authorizer(:accessible_attributes) - accessible_attributes.merge(keys) - end + def accessible_attributes + self._accessible_attributes ||= WhiteList.new.tap { |w| w.logger = logger } + end - # Returns an array of all the attributes that have been protected from mass-assignment. - def protected_attributes - read_inheritable_attribute(:protected_attributes) || begin - authorizer = BlackList.new - authorizer += attributes_protected_by_default - authorizer.logger = logger - write_inheritable_attribute(:protected_attributes, authorizer) + def active_authorizer + self._active_authorizer ||= protected_attributes end - end - # Returns an array of all the attributes that have been made accessible to mass-assignment. - def accessible_attributes - read_inheritable_attribute(:accessible_attributes) || begin - authorizer = WhiteList.new - authorizer.logger = logger - write_inheritable_attribute(:accessible_attributes, authorizer) + def attributes_protected_by_default + [] end end - def mass_assignment_authorizer - protected_attributes - end + protected - private + def sanitize_for_mass_assignment(attributes) + mass_assignment_authorizer.sanitize(attributes) + end - # Sets the active authorizer, (attr_protected or attr_accessible). Subsequent calls - # will raise an exception when using a different authorizer_id. - def use_authorizer(authorizer_id) # :nodoc: - if active_authorizer_id = read_inheritable_attribute(:active_authorizer_id) - unless authorizer_id == active_authorizer_id - raise("Already using #{active_authorizer_id}, cannot use #{authorizer_id}") - end - else - write_inheritable_attribute(:active_authorizer_id, authorizer_id) - end + def mass_assignment_authorizer + self.class.active_authorizer end end diff --git a/activerecord/lib/active_record/mass_assignment_security/permission_set.rb b/activerecord/lib/active_record/mass_assignment_security/permission_set.rb index 1d34dce02e..8446a4103b 100644 --- a/activerecord/lib/active_record/mass_assignment_security/permission_set.rb +++ b/activerecord/lib/active_record/mass_assignment_security/permission_set.rb @@ -2,11 +2,11 @@ require 'active_record/mass_assignment_security/sanitizer' module ActiveRecord module MassAssignmentSecurity - class PermissionSet < Set + class PermissionSet < Set attr_accessor :logger - def merge(values) + def +(values) super(values.map(&:to_s)) end @@ -19,7 +19,6 @@ module ActiveRecord def remove_multiparameter_id(key) key.gsub(/\(.+/, '') end - end class WhiteList < PermissionSet @@ -28,7 +27,6 @@ module ActiveRecord def deny?(key) !include?(key) end - end class BlackList < PermissionSet @@ -37,7 +35,6 @@ module ActiveRecord def deny?(key) include?(key) end - end end diff --git a/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb b/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb index 4a099a147c..11de35f9d6 100644 --- a/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb +++ b/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb @@ -13,15 +13,17 @@ module ActiveRecord def debug_protected_attribute_removal(attributes, sanitized_attributes) removed_keys = attributes.keys - sanitized_attributes.keys - if removed_keys.any? - logger.debug "WARNING: Can't mass-assign protected attributes: #{removed_keys.join(', ')}" - end + warn!(removed_keys) if removed_keys.any? end def debug? logger.present? end + def warn!(attrs) + logger.debug "WARNING: Can't mass-assign protected attributes: #{attrs.join(', ')}" + end + end end end diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index dff39cf54f..2eecb6e344 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -17,6 +17,7 @@ require 'models/comment' require 'models/minimalistic' require 'models/warehouse_thing' require 'models/parrot' +require 'models/mass_assignment_specific' require 'rexml/document' require 'active_support/core_ext/exception' @@ -37,45 +38,12 @@ class Computer < ActiveRecord::Base; end class NonExistentTable < ActiveRecord::Base; end class TestOracleDefault < ActiveRecord::Base; end -class LoosePerson < ActiveRecord::Base - self.table_name = 'people' - self.abstract_class = true - attr_protected :credit_rating, :administrator -end - -class LooseDescendant < LoosePerson - attr_protected :phone_number -end - -class LooseDescendantSecond< LoosePerson - attr_protected :phone_number - attr_protected :name -end - -class TightPerson < ActiveRecord::Base - self.table_name = 'people' - attr_accessible :name, :address -end - -class TightDescendant < TightPerson - attr_accessible :phone_number -end - class ReadonlyTitlePost < Post attr_readonly :title end class Booleantest < ActiveRecord::Base; end -class Task < ActiveRecord::Base - attr_protected :starting -end - -class TopicWithProtectedContent < ActiveRecord::Base - self.table_name = 'topics' - attr_protected :content -end - class BasicsTest < ActiveRecord::TestCase fixtures :topics, :companies, :developers, :projects, :computers, :accounts, :minimalistics, 'warehouse-things', :authors, :categorizations, :categories, :posts @@ -954,89 +922,6 @@ class BasicsTest < ActiveRecord::TestCase Reply.reset_callbacks(:validate) end - def test_mass_assignment_should_raise_exception_if_accessible_and_protected_attribute_writers_are_both_used - assert_raise(RuntimeError) do - TopicWithProtectedContent.attr_accessible :author_name - end - end - - def test_mass_assignment_protection - firm = Firm.new - firm.attributes = { "name" => "Next Angle", "rating" => 5 } - assert_equal 1, firm.rating - end - - def test_mass_assignment_protection_against_class_attribute_writers - [:logger, :configurations, :primary_key_prefix_type, :table_name_prefix, :table_name_suffix, :pluralize_table_names, - :default_timezone, :schema_format, :lock_optimistically, :record_timestamps].each do |method| - assert_respond_to Task, method - assert_respond_to Task, "#{method}=" - assert_respond_to Task.new, method - assert !Task.new.respond_to?("#{method}=") - end - end - - def test_customized_primary_key_remains_protected - subscriber = Subscriber.new(:nick => 'webster123', :name => 'nice try') - assert_nil subscriber.id - - keyboard = Keyboard.new(:key_number => 9, :name => 'nice try') - assert_nil keyboard.id - end - - def test_customized_primary_key_remains_protected_when_referred_to_as_id - subscriber = Subscriber.new(:id => 'webster123', :name => 'nice try') - assert_nil subscriber.id - - keyboard = Keyboard.new(:id => 9, :name => 'nice try') - assert_nil keyboard.id - end - - def test_mass_assigning_invalid_attribute - firm = Firm.new - - assert_raise(ActiveRecord::UnknownAttributeError) do - firm.attributes = { "id" => 5, "type" => "Client", "i_dont_even_exist" => 20 } - end - end - - def test_mass_assignment_protection_on_defaults - firm = Firm.new - firm.attributes = { "id" => 5, "type" => "Client" } - assert_nil firm.id - assert_equal "Firm", firm[:type] - end - - def test_mass_assignment_accessible - reply = Reply.new("title" => "hello", "content" => "world", "approved" => true) - reply.save - - assert reply.approved? - - reply.approved = false - reply.save - - assert !reply.approved? - end - - def test_mass_assignment_protection_inheritance - assert LoosePerson.accessible_attributes.blank? - assert_equal Set.new([ 'credit_rating', 'administrator', *LoosePerson.attributes_protected_by_default ]), LoosePerson.protected_attributes - - assert LooseDescendant.accessible_attributes.blank? - assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', *LoosePerson.attributes_protected_by_default ]), LooseDescendant.protected_attributes - - assert LooseDescendantSecond.accessible_attributes.blank? - assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', 'name', *LoosePerson.attributes_protected_by_default ]), LooseDescendantSecond.protected_attributes, - 'Running attr_protected twice in one class should merge the protections' - - assert (TightPerson.protected_attributes - TightPerson.attributes_protected_by_default).blank? - assert_equal Set.new([ 'name', 'address' ]), TightPerson.accessible_attributes - - assert (TightDescendant.protected_attributes - TightDescendant.attributes_protected_by_default).blank? - assert_equal Set.new([ 'name', 'address', 'phone_number' ]), TightDescendant.accessible_attributes - end - def test_readonly_attributes assert_equal Set.new([ 'title' , 'comments_count' ]), ReadonlyTitlePost.readonly_attributes @@ -1239,15 +1124,6 @@ class BasicsTest < ActiveRecord::TestCase assert_equal Time.local(2004, 6, 24, 16, 24, 0), topic.written_on end - def test_multiparameter_mass_assignment_protector - task = Task.new - time = Time.mktime(2000, 1, 1, 1) - task.starting = time - attributes = { "starting(1i)" => "2004", "starting(2i)" => "6", "starting(3i)" => "24" } - task.attributes = attributes - assert_equal time, task.starting - end - def test_multiparameter_assignment_of_aggregation customer = Customer.new address = Address.new("The Street", "The City", "The Country") diff --git a/activerecord/test/cases/mass_assignment_security/permission_set_test.rb b/activerecord/test/cases/mass_assignment_security/permission_set_test.rb index de7f3982a2..ca8985042a 100644 --- a/activerecord/test/cases/mass_assignment_security/permission_set_test.rb +++ b/activerecord/test/cases/mass_assignment_security/permission_set_test.rb @@ -8,23 +8,23 @@ class PermissionSetTest < ActiveRecord::TestCase test "+ stringifies added collection values" do symbol_collection = [ :admin ] - @permission_list += symbol_collection + new_list = @permission_list += symbol_collection - assert @permission_list.include?('admin'), "did not add collection to #{@permission_list.inspect}}" + assert new_list.include?('admin'), "did not add collection to #{@permission_list.inspect}}" end test "include? normalizes multi-parameter keys" do multi_param_key = 'admin(1)' - @permission_list += [ 'admin' ] + new_list = @permission_list += [ 'admin' ] - assert_equal true, @permission_list.include?(multi_param_key), "#{multi_param_key} not found in #{@permission_list.inspect}" + assert new_list.include?(multi_param_key), "#{multi_param_key} not found in #{@permission_list.inspect}" end test "include? normal keys" do normal_key = 'admin' - @permission_list += [ normal_key ] + new_list = @permission_list += [ normal_key ] - assert_equal true, @permission_list.include?(normal_key), "#{normal_key} not found in #{@permission_list.inspect}" + assert new_list.include?(normal_key), "#{normal_key} not found in #{@permission_list.inspect}" end end diff --git a/activerecord/test/cases/mass_assignment_security_test.rb b/activerecord/test/cases/mass_assignment_security_test.rb new file mode 100644 index 0000000000..07154da93b --- /dev/null +++ b/activerecord/test/cases/mass_assignment_security_test.rb @@ -0,0 +1,96 @@ +require "cases/helper" +require 'models/reply' +require 'models/company' +require 'models/subscriber' +require 'models/keyboard' +require 'models/mass_assignment_specific' + +class MassAssignmentSecurityTest < ActiveRecord::TestCase + + def test_mass_assignment_protection + firm = Firm.new + firm.attributes = { "name" => "Next Angle", "rating" => 5 } + assert_equal 1, firm.rating + end + + def test_mass_assignment_protection_against_class_attribute_writers + [:logger, :configurations, :primary_key_prefix_type, :table_name_prefix, :table_name_suffix, :pluralize_table_names, + :default_timezone, :schema_format, :lock_optimistically, :record_timestamps].each do |method| + assert_respond_to Task, method + assert_respond_to Task, "#{method}=" + assert_respond_to Task.new, method + assert !Task.new.respond_to?("#{method}=") + end + end + + def test_customized_primary_key_remains_protected + subscriber = Subscriber.new(:nick => 'webster123', :name => 'nice try') + assert_nil subscriber.id + + keyboard = Keyboard.new(:key_number => 9, :name => 'nice try') + assert_nil keyboard.id + end + + def test_customized_primary_key_remains_protected_when_referred_to_as_id + subscriber = Subscriber.new(:id => 'webster123', :name => 'nice try') + assert_nil subscriber.id + + keyboard = Keyboard.new(:id => 9, :name => 'nice try') + assert_nil keyboard.id + end + + def test_mass_assigning_invalid_attribute + firm = Firm.new + + assert_raise(ActiveRecord::UnknownAttributeError) do + firm.attributes = { "id" => 5, "type" => "Client", "i_dont_even_exist" => 20 } + end + end + + def test_mass_assignment_protection_on_defaults + firm = Firm.new + firm.attributes = { "id" => 5, "type" => "Client" } + assert_nil firm.id + assert_equal "Firm", firm[:type] + end + + def test_mass_assignment_accessible + reply = Reply.new("title" => "hello", "content" => "world", "approved" => true) + reply.save + + assert reply.approved? + + reply.approved = false + reply.save + + assert !reply.approved? + end + + def test_mass_assignment_protection_inheritance + assert LoosePerson.accessible_attributes.blank? + assert_equal Set.new([ 'credit_rating', 'administrator', *LoosePerson.attributes_protected_by_default ]), LoosePerson.protected_attributes + + assert LooseDescendant.accessible_attributes.blank? + assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', *LoosePerson.attributes_protected_by_default ]), LooseDescendant.protected_attributes + + assert LooseDescendantSecond.accessible_attributes.blank? + assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', 'name', *LoosePerson.attributes_protected_by_default ]), + LooseDescendantSecond.protected_attributes, 'Running attr_protected twice in one class should merge the protections' + + assert (TightPerson.protected_attributes - TightPerson.attributes_protected_by_default).blank? + assert_equal Set.new([ 'name', 'address' ]), TightPerson.accessible_attributes + + assert (TightDescendant.protected_attributes - TightDescendant.attributes_protected_by_default).blank? + assert_equal Set.new([ 'name', 'address', 'phone_number' ]), TightDescendant.accessible_attributes + end + + def test_mass_assignment_multiparameter_protector + task = Task.new + time = Time.mktime(2000, 1, 1, 1) + task.starting = time + attributes = { "starting(1i)" => "2004", "starting(2i)" => "6", "starting(3i)" => "24" } + task.attributes = attributes + assert_equal time, task.starting + end + +end \ No newline at end of file diff --git a/activerecord/test/models/mass_assignment_specific.rb b/activerecord/test/models/mass_assignment_specific.rb new file mode 100644 index 0000000000..13a80e0197 --- /dev/null +++ b/activerecord/test/models/mass_assignment_specific.rb @@ -0,0 +1,32 @@ +class LoosePerson < ActiveRecord::Base + self.table_name = 'people' + self.abstract_class = true + attr_protected :credit_rating, :administrator +end + +class LooseDescendant < LoosePerson + attr_protected :phone_number +end + +class LooseDescendantSecond< LoosePerson + attr_protected :phone_number + attr_protected :name +end + +class TightPerson < ActiveRecord::Base + self.table_name = 'people' + attr_accessible :name, :address +end + +class TightDescendant < TightPerson + attr_accessible :phone_number +end + +class Task < ActiveRecord::Base + attr_protected :starting +end + +class TopicWithProtectedContent < ActiveRecord::Base + self.table_name = 'topics' + attr_protected :content +end \ No newline at end of file -- cgit v1.2.3 From 4b66aab00fa0ea6bcc6ec81df19e44de34fd7864 Mon Sep 17 00:00:00 2001 From: Josh Kalderimis Date: Thu, 8 Jul 2010 18:16:36 +0200 Subject: mass_assignment_security moved from AR to AMo, and minor test cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- activerecord/lib/active_record.rb | 1 - activerecord/lib/active_record/base.rb | 2 +- .../lib/active_record/mass_assignment_security.rb | 142 --------------------- .../mass_assignment_security/permission_set.rb | 41 ------ .../mass_assignment_security/sanitizer.rb | 29 ----- activerecord/test/cases/base_test.rb | 2 +- .../mass_assignment_security/black_list_test.rb | 28 ---- .../permission_set_test.rb | 30 ----- .../mass_assignment_security/sanitizer_test.rb | 36 ------ .../mass_assignment_security/white_list_test.rb | 28 ---- .../test/cases/mass_assignment_security_test.rb | 71 ++--------- activerecord/test/models/loose_person.rb | 24 ++++ .../test/models/mass_assignment_specific.rb | 32 ----- 13 files changed, 35 insertions(+), 431 deletions(-) delete mode 100644 activerecord/lib/active_record/mass_assignment_security.rb delete mode 100644 activerecord/lib/active_record/mass_assignment_security/permission_set.rb delete mode 100644 activerecord/lib/active_record/mass_assignment_security/sanitizer.rb delete mode 100644 activerecord/test/cases/mass_assignment_security/black_list_test.rb delete mode 100644 activerecord/test/cases/mass_assignment_security/permission_set_test.rb delete mode 100644 activerecord/test/cases/mass_assignment_security/sanitizer_test.rb delete mode 100644 activerecord/test/cases/mass_assignment_security/white_list_test.rb create mode 100644 activerecord/test/models/loose_person.rb delete mode 100644 activerecord/test/models/mass_assignment_specific.rb (limited to 'activerecord') diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb index 9ab05a0548..e2f2508ae8 100644 --- a/activerecord/lib/active_record.rb +++ b/activerecord/lib/active_record.rb @@ -64,7 +64,6 @@ module ActiveRecord autoload :CounterCache autoload :DynamicFinderMatch autoload :DynamicScopeMatch - autoload :MassAssignmentSecurity autoload :Migration autoload :Migrator, 'active_record/migration' autoload :NamedScope diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 56b4ba8260..f22a9de7b1 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1797,7 +1797,7 @@ MSG include AttributeMethods::PrimaryKey include AttributeMethods::TimeZoneConversion include AttributeMethods::Dirty - include MassAssignmentSecurity + include ActiveModel::MassAssignmentSecurity include Callbacks, ActiveModel::Observing, Timestamp include Associations, AssociationPreload, NamedScope diff --git a/activerecord/lib/active_record/mass_assignment_security.rb b/activerecord/lib/active_record/mass_assignment_security.rb deleted file mode 100644 index 8f4d6e1c74..0000000000 --- a/activerecord/lib/active_record/mass_assignment_security.rb +++ /dev/null @@ -1,142 +0,0 @@ -require 'active_record/mass_assignment_security/permission_set' - -module ActiveRecord - # = Active Record Mass-Assignment Security - module MassAssignmentSecurity - extend ActiveSupport::Concern - - included do - class_attribute :_accessible_attributes - class_attribute :_protected_attributes - class_attribute :_active_authorizer - end - - # Mass assignment security provides an interface for protecting attributes - # from end-user assignment. For more complex permissions, mass assignment security - # may be handled outside the model by extending a non-ActiveRecord class, - # such as a controller, with this behavior. - # - # For example, a logged in user may need to assign additional attributes depending - # on their role: - # - # class AccountsController < ApplicationController - # include ActiveRecord::MassAssignmentSecurity - # - # attr_accessible :first_name, :last_name - # - # def self.admin_accessible_attributes - # accessible_attributes + [ :plan_id ] - # end - # - # def update - # ... - # @account.update_attributes(account_params) - # ... - # end - # - # protected - # - # def account_params - # sanitize_for_mass_assignment(params[:account]) - # end - # - # def mass_assignment_authorizer - # admin ? admin_accessible_attributes : super - # end - # - # end - # - module ClassMethods - # Attributes named in this macro are protected from mass-assignment, - # such as new(attributes), - # update_attributes(attributes), or - # attributes=(attributes). - # - # Mass-assignment to these attributes will simply be ignored, to assign - # to them you can use direct writer methods. This is meant to protect - # sensitive attributes from being overwritten by malicious users - # tampering with URLs or forms. - # - # class Customer < ActiveRecord::Base - # attr_protected :credit_rating - # end - # - # customer = Customer.new("name" => David, "credit_rating" => "Excellent") - # customer.credit_rating # => nil - # customer.attributes = { "description" => "Jolly fellow", "credit_rating" => "Superb" } - # customer.credit_rating # => nil - # - # customer.credit_rating = "Average" - # customer.credit_rating # => "Average" - # - # To start from an all-closed default and enable attributes as needed, - # have a look at +attr_accessible+. - # - # Note that using Hash#except or Hash#slice in place of +attr_protected+ - # to sanitize attributes won't provide sufficient protection. - def attr_protected(*names) - self._protected_attributes = self.protected_attributes + names - self._active_authorizer = self._protected_attributes - end - - # Specifies a white list of model attributes that can be set via - # mass-assignment, such as new(attributes), - # update_attributes(attributes), or - # attributes=(attributes) - # - # This is the opposite of the +attr_protected+ macro: Mass-assignment - # will only set attributes in this list, to assign to the rest of - # attributes you can use direct writer methods. This is meant to protect - # sensitive attributes from being overwritten by malicious users - # tampering with URLs or forms. If you'd rather start from an all-open - # default and restrict attributes as needed, have a look at - # +attr_protected+. - # - # class Customer < ActiveRecord::Base - # attr_accessible :name, :nickname - # end - # - # customer = Customer.new(:name => "David", :nickname => "Dave", :credit_rating => "Excellent") - # customer.credit_rating # => nil - # customer.attributes = { :name => "Jolly fellow", :credit_rating => "Superb" } - # customer.credit_rating # => nil - # - # customer.credit_rating = "Average" - # customer.credit_rating # => "Average" - # - # Note that using Hash#except or Hash#slice in place of +attr_accessible+ - # to sanitize attributes won't provide sufficient protection. - def attr_accessible(*names) - self._accessible_attributes = self.accessible_attributes + names - self._active_authorizer = self._accessible_attributes - end - - def protected_attributes - self._protected_attributes ||= BlackList.new(attributes_protected_by_default).tap { |w| w.logger = logger } - end - - def accessible_attributes - self._accessible_attributes ||= WhiteList.new.tap { |w| w.logger = logger } - end - - def active_authorizer - self._active_authorizer ||= protected_attributes - end - - def attributes_protected_by_default - [] - end - end - - protected - - def sanitize_for_mass_assignment(attributes) - mass_assignment_authorizer.sanitize(attributes) - end - - def mass_assignment_authorizer - self.class.active_authorizer - end - - end -end diff --git a/activerecord/lib/active_record/mass_assignment_security/permission_set.rb b/activerecord/lib/active_record/mass_assignment_security/permission_set.rb deleted file mode 100644 index 8446a4103b..0000000000 --- a/activerecord/lib/active_record/mass_assignment_security/permission_set.rb +++ /dev/null @@ -1,41 +0,0 @@ -require 'active_record/mass_assignment_security/sanitizer' - -module ActiveRecord - module MassAssignmentSecurity - - class PermissionSet < Set - attr_accessor :logger - - def +(values) - super(values.map(&:to_s)) - end - - def include?(key) - super(remove_multiparameter_id(key)) - end - - protected - - def remove_multiparameter_id(key) - key.gsub(/\(.+/, '') - end - end - - class WhiteList < PermissionSet - include Sanitizer - - def deny?(key) - !include?(key) - end - end - - class BlackList < PermissionSet - include Sanitizer - - def deny?(key) - include?(key) - end - end - - end -end \ No newline at end of file diff --git a/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb b/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb deleted file mode 100644 index 11de35f9d6..0000000000 --- a/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb +++ /dev/null @@ -1,29 +0,0 @@ -module ActiveRecord - module MassAssignmentSecurity - module Sanitizer - - # Returns all attributes not denied by the authorizer. - def sanitize(attributes) - sanitized_attributes = attributes.reject { |key, value| deny?(key) } - debug_protected_attribute_removal(attributes, sanitized_attributes) if debug? - sanitized_attributes - end - - protected - - def debug_protected_attribute_removal(attributes, sanitized_attributes) - removed_keys = attributes.keys - sanitized_attributes.keys - warn!(removed_keys) if removed_keys.any? - end - - def debug? - logger.present? - end - - def warn!(attrs) - logger.debug "WARNING: Can't mass-assign protected attributes: #{attrs.join(', ')}" - end - - end - end -end diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 2eecb6e344..5a72e9c6e0 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -17,7 +17,7 @@ require 'models/comment' require 'models/minimalistic' require 'models/warehouse_thing' require 'models/parrot' -require 'models/mass_assignment_specific' +require 'models/loose_person' require 'rexml/document' require 'active_support/core_ext/exception' diff --git a/activerecord/test/cases/mass_assignment_security/black_list_test.rb b/activerecord/test/cases/mass_assignment_security/black_list_test.rb deleted file mode 100644 index 8b7f48a5f6..0000000000 --- a/activerecord/test/cases/mass_assignment_security/black_list_test.rb +++ /dev/null @@ -1,28 +0,0 @@ -require "cases/helper" - -class BlackListTest < ActiveRecord::TestCase - - def setup - @black_list = ActiveRecord::MassAssignmentSecurity::BlackList.new - @included_key = 'admin' - @black_list += [ @included_key ] - end - - test "deny? is true for included items" do - assert_equal true, @black_list.deny?(@included_key) - end - - test "deny? is false for non-included items" do - assert_equal false, @black_list.deny?('first_name') - end - - test "sanitize attributes" do - original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied', 'admin(1)' => 'denied' } - attributes = @black_list.sanitize(original_attributes) - - assert attributes.key?('first_name'), "Allowed key shouldn't be rejected" - assert !attributes.key?('admin'), "Denied key should be rejected" - assert !attributes.key?('admin(1)'), "Multi-parameter key should be detected" - end - -end diff --git a/activerecord/test/cases/mass_assignment_security/permission_set_test.rb b/activerecord/test/cases/mass_assignment_security/permission_set_test.rb deleted file mode 100644 index ca8985042a..0000000000 --- a/activerecord/test/cases/mass_assignment_security/permission_set_test.rb +++ /dev/null @@ -1,30 +0,0 @@ -require "cases/helper" - -class PermissionSetTest < ActiveRecord::TestCase - - def setup - @permission_list = ActiveRecord::MassAssignmentSecurity::PermissionSet.new - end - - test "+ stringifies added collection values" do - symbol_collection = [ :admin ] - new_list = @permission_list += symbol_collection - - assert new_list.include?('admin'), "did not add collection to #{@permission_list.inspect}}" - end - - test "include? normalizes multi-parameter keys" do - multi_param_key = 'admin(1)' - new_list = @permission_list += [ 'admin' ] - - assert new_list.include?(multi_param_key), "#{multi_param_key} not found in #{@permission_list.inspect}" - end - - test "include? normal keys" do - normal_key = 'admin' - new_list = @permission_list += [ normal_key ] - - assert new_list.include?(normal_key), "#{normal_key} not found in #{@permission_list.inspect}" - end - -end diff --git a/activerecord/test/cases/mass_assignment_security/sanitizer_test.rb b/activerecord/test/cases/mass_assignment_security/sanitizer_test.rb deleted file mode 100644 index 122bc7e114..0000000000 --- a/activerecord/test/cases/mass_assignment_security/sanitizer_test.rb +++ /dev/null @@ -1,36 +0,0 @@ -require "cases/helper" - -class SanitizerTest < ActiveRecord::TestCase - - class SanitizingAuthorizer - include ActiveRecord::MassAssignmentSecurity::Sanitizer - - attr_accessor :logger - - def deny?(key) - [ 'admin' ].include?(key) - end - - end - - def setup - @sanitizer = SanitizingAuthorizer.new - end - - test "sanitize attributes" do - original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied' } - attributes = @sanitizer.sanitize(original_attributes) - - assert attributes.key?('first_name'), "Allowed key shouldn't be rejected" - assert !attributes.key?('admin'), "Denied key should be rejected" - end - - test "debug mass assignment removal" do - original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied' } - log = StringIO.new - @sanitizer.logger = Logger.new(log) - @sanitizer.sanitize(original_attributes) - assert (log.string =~ /admin/), "Should log removed attributes: #{log.string}" - end - -end diff --git a/activerecord/test/cases/mass_assignment_security/white_list_test.rb b/activerecord/test/cases/mass_assignment_security/white_list_test.rb deleted file mode 100644 index 4601263437..0000000000 --- a/activerecord/test/cases/mass_assignment_security/white_list_test.rb +++ /dev/null @@ -1,28 +0,0 @@ -require "cases/helper" - -class WhiteListTest < ActiveRecord::TestCase - - def setup - @white_list = ActiveRecord::MassAssignmentSecurity::WhiteList.new - @included_key = 'first_name' - @white_list += [ @included_key ] - end - - test "deny? is false for included items" do - assert_equal false, @white_list.deny?(@included_key) - end - - test "deny? is true for non-included items" do - assert_equal true, @white_list.deny?('admin') - end - - test "sanitize attributes" do - original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied', 'admin(1)' => 'denied' } - attributes = @white_list.sanitize(original_attributes) - - assert attributes.key?('first_name'), "Allowed key shouldn't be rejected" - assert !attributes.key?('admin'), "Denied key should be rejected" - assert !attributes.key?('admin(1)'), "Multi-parameter key should be detected" - end - -end diff --git a/activerecord/test/cases/mass_assignment_security_test.rb b/activerecord/test/cases/mass_assignment_security_test.rb index 07154da93b..025ec1d3fa 100644 --- a/activerecord/test/cases/mass_assignment_security_test.rb +++ b/activerecord/test/cases/mass_assignment_security_test.rb @@ -1,28 +1,11 @@ require "cases/helper" -require 'models/reply' require 'models/company' require 'models/subscriber' require 'models/keyboard' -require 'models/mass_assignment_specific' +require 'models/task' class MassAssignmentSecurityTest < ActiveRecord::TestCase - def test_mass_assignment_protection - firm = Firm.new - firm.attributes = { "name" => "Next Angle", "rating" => 5 } - assert_equal 1, firm.rating - end - - def test_mass_assignment_protection_against_class_attribute_writers - [:logger, :configurations, :primary_key_prefix_type, :table_name_prefix, :table_name_suffix, :pluralize_table_names, - :default_timezone, :schema_format, :lock_optimistically, :record_timestamps].each do |method| - assert_respond_to Task, method - assert_respond_to Task, "#{method}=" - assert_respond_to Task.new, method - assert !Task.new.respond_to?("#{method}=") - end - end - def test_customized_primary_key_remains_protected subscriber = Subscriber.new(:nick => 'webster123', :name => 'nice try') assert_nil subscriber.id @@ -47,50 +30,14 @@ class MassAssignmentSecurityTest < ActiveRecord::TestCase end end - def test_mass_assignment_protection_on_defaults - firm = Firm.new - firm.attributes = { "id" => 5, "type" => "Client" } - assert_nil firm.id - assert_equal "Firm", firm[:type] - end - - def test_mass_assignment_accessible - reply = Reply.new("title" => "hello", "content" => "world", "approved" => true) - reply.save - - assert reply.approved? - - reply.approved = false - reply.save - - assert !reply.approved? - end - - def test_mass_assignment_protection_inheritance - assert LoosePerson.accessible_attributes.blank? - assert_equal Set.new([ 'credit_rating', 'administrator', *LoosePerson.attributes_protected_by_default ]), LoosePerson.protected_attributes - - assert LooseDescendant.accessible_attributes.blank? - assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', *LoosePerson.attributes_protected_by_default ]), LooseDescendant.protected_attributes - - assert LooseDescendantSecond.accessible_attributes.blank? - assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', 'name', *LoosePerson.attributes_protected_by_default ]), - LooseDescendantSecond.protected_attributes, 'Running attr_protected twice in one class should merge the protections' - - assert (TightPerson.protected_attributes - TightPerson.attributes_protected_by_default).blank? - assert_equal Set.new([ 'name', 'address' ]), TightPerson.accessible_attributes - - assert (TightDescendant.protected_attributes - TightDescendant.attributes_protected_by_default).blank? - assert_equal Set.new([ 'name', 'address', 'phone_number' ]), TightDescendant.accessible_attributes - end - - def test_mass_assignment_multiparameter_protector - task = Task.new - time = Time.mktime(2000, 1, 1, 1) - task.starting = time - attributes = { "starting(1i)" => "2004", "starting(2i)" => "6", "starting(3i)" => "24" } - task.attributes = attributes - assert_equal time, task.starting + def test_protection_against_class_attribute_writers + [:logger, :configurations, :primary_key_prefix_type, :table_name_prefix, :table_name_suffix, :pluralize_table_names, + :default_timezone, :schema_format, :lock_optimistically, :record_timestamps].each do |method| + assert_respond_to Task, method + assert_respond_to Task, "#{method}=" + assert_respond_to Task.new, method + assert !Task.new.respond_to?("#{method}=") + end end end \ No newline at end of file diff --git a/activerecord/test/models/loose_person.rb b/activerecord/test/models/loose_person.rb new file mode 100644 index 0000000000..256c281d0d --- /dev/null +++ b/activerecord/test/models/loose_person.rb @@ -0,0 +1,24 @@ +class LoosePerson < ActiveRecord::Base + self.table_name = 'people' + self.abstract_class = true + + attr_protected :credit_rating, :administrator +end + +class LooseDescendant < LoosePerson + attr_protected :phone_number +end + +class LooseDescendantSecond< LoosePerson + attr_protected :phone_number + attr_protected :name +end + +class TightPerson < ActiveRecord::Base + self.table_name = 'people' + attr_accessible :name, :address +end + +class TightDescendant < TightPerson + attr_accessible :phone_number +end \ No newline at end of file diff --git a/activerecord/test/models/mass_assignment_specific.rb b/activerecord/test/models/mass_assignment_specific.rb deleted file mode 100644 index 13a80e0197..0000000000 --- a/activerecord/test/models/mass_assignment_specific.rb +++ /dev/null @@ -1,32 +0,0 @@ -class LoosePerson < ActiveRecord::Base - self.table_name = 'people' - self.abstract_class = true - attr_protected :credit_rating, :administrator -end - -class LooseDescendant < LoosePerson - attr_protected :phone_number -end - -class LooseDescendantSecond< LoosePerson - attr_protected :phone_number - attr_protected :name -end - -class TightPerson < ActiveRecord::Base - self.table_name = 'people' - attr_accessible :name, :address -end - -class TightDescendant < TightPerson - attr_accessible :phone_number -end - -class Task < ActiveRecord::Base - attr_protected :starting -end - -class TopicWithProtectedContent < ActiveRecord::Base - self.table_name = 'topics' - attr_protected :content -end \ No newline at end of file -- cgit v1.2.3 From 00f1cd71a97bfa79e6b62a870b09ca914c48e421 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 8 Jul 2010 13:58:23 +0200 Subject: fix ActiveRecord `destroy_all` so it returns destroyed records Signed-off-by: Jeremy Kemper --- .../associations/association_collection.rb | 7 ++++--- activerecord/lib/active_record/relation.rb | 3 +-- .../cases/associations/has_many_associations_test.rb | 7 +++++-- activerecord/test/cases/base_test.rb | 20 ++++++++++++++------ 4 files changed, 24 insertions(+), 13 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 a4e08c7d41..615b7d2719 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -253,9 +253,10 @@ module ActiveRecord # See destroy for more info. def destroy_all load_target - destroy(@target) - reset_target! - reset_named_scopes_cache! + destroy(@target).tap do + reset_target! + reset_named_scopes_cache! + end end def create(attrs = {}) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index bc708b573f..d9fc1b4940 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -213,8 +213,7 @@ module ActiveRecord if conditions where(conditions).destroy_all else - to_a.each {|object| object.destroy} - reset + to_a.each {|object| object.destroy }.tap { reset } end end diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 5e3ba778f3..a52cedd8c2 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -817,8 +817,11 @@ class HasManyAssociationsTest < ActiveRecord::TestCase def test_destroy_all force_signal37_to_load_all_clients_of_firm - assert !companies(:first_firm).clients_of_firm.empty?, "37signals has clients after load" - companies(:first_firm).clients_of_firm.destroy_all + clients = companies(:first_firm).clients_of_firm.to_a + assert !clients.empty?, "37signals has clients after load" + destroyed = companies(:first_firm).clients_of_firm.destroy_all + assert_equal clients.sort_by(&:id), destroyed.sort_by(&:id) + assert destroyed.all? { |client| client.frozen? }, "destroyed clients should be frozen" assert companies(:first_firm).clients_of_firm.empty?, "37signals has no clients after destroy all" assert companies(:first_firm).clients_of_firm(true).empty?, "37signals has no clients after destroy all and refresh" end diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 5a72e9c6e0..329ca38f99 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -651,16 +651,24 @@ class BasicsTest < ActiveRecord::TestCase end def test_destroy_all - original_count = Topic.count - topics_by_mary = Topic.count(:conditions => mary = "author_name = 'Mary'") - - Topic.destroy_all mary - assert_equal original_count - topics_by_mary, Topic.count + conditions = "author_name = 'Mary'" + topics_by_mary = Topic.all(:conditions => conditions, :order => 'id') + assert ! topics_by_mary.empty? + + assert_difference('Topic.count', -topics_by_mary.size) do + destroyed = Topic.destroy_all(conditions).sort_by(&:id) + assert_equal topics_by_mary, destroyed + assert destroyed.all? { |topic| topic.frozen? }, "destroyed topics should be frozen" + end end def test_destroy_many + clients = Client.find([2, 3], :order => 'id') + assert_difference('Client.count', -2) do - Client.destroy([2, 3]) + destroyed = Client.destroy([2, 3]).sort_by(&:id) + assert_equal clients, destroyed + assert destroyed.all? { |client| client.frozen? }, "destroyed clients should be frozen" end end -- cgit v1.2.3 From 9ac9c35117a8f232d6df9009b8c55f4ed2ce077b Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 8 Jul 2010 11:44:19 -0700 Subject: removing useless code. [#5070 state:resolved] Signed-off-by: Jeremy Kemper --- activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 0d9a86a1ea..e4fa412a67 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -16,8 +16,7 @@ module ActiveRecord db = SQLite3::Database.new( config[:database], - :results_as_hash => true, - :type_translation => false + :results_as_hash => true ) db.busy_timeout(config[:timeout]) unless config[:timeout].nil? -- cgit v1.2.3 From 7b0f8534c7a806fd04aac7c817c247433d02f641 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 8 Jul 2010 11:50:07 -0700 Subject: moving parse_sqlite_config to the sqlite3_connection method (where it belongs) [#5071 state:resolved] Signed-off-by: Jeremy Kemper --- .../connection_adapters/sqlite3_adapter.rb | 12 +++++++++++- .../connection_adapters/sqlite_adapter.rb | 19 ------------------- 2 files changed, 11 insertions(+), 20 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index e4fa412a67..e5e92f2b1c 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -4,7 +4,17 @@ module ActiveRecord class Base # sqlite3 adapter reuses sqlite_connection. def self.sqlite3_connection(config) # :nodoc: - parse_sqlite_config!(config) + # Require database. + unless config[:database] + raise ArgumentError, "No database file specified. Missing argument: database" + end + + # Allow database path relative to Rails.root, but only if + # the database path is not the special path that tells + # Sqlite to build a database only in memory. + if defined?(Rails.root) && ':memory:' != config[:database] + config[:database] = File.expand_path(config[:database], Rails.root) + end unless 'sqlite3' == config[:adapter] raise ArgumentError, 'adapter name should be "sqlite3"' diff --git a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb index 1927585c49..117cf447df 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb @@ -2,25 +2,6 @@ require 'active_record/connection_adapters/abstract_adapter' require 'active_support/core_ext/kernel/requires' module ActiveRecord - class Base - class << self - private - def parse_sqlite_config!(config) - # Require database. - unless config[:database] - raise ArgumentError, "No database file specified. Missing argument: database" - end - - # Allow database path relative to Rails.root, but only if - # the database path is not the special path that tells - # Sqlite to build a database only in memory. - if defined?(Rails.root) && ':memory:' != config[:database] - config[:database] = File.expand_path(config[:database], Rails.root) - end - end - end - end - module ConnectionAdapters #:nodoc: class SQLiteColumn < Column #:nodoc: class << self -- cgit v1.2.3 From a4f5f0547f205c6dbf264e8dadeb18492d5271ba Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Thu, 1 Jul 2010 23:16:49 -0400 Subject: removing unused method 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 | 5 ----- 1 file changed, 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index f22a9de7b1..573220f460 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1152,11 +1152,6 @@ MSG end end - # Returns the name of the class descending directly from Active Record in the inheritance hierarchy. - def class_name_of_active_record_descendant(klass) #:nodoc: - klass.base_class.name - end - # Accepts an array, hash, or string of SQL conditions and sanitizes # them into a valid SQL fragment for a WHERE clause. # ["name='%s' and group_id='%s'", "foo'bar", 4] returns "name='foo''bar' and group_id='4'" -- cgit v1.2.3 From b09fd9ccbbe988e6edaf244d304458ad9d476c31 Mon Sep 17 00:00:00 2001 From: Josh Kalderimis Date: Thu, 8 Jul 2010 19:50:41 +0200 Subject: removed an old unused method in AR which removed readonly attributes 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 | 9 --------- 1 file changed, 9 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 573220f460..cb74a01388 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1615,15 +1615,6 @@ MSG end end - # Removes attributes which have been marked as readonly. - def remove_readonly_attributes(attributes) - unless self.class.readonly_attributes.nil? - attributes.delete_if { |key, value| self.class.readonly_attributes.include?(key.gsub(/\(.+/,"")) } - else - attributes - end - end - # The primary key and inheritance column can never be set by mass-assignment for security reasons. def self.attributes_protected_by_default default = [ primary_key, inheritance_column ] -- cgit v1.2.3 From 8b2330ebc3dd2f65a605260366d2445ee7a3009b Mon Sep 17 00:00:00 2001 From: Thiago Pradi Date: Thu, 8 Jul 2010 03:26:37 -0300 Subject: Tests to specify the behaviour of ActiveRecord::Migrator.get_all_versions() [#5066 state:resolved] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- activerecord/test/cases/migration_test.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'activerecord') diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index 4ce9bdb46d..2c3fc46831 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -1358,6 +1358,20 @@ if ActiveRecord::Base.connection.supports_migrations? ActiveRecord::Migrator.forward(MIGRATIONS_ROOT + "/valid") assert_equal(3, ActiveRecord::Migrator.current_version) end + + def test_get_all_versions + ActiveRecord::Migrator.migrate(MIGRATIONS_ROOT + "/valid") + assert_equal([1,2,3], ActiveRecord::Migrator.get_all_versions) + + ActiveRecord::Migrator.rollback(MIGRATIONS_ROOT + "/valid") + assert_equal([1,2], ActiveRecord::Migrator.get_all_versions) + + ActiveRecord::Migrator.rollback(MIGRATIONS_ROOT + "/valid") + assert_equal([1], ActiveRecord::Migrator.get_all_versions) + + ActiveRecord::Migrator.rollback(MIGRATIONS_ROOT + "/valid") + assert_equal([], ActiveRecord::Migrator.get_all_versions) + end def test_schema_migrations_table_name ActiveRecord::Base.table_name_prefix = "prefix_" -- cgit v1.2.3 From 0e9bc23c0e5dba228626ffbc2bef069331b2e471 Mon Sep 17 00:00:00 2001 From: Ken Collins Date: Wed, 7 Jul 2010 15:47:57 -0400 Subject: Fix the #using_limitable_reflections? helper to work correctly by not examining the length of an array which contains false/true, hence always passing. [#4869 state:resolved] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- activerecord/lib/active_record/relation/finder_methods.rb | 2 +- activerecord/test/cases/associations_test.rb | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index f39951e16c..3bf4c5bdd1 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -339,7 +339,7 @@ module ActiveRecord end def using_limitable_reflections?(reflections) - reflections.collect(&:collection?).length.zero? + reflections.none?(&:collection?) end end diff --git a/activerecord/test/cases/associations_test.rb b/activerecord/test/cases/associations_test.rb index d99fb44f01..4ae776c35a 100644 --- a/activerecord/test/cases/associations_test.rb +++ b/activerecord/test/cases/associations_test.rb @@ -65,6 +65,16 @@ class AssociationsTest < ActiveRecord::TestCase assert_equal 1, firm.clients(true).size, "New firm should have reloaded clients count" end + def test_using_limitable_reflections_helper + using_limitable_reflections = lambda { |reflections| Tagging.scoped.send :using_limitable_reflections?, reflections } + belongs_to_reflections = [Tagging.reflect_on_association(:tag), Tagging.reflect_on_association(:super_tag)] + has_many_reflections = [Tag.reflect_on_association(:taggings), Developer.reflect_on_association(:projects)] + mixed_reflections = (belongs_to_reflections + has_many_reflections).uniq + assert using_limitable_reflections.call(belongs_to_reflections), "Belong to associations are limitable" + assert !using_limitable_reflections.call(has_many_reflections), "All has many style associations are not limitable" + assert !using_limitable_reflections.call(mixed_reflections), "No collection associations (has many style) should pass" + end + def test_force_reload_is_uncached firm = Firm.create!("name" => "A New Firm, Inc") client = Client.create!("name" => "TheClient.com", :firm => firm) -- cgit v1.2.3 From 17650e394fae26984d506fd0f705bc32e5a5de27 Mon Sep 17 00:00:00 2001 From: Grant Ammons Date: Mon, 28 Jun 2010 16:37:38 -0400 Subject: Eager loading :through associations will join the :source model if there are :conditions. [#2362 state:resolved] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- activerecord/lib/active_record/association_preload.rb | 7 ++++++- activerecord/test/cases/associations/eager_test.rb | 6 ++++++ activerecord/test/models/post.rb | 1 + 3 files changed, 13 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index f13c250ca4..cbec5789fd 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -282,7 +282,12 @@ module ActiveRecord end end else - records.first.class.preload_associations(records, through_association) + options = {} + options[:include] = reflection.options[:include] || reflection.options[:source] if reflection.options[:conditions] + options[:order] = reflection.options[:order] + options[:conditions] = reflection.options[:conditions] + records.first.class.preload_associations(records, through_association, options) + records.each do |record| through_records.concat Array.wrap(record.send(through_association)) end diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index 445e6889c0..40859d425f 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -357,6 +357,12 @@ class EagerAssociationTest < ActiveRecord::TestCase end end + def test_eager_with_has_many_through_with_conditions_join_model_with_include + post_tags = Post.find(posts(:welcome).id).misc_tags + eager_post_tags = Post.find(1, :include => :misc_tags).misc_tags + assert_equal post_tags, eager_post_tags + end + def test_eager_with_has_many_and_limit posts = Post.find(:all, :order => 'posts.id asc', :include => [ :author, :comments ], :limit => 2) assert_equal 2, posts.size diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index dd06822cfd..6c7b93be87 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -58,6 +58,7 @@ class Post < ActiveRecord::Base end end + has_many :misc_tags, :through => :taggings, :source => :tag, :conditions => "tags.name = 'Misc'" has_many :funky_tags, :through => :taggings, :source => :tag has_many :super_tags, :through => :taggings has_one :tagging, :as => :taggable -- cgit v1.2.3 From 690352dce492938ab54a4b7e2befbd0a6931de3c Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Fri, 2 Jul 2010 14:36:13 -0400 Subject: consolidating updated_at and updated_on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- activerecord/lib/active_record/timestamp.rb | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/timestamp.rb b/activerecord/lib/active_record/timestamp.rb index ffd12d2082..da8324ddcc 100644 --- a/activerecord/lib/active_record/timestamp.rb +++ b/activerecord/lib/active_record/timestamp.rb @@ -35,8 +35,7 @@ module ActiveRecord if attribute write_attribute(attribute, current_time) else - write_attribute('updated_at', current_time) if respond_to?(:updated_at) - write_attribute('updated_on', current_time) if respond_to?(:updated_on) + timestamp_attributes_for_update_in_model.each { |column| write_attribute(column.to_s, current_time) } end save! @@ -50,8 +49,9 @@ module ActiveRecord write_attribute('created_at', current_time) if respond_to?(:created_at) && created_at.nil? write_attribute('created_on', current_time) if respond_to?(:created_on) && created_on.nil? - write_attribute('updated_at', current_time) if respond_to?(:updated_at) && updated_at.nil? - write_attribute('updated_on', current_time) if respond_to?(:updated_on) && updated_on.nil? + timestamp_attributes_for_update.each do |column| + write_attribute(column.to_s, current_time) if respond_to?(column) && self.send(column).nil? + end end super @@ -60,16 +60,23 @@ module ActiveRecord def update(*args) #:nodoc: if record_timestamps && (!partial_updates? || changed?) current_time = current_time_from_proper_timezone - - write_attribute('updated_at', current_time) if respond_to?(:updated_at) - write_attribute('updated_on', current_time) if respond_to?(:updated_on) + timestamp_attributes_for_update_in_model.each { |column| write_attribute(column.to_s, current_time) } end super end + + def timestamp_attributes_for_update #:nodoc: + [:updated_at, :updated_on] + end + + def timestamp_attributes_for_update_in_model #:nodoc: + ([:updated_at, :updated_on].inject([]) { |sum, elem| respond_to?(elem) ? sum << elem : sum }) + end - def current_time_from_proper_timezone + def current_time_from_proper_timezone #:nodoc: self.class.default_timezone == :utc ? Time.now.utc : Time.now end end -end \ No newline at end of file +end + -- cgit v1.2.3 From 01629d180468049d17a8be6900e27a4f0d2b18c4 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Wed, 30 Jun 2010 07:14:09 -0400 Subject: This patch changes update_attribute implementatino so: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - it will only save the attribute it has been asked to save and not all dirty attributes - it does not invoke callbacks - it does change updated_at/on Signed-off-by: José Valim --- activerecord/lib/active_record/persistence.rb | 17 +++++++--- activerecord/lib/active_record/timestamp.rb | 12 +++++-- activerecord/test/cases/base_test.rb | 40 +++++++++++++++++++++++ activerecord/test/cases/dirty_test.rb | 7 ++-- activerecord/test/cases/nested_attributes_test.rb | 18 +++++----- 5 files changed, 74 insertions(+), 20 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index 50166c4385..8d093c81de 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -102,12 +102,19 @@ module ActiveRecord became end - # Updates a single attribute and saves the record without going through the normal validation procedure. - # This is especially useful for boolean flags on existing records. The regular +update_attribute+ method - # in Base is replaced with this when the validations module is mixed in, which it is by default. + # Updates a single attribute and saves the record without going through the normal validation procedure + # or callbacks. This is especially useful for boolean flags on existing records. def update_attribute(name, value) send("#{name}=", value) - save(:validate => false) + primary_key = self.class.primary_key + h = {name => value} + if should_record_update_timestamps + self.send(:record_update_timestamps) + current_time = current_time_from_proper_timezone + timestamp_attributes_for_update_in_model.each { |column| h.merge!(column => current_time) } + end + self.class.update_all(h, {primary_key => self[primary_key]}) == 1 + @changed_attributes.delete(name.to_s) end # Updates all the attributes from the passed-in Hash and saves the record. @@ -234,4 +241,4 @@ module ActiveRecord end end end -end \ No newline at end of file +end diff --git a/activerecord/lib/active_record/timestamp.rb b/activerecord/lib/active_record/timestamp.rb index da8324ddcc..e6d52744df 100644 --- a/activerecord/lib/active_record/timestamp.rb +++ b/activerecord/lib/active_record/timestamp.rb @@ -58,14 +58,22 @@ module ActiveRecord end def update(*args) #:nodoc: - if record_timestamps && (!partial_updates? || changed?) + record_update_timestamps + super + end + + def record_update_timestamps + if should_record_update_timestamps current_time = current_time_from_proper_timezone timestamp_attributes_for_update_in_model.each { |column| write_attribute(column.to_s, current_time) } end + end - super + def should_record_update_timestamps + record_timestamps && (!partial_updates? || changed?) end + def timestamp_attributes_for_update #:nodoc: [:updated_at, :updated_on] end diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 329ca38f99..09bdd13cd4 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -893,6 +893,46 @@ class BasicsTest < ActiveRecord::TestCase assert !Topic.find(1).approved? end + def test_update_attribute_with_one_changed_and_one_updated + t = Topic.order('id').limit(1).first + title, author_name = t.title, t.author_name + t.author_name = 'John' + t.update_attribute(:title, 'super_title') + assert_equal 'John', t.author_name + assert_equal 'super_title', t.title + assert t.changed?, "topic should have changed" + assert t.author_name_changed?, "author_name should have changed" + assert !t.title_changed?, "title should not have changed" + assert_nil t.title_change, 'title change should be nil' + assert_equal ['author_name'], t.changed + + t.reload + assert_equal 'David', t.author_name + assert_equal 'super_title', t.title + end + + def test_update_attribute_with_one_updated + t = Topic.first + title = t.title + t.update_attribute(:title, 'super_title') + assert_equal 'super_title', t.title + assert !t.changed?, "topic should not have changed" + assert !t.title_changed?, "title should not have changed" + assert_nil t.title_change, 'title change should be nil' + + t.reload + assert_equal 'super_title', t.title + end + + def test_update_attribute_for_udpated_at_on + developer = Developer.find(1) + updated_at = developer.updated_at + developer.update_attribute(:salary, 80001) + assert_not_equal updated_at, developer.updated_at + developer.reload + assert_not_equal updated_at, developer.updated_at + end + def test_update_attributes topic = Topic.find(1) assert !topic.approved? diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb index 75f7453aa9..837386ed24 100644 --- a/activerecord/test/cases/dirty_test.rb +++ b/activerecord/test/cases/dirty_test.rb @@ -475,10 +475,9 @@ class DirtyTest < ActiveRecord::TestCase pirate = Pirate.find_by_catchphrase("Ahoy!") pirate.update_attribute(:catchphrase, "Ninjas suck!") - assert_equal 2, pirate.previous_changes.size - assert_equal ["Ahoy!", "Ninjas suck!"], pirate.previous_changes['catchphrase'] - assert_not_nil pirate.previous_changes['updated_on'][0] - assert_not_nil pirate.previous_changes['updated_on'][1] + assert_equal 0, pirate.previous_changes.size + assert_nil pirate.previous_changes['catchphrase'] + assert_nil pirate.previous_changes['updated_on'] assert !pirate.previous_changes.key?('parrot_id') assert !pirate.previous_changes.key?('created_on') end diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index 62237f955b..c9ea0d8c40 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -195,7 +195,7 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase [1, '1', true, 'true'].each do |truth| @pirate.reload.create_ship(:name => 'Mister Pablo') assert_difference('Ship.count', -1) do - @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_destroy => truth }) + @pirate.update_attributes(:ship_attributes => { :id => @pirate.ship.id, :_destroy => truth }) end end end @@ -203,7 +203,7 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase def test_should_not_destroy_an_existing_record_if_destroy_is_not_truthy [nil, '0', 0, 'false', false].each do |not_truth| assert_no_difference('Ship.count') do - @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_destroy => not_truth }) + @pirate.update_attributes(:ship_attributes => { :id => @pirate.ship.id, :_destroy => not_truth }) end end end @@ -212,7 +212,7 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase Pirate.accepts_nested_attributes_for :ship, :allow_destroy => false, :reject_if => proc { |attributes| attributes.empty? } assert_no_difference('Ship.count') do - @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_destroy => '1' }) + @pirate.update_attributes(:ship_attributes => { :id => @pirate.ship.id, :_destroy => '1' }) end Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } @@ -247,13 +247,13 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase end def test_should_accept_update_only_option - @pirate.update_attribute(:update_only_ship_attributes, { :id => @pirate.ship.id, :name => 'Mayflower' }) + @pirate.update_attributes(:update_only_ship_attributes => { :id => @pirate.ship.id, :name => 'Mayflower' }) end def test_should_create_new_model_when_nothing_is_there_and_update_only_is_true @ship.delete assert_difference('Ship.count', 1) do - @pirate.reload.update_attribute(:update_only_ship_attributes, { :name => 'Mayflower' }) + @pirate.reload.update_attributes(:update_only_ship_attributes => { :name => 'Mayflower' }) end end @@ -353,7 +353,7 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase [1, '1', true, 'true'].each do |truth| @ship.reload.create_pirate(:catchphrase => 'Arr') assert_difference('Pirate.count', -1) do - @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_destroy => truth }) + @ship.update_attributes(:pirate_attributes => { :id => @ship.pirate.id, :_destroy => truth }) end end end @@ -361,7 +361,7 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase def test_should_not_destroy_an_existing_record_if_destroy_is_not_truthy [nil, '0', 0, 'false', false].each do |not_truth| assert_no_difference('Pirate.count') do - @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_destroy => not_truth }) + @ship.update_attributes(:pirate_attributes => { :id => @ship.pirate.id, :_destroy => not_truth }) end end end @@ -370,7 +370,7 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase Ship.accepts_nested_attributes_for :pirate, :allow_destroy => false, :reject_if => proc { |attributes| attributes.empty? } assert_no_difference('Pirate.count') do - @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_destroy => '1' }) + @ship.update_attributes(:pirate_attributes => { :id => @ship.pirate.id, :_destroy => '1' }) end Ship.accepts_nested_attributes_for :pirate, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } @@ -398,7 +398,7 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase def test_should_create_new_model_when_nothing_is_there_and_update_only_is_true @pirate.delete assert_difference('Pirate.count', 1) do - @ship.reload.update_attribute(:update_only_pirate_attributes, { :catchphrase => 'Arr' }) + @ship.reload.update_attributes(:update_only_pirate_attributes => { :catchphrase => 'Arr' }) end end -- cgit v1.2.3 From 87f64ef05e48ad8b374022ca944e34a7ad68551d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 8 Jul 2010 22:59:41 +0200 Subject: Improve a bit the code in latest commits. --- activerecord/lib/active_record/persistence.rb | 16 +++++++++------- activerecord/lib/active_record/timestamp.rb | 20 +++++++------------- 2 files changed, 16 insertions(+), 20 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index 8d093c81de..828a8b41b6 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -106,15 +106,17 @@ module ActiveRecord # or callbacks. This is especially useful for boolean flags on existing records. def update_attribute(name, value) send("#{name}=", value) - primary_key = self.class.primary_key - h = {name => value} - if should_record_update_timestamps - self.send(:record_update_timestamps) - current_time = current_time_from_proper_timezone - timestamp_attributes_for_update_in_model.each { |column| h.merge!(column => current_time) } + hash = { name => read_attribute(name) } + + if record_update_timestamps + timestamp_attributes_for_update_in_model.each do |column| + hash[column] = read_attribute(column) + end end - self.class.update_all(h, {primary_key => self[primary_key]}) == 1 + @changed_attributes.delete(name.to_s) + primary_key = self.class.primary_key + self.class.update_all(hash, { primary_key => self[primary_key] }) == 1 end # Updates all the attributes from the passed-in Hash and saves the record. diff --git a/activerecord/lib/active_record/timestamp.rb b/activerecord/lib/active_record/timestamp.rb index e6d52744df..1075a60f07 100644 --- a/activerecord/lib/active_record/timestamp.rb +++ b/activerecord/lib/active_record/timestamp.rb @@ -49,8 +49,8 @@ module ActiveRecord write_attribute('created_at', current_time) if respond_to?(:created_at) && created_at.nil? write_attribute('created_on', current_time) if respond_to?(:created_on) && created_on.nil? - timestamp_attributes_for_update.each do |column| - write_attribute(column.to_s, current_time) if respond_to?(column) && self.send(column).nil? + timestamp_attributes_for_update_in_model.each do |column| + write_attribute(column.to_s, current_time) if self.send(column).nil? end end @@ -63,23 +63,17 @@ module ActiveRecord end def record_update_timestamps - if should_record_update_timestamps + if record_timestamps && (!partial_updates? || changed?) current_time = current_time_from_proper_timezone timestamp_attributes_for_update_in_model.each { |column| write_attribute(column.to_s, current_time) } + true + else + false end end - def should_record_update_timestamps - record_timestamps && (!partial_updates? || changed?) - end - - - def timestamp_attributes_for_update #:nodoc: - [:updated_at, :updated_on] - end - def timestamp_attributes_for_update_in_model #:nodoc: - ([:updated_at, :updated_on].inject([]) { |sum, elem| respond_to?(elem) ? sum << elem : sum }) + [:updated_at, :updated_on].select { |elem| respond_to?(elem) } end def current_time_from_proper_timezone #:nodoc: -- cgit v1.2.3 From 1e53404fe9c39ad0849894d73e431315be8c0bf0 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Sun, 27 Jun 2010 11:17:44 -0400 Subject: reset_counter should work with non-traditional belongs_to and polymorphic belongs_to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [#4984 state:resolved] Signed-off-by: José Valim --- activerecord/lib/active_record/counter_cache.rb | 12 +++++++++--- activerecord/test/cases/counter_cache_test.rb | 19 ++++++++++++++++--- activerecord/test/fixtures/cars.yml | 4 ++++ activerecord/test/models/car.rb | 4 ++++ activerecord/test/models/engine.rb | 3 +++ activerecord/test/models/wheel.rb | 3 +++ activerecord/test/schema/schema.rb | 13 +++++++++++++ 7 files changed, 52 insertions(+), 6 deletions(-) create mode 100644 activerecord/test/fixtures/cars.yml create mode 100644 activerecord/test/models/car.rb create mode 100644 activerecord/test/models/engine.rb create mode 100644 activerecord/test/models/wheel.rb (limited to 'activerecord') diff --git a/activerecord/lib/active_record/counter_cache.rb b/activerecord/lib/active_record/counter_cache.rb index 999322129a..808e70d7de 100644 --- a/activerecord/lib/active_record/counter_cache.rb +++ b/activerecord/lib/active_record/counter_cache.rb @@ -17,8 +17,14 @@ module ActiveRecord def reset_counters(id, *counters) object = find(id) counters.each do |association| - child_class = reflect_on_association(association.to_sym).klass - belongs_name = self.name.demodulize.underscore.to_sym + has_many_association = reflect_on_association(association.to_sym) + polymorphic_class = has_many_association.options[:as] + child_class = has_many_association.klass + belongs_to = child_class.reflect_on_all_associations(:belongs_to) + belongs_to_association = belongs_to.detect do |e| + polymorphic_class.nil? ? (e.class_name == self.name) : (e.class_name.to_s.downcase == polymorphic_class.to_s.downcase) + end + belongs_name = belongs_to_association.name counter_name = child_class.reflect_on_association(belongs_name).counter_cache_column self.unscoped.where(arel_table[self.primary_key].eq(object.id)).arel.update({ @@ -103,4 +109,4 @@ module ActiveRecord update_counters(id, counter_name => -1) end end -end \ No newline at end of file +end diff --git a/activerecord/test/cases/counter_cache_test.rb b/activerecord/test/cases/counter_cache_test.rb index 377de168b9..137236255d 100644 --- a/activerecord/test/cases/counter_cache_test.rb +++ b/activerecord/test/cases/counter_cache_test.rb @@ -1,17 +1,20 @@ require 'cases/helper' require 'models/topic' +require 'models/car' +require 'models/wheel' +require 'models/engine' require 'models/reply' require 'models/category' require 'models/categorization' class CounterCacheTest < ActiveRecord::TestCase - fixtures :topics, :categories, :categorizations + fixtures :topics, :categories, :categorizations, :cars - class SpecialTopic < ::Topic + class ::SpecialTopic < ::Topic has_many :special_replies, :foreign_key => 'parent_id' end - class SpecialReply < ::Reply + class ::SpecialReply < ::Reply belongs_to :special_topic, :foreign_key => 'parent_id', :counter_cache => 'replies_count' end @@ -58,6 +61,16 @@ class CounterCacheTest < ActiveRecord::TestCase end end + test "reset counter should with belongs_to which has class_name" do + car = cars(:honda) + assert_nothing_raised do + Car.reset_counters(car.id, :engines) + end + assert_nothing_raised do + Car.reset_counters(car.id, :wheels) + end + end + test "update counter with initial null value" do category = categories(:general) assert_equal 2, category.categorizations.count diff --git a/activerecord/test/fixtures/cars.yml b/activerecord/test/fixtures/cars.yml new file mode 100644 index 0000000000..23c98e8144 --- /dev/null +++ b/activerecord/test/fixtures/cars.yml @@ -0,0 +1,4 @@ +honda: + id: 1 + name: honda + engines_count: 0 diff --git a/activerecord/test/models/car.rb b/activerecord/test/models/car.rb new file mode 100644 index 0000000000..1101180a67 --- /dev/null +++ b/activerecord/test/models/car.rb @@ -0,0 +1,4 @@ +class Car < ActiveRecord::Base + has_many :engines + has_many :wheels, :as => :wheelable +end diff --git a/activerecord/test/models/engine.rb b/activerecord/test/models/engine.rb new file mode 100644 index 0000000000..751c3f02d1 --- /dev/null +++ b/activerecord/test/models/engine.rb @@ -0,0 +1,3 @@ +class Engine < ActiveRecord::Base + belongs_to :my_car, :class_name => 'Car', :foreign_key => 'car_id', :counter_cache => :engines_count +end diff --git a/activerecord/test/models/wheel.rb b/activerecord/test/models/wheel.rb new file mode 100644 index 0000000000..26868bce5e --- /dev/null +++ b/activerecord/test/models/wheel.rb @@ -0,0 +1,3 @@ +class Wheel < ActiveRecord::Base + belongs_to :wheelable, :polymorphic => true, :counter_cache => true +end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index b212e7cff2..bea351b95a 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -82,6 +82,12 @@ ActiveRecord::Schema.define do t.string :name end + create_table :cars, :force => true do |t| + t.string :name + t.integer :engines_count + t.integer :wheels_count + end + create_table :categories, :force => true do |t| t.string :name, :null => false t.string :type @@ -179,6 +185,9 @@ ActiveRecord::Schema.define do end add_index :edges, [:source_id, :sink_id], :unique => true, :name => 'unique_edge_index' + create_table :engines, :force => true do |t| + t.integer :car_id + end create_table :entrants, :force => true do |t| t.string :name, :null => false @@ -566,6 +575,10 @@ ActiveRecord::Schema.define do t.integer :zine_id end + create_table :wheels, :force => true do |t| + t.references :wheelable, :polymorphic => true + end + create_table :zines, :force => true do |t| t.string :title end -- cgit v1.2.3 From 786342e17f42799ef889cf6127fe97e9598272e0 Mon Sep 17 00:00:00 2001 From: David Trasbo Date: Wed, 30 Jun 2010 21:33:49 +0200 Subject: Return from ActiveRecord::Base#attributes= unless value is a hash [#4070 state:committed] 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 | 2 +- activerecord/test/cases/base_test.rb | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index cb74a01388..c78060c956 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1471,7 +1471,7 @@ MSG # user.send(:attributes=, { :username => 'Phusion', :is_admin => true }, false) # user.is_admin? # => true def attributes=(new_attributes, guard_protected_attributes = true) - return if new_attributes.nil? + return unless new_attributes.is_a? Hash attributes = new_attributes.stringify_keys multi_parameter_attributes = [] diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 09bdd13cd4..a4cf5120e1 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -61,6 +61,13 @@ class BasicsTest < ActiveRecord::TestCase assert_equal(topics(:first).author_email_address, Topic.find(1).author_email_address) end + def test_set_attributes_without_hash + topic = Topic.new + assert_nothing_raised do + topic.attributes = '' + end + end + def test_integers_as_nil test = AutoId.create('value' => '') assert_nil AutoId.find(test.id).value -- cgit v1.2.3 From d9ebc76d70a122544452189f0cb93d3854f2043f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 8 Jul 2010 23:45:10 +0200 Subject: Refactor previous commits a bit. --- activerecord/lib/active_record/counter_cache.rb | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/counter_cache.rb b/activerecord/lib/active_record/counter_cache.rb index 808e70d7de..237cd56683 100644 --- a/activerecord/lib/active_record/counter_cache.rb +++ b/activerecord/lib/active_record/counter_cache.rb @@ -18,14 +18,17 @@ module ActiveRecord object = find(id) counters.each do |association| has_many_association = reflect_on_association(association.to_sym) - polymorphic_class = has_many_association.options[:as] - child_class = has_many_association.klass - belongs_to = child_class.reflect_on_all_associations(:belongs_to) - belongs_to_association = belongs_to.detect do |e| - polymorphic_class.nil? ? (e.class_name == self.name) : (e.class_name.to_s.downcase == polymorphic_class.to_s.downcase) + + expected_name = if has_many_association.options[:as] + has_many_association.options[:as].to_s.classify + else + self.name end - belongs_name = belongs_to_association.name - counter_name = child_class.reflect_on_association(belongs_name).counter_cache_column + + child_class = has_many_association.klass + belongs_to = child_class.reflect_on_all_associations(:belongs_to) + reflection = belongs_to.find { |e| e.class_name == expected_name } + counter_name = reflection.counter_cache_column self.unscoped.where(arel_table[self.primary_key].eq(object.id)).arel.update({ arel_table[counter_name] => object.send(association).count -- cgit v1.2.3 From 1e1af8f612ce5649f529fd2dcb573c9b42b455ad Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 9 Jul 2010 17:12:29 -0700 Subject: adding more behavioral tests for the sqlite adapter --- .../cases/adapters/sqlite/sqlite_adapter_test.rb | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) (limited to 'activerecord') diff --git a/activerecord/test/cases/adapters/sqlite/sqlite_adapter_test.rb b/activerecord/test/cases/adapters/sqlite/sqlite_adapter_test.rb index 69cfb00faf..2505372b7e 100644 --- a/activerecord/test/cases/adapters/sqlite/sqlite_adapter_test.rb +++ b/activerecord/test/cases/adapters/sqlite/sqlite_adapter_test.rb @@ -83,6 +83,26 @@ module ActiveRecord assert_equal 0, @ctx.select_rows(count_sql).first.first end + def test_tables + assert_equal %w{ items }, @ctx.tables + + @ctx.execute <<-eosql + CREATE TABLE people ( + id integer PRIMARY KEY AUTOINCREMENT, + number integer + ) + eosql + assert_equal %w{ items people }.sort, @ctx.tables.sort + end + + def test_tables_logs_name + name = "hello" + assert_logged [[name]] do + @ctx.tables(name) + assert_not_nil @ctx.logged.first.shift + end + end + def assert_logged logs @ctx.extend(Module.new { attr_reader :logged -- cgit v1.2.3