diff options
Diffstat (limited to 'activerecord')
-rw-r--r-- | activerecord/CHANGELOG.md | 40 | ||||
-rw-r--r-- | activerecord/lib/active_record/attribute_methods.rb | 26 | ||||
-rw-r--r-- | activerecord/lib/active_record/dynamic_matchers.rb | 8 | ||||
-rw-r--r-- | activerecord/lib/active_record/enum.rb | 46 | ||||
-rw-r--r-- | activerecord/lib/active_record/persistence.rb | 2 | ||||
-rw-r--r-- | activerecord/lib/active_record/relation/batches.rb | 11 | ||||
-rw-r--r-- | activerecord/lib/active_record/relation/query_methods.rb | 6 | ||||
-rw-r--r-- | activerecord/lib/active_record/scoping/named.rb | 6 | ||||
-rw-r--r-- | activerecord/test/cases/batches_test.rb | 13 | ||||
-rw-r--r-- | activerecord/test/cases/enum_test.rb | 59 | ||||
-rw-r--r-- | activerecord/test/cases/finder_respond_to_test.rb | 5 | ||||
-rw-r--r-- | activerecord/test/cases/locking_test.rb | 11 | ||||
-rw-r--r-- | activerecord/test/cases/relation/mutation_test.rb | 4 | ||||
-rw-r--r-- | activerecord/test/cases/relations_test.rb | 31 | ||||
-rw-r--r-- | activerecord/test/cases/scoping/named_scoping_test.rb | 57 |
15 files changed, 314 insertions, 11 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 7db99d4aeb..fe0d7b2b35 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,43 @@ +* `find_in_batches` now returns an `Enumerator` when called without a block, so that it + can be chained with other `Enumerable` methods. + + *Marc-AndrĂ© Lafortune* + +* `enum` now raises on "dangerous" name conflicts + + Dangerous name conflicts includes instance or class method conflicts + with methods defined within `ActiveRecord::Base` but not its ancestors, + as well as conflicts with methods generated by other enums on the same + class. + + Fixes #13389. + + *Godfrey Chan* + +* `scope` now raises on "dangerous" name conflicts + + Similar to dangerous attribute methods, a scope name conflict is + dangerous if it conflicts with an existing class method defined within + `ActiveRecord::Base` but not its ancestors. + + See also #13389. + + *Godfrey Chan*, *Philippe Creux* + +* Correctly send an user provided statement to a `lock!()` call. + + person.lock! 'FOR SHARE NOWAIT' + # Before: SELECT * ... LIMIT 1 FOR UPDATE + # After: SELECT * ... LIMIT 1 FOR SHARE NOWAIT + + Fixes #13788. + + *MaurĂcio Linhares* + +* Handle aliased attributes `select()`, `order()` and `reorder()`. + + *Tsutomu Kuroda* + * Reset the collection association when calling `reset` on it. Before: diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb index 73761520f7..ccbff8d1ff 100644 --- a/activerecord/lib/active_record/attribute_methods.rb +++ b/activerecord/lib/active_record/attribute_methods.rb @@ -110,16 +110,34 @@ module ActiveRecord end end - # A method name is 'dangerous' if it is already defined by Active Record, but + # A method name is 'dangerous' if it is already (re)defined by Active Record, but # not by any ancestors. (So 'puts' is not dangerous but 'save' is.) def dangerous_attribute_method?(name) # :nodoc: method_defined_within?(name, Base) end - def method_defined_within?(name, klass, sup = klass.superclass) # :nodoc: + def method_defined_within?(name, klass, superklass = klass.superclass) # :nodoc: if klass.method_defined?(name) || klass.private_method_defined?(name) - if sup.method_defined?(name) || sup.private_method_defined?(name) - klass.instance_method(name).owner != sup.instance_method(name).owner + if superklass.method_defined?(name) || superklass.private_method_defined?(name) + klass.instance_method(name).owner != superklass.instance_method(name).owner + else + true + end + else + false + end + end + + # A class method is 'dangerous' if it is already (re)defined by Active Record, but + # not by any ancestors. (So 'puts' is not dangerous but 'new' is.) + def dangerous_class_method?(method_name) + class_method_defined_within?(method_name, Base) + end + + def class_method_defined_within?(name, klass, superklass = klass.superclass) # :nodoc + if klass.respond_to?(name, true) + if superklass.respond_to?(name, true) + klass.method(name).owner != superklass.method(name).owner else true end diff --git a/activerecord/lib/active_record/dynamic_matchers.rb b/activerecord/lib/active_record/dynamic_matchers.rb index 5caab09038..e94b74063e 100644 --- a/activerecord/lib/active_record/dynamic_matchers.rb +++ b/activerecord/lib/active_record/dynamic_matchers.rb @@ -6,8 +6,12 @@ module ActiveRecord # then we can remove the indirection. def respond_to?(name, include_private = false) - match = Method.match(self, name) - match && match.valid? || super + if self == Base + super + else + match = Method.match(self, name) + match && match.valid? || super + end end private diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb index 53dde5e564..059bfe9a0f 100644 --- a/activerecord/lib/active_record/enum.rb +++ b/activerecord/lib/active_record/enum.rb @@ -77,10 +77,12 @@ module ActiveRecord name = name.to_sym # def self.statuses statuses end + detect_enum_conflict!(name, name.to_s.pluralize, true) klass.singleton_class.send(:define_method, name.to_s.pluralize) { enum_values } _enum_methods_module.module_eval do # def status=(value) self[:status] = statuses[value] end + klass.send(:detect_enum_conflict!, name, "#{name}=") define_method("#{name}=") { |value| if enum_values.has_key?(value) || value.blank? self[name] = enum_values[value] @@ -95,23 +97,28 @@ module ActiveRecord } # def status() statuses.key self[:status] end + klass.send(:detect_enum_conflict!, name, name) define_method(name) { enum_values.key self[name] } # def status_before_type_cast() statuses.key self[:status] end + klass.send(:detect_enum_conflict!, name, "#{name}_before_type_cast") define_method("#{name}_before_type_cast") { enum_values.key self[name] } pairs = values.respond_to?(:each_pair) ? values.each_pair : values.each_with_index pairs.each do |value, i| enum_values[value] = i - # scope :active, -> { where status: 0 } - klass.scope value, -> { klass.where name => i } - # def active?() status == 0 end + klass.send(:detect_enum_conflict!, name, "#{value}?") define_method("#{value}?") { self[name] == i } # def active!() update! status: :active end + klass.send(:detect_enum_conflict!, name, "#{value}!") define_method("#{value}!") { update! name => value } + + # scope :active, -> { where status: 0 } + klass.send(:detect_enum_conflict!, name, value, true) + klass.scope value, -> { klass.where name => i } end DEFINED_ENUMS[name.to_s] = enum_values @@ -148,5 +155,38 @@ module ActiveRecord mod end end + + ENUM_CONFLICT_MESSAGE = \ + "You tried to define an enum named \"%{enum}\" on the model \"%{klass}\", but " \ + "this will generate a %{type} method \"%{method}\", which is already defined " \ + "by %{source}." + + def detect_enum_conflict!(enum_name, method_name, klass_method = false) + if klass_method && dangerous_class_method?(method_name) + raise ArgumentError, ENUM_CONFLICT_MESSAGE % { + enum: enum_name, + klass: self.name, + type: 'class', + method: method_name, + source: 'Active Record' + } + elsif !klass_method && dangerous_attribute_method?(method_name) + raise ArgumentError, ENUM_CONFLICT_MESSAGE % { + enum: enum_name, + klass: self.name, + type: 'instance', + method: method_name, + source: 'Active Record' + } + elsif !klass_method && method_defined_within?(method_name, _enum_methods_module, Module) + raise ArgumentError, ENUM_CONFLICT_MESSAGE % { + enum: enum_name, + klass: self.name, + type: 'instance', + method: method_name, + source: 'another enum' + } + end + end end end diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index 460fbdb3f8..b1b35ed940 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -389,7 +389,7 @@ module ActiveRecord fresh_object = if options && options[:lock] - self.class.unscoped { self.class.lock.find(id) } + self.class.unscoped { self.class.lock(options[:lock]).find(id) } else self.class.unscoped { self.class.find(id) } end diff --git a/activerecord/lib/active_record/relation/batches.rb b/activerecord/lib/active_record/relation/batches.rb index f02e2365f7..dfcfef2ad2 100644 --- a/activerecord/lib/active_record/relation/batches.rb +++ b/activerecord/lib/active_record/relation/batches.rb @@ -64,6 +64,16 @@ module ActiveRecord # group.each { |person| person.party_all_night! } # end # + # If you do not provide a block to #find_in_batches, it will return an Enumerator + # for chaining with other methods: + # + # Person.find_in_batches.with_index do |group, batch| + # puts "Processing group ##{batch}" + # group.each(&:recover_from_last_night!) + # end + # + # To be yielded each record one by one, use #find_each instead. + # # ==== Options # * <tt>:batch_size</tt> - Specifies the size of the batch. Default to 1000. # * <tt>:start</tt> - Specifies the starting point for the batch processing. @@ -86,6 +96,7 @@ module ActiveRecord # the batch sizes. def find_in_batches(options = {}) options.assert_valid_keys(:start, :batch_size) + return to_enum(:find_in_batches, options) unless block_given? relation = self diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 993f628fa3..88fc47fada 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -234,7 +234,9 @@ module ActiveRecord def select!(*fields) # :nodoc: fields.flatten! - + fields.map! do |field| + klass.attribute_alias?(field) ? klass.attribute_alias(field).to_sym : field + end self.select_values += fields self end @@ -1048,9 +1050,11 @@ module ActiveRecord order_args.map! do |arg| case arg when Symbol + arg = klass.attribute_alias(arg).to_sym if klass.attribute_alias?(arg) table[arg].asc when Hash arg.map { |field, dir| + field = klass.attribute_alias(field).to_sym if klass.attribute_alias?(field) table[field].send(dir) } else diff --git a/activerecord/lib/active_record/scoping/named.rb b/activerecord/lib/active_record/scoping/named.rb index 2a5718f388..49cadb66d0 100644 --- a/activerecord/lib/active_record/scoping/named.rb +++ b/activerecord/lib/active_record/scoping/named.rb @@ -139,6 +139,12 @@ module ActiveRecord # Article.published.featured.latest_article # Article.featured.titles def scope(name, body, &block) + if dangerous_class_method?(name) + raise ArgumentError, "You tried to define a scope named \"#{name}\" " \ + "on the model \"#{self.name}\", but Active Record already defined " \ + "a class method with the same name." + end + extension = Module.new(&block) if block singleton_class.send(:define_method, name) do |*args| diff --git a/activerecord/test/cases/batches_test.rb b/activerecord/test/cases/batches_test.rb index ebb36e4940..1161b57514 100644 --- a/activerecord/test/cases/batches_test.rb +++ b/activerecord/test/cases/batches_test.rb @@ -172,4 +172,17 @@ class EachTest < ActiveRecord::TestCase end end end + + def test_find_in_batches_should_return_an_enumerator + enum = nil + assert_queries(0) do + enum = Post.find_in_batches(:batch_size => 1) + end + assert_queries(4) do + enum.first(4) do |batch| + assert_kind_of Array, batch + assert_kind_of Post, batch.first + end + end + end end diff --git a/activerecord/test/cases/enum_test.rb b/activerecord/test/cases/enum_test.rb index 8719f45e76..5cac630a3a 100644 --- a/activerecord/test/cases/enum_test.rb +++ b/activerecord/test/cases/enum_test.rb @@ -163,4 +163,63 @@ class EnumTest < ActiveRecord::TestCase test "_before_type_cast returns the enum label (required for form fields)" do assert_equal "proposed", @book.status_before_type_cast end + + test "reserved enum names" do + klass = Class.new(ActiveRecord::Base) do + self.table_name = "books" + enum status: [:proposed, :written, :published] + end + + conflicts = [ + :column, # generates class method .columns, which conflicts with an AR method + :logger, # generates #logger, which conflicts with an AR method + :attributes, # generates #attributes=, which conflicts with an AR method + ] + + conflicts.each_with_index do |name, i| + assert_raises(ArgumentError, "enum name `#{name}` should not be allowed") do + klass.class_eval { enum name => ["value_#{i}"] } + end + end + end + + test "reserved enum values" do + klass = Class.new(ActiveRecord::Base) do + self.table_name = "books" + enum status: [:proposed, :written, :published] + end + + conflicts = [ + :new, # generates a scope that conflicts with an AR class method + :valid, # generates #valid?, which conflicts with an AR method + :save, # generates #save!, which conflicts with an AR method + :proposed, # same value as an existing enum + ] + + conflicts.each_with_index do |value, i| + assert_raises(ArgumentError, "enum value `#{value}` should not be allowed") do + klass.class_eval { enum "status_#{i}" => [value] } + end + end + end + + test "overriding enum method should not raise" do + assert_nothing_raised do + klass = Class.new(ActiveRecord::Base) do + self.table_name = "books" + + def published! + super + "do publish work..." + end + + enum status: [:proposed, :written, :published] + + def written! + super + "do written work..." + end + end + end + end end diff --git a/activerecord/test/cases/finder_respond_to_test.rb b/activerecord/test/cases/finder_respond_to_test.rb index 3ff22f222f..6ab2657c44 100644 --- a/activerecord/test/cases/finder_respond_to_test.rb +++ b/activerecord/test/cases/finder_respond_to_test.rb @@ -5,6 +5,11 @@ class FinderRespondToTest < ActiveRecord::TestCase fixtures :topics + def test_should_preserve_normal_respond_to_behaviour_on_base + assert_respond_to ActiveRecord::Base, :new + assert !ActiveRecord::Base.respond_to?(:find_by_something) + end + def test_should_preserve_normal_respond_to_behaviour_and_respond_to_newly_added_method class << Topic; self; end.send(:define_method, :method_added_for_finder_respond_to_test) { } assert_respond_to Topic, :method_added_for_finder_respond_to_test diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb index a16ed963fe..c373dc1511 100644 --- a/activerecord/test/cases/locking_test.rb +++ b/activerecord/test/cases/locking_test.rb @@ -431,6 +431,17 @@ unless current_adapter?(:SybaseAdapter, :OpenBaseAdapter) || in_memory_db? assert_equal old, person.reload.first_name end + if current_adapter?(:PostgreSQLAdapter) + def test_lock_sending_custom_lock_statement + Person.transaction do + person = Person.find(1) + assert_sql(/LIMIT 1 FOR SHARE NOWAIT/) do + person.lock!('FOR SHARE NOWAIT') + end + end + end + end + if current_adapter?(:PostgreSQLAdapter, :OracleAdapter) def test_no_locks_no_wait first, second = duel { Person.find 1 } diff --git a/activerecord/test/cases/relation/mutation_test.rb b/activerecord/test/cases/relation/mutation_test.rb index 7cb2a19bee..4fafa668fb 100644 --- a/activerecord/test/cases/relation/mutation_test.rb +++ b/activerecord/test/cases/relation/mutation_test.rb @@ -14,6 +14,10 @@ module ActiveRecord def relation_delegate_class(klass) self.class.relation_delegate_class(klass) end + + def attribute_alias?(name) + false + end end def relation diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index e874c93110..e390d37871 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -206,12 +206,36 @@ class RelationTest < ActiveRecord::TestCase assert_equal topics(:fourth).title, topics.first.title end + def test_finding_with_order_by_aliased_attributes + topics = Topic.order(:heading) + assert_equal 5, topics.to_a.size + assert_equal topics(:fifth).title, topics.first.title + end + + def test_finding_with_assoc_order_by_aliased_attributes + topics = Topic.order(heading: :desc) + assert_equal 5, topics.to_a.size + assert_equal topics(:third).title, topics.first.title + end + def test_finding_with_reorder topics = Topic.order('author_name').order('title').reorder('id').to_a topics_titles = topics.map{ |t| t.title } assert_equal ['The First Topic', 'The Second Topic of the day', 'The Third Topic of the day', 'The Fourth Topic of the day', 'The Fifth Topic of the day'], topics_titles end + def test_finding_with_reorder_by_aliased_attributes + topics = Topic.order('author_name').reorder(:heading) + assert_equal 5, topics.to_a.size + assert_equal topics(:fifth).title, topics.first.title + end + + def test_finding_with_assoc_reorder_by_aliased_attributes + topics = Topic.order('author_name').reorder(heading: :desc) + assert_equal 5, topics.to_a.size + assert_equal topics(:third).title, topics.first.title + end + def test_finding_with_order_and_take entrants = Entrant.order("id ASC").limit(2).to_a @@ -775,6 +799,13 @@ class RelationTest < ActiveRecord::TestCase assert_equal david.salary, developer.salary end + def test_select_takes_an_aliased_attribute + first = topics(:first) + + topic = Topic.where(id: first.id).select(:heading).first + assert_equal first.heading, topic.heading + end + def test_select_argument_error assert_raises(ArgumentError) { Developer.select } end diff --git a/activerecord/test/cases/scoping/named_scoping_test.rb b/activerecord/test/cases/scoping/named_scoping_test.rb index 086977d9a2..9dc26cfd4d 100644 --- a/activerecord/test/cases/scoping/named_scoping_test.rb +++ b/activerecord/test/cases/scoping/named_scoping_test.rb @@ -266,6 +266,63 @@ class NamedScopingTest < ActiveRecord::TestCase assert_equal 'lifo', topic.author_name end + def test_reserved_scope_names + klass = Class.new(ActiveRecord::Base) do + self.table_name = "topics" + + scope :approved, -> { where(approved: true) } + + class << self + public + def pub; end + + private + def pri; end + + protected + def pro; end + end + end + + subklass = Class.new(klass) + + conflicts = [ + :create, # public class method on AR::Base + :relation, # private class method on AR::Base + :new, # redefined class method on AR::Base + :all, # a default scope + ] + + non_conflicts = [ + :find_by_title, # dynamic finder method + :approved, # existing scope + :pub, # existing public class method + :pri, # existing private class method + :pro, # existing protected class method + :open, # a ::Kernel method + ] + + conflicts.each do |name| + assert_raises(ArgumentError, "scope `#{name}` should not be allowed") do + klass.class_eval { scope name, ->{ where(approved: true) } } + end + + assert_raises(ArgumentError, "scope `#{name}` should not be allowed") do + subklass.class_eval { scope name, ->{ where(approved: true) } } + end + end + + non_conflicts.each do |name| + assert_nothing_raised do + klass.class_eval { scope name, ->{ where(approved: true) } } + end + + assert_nothing_raised do + subklass.class_eval { scope name, ->{ where(approved: true) } } + end + end + end + # Method delegation for scope names which look like /\A[a-zA-Z_]\w*[!?]?\z/ # has been done by evaluating a string with a plain def statement. For scope # names which contain spaces this approach doesn't work. |