From 2476c5312dcbd29f49672f71617a3d34c6a60cc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 22 Dec 2009 23:12:21 +0100 Subject: Validator is simply sent to validate method. However, the API needs to change, so validate accepts a record. --- activemodel/lib/active_model/validations/with.rb | 8 +++----- activemodel/lib/active_model/validator.rb | 7 +++---- activemodel/test/cases/validations/with_validation_test.rb | 12 ++++++------ 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/activemodel/lib/active_model/validations/with.rb b/activemodel/lib/active_model/validations/with.rb index edc2133ddc..8313aded11 100644 --- a/activemodel/lib/active_model/validations/with.rb +++ b/activemodel/lib/active_model/validations/with.rb @@ -49,12 +49,10 @@ module ActiveModel # end # def validates_with(*args) - configuration = args.extract_options! + options = args.extract_options! - validate configuration do |record| - args.each do |klass| - klass.new(record, configuration.except(:on, :if, :unless)).validate - end + args.each do |klass| + validate klass.new(options.except(:on, :if, :unless)), options end end end diff --git a/activemodel/lib/active_model/validator.rb b/activemodel/lib/active_model/validator.rb index 09de72b757..80a538fcb6 100644 --- a/activemodel/lib/active_model/validator.rb +++ b/activemodel/lib/active_model/validator.rb @@ -54,14 +54,13 @@ module ActiveModel #:nodoc: # end # class Validator - attr_reader :record, :options + attr_reader :options - def initialize(record, options) - @record = record + def initialize(options) @options = options end - def validate + def validate(record) raise "You must override this method" end end diff --git a/activemodel/test/cases/validations/with_validation_test.rb b/activemodel/test/cases/validations/with_validation_test.rb index fae87a6188..4350ad69ec 100644 --- a/activemodel/test/cases/validations/with_validation_test.rb +++ b/activemodel/test/cases/validations/with_validation_test.rb @@ -14,24 +14,24 @@ class ValidatesWithTest < ActiveRecord::TestCase OTHER_ERROR_MESSAGE = "Validation error from other validator" class ValidatorThatAddsErrors < ActiveModel::Validator - def validate() + def validate(record) record.errors[:base] << ERROR_MESSAGE end end class OtherValidatorThatAddsErrors < ActiveModel::Validator - def validate() + def validate(record) record.errors[:base] << OTHER_ERROR_MESSAGE end end class ValidatorThatDoesNotAddErrors < ActiveModel::Validator - def validate() + def validate(record) end end class ValidatorThatValidatesOptions < ActiveModel::Validator - def validate() + def validate(record) if options[:field] == :first_name record.errors[:base] << ERROR_MESSAGE end @@ -101,8 +101,8 @@ class ValidatesWithTest < ActiveRecord::TestCase test "passes all non-standard configuration options to the validator class" do topic = Topic.new validator = mock() - validator.expects(:new).with(topic, {:foo => :bar}).returns(validator) - validator.expects(:validate) + validator.expects(:new).with({:foo => :bar}).returns(validator) + validator.expects(:validate).with(topic) Topic.validates_with(validator, :if => "1 == 1", :foo => :bar) assert topic.valid? -- cgit v1.2.3 From f1085f41287687835659fa23079080204fe32e96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 23 Dec 2009 00:36:51 +0100 Subject: Move validations in ActiveModel to validators, however all validatity checks are still in the class method. --- activemodel/lib/active_model.rb | 5 +- .../lib/active_model/validations/acceptance.rb | 18 ++-- .../lib/active_model/validations/confirmation.rb | 20 ++-- .../lib/active_model/validations/exclusion.rb | 21 +++-- activemodel/lib/active_model/validations/format.rb | 28 +++--- .../lib/active_model/validations/inclusion.rb | 21 +++-- activemodel/lib/active_model/validations/length.rb | 91 +++++++++--------- .../lib/active_model/validations/numericality.rb | 105 +++++++++++---------- .../lib/active_model/validations/presence.rb | 15 +-- activemodel/lib/active_model/validations/with.rb | 5 +- activemodel/lib/active_model/validator.rb | 24 ++++- .../test/cases/validations/with_validation_test.rb | 4 +- 12 files changed, 200 insertions(+), 157 deletions(-) diff --git a/activemodel/lib/active_model.rb b/activemodel/lib/active_model.rb index e0de27b96d..6aa80da23f 100644 --- a/activemodel/lib/active_model.rb +++ b/activemodel/lib/active_model.rb @@ -35,9 +35,9 @@ module ActiveModel autoload :Dirty autoload :Errors autoload :Lint - autoload :Name, 'active_model/naming' + autoload :Name, 'active_model/naming' autoload :Naming - autoload :Observer, 'active_model/observing' + autoload :Observer, 'active_model/observing' autoload :Observing autoload :Serialization autoload :StateMachine @@ -45,6 +45,7 @@ module ActiveModel autoload :Validations autoload :ValidationsRepairHelper autoload :Validator + autoload :EachValidator, 'active_model/validator' autoload :VERSION module Serializers diff --git a/activemodel/lib/active_model/validations/acceptance.rb b/activemodel/lib/active_model/validations/acceptance.rb index b65c9b933d..a5de58cd41 100644 --- a/activemodel/lib/active_model/validations/acceptance.rb +++ b/activemodel/lib/active_model/validations/acceptance.rb @@ -1,5 +1,13 @@ module ActiveModel module Validations + class AcceptanceValidator < EachValidator + def validate_each(record, attribute, value) + unless value == options[:accept] + record.errors.add(attribute, :accepted, :default => options[:message]) + end + end + end + module ClassMethods # Encapsulates the pattern of wanting to validate the acceptance of a terms of service check box (or similar agreement). Example: # @@ -25,8 +33,8 @@ module ActiveModel # not occur (e.g. :unless => :skip_validation, or :unless => Proc.new { |user| user.signup_step <= 2 }). The # method, proc or string should return or evaluate to a true or false value. def validates_acceptance_of(*attr_names) - configuration = { :allow_nil => true, :accept => "1" } - configuration.update(attr_names.extract_options!) + options = { :allow_nil => true, :accept => "1" } + options.update(attr_names.extract_options!) db_cols = begin column_names @@ -37,11 +45,7 @@ module ActiveModel names = attr_names.reject { |name| db_cols.include?(name.to_s) } attr_accessor(*names) - validates_each(attr_names,configuration) do |record, attr_name, value| - unless value == configuration[:accept] - record.errors.add(attr_name, :accepted, :default => configuration[:message]) - end - end + validates_with AcceptanceValidator, options.merge(:attributes => attr_names) end end end diff --git a/activemodel/lib/active_model/validations/confirmation.rb b/activemodel/lib/active_model/validations/confirmation.rb index d414224dd2..b06effdceb 100644 --- a/activemodel/lib/active_model/validations/confirmation.rb +++ b/activemodel/lib/active_model/validations/confirmation.rb @@ -1,5 +1,13 @@ module ActiveModel module Validations + class ConfirmationValidator < EachValidator + def validate_each(record, attribute, value) + confirmed = record.send(:"#{attribute}_confirmation") + return if confirmed.nil? || value == confirmed + record.errors.add(attribute, :confirmation, :default => options[:message]) + end + end + module ClassMethods # Encapsulates the pattern of wanting to validate a password or email address field with a confirmation. Example: # @@ -30,15 +38,9 @@ module ActiveModel # not occur (e.g. :unless => :skip_validation, or :unless => Proc.new { |user| user.signup_step <= 2 }). The # method, proc or string should return or evaluate to a true or false value. def validates_confirmation_of(*attr_names) - configuration = attr_names.extract_options! - - attr_accessor(*(attr_names.map { |n| "#{n}_confirmation" })) - - validates_each(attr_names, configuration) do |record, attr_name, value| - unless record.send("#{attr_name}_confirmation").nil? or value == record.send("#{attr_name}_confirmation") - record.errors.add(attr_name, :confirmation, :default => configuration[:message]) - end - end + options = attr_names.extract_options! + attr_accessor(*(attr_names.map { |n| :"#{n}_confirmation" })) + validates_with ConfirmationValidator, options.merge(:attributes => attr_names) end end end diff --git a/activemodel/lib/active_model/validations/exclusion.rb b/activemodel/lib/active_model/validations/exclusion.rb index 2cfdec97a5..209b1c76f9 100644 --- a/activemodel/lib/active_model/validations/exclusion.rb +++ b/activemodel/lib/active_model/validations/exclusion.rb @@ -1,5 +1,12 @@ module ActiveModel module Validations + class ExclusionValidator < EachValidator + def validate_each(record, attribute, value) + return unless options[:in].include?(value) + record.errors.add(attribute, :exclusion, :default => options[:message], :value => value) + end + end + module ClassMethods # Validates that the value of the specified attribute is not in a particular enumerable object. # @@ -21,17 +28,13 @@ module ActiveModel # not occur (e.g. :unless => :skip_validation, or :unless => Proc.new { |user| user.signup_step <= 2 }). The # method, proc or string should return or evaluate to a true or false value. def validates_exclusion_of(*attr_names) - configuration = attr_names.extract_options! - - enum = configuration[:in] || configuration[:within] + options = attr_names.extract_options! + options[:in] ||= options.delete(:within) - raise(ArgumentError, "An object with the method include? is required must be supplied as the :in option of the configuration hash") unless enum.respond_to?(:include?) + raise ArgumentError, "An object with the method include? is required must be supplied as the " << + ":in option of the configuration hash" unless options[:in].respond_to?(:include?) - validates_each(attr_names, configuration) do |record, attr_name, value| - if enum.include?(value) - record.errors.add(attr_name, :exclusion, :default => configuration[:message], :value => value) - end - end + validates_with ExclusionValidator, options.merge(:attributes => attr_names) end end end diff --git a/activemodel/lib/active_model/validations/format.rb b/activemodel/lib/active_model/validations/format.rb index c670dafc7c..d5427c2b03 100644 --- a/activemodel/lib/active_model/validations/format.rb +++ b/activemodel/lib/active_model/validations/format.rb @@ -1,5 +1,15 @@ module ActiveModel module Validations + class FormatValidator < EachValidator + def validate_each(record, attribute, value) + if options[:with] && value.to_s !~ options[:with] + record.errors.add(attribute, :invalid, :default => options[:message], :value => value) + elsif options[:without] && value.to_s =~ options[:without] + record.errors.add(attribute, :invalid, :default => options[:message], :value => value) + end + end + end + module ClassMethods # Validates whether the value of the specified attribute is of the correct form, going by the regular expression provided. # You can require that the attribute matches the regular expression: @@ -33,29 +43,21 @@ module ActiveModel # not occur (e.g. :unless => :skip_validation, or :unless => Proc.new { |user| user.signup_step <= 2 }). The # method, proc or string should return or evaluate to a true or false value. def validates_format_of(*attr_names) - configuration = attr_names.extract_options! + options = attr_names.extract_options! - unless configuration.include?(:with) ^ configuration.include?(:without) # ^ == xor, or "exclusive or" + unless options.include?(:with) ^ options.include?(:without) # ^ == xor, or "exclusive or" raise ArgumentError, "Either :with or :without must be supplied (but not both)" end - if configuration[:with] && !configuration[:with].is_a?(Regexp) + if options[:with] && !options[:with].is_a?(Regexp) raise ArgumentError, "A regular expression must be supplied as the :with option of the configuration hash" end - if configuration[:without] && !configuration[:without].is_a?(Regexp) + if options[:without] && !options[:without].is_a?(Regexp) raise ArgumentError, "A regular expression must be supplied as the :without option of the configuration hash" end - if configuration[:with] - validates_each(attr_names, configuration) do |record, attr_name, value| - record.errors.add(attr_name, :invalid, :default => configuration[:message], :value => value) if value.to_s !~ configuration[:with] - end - elsif configuration[:without] - validates_each(attr_names, configuration) do |record, attr_name, value| - record.errors.add(attr_name, :invalid, :default => configuration[:message], :value => value) if value.to_s =~ configuration[:without] - end - end + validates_with FormatValidator, options.merge(:attributes => attr_names) end end end diff --git a/activemodel/lib/active_model/validations/inclusion.rb b/activemodel/lib/active_model/validations/inclusion.rb index 0d7dc5cd64..d42c95357c 100644 --- a/activemodel/lib/active_model/validations/inclusion.rb +++ b/activemodel/lib/active_model/validations/inclusion.rb @@ -1,5 +1,12 @@ module ActiveModel module Validations + class InclusionValidator < EachValidator + def validate_each(record, attribute, value) + return if options[:in].include?(value) + record.errors.add(attribute, :inclusion, :default => options[:message], :value => value) + end + end + module ClassMethods # Validates whether the value of the specified attribute is available in a particular enumerable object. # @@ -21,17 +28,13 @@ module ActiveModel # not occur (e.g. :unless => :skip_validation, or :unless => Proc.new { |user| user.signup_step <= 2 }). The # method, proc or string should return or evaluate to a true or false value. def validates_inclusion_of(*attr_names) - configuration = attr_names.extract_options! - - enum = configuration[:in] || configuration[:within] + options = attr_names.extract_options! + options[:in] ||= options.delete(:within) - raise(ArgumentError, "An object with the method include? is required must be supplied as the :in option of the configuration hash") unless enum.respond_to?(:include?) + raise ArgumentError, "An object with the method include? is required must be supplied as the " << + ":in option of the configuration hash" unless options[:in].respond_to?(:include?) - validates_each(attr_names, configuration) do |record, attr_name, value| - unless enum.include?(value) - record.errors.add(attr_name, :inclusion, :default => configuration[:message], :value => value) - end - end + validates_with InclusionValidator, options.merge(:attributes => attr_names) end end end diff --git a/activemodel/lib/active_model/validations/length.rb b/activemodel/lib/active_model/validations/length.rb index e91841bd1c..66b2ae5b18 100644 --- a/activemodel/lib/active_model/validations/length.rb +++ b/activemodel/lib/active_model/validations/length.rb @@ -1,7 +1,44 @@ module ActiveModel module Validations + class LengthValidator < EachValidator + MESSAGES = { :is => :wrong_length, :minimum => :too_short, :maximum => :too_long }.freeze + CHECKS = { :is => :==, :minimum => :>=, :maximum => :<= }.freeze + + attr_reader :type + + def initialize(options) + @type = options.delete(:type) + super + end + + def validate_each(record, attribute, value) + checks = options.slice(:minimum, :maximum, :is) + value = options[:tokenizer].call(value) if value.kind_of?(String) + + if [:within, :in].include?(type) + range = options[type] + checks[:minimum], checks[:maximum] = range.begin, range.end + checks[:maximum] -= 1 if range.exclude_end? + end + + checks.each do |key, check_value| + custom_message = options[:message] || options[MESSAGES[key]] + validity_check = CHECKS[key] + + valid_value = if key == :maximum + value.nil? || value.size.send(validity_check, check_value) + else + value && value.size.send(validity_check, check_value) + end + + record.errors.add(attribute, MESSAGES[key], :default => custom_message, :count => check_value) unless valid_value + end + end + end + module ClassMethods ALL_RANGE_OPTIONS = [ :is, :within, :in, :minimum, :maximum ].freeze + DEFAULT_TOKENIZER = lambda { |value| value.split(//) } # Validates that the specified attribute matches the length restrictions supplied. Only one option can be used at a time: # @@ -38,62 +75,32 @@ module ActiveModel # * :tokenizer - Specifies how to split up the attribute string. (e.g. :tokenizer => lambda {|str| str.scan(/\w+/)} to # count words as in above example.) # Defaults to lambda{ |value| value.split(//) } which counts individual characters. - def validates_length_of(*attrs) - # Merge given options with defaults. - options = { :tokenizer => lambda {|value| value.split(//)} } - options.update(attrs.extract_options!.symbolize_keys) + def validates_length_of(*attr_names) + options = { :tokenizer => DEFAULT_TOKENIZER } + options.update(attr_names.extract_options!) # Ensure that one and only one range option is specified. range_options = ALL_RANGE_OPTIONS & options.keys case range_options.size when 0 - raise ArgumentError, 'Range unspecified. Specify the :within, :maximum, :minimum, or :is option.' + raise ArgumentError, 'Range unspecified. Specify the :within, :maximum, :minimum, or :is option.' when 1 # Valid number of options; do nothing. else - raise ArgumentError, 'Too many range options specified. Choose only one.' + raise ArgumentError, 'Too many range options specified. Choose only one.' end - # Get range option and value. - option = range_options.first - option_value = options[range_options.first] - key = {:is => :wrong_length, :minimum => :too_short, :maximum => :too_long}[option] - custom_message = options[:message] || options[key] + type = range_options.first + value = options[type] - case option + case type when :within, :in - raise ArgumentError, ":#{option} must be a Range" unless option_value.is_a?(Range) - - validates_each(attrs, options) do |record, attr, value| - value = options[:tokenizer].call(value) if value.kind_of?(String) - - min, max = option_value.begin, option_value.end - max = max - 1 if option_value.exclude_end? - - if value.nil? || value.size < min - record.errors.add(attr, :too_short, :default => custom_message || options[:too_short], :count => min) - elsif value.size > max - record.errors.add(attr, :too_long, :default => custom_message || options[:too_long], :count => max) - end - end + raise ArgumentError, ":#{type} must be a Range" unless value.is_a?(Range) when :is, :minimum, :maximum - raise ArgumentError, ":#{option} must be a nonnegative Integer" unless option_value.is_a?(Integer) and option_value >= 0 - - # Declare different validations per option. - validity_checks = { :is => "==", :minimum => ">=", :maximum => "<=" } - - validates_each(attrs, options) do |record, attr, value| - value = options[:tokenizer].call(value) if value.kind_of?(String) - - valid_value = if option == :maximum - value.nil? || value.size.send(validity_checks[option], option_value) - else - value && value.size.send(validity_checks[option], option_value) - end - - record.errors.add(attr, key, :default => custom_message, :count => option_value) unless valid_value - end + raise ArgumentError, ":#{type} must be a nonnegative Integer" unless value.is_a?(Integer) && value >= 0 end + + validates_with LengthValidator, options.merge(:attributes => attr_names, :type => type) end alias_method :validates_size_of, :validates_length_of diff --git a/activemodel/lib/active_model/validations/numericality.rb b/activemodel/lib/active_model/validations/numericality.rb index 32dbcd82d0..8ffa395a2d 100644 --- a/activemodel/lib/active_model/validations/numericality.rb +++ b/activemodel/lib/active_model/validations/numericality.rb @@ -1,9 +1,55 @@ module ActiveModel module Validations + class NumericalityValidator < EachValidator + CHECKS = { :greater_than => '>', :greater_than_or_equal_to => '>=', + :equal_to => '==', :less_than => '<', :less_than_or_equal_to => '<=', + :odd => 'odd?', :even => 'even?' }.freeze + + def validate_each(record, attr_name, value) + before_type_cast = "#{attr_name}_before_type_cast" + + if record.respond_to?(before_type_cast.to_sym) + raw_value = record.send("#{attr_name}_before_type_cast") || value + else + raw_value = value + end + + return if options[:allow_nil] and raw_value.nil? + + if options[:only_integer] + unless raw_value.to_s =~ /\A[+-]?\d+\Z/ + record.errors.add(attr_name, :not_a_number, :value => raw_value, :default => options[:message]) + return + end + raw_value = raw_value.to_i + else + begin + raw_value = Kernel.Float(raw_value) + rescue ArgumentError, TypeError + record.errors.add(attr_name, :not_a_number, :value => raw_value, :default => options[:message]) + return + end + end + + options.slice(*CHECKS.keys).each do |option, option_value| + case option + when :odd, :even + unless raw_value.to_i.method(CHECKS[option])[] + record.errors.add(attr_name, option, :value => raw_value, :default => options[:message]) + end + else + option_value = option_value.call(record) if option_value.is_a? Proc + option_value = record.method(option_value).call if option_value.is_a? Symbol + + unless raw_value.method(CHECKS[option])[option_value] + record.errors.add(attr_name, option, :default => options[:message], :value => raw_value, :count => option_value) + end + end + end + end + end module ClassMethods - ALL_NUMERICALITY_CHECKS = { :greater_than => '>', :greater_than_or_equal_to => '>=', - :equal_to => '==', :less_than => '<', :less_than_or_equal_to => '<=', - :odd => 'odd?', :even => 'even?' }.freeze + # Validates whether the value of the specified attribute is numeric by trying to convert it to # a float with Kernel.Float (if only_integer is false) or applying it to the regular expression @@ -44,61 +90,18 @@ module ActiveModel # validates_numericality_of :width, :greater_than => :minimum_weight # end # - # - def validates_numericality_of(*attr_names) - configuration = { :only_integer => false, :allow_nil => false } - configuration.update(attr_names.extract_options!) + options = { :only_integer => false, :allow_nil => false } + options.update(attr_names.extract_options!) - numericality_options = ALL_NUMERICALITY_CHECKS.keys & configuration.keys + numericality_options = NumericalityValidator::CHECKS.keys & options.keys (numericality_options - [ :odd, :even ]).each do |option| - value = configuration[option] + value = options[option] raise ArgumentError, ":#{option} must be a number, a symbol or a proc" unless value.is_a?(Numeric) || value.is_a?(Proc) || value.is_a?(Symbol) end - validates_each(attr_names,configuration) do |record, attr_name, value| - before_type_cast = "#{attr_name}_before_type_cast" - - if record.respond_to?(before_type_cast.to_sym) - raw_value = record.send("#{attr_name}_before_type_cast") || value - else - raw_value = value - end - - next if configuration[:allow_nil] and raw_value.nil? - - if configuration[:only_integer] - unless raw_value.to_s =~ /\A[+-]?\d+\Z/ - record.errors.add(attr_name, :not_a_number, :value => raw_value, :default => configuration[:message]) - next - end - raw_value = raw_value.to_i - else - begin - raw_value = Kernel.Float(raw_value) - rescue ArgumentError, TypeError - record.errors.add(attr_name, :not_a_number, :value => raw_value, :default => configuration[:message]) - next - end - end - - numericality_options.each do |option| - case option - when :odd, :even - unless raw_value.to_i.method(ALL_NUMERICALITY_CHECKS[option])[] - record.errors.add(attr_name, option, :value => raw_value, :default => configuration[:message]) - end - else - configuration[option] = configuration[option].call(record) if configuration[option].is_a? Proc - configuration[option] = record.method(configuration[option]).call if configuration[option].is_a? Symbol - - unless raw_value.method(ALL_NUMERICALITY_CHECKS[option])[configuration[option]] - record.errors.add(attr_name, option, :default => configuration[:message], :value => raw_value, :count => configuration[option]) - end - end - end - end + validates_with NumericalityValidator, options.merge(:attributes => attr_names) end end end diff --git a/activemodel/lib/active_model/validations/presence.rb b/activemodel/lib/active_model/validations/presence.rb index 3ff677c137..a4c6f866a7 100644 --- a/activemodel/lib/active_model/validations/presence.rb +++ b/activemodel/lib/active_model/validations/presence.rb @@ -2,6 +2,12 @@ require 'active_support/core_ext/object/blank' module ActiveModel module Validations + class PresenceValidator < EachValidator + def validate(record) + record.errors.add_on_blank(attributes, options[:message]) + end + end + module ClassMethods # Validates that the specified attributes are not blank (as defined by Object#blank?). Happens by default on save. Example: # @@ -28,13 +34,8 @@ module ActiveModel # The method, proc or string should return or evaluate to a true or false value. # def validates_presence_of(*attr_names) - configuration = attr_names.extract_options! - - # can't use validates_each here, because it cannot cope with nonexistent attributes, - # while errors.add_on_empty can - validate configuration do |record| - record.errors.add_on_blank(attr_names, configuration[:message]) - end + options = attr_names.extract_options! + validates_with PresenceValidator, options.merge(:attributes => attr_names) end end end diff --git a/activemodel/lib/active_model/validations/with.rb b/activemodel/lib/active_model/validations/with.rb index 8313aded11..626e9d5731 100644 --- a/activemodel/lib/active_model/validations/with.rb +++ b/activemodel/lib/active_model/validations/with.rb @@ -50,10 +50,7 @@ module ActiveModel # def validates_with(*args) options = args.extract_options! - - args.each do |klass| - validate klass.new(options.except(:on, :if, :unless)), options - end + args.each { |klass| validate(klass.new(options), options) } end end end diff --git a/activemodel/lib/active_model/validator.rb b/activemodel/lib/active_model/validator.rb index 80a538fcb6..6885c6800e 100644 --- a/activemodel/lib/active_model/validator.rb +++ b/activemodel/lib/active_model/validator.rb @@ -1,5 +1,4 @@ module ActiveModel #:nodoc: - # A simple base class that can be used along with ActiveModel::Base.validates_with # # class Person < ActiveModel::Base @@ -61,7 +60,28 @@ module ActiveModel #:nodoc: end def validate(record) - raise "You must override this method" + raise NotImplementedError + end + end + + class EachValidator < Validator + attr_reader :attributes + + def initialize(options) + @attributes = options.delete(:attributes) + super + end + + def validate(record) + attributes.each do |attribute| + value = record.send(:read_attribute_for_validation, attribute) + next if (value.nil? && options[:allow_nil]) || (value.blank? && options[:allow_blank]) + validate_each(record, attribute, value) + end + end + + def validate_each(record) + raise NotImplementedError end end end diff --git a/activemodel/test/cases/validations/with_validation_test.rb b/activemodel/test/cases/validations/with_validation_test.rb index 4350ad69ec..953ff8aa2d 100644 --- a/activemodel/test/cases/validations/with_validation_test.rb +++ b/activemodel/test/cases/validations/with_validation_test.rb @@ -98,10 +98,10 @@ class ValidatesWithTest < ActiveRecord::TestCase assert topic.errors[:base].include?(ERROR_MESSAGE) end - test "passes all non-standard configuration options to the validator class" do + test "passes all configuration options to the validator class" do topic = Topic.new validator = mock() - validator.expects(:new).with({:foo => :bar}).returns(validator) + validator.expects(:new).with(:foo => :bar, :if => "1 == 1").returns(validator) validator.expects(:validate).with(topic) Topic.validates_with(validator, :if => "1 == 1", :foo => :bar) -- cgit v1.2.3 From 977a5c43b160d8aa8d1b87bb0feb54db85fe203c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 23 Dec 2009 01:08:27 +0100 Subject: Added check_validity! to EachValidator and refactor existing ones. --- .../lib/active_model/validations/exclusion.rb | 9 +-- .../lib/active_model/validations/inclusion.rb | 9 +-- activemodel/lib/active_model/validations/length.rb | 64 ++++++++++-------- .../lib/active_model/validations/numericality.rb | 78 +++++++++++----------- activemodel/lib/active_model/validator.rb | 4 ++ 5 files changed, 90 insertions(+), 74 deletions(-) diff --git a/activemodel/lib/active_model/validations/exclusion.rb b/activemodel/lib/active_model/validations/exclusion.rb index 209b1c76f9..f8759f253b 100644 --- a/activemodel/lib/active_model/validations/exclusion.rb +++ b/activemodel/lib/active_model/validations/exclusion.rb @@ -1,6 +1,11 @@ module ActiveModel module Validations class ExclusionValidator < EachValidator + def check_validity! + raise ArgumentError, "An object with the method include? is required must be supplied as the " << + ":in option of the configuration hash" unless options[:in].respond_to?(:include?) + end + def validate_each(record, attribute, value) return unless options[:in].include?(value) record.errors.add(attribute, :exclusion, :default => options[:message], :value => value) @@ -30,10 +35,6 @@ module ActiveModel def validates_exclusion_of(*attr_names) options = attr_names.extract_options! options[:in] ||= options.delete(:within) - - raise ArgumentError, "An object with the method include? is required must be supplied as the " << - ":in option of the configuration hash" unless options[:in].respond_to?(:include?) - validates_with ExclusionValidator, options.merge(:attributes => attr_names) end end diff --git a/activemodel/lib/active_model/validations/inclusion.rb b/activemodel/lib/active_model/validations/inclusion.rb index d42c95357c..a122e9e737 100644 --- a/activemodel/lib/active_model/validations/inclusion.rb +++ b/activemodel/lib/active_model/validations/inclusion.rb @@ -1,6 +1,11 @@ module ActiveModel module Validations class InclusionValidator < EachValidator + def check_validity! + raise ArgumentError, "An object with the method include? is required must be supplied as the " << + ":in option of the configuration hash" unless options[:in].respond_to?(:include?) + end + def validate_each(record, attribute, value) return if options[:in].include?(value) record.errors.add(attribute, :inclusion, :default => options[:message], :value => value) @@ -30,10 +35,6 @@ module ActiveModel def validates_inclusion_of(*attr_names) options = attr_names.extract_options! options[:in] ||= options.delete(:within) - - raise ArgumentError, "An object with the method include? is required must be supplied as the " << - ":in option of the configuration hash" unless options[:in].respond_to?(:include?) - validates_with InclusionValidator, options.merge(:attributes => attr_names) end end diff --git a/activemodel/lib/active_model/validations/length.rb b/activemodel/lib/active_model/validations/length.rb index 66b2ae5b18..1214c5f4bf 100644 --- a/activemodel/lib/active_model/validations/length.rb +++ b/activemodel/lib/active_model/validations/length.rb @@ -1,16 +1,24 @@ module ActiveModel module Validations class LengthValidator < EachValidator - MESSAGES = { :is => :wrong_length, :minimum => :too_short, :maximum => :too_long }.freeze - CHECKS = { :is => :==, :minimum => :>=, :maximum => :<= }.freeze + OPTIONS = [ :is, :within, :in, :minimum, :maximum ].freeze + MESSAGES = { :is => :wrong_length, :minimum => :too_short, :maximum => :too_long }.freeze + CHECKS = { :is => :==, :minimum => :>=, :maximum => :<= }.freeze + DEFAULT_TOKENIZER = lambda { |value| value.split(//) } attr_reader :type def initialize(options) - @type = options.delete(:type) + options[:tokenizer] ||= DEFAULT_TOKENIZER super + @type = (OPTIONS & options.keys).first end + def check_validity! + ensure_one_range_option! + ensure_argument_types! + end + def validate_each(record, attribute, value) checks = options.slice(:minimum, :maximum, :is) value = options[:tokenizer].call(value) if value.kind_of?(String) @@ -34,11 +42,35 @@ module ActiveModel record.errors.add(attribute, MESSAGES[key], :default => custom_message, :count => check_value) unless valid_value end end + + protected + + def ensure_one_range_option! #:nodoc: + range_options = OPTIONS & options.keys + + case range_options.size + when 0 + raise ArgumentError, 'Range unspecified. Specify the :within, :maximum, :minimum, or :is option.' + when 1 + # Valid number of options; do nothing. + else + raise ArgumentError, 'Too many range options specified. Choose only one.' + end + end + + def ensure_argument_types! #:nodoc: + value = options[type] + + case type + when :within, :in + raise ArgumentError, ":#{type} must be a Range" unless value.is_a?(Range) + when :is, :minimum, :maximum + raise ArgumentError, ":#{type} must be a nonnegative Integer" unless value.is_a?(Integer) && value >= 0 + end + end end module ClassMethods - ALL_RANGE_OPTIONS = [ :is, :within, :in, :minimum, :maximum ].freeze - DEFAULT_TOKENIZER = lambda { |value| value.split(//) } # Validates that the specified attribute matches the length restrictions supplied. Only one option can be used at a time: # @@ -78,28 +110,6 @@ module ActiveModel def validates_length_of(*attr_names) options = { :tokenizer => DEFAULT_TOKENIZER } options.update(attr_names.extract_options!) - - # Ensure that one and only one range option is specified. - range_options = ALL_RANGE_OPTIONS & options.keys - case range_options.size - when 0 - raise ArgumentError, 'Range unspecified. Specify the :within, :maximum, :minimum, or :is option.' - when 1 - # Valid number of options; do nothing. - else - raise ArgumentError, 'Too many range options specified. Choose only one.' - end - - type = range_options.first - value = options[type] - - case type - when :within, :in - raise ArgumentError, ":#{type} must be a Range" unless value.is_a?(Range) - when :is, :minimum, :maximum - raise ArgumentError, ":#{type} must be a nonnegative Integer" unless value.is_a?(Integer) && value >= 0 - end - validates_with LengthValidator, options.merge(:attributes => attr_names, :type => type) end diff --git a/activemodel/lib/active_model/validations/numericality.rb b/activemodel/lib/active_model/validations/numericality.rb index 8ffa395a2d..914a3133cf 100644 --- a/activemodel/lib/active_model/validations/numericality.rb +++ b/activemodel/lib/active_model/validations/numericality.rb @@ -1,56 +1,64 @@ module ActiveModel module Validations class NumericalityValidator < EachValidator - CHECKS = { :greater_than => '>', :greater_than_or_equal_to => '>=', - :equal_to => '==', :less_than => '<', :less_than_or_equal_to => '<=', - :odd => 'odd?', :even => 'even?' }.freeze + CHECKS = { :greater_than => :>, :greater_than_or_equal_to => :>=, + :equal_to => :==, :less_than => :<, :less_than_or_equal_to => :<=, + :odd => :odd?, :even => :even? }.freeze + + def check_validity! + options.slice(*CHECKS.keys) do |option, value| + next if [:odd, :even].include?(option) + raise ArgumentError, ":#{option} must be a number, a symbol or a proc" unless value.is_a?(Numeric) || value.is_a?(Proc) || value.is_a?(Symbol) + end + end def validate_each(record, attr_name, value) before_type_cast = "#{attr_name}_before_type_cast" - if record.respond_to?(before_type_cast.to_sym) - raw_value = record.send("#{attr_name}_before_type_cast") || value - else - raw_value = value - end + raw_value = record.send("#{attr_name}_before_type_cast") if record.respond_to?(before_type_cast.to_sym) + raw_value ||= value - return if options[:allow_nil] and raw_value.nil? + return if options[:allow_nil] && raw_value.nil? - if options[:only_integer] - unless raw_value.to_s =~ /\A[+-]?\d+\Z/ - record.errors.add(attr_name, :not_a_number, :value => raw_value, :default => options[:message]) - return - end - raw_value = raw_value.to_i - else - begin - raw_value = Kernel.Float(raw_value) - rescue ArgumentError, TypeError - record.errors.add(attr_name, :not_a_number, :value => raw_value, :default => options[:message]) - return - end + unless value = parse_raw_value(raw_value, options) + record.errors.add(attr_name, :not_a_number, :value => raw_value, :default => options[:message]) + return end options.slice(*CHECKS.keys).each do |option, option_value| case option when :odd, :even - unless raw_value.to_i.method(CHECKS[option])[] - record.errors.add(attr_name, option, :value => raw_value, :default => options[:message]) + unless value.to_i.send(CHECKS[option]) + record.errors.add(attr_name, option, :value => value, :default => options[:message]) end else - option_value = option_value.call(record) if option_value.is_a? Proc - option_value = record.method(option_value).call if option_value.is_a? Symbol - - unless raw_value.method(CHECKS[option])[option_value] - record.errors.add(attr_name, option, :default => options[:message], :value => raw_value, :count => option_value) + option_value = option_value.call(record) if option_value.is_a?(Proc) + option_value = record.send(option_value) if option_value.is_a?(Symbol) + + unless value.send(CHECKS[option], option_value) + record.errors.add(attr_name, option, :default => options[:message], :value => value, :count => option_value) end end end end - end - module ClassMethods + protected + def parse_raw_value(raw_value, options) + if options[:only_integer] + raw_value.to_i if raw_value.to_s =~ /\A[+-]?\d+\Z/ + else + begin + Kernel.Float(raw_value) + rescue ArgumentError, TypeError + nil + end + end + end + + end + + module ClassMethods # Validates whether the value of the specified attribute is numeric by trying to convert it to # a float with Kernel.Float (if only_integer is false) or applying it to the regular expression # /\A[\+\-]?\d+\Z/ (if only_integer is set to true). @@ -93,14 +101,6 @@ module ActiveModel def validates_numericality_of(*attr_names) options = { :only_integer => false, :allow_nil => false } options.update(attr_names.extract_options!) - - numericality_options = NumericalityValidator::CHECKS.keys & options.keys - - (numericality_options - [ :odd, :even ]).each do |option| - value = options[option] - raise ArgumentError, ":#{option} must be a number, a symbol or a proc" unless value.is_a?(Numeric) || value.is_a?(Proc) || value.is_a?(Symbol) - end - validates_with NumericalityValidator, options.merge(:attributes => attr_names) end end diff --git a/activemodel/lib/active_model/validator.rb b/activemodel/lib/active_model/validator.rb index 6885c6800e..342c4691ff 100644 --- a/activemodel/lib/active_model/validator.rb +++ b/activemodel/lib/active_model/validator.rb @@ -70,6 +70,7 @@ module ActiveModel #:nodoc: def initialize(options) @attributes = options.delete(:attributes) super + check_validity! end def validate(record) @@ -83,5 +84,8 @@ module ActiveModel #:nodoc: def validate_each(record) raise NotImplementedError end + + def check_validity! + end end end -- cgit v1.2.3 From 279067639f319f3b4bbcaf90c26f286e96df2c77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 23 Dec 2009 01:37:19 +0100 Subject: validates_each uses a BlockValidator. --- activemodel/lib/active_model.rb | 1 + activemodel/lib/active_model/validations.rb | 56 +++++++++------------- activemodel/lib/active_model/validations/length.rb | 7 ++- activemodel/lib/active_model/validations/with.rb | 4 +- activemodel/lib/active_model/validator.rb | 22 ++++++++- 5 files changed, 49 insertions(+), 41 deletions(-) diff --git a/activemodel/lib/active_model.rb b/activemodel/lib/active_model.rb index 6aa80da23f..abb04cd624 100644 --- a/activemodel/lib/active_model.rb +++ b/activemodel/lib/active_model.rb @@ -46,6 +46,7 @@ module ActiveModel autoload :ValidationsRepairHelper autoload :Validator autoload :EachValidator, 'active_model/validator' + autoload :BlockValidator, 'active_model/validator' autoload :VERSION module Serializers diff --git a/activemodel/lib/active_model/validations.rb b/activemodel/lib/active_model/validations.rb index 064ec98f3a..f1a15ec1d8 100644 --- a/activemodel/lib/active_model/validations.rb +++ b/activemodel/lib/active_model/validations.rb @@ -12,6 +12,29 @@ module ActiveModel end module ClassMethods + # Validates each attribute against a block. + # + # class Person < ActiveRecord::Base + # validates_each :first_name, :last_name do |record, attr, value| + # record.errors.add attr, 'starts with z.' if value[0] == ?z + # end + # end + # + # Options: + # * :on - Specifies when this validation is active (default is :save, other options :create, :update). + # * :allow_nil - Skip validation if attribute is +nil+. + # * :allow_blank - Skip validation if attribute is blank. + # * :if - Specifies a method, proc or string to call to determine if the validation should + # occur (e.g. :if => :allow_validation, or :if => Proc.new { |user| user.signup_step > 2 }). The + # method, proc or string should return or evaluate to a true or false value. + # * :unless - Specifies a method, proc or string to call to determine if the validation should + # not occur (e.g. :unless => :skip_validation, or :unless => Proc.new { |user| user.signup_step <= 2 }). The + # method, proc or string should return or evaluate to a true or false value. + def validates_each(*attr_names, &block) + options = attr_names.extract_options!.symbolize_keys + validates_with BlockValidator, options.merge(:attributes => attr_names.flatten), &block + end + # Adds a validation method or block to the class. This is useful when # overriding the +validate+ instance method becomes too unwieldly and # you're looking for more descriptive declaration of your validations. @@ -39,39 +62,6 @@ module ActiveModel # end # # This usage applies to +validate_on_create+ and +validate_on_update as well+. - - # Validates each attribute against a block. - # - # class Person < ActiveRecord::Base - # validates_each :first_name, :last_name do |record, attr, value| - # record.errors.add attr, 'starts with z.' if value[0] == ?z - # end - # end - # - # Options: - # * :on - Specifies when this validation is active (default is :save, other options :create, :update). - # * :allow_nil - Skip validation if attribute is +nil+. - # * :allow_blank - Skip validation if attribute is blank. - # * :if - Specifies a method, proc or string to call to determine if the validation should - # occur (e.g. :if => :allow_validation, or :if => Proc.new { |user| user.signup_step > 2 }). The - # method, proc or string should return or evaluate to a true or false value. - # * :unless - Specifies a method, proc or string to call to determine if the validation should - # not occur (e.g. :unless => :skip_validation, or :unless => Proc.new { |user| user.signup_step <= 2 }). The - # method, proc or string should return or evaluate to a true or false value. - def validates_each(*attrs) - options = attrs.extract_options!.symbolize_keys - attrs = attrs.flatten - - # Declare the validation. - validate options do |record| - attrs.each do |attr| - value = record.send(:read_attribute_for_validation, attr) - next if (value.nil? && options[:allow_nil]) || (value.blank? && options[:allow_blank]) - yield record, attr, value - end - end - end - def validate(*args, &block) options = args.last if options.is_a?(Hash) && options.key?(:on) diff --git a/activemodel/lib/active_model/validations/length.rb b/activemodel/lib/active_model/validations/length.rb index 1214c5f4bf..04280b401b 100644 --- a/activemodel/lib/active_model/validations/length.rb +++ b/activemodel/lib/active_model/validations/length.rb @@ -10,8 +10,8 @@ module ActiveModel def initialize(options) options[:tokenizer] ||= DEFAULT_TOKENIZER - super @type = (OPTIONS & options.keys).first + super end def check_validity! @@ -108,9 +108,8 @@ module ActiveModel # count words as in above example.) # Defaults to lambda{ |value| value.split(//) } which counts individual characters. def validates_length_of(*attr_names) - options = { :tokenizer => DEFAULT_TOKENIZER } - options.update(attr_names.extract_options!) - validates_with LengthValidator, options.merge(:attributes => attr_names, :type => type) + options = attr_names.extract_options! + validates_with LengthValidator, options.merge(:attributes => attr_names) end alias_method :validates_size_of, :validates_length_of diff --git a/activemodel/lib/active_model/validations/with.rb b/activemodel/lib/active_model/validations/with.rb index 626e9d5731..8d521173c6 100644 --- a/activemodel/lib/active_model/validations/with.rb +++ b/activemodel/lib/active_model/validations/with.rb @@ -48,9 +48,9 @@ module ActiveModel # end # end # - def validates_with(*args) + def validates_with(*args, &block) options = args.extract_options! - args.each { |klass| validate(klass.new(options), options) } + args.each { |klass| validate(klass.new(options, &block), options) } end end end diff --git a/activemodel/lib/active_model/validator.rb b/activemodel/lib/active_model/validator.rb index 342c4691ff..8c9f9c7fb3 100644 --- a/activemodel/lib/active_model/validator.rb +++ b/activemodel/lib/active_model/validator.rb @@ -51,7 +51,6 @@ module ActiveModel #:nodoc: # @my_custom_field = options[:field_name] || :first_name # end # end - # class Validator attr_reader :options @@ -64,6 +63,11 @@ module ActiveModel #:nodoc: end end + # EachValidator is a validator which iterates through the attributes given + # in the options hash invoking the validate_each method passing in the + # record, attribute and value. + # + # All ActiveModel validations are built on top of this Validator. class EachValidator < Validator attr_reader :attributes @@ -81,11 +85,25 @@ module ActiveModel #:nodoc: end end - def validate_each(record) + def validate_each(record, attribute, value) raise NotImplementedError end def check_validity! end end + + # BlockValidator is a special EachValidator which receives a block on initialization + # and call this block for each attribute being validated. +validates_each+ uses this + # Validator. + class BlockValidator < EachValidator + def initialize(options, &block) + @block = block + super + end + + def validate_each(record, attribute, value) + @block.call(record, attribute, value) + end + end end -- cgit v1.2.3 From 44cd9e0e7132abe632664377f13f3edd1106685a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 23 Dec 2009 12:14:00 +0100 Subject: ActiveRecord::Validations are now built on top of Validator as well. --- .../lib/active_model/validations/acceptance.rb | 7 +- activemodel/lib/active_model/validations/length.rb | 3 +- .../lib/active_model/validations/numericality.rb | 7 +- .../lib/active_record/validations/associated.rb | 16 +-- .../lib/active_record/validations/uniqueness.rb | 134 ++++++++++++--------- 5 files changed, 96 insertions(+), 71 deletions(-) diff --git a/activemodel/lib/active_model/validations/acceptance.rb b/activemodel/lib/active_model/validations/acceptance.rb index a5de58cd41..bd9463ed27 100644 --- a/activemodel/lib/active_model/validations/acceptance.rb +++ b/activemodel/lib/active_model/validations/acceptance.rb @@ -1,6 +1,10 @@ module ActiveModel module Validations class AcceptanceValidator < EachValidator + def initialize(options) + super(options.reverse_merge(:allow_nil => true, :accept => "1")) + end + def validate_each(record, attribute, value) unless value == options[:accept] record.errors.add(attribute, :accepted, :default => options[:message]) @@ -33,8 +37,7 @@ module ActiveModel # not occur (e.g. :unless => :skip_validation, or :unless => Proc.new { |user| user.signup_step <= 2 }). The # method, proc or string should return or evaluate to a true or false value. def validates_acceptance_of(*attr_names) - options = { :allow_nil => true, :accept => "1" } - options.update(attr_names.extract_options!) + options = attr_names.extract_options! db_cols = begin column_names diff --git a/activemodel/lib/active_model/validations/length.rb b/activemodel/lib/active_model/validations/length.rb index 04280b401b..6e90a75c17 100644 --- a/activemodel/lib/active_model/validations/length.rb +++ b/activemodel/lib/active_model/validations/length.rb @@ -9,9 +9,8 @@ module ActiveModel attr_reader :type def initialize(options) - options[:tokenizer] ||= DEFAULT_TOKENIZER @type = (OPTIONS & options.keys).first - super + super(options.reverse_merge(:tokenizer => DEFAULT_TOKENIZER)) end def check_validity! diff --git a/activemodel/lib/active_model/validations/numericality.rb b/activemodel/lib/active_model/validations/numericality.rb index 914a3133cf..f2aab8c5b8 100644 --- a/activemodel/lib/active_model/validations/numericality.rb +++ b/activemodel/lib/active_model/validations/numericality.rb @@ -5,6 +5,10 @@ module ActiveModel :equal_to => :==, :less_than => :<, :less_than_or_equal_to => :<=, :odd => :odd?, :even => :even? }.freeze + def initialize(options) + super(options.reverse_merge(:only_integer => false, :allow_nil => false)) + end + def check_validity! options.slice(*CHECKS.keys) do |option, value| next if [:odd, :even].include?(option) @@ -99,8 +103,7 @@ module ActiveModel # end # def validates_numericality_of(*attr_names) - options = { :only_integer => false, :allow_nil => false } - options.update(attr_names.extract_options!) + options = attr_names.extract_options! validates_with NumericalityValidator, options.merge(:attributes => attr_names) end end diff --git a/activerecord/lib/active_record/validations/associated.rb b/activerecord/lib/active_record/validations/associated.rb index 92f47d770f..6e6f4df415 100644 --- a/activerecord/lib/active_record/validations/associated.rb +++ b/activerecord/lib/active_record/validations/associated.rb @@ -1,5 +1,12 @@ module ActiveRecord module Validations + class AssociatedValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + return if (value.is_a?(Array) ? value : [value]).compact.all?{ |r| r.valid? } + record.errors.add(attribute, :invalid, :default => options[:message], :value => value) + end + end + module ClassMethods # Validates whether the associated object or objects are all valid themselves. Works with any kind of association. # @@ -33,13 +40,8 @@ module ActiveRecord # not occur (e.g. :unless => :skip_validation, or :unless => Proc.new { |user| user.signup_step <= 2 }). The # method, proc or string should return or evaluate to a true or false value. def validates_associated(*attr_names) - configuration = attr_names.extract_options! - - validates_each(attr_names, configuration) do |record, attr_name, value| - unless (value.is_a?(Array) ? value : [value]).collect { |r| r.nil? || r.valid? }.all? - record.errors.add(attr_name, :invalid, :default => configuration[:message], :value => value) - end - end + options = attr_names.extract_options! + validates_with AssociatedValidator, options.merge(:attributes => attr_names) end end end diff --git a/activerecord/lib/active_record/validations/uniqueness.rb b/activerecord/lib/active_record/validations/uniqueness.rb index 711086dc2c..b3a34501dc 100644 --- a/activerecord/lib/active_record/validations/uniqueness.rb +++ b/activerecord/lib/active_record/validations/uniqueness.rb @@ -1,5 +1,77 @@ module ActiveRecord module Validations + class UniquenessValidator < ActiveModel::EachValidator + def initialize(options) + @klass = options.delete(:klass) + super(options.reverse_merge(:case_sensitive => true)) + end + + def validate_each(record, attribute, value) + finder_class = find_finder_class_for(record) + table_name = record.class.quoted_table_name + sql, params = mount_sql_and_params(finder_class, table_name, attribute, value) + + Array(options[:scope]).each do |scope_item| + scope_value = record.send(scope_item) + sql << " AND " << record.class.send(:attribute_condition, "#{table_name}.#{scope_item}", scope_value) + params << scope_value + end + + unless record.new_record? + sql << " AND #{record.class.quoted_table_name}.#{record.class.primary_key} <> ?" + params << record.send(:id) + end + + finder_class.with_exclusive_scope do + if finder_class.exists?([sql, *params]) + record.errors.add(attribute, :taken, :default => options[:message], :value => value) + end + end + end + + protected + + # The check for an existing value should be run from a class that + # isn't abstract. This means working down from the current class + # (self), to the first non-abstract class. Since classes don't know + # their subclasses, we have to build the hierarchy between self and + # the record's class. + def find_finder_class_for(record) #:nodoc: + class_hierarchy = [record.class] + + while class_hierarchy.first != @klass + class_hierarchy.insert(0, class_hierarchy.first.superclass) + end + + class_hierarchy.detect { |klass| !klass.abstract_class? } + end + + def mount_sql_and_params(klass, table_name, attribute, value) #:nodoc: + column = klass.columns_hash[attribute.to_s] + + operator = if value.nil? + "IS ?" + elsif column.text? + value = column.limit ? value.to_s.mb_chars[0, column.limit] : value.to_s + "#{klass.connection.case_sensitive_equality_operator} ?" + else + "= ?" + end + + sql_attribute = "#{table_name}.#{klass.connection.quote_column_name(attribute)}" + + if value.nil? || (options[:case_sensitive] || !column.text?) + sql = "#{sql_attribute} #{operator}" + params = [value] + else + sql = "LOWER(#{sql_attribute}) #{operator}" + params = [value.mb_chars.downcase] + end + + [sql, params] + end + end + module ClassMethods # Validates whether the value of the specified attributes are unique across the system. Useful for making sure that only one user # can be named "davidhh". @@ -69,6 +141,7 @@ module ActiveRecord # # This could even happen if you use transactions with the 'serializable' # isolation level. There are several ways to get around this problem: + # # - By locking the database table before validating, and unlocking it after # saving. However, table locking is very expensive, and thus not # recommended. @@ -94,65 +167,10 @@ module ActiveRecord # index constraint errors from other types of database errors, so you # will have to parse the (database-specific) exception message to detect # such a case. + # def validates_uniqueness_of(*attr_names) - configuration = { :case_sensitive => true } - configuration.update(attr_names.extract_options!) - - validates_each(attr_names,configuration) do |record, attr_name, value| - # The check for an existing value should be run from a class that - # isn't abstract. This means working down from the current class - # (self), to the first non-abstract class. Since classes don't know - # their subclasses, we have to build the hierarchy between self and - # the record's class. - class_hierarchy = [record.class] - while class_hierarchy.first != self - class_hierarchy.insert(0, class_hierarchy.first.superclass) - end - - # Now we can work our way down the tree to the first non-abstract - # class (which has a database table to query from). - finder_class = class_hierarchy.detect { |klass| !klass.abstract_class? } - - column = finder_class.columns_hash[attr_name.to_s] - - if value.nil? - comparison_operator = "IS ?" - elsif column.text? - comparison_operator = "#{connection.case_sensitive_equality_operator} ?" - value = column.limit ? value.to_s.mb_chars[0, column.limit] : value.to_s - else - comparison_operator = "= ?" - end - - sql_attribute = "#{record.class.quoted_table_name}.#{connection.quote_column_name(attr_name)}" - - if value.nil? || (configuration[:case_sensitive] || !column.text?) - condition_sql = "#{sql_attribute} #{comparison_operator}" - condition_params = [value] - else - condition_sql = "LOWER(#{sql_attribute}) #{comparison_operator}" - condition_params = [value.mb_chars.downcase] - end - - if scope = configuration[:scope] - Array(scope).map do |scope_item| - scope_value = record.send(scope_item) - condition_sql << " AND " << attribute_condition("#{record.class.quoted_table_name}.#{scope_item}", scope_value) - condition_params << scope_value - end - end - - unless record.new_record? - condition_sql << " AND #{record.class.quoted_table_name}.#{record.class.primary_key} <> ?" - condition_params << record.send(:id) - end - - finder_class.with_exclusive_scope do - if finder_class.exists?([condition_sql, *condition_params]) - record.errors.add(attr_name, :taken, :default => configuration[:message], :value => value) - end - end - end + options = attr_names.extract_options! + validates_with UniquenessValidator, options.merge(:attributes => attr_names, :klass => self) end end end -- cgit v1.2.3 From e31077c9aaec05bdf5ea0386eb42fcc039d86a0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 23 Dec 2009 12:28:02 +0100 Subject: Small clean up in Naming and TTranslation tests. --- activemodel/lib/active_model/naming.rb | 26 +++++++++++++++++---- activemodel/lib/active_model/translation.rb | 22 +----------------- activemodel/test/cases/naming_test.rb | 3 ++- activemodel/test/cases/translation_test.rb | 36 ++++++++++++----------------- activemodel/test/models/person.rb | 4 ++++ activemodel/test/models/track_back.rb | 4 ++++ 6 files changed, 48 insertions(+), 47 deletions(-) create mode 100644 activemodel/test/models/track_back.rb diff --git a/activemodel/lib/active_model/naming.rb b/activemodel/lib/active_model/naming.rb index 675d62b9a6..4cd68a0c89 100644 --- a/activemodel/lib/active_model/naming.rb +++ b/activemodel/lib/active_model/naming.rb @@ -2,11 +2,11 @@ require 'active_support/inflector' module ActiveModel class Name < String - attr_reader :singular, :plural, :element, :collection, :partial_path, :human + attr_reader :singular, :plural, :element, :collection, :partial_path alias_method :cache_key, :collection - def initialize(klass, name) - super(name) + def initialize(klass) + super(klass.name) @klass = klass @singular = ActiveSupport::Inflector.underscore(self).tr('/', '_').freeze @plural = ActiveSupport::Inflector.pluralize(@singular).freeze @@ -15,13 +15,31 @@ module ActiveModel @collection = ActiveSupport::Inflector.tableize(self).freeze @partial_path = "#{@collection}/#{@element}".freeze end + + # Transform the model name into a more humane format, using I18n. By default, + # it will underscore then humanize the class name (BlogPost.model_name.human #=> "Blog post"). + # Specify +options+ with additional translating options. + def human(options={}) + return @human unless @klass.respond_to?(:lookup_ancestors) && + @klass.respond_to?(:i18n_scope) + + defaults = @klass.lookup_ancestors.map do |klass| + klass.model_name.underscore.to_sym + end + + defaults << options.delete(:default) if options[:default] + defaults << @human + + options.reverse_merge! :scope => [@klass.i18n_scope, :models], :count => 1, :default => defaults + I18n.translate(defaults.shift, options) + end end module Naming # Returns an ActiveModel::Name object for module. It can be # used to retrieve all kinds of naming-related information. def model_name - @_model_name ||= ActiveModel::Name.new(self, name) + @_model_name ||= ActiveModel::Name.new(self) end end end diff --git a/activemodel/lib/active_model/translation.rb b/activemodel/lib/active_model/translation.rb index 42ca463f82..e5ef1e6114 100644 --- a/activemodel/lib/active_model/translation.rb +++ b/activemodel/lib/active_model/translation.rb @@ -37,28 +37,8 @@ module ActiveModel # Model.human_name is deprecated. Use Model.model_name.human instead. def human_name(*args) - ActiveSupport::Deprecation.warn("human_name has been deprecated, please use model_name.human instead", caller[0,1]) + ActiveSupport::Deprecation.warn("human_name has been deprecated, please use model_name.human instead", caller[0,5]) model_name.human(*args) end end - - class Name < String - # Transform the model name into a more humane format, using I18n. By default, - # it will underscore then humanize the class name (BlogPost.human_name #=> "Blog post"). - # Specify +options+ with additional translating options. - def human(options={}) - return @human unless @klass.respond_to?(:lookup_ancestors) && - @klass.respond_to?(:i18n_scope) - - defaults = @klass.lookup_ancestors.map do |klass| - klass.model_name.underscore.to_sym - end - - defaults << options.delete(:default) if options[:default] - defaults << @human - - options.reverse_merge! :scope => [@klass.i18n_scope, :models], :count => 1, :default => defaults - I18n.translate(defaults.shift, options) - end - end end diff --git a/activemodel/test/cases/naming_test.rb b/activemodel/test/cases/naming_test.rb index fe1ea36450..dc39b84ed8 100644 --- a/activemodel/test/cases/naming_test.rb +++ b/activemodel/test/cases/naming_test.rb @@ -1,8 +1,9 @@ require 'cases/helper' +require 'models/track_back' class NamingTest < ActiveModel::TestCase def setup - @model_name = ActiveModel::Name.new(self, 'Post::TrackBack') + @model_name = ActiveModel::Name.new(Post::TrackBack) end def test_singular diff --git a/activemodel/test/cases/translation_test.rb b/activemodel/test/cases/translation_test.rb index d171784963..bfc1ca12e6 100644 --- a/activemodel/test/cases/translation_test.rb +++ b/activemodel/test/cases/translation_test.rb @@ -1,11 +1,5 @@ require 'cases/helper' - -class SuperUser - extend ActiveModel::Translation -end - -class User < SuperUser -end +require 'models/person' class ActiveModelI18nTests < ActiveModel::TestCase @@ -14,38 +8,38 @@ class ActiveModelI18nTests < ActiveModel::TestCase end def test_translated_model_attributes - I18n.backend.store_translations 'en', :activemodel => {:attributes => {:super_user => {:name => 'super_user name attribute'} } } - assert_equal 'super_user name attribute', SuperUser.human_attribute_name('name') + I18n.backend.store_translations 'en', :activemodel => {:attributes => {:person => {:name => 'person name attribute'} } } + assert_equal 'person name attribute', Person.human_attribute_name('name') end def test_translated_model_attributes_with_symbols - I18n.backend.store_translations 'en', :activemodel => {:attributes => {:super_user => {:name => 'super_user name attribute'} } } - assert_equal 'super_user name attribute', SuperUser.human_attribute_name(:name) + I18n.backend.store_translations 'en', :activemodel => {:attributes => {:person => {:name => 'person name attribute'} } } + assert_equal 'person name attribute', Person.human_attribute_name(:name) end def test_translated_model_attributes_with_ancestor - I18n.backend.store_translations 'en', :activemodel => {:attributes => {:user => {:name => 'user name attribute'} } } - assert_equal 'user name attribute', User.human_attribute_name('name') + I18n.backend.store_translations 'en', :activemodel => {:attributes => {:child => {:name => 'child name attribute'} } } + assert_equal 'child name attribute', Child.human_attribute_name('name') end def test_translated_model_attributes_with_ancestors_fallback - I18n.backend.store_translations 'en', :activemodel => {:attributes => {:super_user => {:name => 'super_user name attribute'} } } - assert_equal 'super_user name attribute', User.human_attribute_name('name') + I18n.backend.store_translations 'en', :activemodel => {:attributes => {:person => {:name => 'person name attribute'} } } + assert_equal 'person name attribute', Child.human_attribute_name('name') end def test_translated_model_names - I18n.backend.store_translations 'en', :activemodel => {:models => {:super_user => 'super_user model'} } - assert_equal 'super_user model', SuperUser.model_name.human + I18n.backend.store_translations 'en', :activemodel => {:models => {:person => 'person model'} } + assert_equal 'person model', Person.model_name.human end def test_translated_model_names_with_sti - I18n.backend.store_translations 'en', :activemodel => {:models => {:user => 'user model'} } - assert_equal 'user model', User.model_name.human + I18n.backend.store_translations 'en', :activemodel => {:models => {:child => 'child model'} } + assert_equal 'child model', Child.model_name.human end def test_translated_model_names_with_ancestors_fallback - I18n.backend.store_translations 'en', :activemodel => {:models => {:super_user => 'super_user model'} } - assert_equal 'super_user model', User.model_name.human + I18n.backend.store_translations 'en', :activemodel => {:models => {:person => 'person model'} } + assert_equal 'person model', Child.model_name.human end end diff --git a/activemodel/test/models/person.rb b/activemodel/test/models/person.rb index d98420f900..fad29a51ec 100644 --- a/activemodel/test/models/person.rb +++ b/activemodel/test/models/person.rb @@ -1,5 +1,9 @@ class Person include ActiveModel::Validations + extend ActiveModel::Translation attr_accessor :title, :karma end + +class Child < Person +end diff --git a/activemodel/test/models/track_back.rb b/activemodel/test/models/track_back.rb new file mode 100644 index 0000000000..d137e4ff8f --- /dev/null +++ b/activemodel/test/models/track_back.rb @@ -0,0 +1,4 @@ +class Post + class TrackBack + end +end \ No newline at end of file -- cgit v1.2.3 From 74098e4cb6de01745db8f1d8d567645553ade7c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 23 Dec 2009 13:30:58 +0100 Subject: No need to use ValidationsRepairHelper hack on ActiveModel anymore, Model.reset_callbacks(:validate) is enough. However, tests in ActiveRecord are still coupled, so moved ValidationsRepairHelper back there. --- activemodel/lib/active_model.rb | 1 - .../lib/active_model/validations_repair_helper.rb | 35 -------------- .../validations/acceptance_validation_test.rb | 33 +++++-------- .../validations/conditional_validation_test.rb | 5 +- .../validations/confirmation_validation_test.rb | 34 +++++--------- .../cases/validations/exclusion_validation_test.rb | 23 ++++----- .../cases/validations/format_validation_test.rb | 33 +++++-------- .../test/cases/validations/i18n_validation_test.rb | 1 - .../cases/validations/inclusion_validation_test.rb | 33 +++++-------- .../cases/validations/length_validation_test.rb | 53 +++++---------------- .../validations/numericality_validation_test.rb | 41 +++++----------- .../cases/validations/presence_validation_test.rb | 54 +++++++++++----------- .../test/cases/validations/with_validation_test.rb | 5 +- activemodel/test/cases/validations_test.rb | 5 +- activemodel/test/models/person.rb | 2 +- .../lib/active_record/validations/associated.rb | 2 +- activerecord/test/cases/helper.rb | 3 +- .../test/cases/validations_repair_helper.rb | 35 ++++++++++++++ 18 files changed, 158 insertions(+), 240 deletions(-) delete mode 100644 activemodel/lib/active_model/validations_repair_helper.rb create mode 100644 activerecord/test/cases/validations_repair_helper.rb diff --git a/activemodel/lib/active_model.rb b/activemodel/lib/active_model.rb index abb04cd624..ee83e3eddd 100644 --- a/activemodel/lib/active_model.rb +++ b/activemodel/lib/active_model.rb @@ -43,7 +43,6 @@ module ActiveModel autoload :StateMachine autoload :Translation autoload :Validations - autoload :ValidationsRepairHelper autoload :Validator autoload :EachValidator, 'active_model/validator' autoload :BlockValidator, 'active_model/validator' diff --git a/activemodel/lib/active_model/validations_repair_helper.rb b/activemodel/lib/active_model/validations_repair_helper.rb deleted file mode 100644 index 40741e6dbe..0000000000 --- a/activemodel/lib/active_model/validations_repair_helper.rb +++ /dev/null @@ -1,35 +0,0 @@ -module ActiveModel - module ValidationsRepairHelper - extend ActiveSupport::Concern - - module ClassMethods - def repair_validations(*model_classes) - setup do - @_stored_callbacks = {} - model_classes.each do |k| - @_stored_callbacks[k] = k._validate_callbacks.dup - end - end - teardown do - model_classes.each do |k| - k._validate_callbacks = @_stored_callbacks[k] - k.__update_callbacks(:validate) - end - end - end - end - - def repair_validations(*model_classes, &block) - @__stored_callbacks = {} - model_classes.each do |k| - @__stored_callbacks[k] = k._validate_callbacks.dup - end - return block.call - ensure - model_classes.each do |k| - k._validate_callbacks = @__stored_callbacks[k] - k.__update_callbacks(:validate) - end - end - end -end diff --git a/activemodel/test/cases/validations/acceptance_validation_test.rb b/activemodel/test/cases/validations/acceptance_validation_test.rb index 88e5fdb358..11c9c1edfd 100644 --- a/activemodel/test/cases/validations/acceptance_validation_test.rb +++ b/activemodel/test/cases/validations/acceptance_validation_test.rb @@ -9,9 +9,10 @@ require 'models/person' class AcceptanceValidationTest < ActiveModel::TestCase include ActiveModel::TestsDatabase - include ActiveModel::ValidationsRepairHelper - repair_validations(Topic) + def teardown + Topic.reset_callbacks(:validate) + end def test_terms_of_service_agreement_no_acceptance Topic.validates_acceptance_of(:terms_of_service, :on => :create) @@ -53,28 +54,18 @@ class AcceptanceValidationTest < ActiveModel::TestCase assert t.save end - def test_validates_acceptance_of_with_custom_error_using_quotes - repair_validations(Developer) do - Developer.validates_acceptance_of :salary, :message=> "This string contains 'single' and \"double\" quotes" - d = Developer.new - d.salary = "0" - assert !d.valid? - assert_equal "This string contains 'single' and \"double\" quotes", d.errors[:salary].last - end - end - def test_validates_acceptance_of_for_ruby_class - repair_validations(Person) do - Person.validates_acceptance_of :karma + Person.validates_acceptance_of :karma - p = Person.new - p.karma = "" + p = Person.new + p.karma = "" - assert p.invalid? - assert_equal ["must be accepted"], p.errors[:karma] + assert p.invalid? + assert_equal ["must be accepted"], p.errors[:karma] - p.karma = "1" - assert p.valid? - end + p.karma = "1" + assert p.valid? + ensure + Person.reset_callbacks(:validate) end end diff --git a/activemodel/test/cases/validations/conditional_validation_test.rb b/activemodel/test/cases/validations/conditional_validation_test.rb index 4c716d5d48..5260162a58 100644 --- a/activemodel/test/cases/validations/conditional_validation_test.rb +++ b/activemodel/test/cases/validations/conditional_validation_test.rb @@ -6,9 +6,10 @@ require 'models/topic' class ConditionalValidationTest < ActiveModel::TestCase include ActiveModel::TestsDatabase - include ActiveModel::ValidationsRepairHelper - repair_validations(Topic) + def teardown + Topic.reset_callbacks(:validate) + end def test_if_validation_using_method_true # When the method returns true diff --git a/activemodel/test/cases/validations/confirmation_validation_test.rb b/activemodel/test/cases/validations/confirmation_validation_test.rb index 1d6f2a6ec5..55554d5054 100644 --- a/activemodel/test/cases/validations/confirmation_validation_test.rb +++ b/activemodel/test/cases/validations/confirmation_validation_test.rb @@ -8,9 +8,10 @@ require 'models/person' class ConfirmationValidationTest < ActiveModel::TestCase include ActiveModel::TestsDatabase - include ActiveModel::ValidationsRepairHelper - repair_validations(Topic) + def teardown + Topic.reset_callbacks(:validate) + end def test_no_title_confirmation Topic.validates_confirmation_of(:title) @@ -39,30 +40,19 @@ class ConfirmationValidationTest < ActiveModel::TestCase assert t.save end - def test_validates_confirmation_of_with_custom_error_using_quotes - repair_validations(Developer) do - Developer.validates_confirmation_of :name, :message=> "confirm 'single' and \"double\" quotes" - d = Developer.new - d.name = "John" - d.name_confirmation = "Johnny" - assert !d.valid? - assert_equal ["confirm 'single' and \"double\" quotes"], d.errors[:name] - end - end - def test_validates_confirmation_of_for_ruby_class - repair_validations(Person) do - Person.validates_confirmation_of :karma + Person.validates_confirmation_of :karma - p = Person.new - p.karma_confirmation = "None" - assert p.invalid? + p = Person.new + p.karma_confirmation = "None" + assert p.invalid? - assert_equal ["doesn't match confirmation"], p.errors[:karma] + assert_equal ["doesn't match confirmation"], p.errors[:karma] - p.karma = "None" - assert p.valid? - end + p.karma = "None" + assert p.valid? + ensure + Person.reset_callbacks(:validate) end end diff --git a/activemodel/test/cases/validations/exclusion_validation_test.rb b/activemodel/test/cases/validations/exclusion_validation_test.rb index 584f009e84..7d851f546c 100644 --- a/activemodel/test/cases/validations/exclusion_validation_test.rb +++ b/activemodel/test/cases/validations/exclusion_validation_test.rb @@ -7,9 +7,10 @@ require 'models/person' class ExclusionValidationTest < ActiveModel::TestCase include ActiveModel::TestsDatabase - include ActiveModel::ValidationsRepairHelper - repair_validations(Topic) + def teardown + Topic.reset_callbacks(:validate) + end def test_validates_exclusion_of Topic.validates_exclusion_of( :title, :in => %w( abe monkey ) ) @@ -30,17 +31,17 @@ class ExclusionValidationTest < ActiveModel::TestCase end def test_validates_exclusion_of_for_ruby_class - repair_validations(Person) do - Person.validates_exclusion_of :karma, :in => %w( abe monkey ) + Person.validates_exclusion_of :karma, :in => %w( abe monkey ) - p = Person.new - p.karma = "abe" - assert p.invalid? + p = Person.new + p.karma = "abe" + assert p.invalid? - assert_equal ["is reserved"], p.errors[:karma] + assert_equal ["is reserved"], p.errors[:karma] - p.karma = "Lifo" - assert p.valid? - end + p.karma = "Lifo" + assert p.valid? + ensure + Person.reset_callbacks(:validate) end end diff --git a/activemodel/test/cases/validations/format_validation_test.rb b/activemodel/test/cases/validations/format_validation_test.rb index e19e4bf7b3..e10089208a 100644 --- a/activemodel/test/cases/validations/format_validation_test.rb +++ b/activemodel/test/cases/validations/format_validation_test.rb @@ -8,9 +8,10 @@ require 'models/person' class PresenceValidationTest < ActiveModel::TestCase include ActiveModel::TestsDatabase - include ActiveModel::ValidationsRepairHelper - repair_validations(Topic) + def teardown + Topic.reset_callbacks(:validate) + end def test_validate_format Topic.validates_format_of(:title, :content, :with => /^Validation\smacros \w+!$/, :message => "is bad data") @@ -100,28 +101,18 @@ class PresenceValidationTest < ActiveModel::TestCase assert_raise(ArgumentError) { Topic.validates_format_of(:title, :without => "clearly not a regexp") } end - def test_validates_format_of_with_custom_error_using_quotes - repair_validations(Developer) do - Developer.validates_format_of :name, :with => /^(A-Z*)$/, :message=> "format 'single' and \"double\" quotes" - d = Developer.new - d.name = d.name_confirmation = "John 32" - assert !d.valid? - assert_equal ["format 'single' and \"double\" quotes"], d.errors[:name] - end - end - def test_validates_format_of_for_ruby_class - repair_validations(Person) do - Person.validates_format_of :karma, :with => /\A\d+\Z/ + Person.validates_format_of :karma, :with => /\A\d+\Z/ - p = Person.new - p.karma = "Pixies" - assert p.invalid? + p = Person.new + p.karma = "Pixies" + assert p.invalid? - assert_equal ["is invalid"], p.errors[:karma] + assert_equal ["is invalid"], p.errors[:karma] - p.karma = "1234" - assert p.valid? - end + p.karma = "1234" + assert p.valid? + ensure + Person.reset_callbacks(:validate) end end diff --git a/activemodel/test/cases/validations/i18n_validation_test.rb b/activemodel/test/cases/validations/i18n_validation_test.rb index 68b1b27f75..2717a09331 100644 --- a/activemodel/test/cases/validations/i18n_validation_test.rb +++ b/activemodel/test/cases/validations/i18n_validation_test.rb @@ -1,6 +1,5 @@ require "cases/helper" require 'cases/tests_database' - require 'models/person' class I18nValidationTest < ActiveModel::TestCase diff --git a/activemodel/test/cases/validations/inclusion_validation_test.rb b/activemodel/test/cases/validations/inclusion_validation_test.rb index bc1b0365d2..6b2bcd9c60 100644 --- a/activemodel/test/cases/validations/inclusion_validation_test.rb +++ b/activemodel/test/cases/validations/inclusion_validation_test.rb @@ -8,9 +8,10 @@ require 'models/person' class InclusionValidationTest < ActiveModel::TestCase include ActiveModel::TestsDatabase - include ActiveModel::ValidationsRepairHelper - repair_validations(Topic) + def teardown + Topic.reset_callbacks(:validate) + end def test_validates_inclusion_of Topic.validates_inclusion_of( :title, :in => %w( a b c d e f g ) ) @@ -53,28 +54,18 @@ class InclusionValidationTest < ActiveModel::TestCase assert_equal ["option uhoh is not in the list"], t.errors[:title] end - def test_validates_inclusion_of_with_custom_error_using_quotes - repair_validations(Developer) do - Developer.validates_inclusion_of :salary, :in => 1000..80000, :message=> "This string contains 'single' and \"double\" quotes" - d = Developer.new - d.salary = "90,000" - assert !d.valid? - assert_equal "This string contains 'single' and \"double\" quotes", d.errors[:salary].last - end - end - def test_validates_inclusion_of_for_ruby_class - repair_validations(Person) do - Person.validates_inclusion_of :karma, :in => %w( abe monkey ) + Person.validates_inclusion_of :karma, :in => %w( abe monkey ) - p = Person.new - p.karma = "Lifo" - assert p.invalid? + p = Person.new + p.karma = "Lifo" + assert p.invalid? - assert_equal ["is not included in the list"], p.errors[:karma] + assert_equal ["is not included in the list"], p.errors[:karma] - p.karma = "monkey" - assert p.valid? - end + p.karma = "monkey" + assert p.valid? + ensure + Person.reset_callbacks(:validate) end end diff --git a/activemodel/test/cases/validations/length_validation_test.rb b/activemodel/test/cases/validations/length_validation_test.rb index 2c97b762f1..f3ef5e648a 100644 --- a/activemodel/test/cases/validations/length_validation_test.rb +++ b/activemodel/test/cases/validations/length_validation_test.rb @@ -8,9 +8,10 @@ require 'models/person' class LengthValidationTest < ActiveModel::TestCase include ActiveModel::TestsDatabase - include ActiveModel::ValidationsRepairHelper - repair_validations(Topic) + def teardown + Topic.reset_callbacks(:validate) + end def test_validates_length_of_with_allow_nil Topic.validates_length_of( :title, :is => 5, :allow_nil=>true ) @@ -419,48 +420,18 @@ class LengthValidationTest < ActiveModel::TestCase assert_equal ["Your essay must be at least 5 words."], t.errors[:content] end - def test_validates_length_of_with_custom_too_long_using_quotes - repair_validations(Developer) do - Developer.validates_length_of :name, :maximum => 4, :too_long=> "This string contains 'single' and \"double\" quotes" - d = Developer.new - d.name = "Jeffrey" - assert !d.valid? - assert_equal ["This string contains 'single' and \"double\" quotes"], d.errors[:name] - end - end - - def test_validates_length_of_with_custom_too_short_using_quotes - repair_validations(Developer) do - Developer.validates_length_of :name, :minimum => 4, :too_short=> "This string contains 'single' and \"double\" quotes" - d = Developer.new - d.name = "Joe" - assert !d.valid? - assert_equal ["This string contains 'single' and \"double\" quotes"], d.errors[:name] - end - end - - def test_validates_length_of_with_custom_message_using_quotes - repair_validations(Developer) do - Developer.validates_length_of :name, :minimum => 4, :message=> "This string contains 'single' and \"double\" quotes" - d = Developer.new - d.name = "Joe" - assert !d.valid? - assert_equal ["This string contains 'single' and \"double\" quotes"], d.errors[:name] - end - end - def test_validates_length_of_for_ruby_class - repair_validations(Person) do - Person.validates_length_of :karma, :minimum => 5 + Person.validates_length_of :karma, :minimum => 5 - p = Person.new - p.karma = "Pix" - assert p.invalid? + p = Person.new + p.karma = "Pix" + assert p.invalid? - assert_equal ["is too short (minimum is 5 characters)"], p.errors[:karma] + assert_equal ["is too short (minimum is 5 characters)"], p.errors[:karma] - p.karma = "The Smiths" - assert p.valid? - end + p.karma = "The Smiths" + assert p.valid? + ensure + Person.reset_callbacks(:validate) end end diff --git a/activemodel/test/cases/validations/numericality_validation_test.rb b/activemodel/test/cases/validations/numericality_validation_test.rb index d3201966dc..75cd654f98 100644 --- a/activemodel/test/cases/validations/numericality_validation_test.rb +++ b/activemodel/test/cases/validations/numericality_validation_test.rb @@ -8,9 +8,10 @@ require 'models/person' class NumericalityValidationTest < ActiveModel::TestCase include ActiveModel::TestsDatabase - include ActiveModel::ValidationsRepairHelper - repair_validations(Topic) + def teardown + Topic.reset_callbacks(:validate) + end NIL = [nil] BLANK = ["", " ", " \t \r \n"] @@ -138,37 +139,19 @@ class NumericalityValidationTest < ActiveModel::TestCase assert_equal ["greater than 4"], topic.errors[:approved] end - def test_numericality_with_getter_method - repair_validations(Developer) do - Developer.validates_numericality_of( :salary ) - developer = Developer.new("name" => "michael", "salary" => nil) - developer.instance_eval("def salary; read_attribute('salary') ? read_attribute('salary') : 100000; end") - assert developer.valid? - end - end - - def test_numericality_with_allow_nil_and_getter_method - repair_validations(Developer) do - Developer.validates_numericality_of( :salary, :allow_nil => true) - developer = Developer.new("name" => "michael", "salary" => nil) - developer.instance_eval("def salary; read_attribute('salary') ? read_attribute('salary') : 100000; end") - assert developer.valid? - end - end - def test_validates_numericality_of_for_ruby_class - repair_validations(Person) do - Person.validates_numericality_of :karma, :allow_nil => false + Person.validates_numericality_of :karma, :allow_nil => false - p = Person.new - p.karma = "Pix" - assert p.invalid? + p = Person.new + p.karma = "Pix" + assert p.invalid? - assert_equal ["is not a number"], p.errors[:karma] + assert_equal ["is not a number"], p.errors[:karma] - p.karma = "1234" - assert p.valid? - end + p.karma = "1234" + assert p.valid? + ensure + Person.reset_callbacks(:validate) end private diff --git a/activemodel/test/cases/validations/presence_validation_test.rb b/activemodel/test/cases/validations/presence_validation_test.rb index 90b0951a77..8b9795a90c 100644 --- a/activemodel/test/cases/validations/presence_validation_test.rb +++ b/activemodel/test/cases/validations/presence_validation_test.rb @@ -9,9 +9,6 @@ require 'models/custom_reader' class PresenceValidationTest < ActiveModel::TestCase include ActiveModel::TestsDatabase - include ActiveModel::ValidationsRepairHelper - - repair_validations(Topic) def test_validate_presences Topic.validates_presence_of(:title, :content) @@ -30,43 +27,44 @@ class PresenceValidationTest < ActiveModel::TestCase t.content = "like stuff" assert t.save + ensure + Topic.reset_callbacks(:validate) end - # def test_validates_presence_of_with_custom_message_using_quotes - # repair_validations(Developer) do - # Developer.validates_presence_of :non_existent, :message=> "This string contains 'single' and \"double\" quotes" - # d = Developer.new - # d.name = "Joe" - # assert !d.valid? - # assert_equal ["This string contains 'single' and \"double\" quotes"], d.errors[:non_existent] - # end - # end + def test_validates_acceptance_of_with_custom_error_using_quotes + Person.validates_presence_of :karma, :message=> "This string contains 'single' and \"double\" quotes" + p = Person.new + assert !p.valid? + assert_equal "This string contains 'single' and \"double\" quotes", p.errors[:karma].last + ensure + Person.reset_callbacks(:validate) + end def test_validates_presence_of_for_ruby_class - repair_validations(Person) do - Person.validates_presence_of :karma + Person.validates_presence_of :karma - p = Person.new - assert p.invalid? + p = Person.new + assert p.invalid? - assert_equal ["can't be blank"], p.errors[:karma] + assert_equal ["can't be blank"], p.errors[:karma] - p.karma = "Cold" - assert p.valid? - end + p.karma = "Cold" + assert p.valid? + ensure + Person.reset_callbacks(:validate) end def test_validates_presence_of_for_ruby_class_with_custom_reader - repair_validations(Person) do - CustomReader.validates_presence_of :karma + CustomReader.validates_presence_of :karma - p = CustomReader.new - assert p.invalid? + p = CustomReader.new + assert p.invalid? - assert_equal ["can't be blank"], p.errors[:karma] + assert_equal ["can't be blank"], p.errors[:karma] - p[:karma] = "Cold" - assert p.valid? - end + p[:karma] = "Cold" + assert p.valid? + ensure + CustomReader.reset_callbacks(:validate) end end diff --git a/activemodel/test/cases/validations/with_validation_test.rb b/activemodel/test/cases/validations/with_validation_test.rb index 953ff8aa2d..9e9b925df2 100644 --- a/activemodel/test/cases/validations/with_validation_test.rb +++ b/activemodel/test/cases/validations/with_validation_test.rb @@ -6,9 +6,10 @@ require 'models/topic' class ValidatesWithTest < ActiveRecord::TestCase include ActiveModel::TestsDatabase - include ActiveModel::ValidationsRepairHelper - repair_validations(Topic) + def teardown + Topic.reset_callbacks(:validate) + end ERROR_MESSAGE = "Validation error from validator" OTHER_ERROR_MESSAGE = "Validation error from other validator" diff --git a/activemodel/test/cases/validations_test.rb b/activemodel/test/cases/validations_test.rb index 78565177d8..61910395b5 100644 --- a/activemodel/test/cases/validations_test.rb +++ b/activemodel/test/cases/validations_test.rb @@ -9,11 +9,12 @@ require 'models/custom_reader' class ValidationsTest < ActiveModel::TestCase include ActiveModel::TestsDatabase - include ActiveModel::ValidationsRepairHelper # Most of the tests mess with the validations of Topic, so lets repair it all the time. # Other classes we mess with will be dealt with in the specific tests - repair_validations(Topic) + def teardown + Topic.reset_callbacks(:validate) + end def test_single_field_validation r = Reply.new diff --git a/activemodel/test/models/person.rb b/activemodel/test/models/person.rb index fad29a51ec..c83d768379 100644 --- a/activemodel/test/models/person.rb +++ b/activemodel/test/models/person.rb @@ -2,7 +2,7 @@ class Person include ActiveModel::Validations extend ActiveModel::Translation - attr_accessor :title, :karma + attr_accessor :title, :karma, :salary end class Child < Person diff --git a/activerecord/lib/active_record/validations/associated.rb b/activerecord/lib/active_record/validations/associated.rb index 6e6f4df415..66b78682ad 100644 --- a/activerecord/lib/active_record/validations/associated.rb +++ b/activerecord/lib/active_record/validations/associated.rb @@ -2,7 +2,7 @@ module ActiveRecord module Validations class AssociatedValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - return if (value.is_a?(Array) ? value : [value]).compact.all?{ |r| r.valid? } + return if (value.is_a?(Array) ? value : [value]).collect{ |r| r.nil? || r.valid? }.all? record.errors.add(attribute, :invalid, :default => options[:message], :value => value) end end diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index 307320b964..243c05e665 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -62,9 +62,10 @@ unless ENV['FIXTURE_DEBUG'] end end +require "cases/validations_repair_helper" class ActiveSupport::TestCase include ActiveRecord::TestFixtures - include ActiveModel::ValidationsRepairHelper + include ActiveRecord::ValidationsRepairHelper self.fixture_path = FIXTURES_ROOT self.use_instantiated_fixtures = false diff --git a/activerecord/test/cases/validations_repair_helper.rb b/activerecord/test/cases/validations_repair_helper.rb new file mode 100644 index 0000000000..e04738d209 --- /dev/null +++ b/activerecord/test/cases/validations_repair_helper.rb @@ -0,0 +1,35 @@ +module ActiveRecord + module ValidationsRepairHelper + extend ActiveSupport::Concern + + module ClassMethods + def repair_validations(*model_classes) + setup do + @_stored_callbacks = {} + model_classes.each do |k| + @_stored_callbacks[k] = k._validate_callbacks.dup + end + end + teardown do + model_classes.each do |k| + k._validate_callbacks = @_stored_callbacks[k] + k.__update_callbacks(:validate) + end + end + end + end + + def repair_validations(*model_classes, &block) + @__stored_callbacks = {} + model_classes.each do |k| + @__stored_callbacks[k] = k._validate_callbacks.dup + end + return block.call + ensure + model_classes.each do |k| + k._validate_callbacks = @__stored_callbacks[k] + k.__update_callbacks(:validate) + end + end + end +end -- cgit v1.2.3 From a3c1db4e444baa8ae4f8d968fab786e03c93f413 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 28 Dec 2009 12:33:35 +0530 Subject: Add Model.lock and relation#lock now that arel has locking --- activerecord/CHANGELOG | 7 +++++++ .../lib/active_record/associations/association_collection.rb | 2 +- activerecord/lib/active_record/base.rb | 5 ++++- activerecord/lib/active_record/relation.rb | 11 +++++++++++ 4 files changed, 23 insertions(+), 2 deletions(-) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 3f4d77979b..2f0bdb6582 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,12 @@ *Edge* +* Add .lock finder method [Pratik Naik] + + User.lock.where(:name => 'lifo').to_a + + old_items = Item.where("age > 100") + old_items.locked.each {|i| .. } + * Add Model.from and association_collection#from finder methods [Pratik Naik] user = User.scoped diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index d2c61cdc78..b3a69ab0c0 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -21,7 +21,7 @@ module ActiveRecord construct_sql end - delegate :group, :order, :limit, :joins, :where, :preload, :eager_load, :from, :to => :scoped + delegate :group, :order, :limit, :joins, :where, :preload, :eager_load, :from, :lock, :to => :scoped def select(select = nil, &block) if block_given? diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 3b880ce17f..a887b0c571 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -652,7 +652,7 @@ module ActiveRecord #:nodoc: end end - delegate :select, :group, :order, :limit, :joins, :where, :preload, :eager_load, :from, :to => :scoped + delegate :select, :group, :order, :limit, :joins, :where, :preload, :eager_load, :from, :lock, :to => :scoped # A convenience wrapper for find(:first, *args). You can pass in all the # same arguments to this method as you can to find(:first). @@ -1573,6 +1573,9 @@ module ActiveRecord #:nodoc: offset(construct_offset(options[:offset], scope)). from(options[:from]) + lock = (scope && scope[:lock]) || options[:lock] + relation = relation.lock if lock.present? + relation = relation.readonly if options[:readonly] relation diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 530402bf5d..a7f62abe1d 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -55,6 +55,17 @@ module ActiveRecord orders.present? ? create_new_relation(@relation.order(orders)) : create_new_relation end + def lock(locks = true) + case locks + when String + create_new_relation(@relation.lock(locks)) + when TrueClass, NilClass + create_new_relation(@relation.lock) + else + create_new_relation + end + end + def reverse_order relation = create_new_relation relation.instance_variable_set(:@orders, nil) -- cgit v1.2.3 From d6b0a7d67c57470694843606d8079d887bd4579b Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 28 Dec 2009 12:44:43 +0530 Subject: Fix a typo in CHANGELOG --- activerecord/CHANGELOG | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 2f0bdb6582..1e3d7a51b9 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -5,7 +5,7 @@ User.lock.where(:name => 'lifo').to_a old_items = Item.where("age > 100") - old_items.locked.each {|i| .. } + old_items.lock.each {|i| .. } * Add Model.from and association_collection#from finder methods [Pratik Naik] -- cgit v1.2.3 From 9f4e98330b3a7f85a4bca91fca062106ffbee2bf Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 28 Dec 2009 12:57:35 +0530 Subject: Remove unused construct_finder_sql --- activerecord/lib/active_record/base.rb | 4 ---- .../associations/inner_join_association_test.rb | 28 +++++++++++----------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index a887b0c571..da1341a846 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1596,10 +1596,6 @@ module ActiveRecord #:nodoc: relation end - def construct_finder_sql(options, scope = scope(:find)) - construct_finder_arel(options, scope).to_sql - end - def construct_join(joins, scope) merged_joins = scope && scope[:joins] && joins ? merge_joins(scope[:joins], joins) : (joins || scope && scope[:joins]) case merged_joins diff --git a/activerecord/test/cases/associations/inner_join_association_test.rb b/activerecord/test/cases/associations/inner_join_association_test.rb index 5f08c40005..6220fab383 100644 --- a/activerecord/test/cases/associations/inner_join_association_test.rb +++ b/activerecord/test/cases/associations/inner_join_association_test.rb @@ -9,56 +9,56 @@ class InnerJoinAssociationTest < ActiveRecord::TestCase fixtures :authors, :posts, :comments, :categories, :categories_posts, :categorizations def test_construct_finder_sql_creates_inner_joins - sql = Author.send(:construct_finder_sql, :joins => :posts) + sql = Author.joins(:posts).to_sql assert_match /INNER JOIN .?posts.? ON .?posts.?.author_id = authors.id/, sql end def test_construct_finder_sql_cascades_inner_joins - sql = Author.send(:construct_finder_sql, :joins => {:posts => :comments}) + sql = Author.joins(:posts => :comments).to_sql assert_match /INNER JOIN .?posts.? ON .?posts.?.author_id = authors.id/, sql assert_match /INNER JOIN .?comments.? ON .?comments.?.post_id = posts.id/, sql end def test_construct_finder_sql_inner_joins_through_associations - sql = Author.send(:construct_finder_sql, :joins => :categorized_posts) + sql = Author.joins(:categorized_posts).to_sql assert_match /INNER JOIN .?categorizations.?.*INNER JOIN .?posts.?/, sql end def test_construct_finder_sql_applies_association_conditions - sql = Author.send(:construct_finder_sql, :joins => :categories_like_general, :conditions => "TERMINATING_MARKER") + sql = Author.joins(:categories_like_general).where("TERMINATING_MARKER").to_sql assert_match /INNER JOIN .?categories.? ON.*AND.*.?General.?(.|\n)*TERMINATING_MARKER/, sql end def test_construct_finder_sql_applies_aliases_tables_on_association_conditions - result = Author.find(:all, :joins => [:thinking_posts, :welcome_posts]) + result = Author.joins(:thinking_posts, :welcome_posts).to_a assert_equal authors(:david), result.first end def test_construct_finder_sql_unpacks_nested_joins - sql = Author.send(:construct_finder_sql, :joins => {:posts => [[:comments]]}) + sql = Author.joins(:posts => [[:comments]]).to_sql assert_no_match /inner join.*inner join.*inner join/i, sql, "only two join clauses should be present" assert_match /INNER JOIN .?posts.? ON .?posts.?.author_id = authors.id/, sql assert_match /INNER JOIN .?comments.? ON .?comments.?.post_id = .?posts.?.id/, sql end def test_construct_finder_sql_ignores_empty_joins_hash - sql = Author.send(:construct_finder_sql, :joins => {}) + sql = Author.joins({}).to_sql assert_no_match /JOIN/i, sql end def test_construct_finder_sql_ignores_empty_joins_array - sql = Author.send(:construct_finder_sql, :joins => []) + sql = Author.joins([]).to_sql assert_no_match /JOIN/i, sql end def test_find_with_implicit_inner_joins_honors_readonly_without_select - authors = Author.find(:all, :joins => :posts) + authors = Author.joins(:posts).to_a assert !authors.empty?, "expected authors to be non-empty" assert authors.all? {|a| a.readonly? }, "expected all authors to be readonly" end def test_find_with_implicit_inner_joins_honors_readonly_with_select - authors = Author.find(:all, :select => 'authors.*', :joins => :posts) + authors = Author.joins(:posts).select('authors.*').to_a assert !authors.empty?, "expected authors to be non-empty" assert authors.all? {|a| !a.readonly? }, "expected no authors to be readonly" end @@ -70,23 +70,23 @@ class InnerJoinAssociationTest < ActiveRecord::TestCase end def test_find_with_implicit_inner_joins_does_not_set_associations - authors = Author.find(:all, :select => 'authors.*', :joins => :posts) + authors = Author.joins(:posts).select('authors.*') assert !authors.empty?, "expected authors to be non-empty" assert authors.all? {|a| !a.send(:instance_variable_names).include?("@posts")}, "expected no authors to have the @posts association loaded" end def test_count_honors_implicit_inner_joins - real_count = Author.find(:all).sum{|a| a.posts.count } + real_count = Author.scoped.to_a.sum{|a| a.posts.count } assert_equal real_count, Author.count(:joins => :posts), "plain inner join count should match the number of referenced posts records" end def test_calculate_honors_implicit_inner_joins - real_count = Author.find(:all).sum{|a| a.posts.count } + real_count = Author.scoped.to_a.sum{|a| a.posts.count } assert_equal real_count, Author.calculate(:count, 'authors.id', :joins => :posts), "plain inner join count should match the number of referenced posts records" end def test_calculate_honors_implicit_inner_joins_and_distinct_and_conditions - real_count = Author.find(:all).select {|a| a.posts.any? {|p| p.title =~ /^Welcome/} }.length + real_count = Author.scoped.to_a.select {|a| a.posts.any? {|p| p.title =~ /^Welcome/} }.length authors_with_welcoming_post_titles = Author.calculate(:count, 'authors.id', :joins => :posts, :distinct => true, :conditions => "posts.title like 'Welcome%'") assert_equal real_count, authors_with_welcoming_post_titles, "inner join and conditions should have only returned authors posting titles starting with 'Welcome'" end -- cgit v1.2.3 From 92c982d973c3e3125982309ff8bb6c22608696c5 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 28 Dec 2009 14:20:54 +0530 Subject: Relation#readonly(false) should toggle the readonly flag --- activerecord/lib/active_record/relation.rb | 13 +++++++++---- .../test/cases/associations/inner_join_association_test.rb | 2 +- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index a7f62abe1d..ff10df96f2 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -35,12 +35,17 @@ module ActiveRecord create_new_relation(@relation, @readonly, @associations_to_preload, @eager_load_associations + Array.wrap(associations)) end - def readonly - create_new_relation(@relation, true) + def readonly(status = true) + status.nil? ? create_new_relation : create_new_relation(@relation, status) end def select(selects) - selects.present? ? create_new_relation(@relation.project(selects)) : create_new_relation + if selects.present? + frozen = @relation.joins(relation).present? ? false : @readonly + create_new_relation(@relation.project(selects), frozen) + else + create_new_relation + end end def from(from) @@ -106,7 +111,7 @@ module ActiveRecord @relation.join(join, join_type) end - create_new_relation(join_relation) + create_new_relation(join_relation, true) end def where(*args) diff --git a/activerecord/test/cases/associations/inner_join_association_test.rb b/activerecord/test/cases/associations/inner_join_association_test.rb index 6220fab383..18a1cd3cd0 100644 --- a/activerecord/test/cases/associations/inner_join_association_test.rb +++ b/activerecord/test/cases/associations/inner_join_association_test.rb @@ -64,7 +64,7 @@ class InnerJoinAssociationTest < ActiveRecord::TestCase end def test_find_with_implicit_inner_joins_honors_readonly_false - authors = Author.find(:all, :joins => :posts, :readonly => false) + authors = Author.joins(:posts).readonly(false).to_a assert !authors.empty?, "expected authors to be non-empty" assert authors.all? {|a| !a.readonly? }, "expected no authors to be readonly" end -- cgit v1.2.3 From b95cc72429f83304b8e882c3637dfb3135a571ed Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 28 Dec 2009 14:24:52 +0530 Subject: Raise ArgumentError when trying to merge relations of different classes --- activerecord/lib/active_record/relation.rb | 2 ++ activerecord/test/cases/relations_test.rb | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index ff10df96f2..ef480cfb29 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -13,6 +13,8 @@ module ActiveRecord end def merge(r) + raise ArgumentError, "Cannot merge a #{r.klass.name} relation with #{@klass.name} relation" if r.klass != @klass + joins(r.relation.joins(r.relation)). group(r.send(:group_clauses).join(', ')). order(r.send(:order_clauses).join(', ')). diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 61fcc7ca46..cf6708cc8b 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -353,4 +353,9 @@ class RelationTest < ActiveRecord::TestCase assert_queries(2) { assert posts.first.author } end end + + def test_invalid_merge + assert_raises(ArgumentError) { Post.scoped & Developer.scoped } + end + end -- cgit v1.2.3 From 5156507e13bb17f1852576acfd921a39e06de175 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 28 Dec 2009 14:33:56 +0530 Subject: Remove locking related unused code --- activerecord/lib/active_record/base.rb | 8 -------- .../connection_adapters/abstract/database_statements.rb | 12 ------------ .../lib/active_record/connection_adapters/sqlite_adapter.rb | 6 ------ 3 files changed, 26 deletions(-) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index da1341a846..30f0207aab 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1702,14 +1702,6 @@ module ActiveRecord #:nodoc: o.is_a?(Array) && o.all?{|obj| obj.is_a?(String)} end - # The optional scope argument is for the current :find scope. - # The :lock option has precedence over a scoped :lock. - def add_lock!(sql, options, scope = :auto) - scope = scope(:find) if :auto == scope - options = options.reverse_merge(:lock => scope[:lock]) if scope - connection.add_lock!(sql, options) - end - def type_condition(table_alias=nil) quoted_table_alias = self.connection.quote_table_name(table_alias || table_name) quoted_inheritance_column = connection.quote_column_name(inheritance_column) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index be89873632..027d736484 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -181,18 +181,6 @@ module ActiveRecord # done if the transaction block raises an exception or returns false. def rollback_db_transaction() end - # Appends a locking clause to an SQL statement. - # This method *modifies* the +sql+ parameter. - # # SELECT * FROM suppliers FOR UPDATE - # add_lock! 'SELECT * FROM suppliers', :lock => true - # add_lock! 'SELECT * FROM suppliers', :lock => ' FOR UPDATE' - def add_lock!(sql, options) - case lock = options[:lock] - when true; sql << ' FOR UPDATE' - when String; sql << " #{lock}" - end - end - def default_sequence_name(table, column) nil end diff --git a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb index c9c2892ba4..78b897add6 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb @@ -183,12 +183,6 @@ module ActiveRecord catch_schema_changes { @connection.rollback } end - # SELECT ... FOR UPDATE is redundant since the table is locked. - def add_lock!(sql, options) #:nodoc: - sql - end - - # SCHEMA STATEMENTS ======================================== def tables(name = nil) #:nodoc: -- cgit v1.2.3 From 02207dc02c10f2b00a84594962d7bf6ffcf539a9 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 28 Dec 2009 16:16:21 +0530 Subject: Add Model.readonly and association_collection#readonly finder method --- activerecord/CHANGELOG | 5 ++++ .../associations/association_collection.rb | 2 +- activerecord/lib/active_record/base.rb | 2 +- activerecord/lib/active_record/relation.rb | 2 +- activerecord/test/cases/readonly_test.rb | 33 +++++++++++----------- 5 files changed, 25 insertions(+), 19 deletions(-) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 1e3d7a51b9..93c4696eca 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,10 @@ *Edge* +* Add Model.readonly and association_collection#readonly finder method. [Pratik Naik] + + Post.readonly.to_a # Load all posts in readonly mode + @user.items.readonly(false).to_a # Load all the user items in writable mode + * Add .lock finder method [Pratik Naik] User.lock.where(:name => 'lifo').to_a diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index b3a69ab0c0..8bc64a3a93 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -21,7 +21,7 @@ module ActiveRecord construct_sql end - delegate :group, :order, :limit, :joins, :where, :preload, :eager_load, :from, :lock, :to => :scoped + delegate :group, :order, :limit, :joins, :where, :preload, :eager_load, :from, :lock, :readonly, :to => :scoped def select(select = nil, &block) if block_given? diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 30f0207aab..a9b011dd76 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -652,7 +652,7 @@ module ActiveRecord #:nodoc: end end - delegate :select, :group, :order, :limit, :joins, :where, :preload, :eager_load, :from, :lock, :to => :scoped + delegate :select, :group, :order, :limit, :joins, :where, :preload, :eager_load, :from, :lock, :readonly, :to => :scoped # A convenience wrapper for find(:first, *args). You can pass in all the # same arguments to this method as you can to find(:first). diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index ef480cfb29..93f7b74c68 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -1,7 +1,7 @@ module ActiveRecord class Relation delegate :to_sql, :to => :relation - delegate :length, :collect, :map, :each, :to => :to_a + delegate :length, :collect, :map, :each, :all?, :to => :to_a attr_reader :relation, :klass, :associations_to_preload, :eager_load_associations def initialize(klass, relation, readonly = false, preload = [], eager_load = []) diff --git a/activerecord/test/cases/readonly_test.rb b/activerecord/test/cases/readonly_test.rb index b921cbdc9c..d2ef4fd6d2 100644 --- a/activerecord/test/cases/readonly_test.rb +++ b/activerecord/test/cases/readonly_test.rb @@ -33,19 +33,20 @@ class ReadOnlyTest < ActiveRecord::TestCase def test_find_with_readonly_option Developer.find(:all).each { |d| assert !d.readonly? } - Developer.find(:all, :readonly => false).each { |d| assert !d.readonly? } - Developer.find(:all, :readonly => true).each { |d| assert d.readonly? } + Developer.readonly(false).each { |d| assert !d.readonly? } + Developer.readonly(true).each { |d| assert d.readonly? } + Developer.readonly.each { |d| assert d.readonly? } end def test_find_with_joins_option_implies_readonly # Blank joins don't count. - Developer.find(:all, :joins => ' ').each { |d| assert !d.readonly? } - Developer.find(:all, :joins => ' ', :readonly => false).each { |d| assert !d.readonly? } + Developer.joins(' ').each { |d| assert !d.readonly? } + Developer.joins(' ').readonly(false).each { |d| assert !d.readonly? } # Others do. - Developer.find(:all, :joins => ', projects').each { |d| assert d.readonly? } - Developer.find(:all, :joins => ', projects', :readonly => false).each { |d| assert !d.readonly? } + Developer.joins(', projects').each { |d| assert d.readonly? } + Developer.joins(', projects').readonly(false).each { |d| assert !d.readonly? } end @@ -54,7 +55,7 @@ class ReadOnlyTest < ActiveRecord::TestCase assert !dev.projects.empty? assert dev.projects.all?(&:readonly?) assert dev.projects.find(:all).all?(&:readonly?) - assert dev.projects.find(:all, :readonly => true).all?(&:readonly?) + assert dev.projects.readonly(true).all?(&:readonly?) end def test_has_many_find_readonly @@ -62,7 +63,7 @@ class ReadOnlyTest < ActiveRecord::TestCase assert !post.comments.empty? assert !post.comments.any?(&:readonly?) assert !post.comments.find(:all).any?(&:readonly?) - assert post.comments.find(:all, :readonly => true).all?(&:readonly?) + assert post.comments.readonly(true).all?(&:readonly?) end def test_has_many_with_through_is_not_implicitly_marked_readonly @@ -73,14 +74,14 @@ class ReadOnlyTest < ActiveRecord::TestCase def test_readonly_scoping Post.with_scope(:find => { :conditions => '1=1' }) do assert !Post.find(1).readonly? - assert Post.find(1, :readonly => true).readonly? - assert !Post.find(1, :readonly => false).readonly? + assert Post.readonly(true).find(1).readonly? + assert !Post.readonly(false).find(1).readonly? end Post.with_scope(:find => { :joins => ' ' }) do assert !Post.find(1).readonly? - assert Post.find(1, :readonly => true).readonly? - assert !Post.find(1, :readonly => false).readonly? + assert Post.readonly.find(1).readonly? + assert !Post.readonly(false).find(1).readonly? end # Oracle barfs on this because the join includes unqualified and @@ -88,15 +89,15 @@ class ReadOnlyTest < ActiveRecord::TestCase unless current_adapter?(:OracleAdapter) Post.with_scope(:find => { :joins => ', developers' }) do assert Post.find(1).readonly? - assert Post.find(1, :readonly => true).readonly? - assert !Post.find(1, :readonly => false).readonly? + assert Post.readonly.find(1).readonly? + assert !Post.readonly(false).find(1).readonly? end end Post.with_scope(:find => { :readonly => true }) do assert Post.find(1).readonly? - assert Post.find(1, :readonly => true).readonly? - assert !Post.find(1, :readonly => false).readonly? + assert Post.readonly.find(1).readonly? + assert !Post.readonly(false).find(1).readonly? end end -- cgit v1.2.3 From aefa975fdde01b1beaacbe065fe4b2bad69295d3 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 28 Dec 2009 16:20:40 +0530 Subject: Remove the todo note for arel#lock --- activerecord/lib/active_record/base.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index a9b011dd76..3135234706 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1560,7 +1560,6 @@ module ActiveRecord #:nodoc: end def construct_finder_arel(options = {}, scope = scope(:find)) - # TODO add lock to Arel validate_find_options(options) relation = arel_table. -- cgit v1.2.3 From 8f5d9eb0e2d05c5ca10c313bb47dbcab335c6fa7 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 28 Dec 2009 18:38:28 +0530 Subject: Add Relation#count --- activerecord/CHANGELOG | 7 +++++ activerecord/lib/active_record/relation.rb | 45 ++++++++++++++++++++++++++++++ activerecord/test/cases/relations_test.rb | 30 ++++++++++++++++++++ 3 files changed, 82 insertions(+) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 93c4696eca..5af66c3519 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,12 @@ *Edge* +* Add Relation#count. [Pratik Naik] + + legends = People.where("age > 100") + legends.count + legends.count(:age, :distinct => true) + legends.select('id').count + * Add Model.readonly and association_collection#readonly finder method. [Pratik Naik] Post.readonly.to_a # Load all posts in readonly mode diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 93f7b74c68..cb252eea70 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -204,6 +204,20 @@ module ActiveRecord end end + def count(*args) + column_name, options = construct_count_options_from_args(*args) + distinct = options[:distinct] ? true : false + + column = if @klass.column_names.include?(column_name.to_s) + Arel::Attribute.new(@relation.table, column_name) + else + Arel::SqlLiteral.new(column_name == :all ? "*" : column_name.to_s) + end + + relation = select(column.count(distinct)) + @klass.connection.select_value(relation.to_sql).to_i + end + def destroy_all to_a.each {|object| object.destroy} reset @@ -337,5 +351,36 @@ module ActiveRecord }.join(',') end + def construct_count_options_from_args(*args) + options = {} + column_name = :all + + # We need to handle + # count() + # count(:column_name=:all) + # count(options={}) + # count(column_name=:all, options={}) + # selects specified by scopes + + # TODO : relation.projections only works when .select() was last in the chain. Fix it! + case args.size + when 0 + column_name = @relation.send(:select_clauses).join(', ') if @relation.respond_to?(:projections) && @relation.projections.present? + when 1 + if args[0].is_a?(Hash) + column_name = @relation.send(:select_clauses).join(', ') if @relation.respond_to?(:projections) && @relation.projections.present? + options = args[0] + else + column_name = args[0] + end + when 2 + column_name, options = args + else + raise ArgumentError, "Unexpected parameters passed to count(): #{args.inspect}" + end + + [column_name || :all, options] + end + end end diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index cf6708cc8b..ded4f2f479 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -358,4 +358,34 @@ class RelationTest < ActiveRecord::TestCase assert_raises(ArgumentError) { Post.scoped & Developer.scoped } end + def test_count + posts = Post.scoped + + assert_equal 7, posts.count + assert_equal 7, posts.count(:all) + assert_equal 7, posts.count(:id) + + assert_equal 1, posts.where('comments_count > 1').count + assert_equal 5, posts.where(:comments_count => 0).count + end + + def test_count_with_distinct + posts = Post.scoped + + assert_equal 3, posts.count(:comments_count, :distinct => true) + assert_equal 7, posts.count(:comments_count, :distinct => false) + + assert_equal 3, posts.select(:comments_count).count(:distinct => true) + assert_equal 7, posts.select(:comments_count).count(:distinct => false) + end + + def test_count_explicit_columns + Post.update_all(:comments_count => nil) + posts = Post.scoped + + assert_equal 7, posts.select('comments_count').count('id') + assert_equal 0, posts.select('comments_count').count + assert_equal 0, posts.count(:comments_count) + assert_equal 0, posts.count('comments_count') + end end -- cgit v1.2.3 From e8ca22d129c1e93574e770dd69dc964be6686469 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 28 Dec 2009 19:12:15 +0530 Subject: Move Relation calculation methods to a separate module --- activerecord/lib/active_record.rb | 1 + activerecord/lib/active_record/relation.rb | 46 +------------------ .../lib/active_record/relational_calculations.rb | 52 ++++++++++++++++++++++ 3 files changed, 54 insertions(+), 45 deletions(-) create mode 100644 activerecord/lib/active_record/relational_calculations.rb diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb index 2cfd528f2c..7031c67539 100644 --- a/activerecord/lib/active_record.rb +++ b/activerecord/lib/active_record.rb @@ -48,6 +48,7 @@ module ActiveRecord autoload :Attributes autoload :AutosaveAssociation autoload :Relation + autoload :RelationalCalculations autoload :Base autoload :Batches autoload :Calculations diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index cb252eea70..291e97ff83 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -4,6 +4,7 @@ module ActiveRecord delegate :length, :collect, :map, :each, :all?, :to => :to_a attr_reader :relation, :klass, :associations_to_preload, :eager_load_associations + include RelationalCalculations def initialize(klass, relation, readonly = false, preload = [], eager_load = []) @klass, @relation = klass, relation @readonly = readonly @@ -204,20 +205,6 @@ module ActiveRecord end end - def count(*args) - column_name, options = construct_count_options_from_args(*args) - distinct = options[:distinct] ? true : false - - column = if @klass.column_names.include?(column_name.to_s) - Arel::Attribute.new(@relation.table, column_name) - else - Arel::SqlLiteral.new(column_name == :all ? "*" : column_name.to_s) - end - - relation = select(column.count(distinct)) - @klass.connection.select_value(relation.to_sql).to_i - end - def destroy_all to_a.each {|object| object.destroy} reset @@ -351,36 +338,5 @@ module ActiveRecord }.join(',') end - def construct_count_options_from_args(*args) - options = {} - column_name = :all - - # We need to handle - # count() - # count(:column_name=:all) - # count(options={}) - # count(column_name=:all, options={}) - # selects specified by scopes - - # TODO : relation.projections only works when .select() was last in the chain. Fix it! - case args.size - when 0 - column_name = @relation.send(:select_clauses).join(', ') if @relation.respond_to?(:projections) && @relation.projections.present? - when 1 - if args[0].is_a?(Hash) - column_name = @relation.send(:select_clauses).join(', ') if @relation.respond_to?(:projections) && @relation.projections.present? - options = args[0] - else - column_name = args[0] - end - when 2 - column_name, options = args - else - raise ArgumentError, "Unexpected parameters passed to count(): #{args.inspect}" - end - - [column_name || :all, options] - end - end end diff --git a/activerecord/lib/active_record/relational_calculations.rb b/activerecord/lib/active_record/relational_calculations.rb new file mode 100644 index 0000000000..10eb992167 --- /dev/null +++ b/activerecord/lib/active_record/relational_calculations.rb @@ -0,0 +1,52 @@ +module ActiveRecord + module RelationalCalculations + + def count(*args) + column_name, options = construct_count_options_from_args(*args) + distinct = options[:distinct] ? true : false + + column = if @klass.column_names.include?(column_name.to_s) + Arel::Attribute.new(@relation.table, column_name) + else + Arel::SqlLiteral.new(column_name == :all ? "*" : column_name.to_s) + end + + relation = select(column.count(distinct)) + @klass.connection.select_value(relation.to_sql).to_i + end + + private + + def construct_count_options_from_args(*args) + options = {} + column_name = :all + + # We need to handle + # count() + # count(:column_name=:all) + # count(options={}) + # count(column_name=:all, options={}) + # selects specified by scopes + + # TODO : relation.projections only works when .select() was last in the chain. Fix it! + case args.size + when 0 + column_name = @relation.send(:select_clauses).join(', ') if @relation.respond_to?(:projections) && @relation.projections.present? + when 1 + if args[0].is_a?(Hash) + column_name = @relation.send(:select_clauses).join(', ') if @relation.respond_to?(:projections) && @relation.projections.present? + options = args[0] + else + column_name = args[0] + end + when 2 + column_name, options = args + else + raise ArgumentError, "Unexpected parameters passed to count(): #{args.inspect}" + end + + [column_name || :all, options] + end + + end +end -- cgit v1.2.3 From fc85c665271578e55e7fe90a721ca1533289d923 Mon Sep 17 00:00:00 2001 From: George Ogata Date: Thu, 26 Nov 2009 00:05:57 -0500 Subject: Set inverse for #replace on a has_one association. [#3513 state:resolved] Signed-off-by: Eloy Duran --- .../lib/active_record/associations/has_one_association.rb | 1 + .../test/cases/associations/inverse_associations_test.rb | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index b85a40b2e5..081d6233c4 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -57,6 +57,7 @@ module ActiveRecord @target = (AssociationProxy === obj ? obj.target : obj) end + set_inverse_instance(obj, @owner) @loaded = true unless @owner.new_record? or obj.nil? or dont_save diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index 47f83db112..ee360dff10 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -135,6 +135,21 @@ class InverseHasOneTests < ActiveRecord::TestCase assert_equal m.name, f.man.name, "Name of man should be the same after changes to newly-created-child-owned instance" end + def test_parent_instance_should_be_shared_with_replaced_child + man = Man.find(:first) + old_face = man.face + new_face = Face.new + + assert_not_nil man.face + man.face.replace(new_face) + + assert_equal man.name, new_face.man.name, "Name of man should be the same before changes to parent instance" + man.name = 'Bongo' + assert_equal man.name, new_face.man.name, "Name of man should be the same after changes to parent instance" + new_face.man.name = 'Mungo' + assert_equal man.name, new_face.man.name, "Name of man should be the same after changes to replaced-parent-owned instance" + end + def test_trying_to_use_inverses_that_dont_exist_should_raise_an_error assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { Man.find(:first).dirty_face } end -- cgit v1.2.3 From 6c8c85bc1eaf1639ea0df5f356e7105c74d128b2 Mon Sep 17 00:00:00 2001 From: Murray Steele Date: Thu, 17 Dec 2009 11:38:44 +0000 Subject: Add more tests for the various ways we can assign objects to associations. [#3513 state:resolved] Get rid of a duplicate set_inverse_instance call if you use new_record(true) (e.g. you want to replace the existing instance). Signed-off-by: Eloy Duran --- .../associations/has_one_association.rb | 3 +- .../associations/inverse_associations_test.rb | 170 +++++++++++++++++++-- 2 files changed, 160 insertions(+), 13 deletions(-) diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index 081d6233c4..ea769fd48b 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -121,10 +121,9 @@ module ActiveRecord else record[@reflection.primary_key_name] = @owner.id unless @owner.new_record? self.target = record + set_inverse_instance(record, @owner) end - set_inverse_instance(record, @owner) - record end diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index ee360dff10..c3d0d61cec 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -135,19 +135,84 @@ class InverseHasOneTests < ActiveRecord::TestCase assert_equal m.name, f.man.name, "Name of man should be the same after changes to newly-created-child-owned instance" end - def test_parent_instance_should_be_shared_with_replaced_child - man = Man.find(:first) - old_face = man.face - new_face = Face.new + def test_parent_instance_should_be_shared_with_newly_created_child_via_bang_method + m = Man.find(:first) + f = m.face.create!(:description => 'haunted') + assert_not_nil f.man + assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance" + m.name = 'Bongo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to parent instance" + f.man.name = 'Mungo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to newly-created-child-owned instance" + end + + def test_parent_instance_should_be_shared_with_newly_built_child_when_we_dont_replace_existing + m = Man.find(:first) + f = m.build_face({:description => 'haunted'}, false) + assert_not_nil f.man + assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance" + m.name = 'Bongo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to parent instance" + f.man.name = 'Mungo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to just-built-child-owned instance" + end + + def test_parent_instance_should_be_shared_with_newly_created_child_when_we_dont_replace_existing + m = Man.find(:first) + f = m.create_face({:description => 'haunted'}, false) + assert_not_nil f.man + assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance" + m.name = 'Bongo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to parent instance" + f.man.name = 'Mungo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to newly-created-child-owned instance" + end + + def test_parent_instance_should_be_shared_with_newly_created_child_via_bang_method_when_we_dont_replace_existing + m = Man.find(:first) + f = m.face.create!({:description => 'haunted'}, false) + assert_not_nil f.man + assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance" + m.name = 'Bongo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to parent instance" + f.man.name = 'Mungo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to newly-created-child-owned instance" + end - assert_not_nil man.face - man.face.replace(new_face) + def test_parent_instance_should_be_shared_with_replaced_via_accessor_child + m = Man.find(:first) + f = Face.new(:description => 'haunted') + m.face = f + assert_not_nil f.man + assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance" + m.name = 'Bongo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to parent instance" + f.man.name = 'Mungo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to replaced-child-owned instance" + end - assert_equal man.name, new_face.man.name, "Name of man should be the same before changes to parent instance" - man.name = 'Bongo' - assert_equal man.name, new_face.man.name, "Name of man should be the same after changes to parent instance" - new_face.man.name = 'Mungo' - assert_equal man.name, new_face.man.name, "Name of man should be the same after changes to replaced-parent-owned instance" + def test_parent_instance_should_be_shared_with_replaced_via_method_child + m = Man.find(:first) + f = Face.new(:description => 'haunted') + m.face.replace(f) + assert_not_nil f.man + assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance" + m.name = 'Bongo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to parent instance" + f.man.name = 'Mungo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to replaced-child-owned instance" + end + + def test_parent_instance_should_be_shared_with_replaced_via_method_child_when_we_dont_replace_existing + m = Man.find(:first) + f = Face.new(:description => 'haunted') + m.face.replace(f, false) + assert_not_nil f.man + assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance" + m.name = 'Bongo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to parent instance" + f.man.name = 'Mungo' + assert_equal m.name, f.man.name, "Name of man should be the same after changes to replaced-child-owned instance" end def test_trying_to_use_inverses_that_dont_exist_should_raise_an_error @@ -204,6 +269,18 @@ class InverseHasManyTests < ActiveRecord::TestCase assert_equal m.name, i.man.name, "Name of man should be the same after changes to just-built-child-owned instance" end + def test_parent_instance_should_be_shared_with_newly_block_style_built_child + m = Man.find(:first) + i = m.interests.build {|ii| ii.topic = 'Industrial Revolution Re-enactment'} + assert_not_nil i.topic, "Child attributes supplied to build via blocks should be populated" + assert_not_nil i.man + assert_equal m.name, i.man.name, "Name of man should be the same before changes to parent instance" + m.name = 'Bongo' + assert_equal m.name, i.man.name, "Name of man should be the same after changes to parent instance" + i.man.name = 'Mungo' + assert_equal m.name, i.man.name, "Name of man should be the same after changes to just-built-child-owned instance" + end + def test_parent_instance_should_be_shared_with_newly_created_child m = Man.find(:first) i = m.interests.create(:topic => 'Industrial Revolution Re-enactment') @@ -215,6 +292,29 @@ class InverseHasManyTests < ActiveRecord::TestCase assert_equal m.name, i.man.name, "Name of man should be the same after changes to newly-created-child-owned instance" end + def test_parent_instance_should_be_shared_with_newly_created_via_bang_method_child + m = Man.find(:first) + i = m.interests.create!(:topic => 'Industrial Revolution Re-enactment') + assert_not_nil i.man + assert_equal m.name, i.man.name, "Name of man should be the same before changes to parent instance" + m.name = 'Bongo' + assert_equal m.name, i.man.name, "Name of man should be the same after changes to parent instance" + i.man.name = 'Mungo' + assert_equal m.name, i.man.name, "Name of man should be the same after changes to newly-created-child-owned instance" + end + + def test_parent_instance_should_be_shared_with_newly_block_style_created_child + m = Man.find(:first) + i = m.interests.create {|ii| ii.topic = 'Industrial Revolution Re-enactment'} + assert_not_nil i.topic, "Child attributes supplied to create via blocks should be populated" + assert_not_nil i.man + assert_equal m.name, i.man.name, "Name of man should be the same before changes to parent instance" + m.name = 'Bongo' + assert_equal m.name, i.man.name, "Name of man should be the same after changes to parent instance" + i.man.name = 'Mungo' + assert_equal m.name, i.man.name, "Name of man should be the same after changes to newly-created-child-owned instance" + end + def test_parent_instance_should_be_shared_with_poked_in_child m = Man.find(:first) i = Interest.create(:topic => 'Industrial Revolution Re-enactment') @@ -227,6 +327,30 @@ class InverseHasManyTests < ActiveRecord::TestCase assert_equal m.name, i.man.name, "Name of man should be the same after changes to newly-created-child-owned instance" end + def test_parent_instance_should_be_shared_with_replaced_via_accessor_children + m = Man.find(:first) + i = Interest.new(:topic => 'Industrial Revolution Re-enactment') + m.interests = [i] + assert_not_nil i.man + assert_equal m.name, i.man.name, "Name of man should be the same before changes to parent instance" + m.name = 'Bongo' + assert_equal m.name, i.man.name, "Name of man should be the same after changes to parent instance" + i.man.name = 'Mungo' + assert_equal m.name, i.man.name, "Name of man should be the same after changes to replaced-child-owned instance" + end + + def test_parent_instance_should_be_shared_with_replaced_via_method_children + m = Man.find(:first) + i = Interest.new(:topic => 'Industrial Revolution Re-enactment') + m.interests.replace([i]) + assert_not_nil i.man + assert_equal m.name, i.man.name, "Name of man should be the same before changes to parent instance" + m.name = 'Bongo' + assert_equal m.name, i.man.name, "Name of man should be the same after changes to parent instance" + i.man.name = 'Mungo' + assert_equal m.name, i.man.name, "Name of man should be the same after changes to replaced-child-owned instance" + end + def test_trying_to_use_inverses_that_dont_exist_should_raise_an_error assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { Man.find(:first).secret_interests } end @@ -299,6 +423,30 @@ class InverseBelongsToTests < ActiveRecord::TestCase assert_not_equal i.topic, iz.topic, "Interest topics should not be the same after changes to parent-owned instance" end + def test_child_instance_should_be_shared_with_replaced_via_accessor_parent + f = Face.find(:first) + m = Man.new(:name => 'Charles') + f.man = m + assert_not_nil m.face + assert_equal f.description, m.face.description, "Description of face should be the same before changes to child instance" + f.description = 'gormless' + assert_equal f.description, m.face.description, "Description of face should be the same after changes to child instance" + m.face.description = 'pleasing' + assert_equal f.description, m.face.description, "Description of face should be the same after changes to replaced-parent-owned instance" + end + + def test_child_instance_should_be_shared_with_replaced_via_method_parent + f = Face.find(:first) + m = Man.new(:name => 'Charles') + f.man.replace(m) + assert_not_nil m.face + assert_equal f.description, m.face.description, "Description of face should be the same before changes to child instance" + f.description = 'gormless' + assert_equal f.description, m.face.description, "Description of face should be the same after changes to child instance" + m.face.description = 'pleasing' + assert_equal f.description, m.face.description, "Description of face should be the same after changes to replaced-parent-owned instance" + end + def test_trying_to_use_inverses_that_dont_exist_should_raise_an_error assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { Face.find(:first).horrible_man } end -- cgit v1.2.3 From 81ca0cf2b074f4b868a84c427ef155607a956119 Mon Sep 17 00:00:00 2001 From: George Ogata Date: Sun, 29 Nov 2009 00:46:09 -0500 Subject: Add inverse polymorphic association support. [#3520 state:resolved] Signed-off-by: Eloy Duran --- .../belongs_to_polymorphic_association.rb | 39 +++++--- activerecord/lib/active_record/reflection.rb | 14 ++- .../associations/inverse_associations_test.rb | 100 +++++++++++++++++---- activerecord/test/fixtures/faces.yml | 4 + activerecord/test/fixtures/interests.yml | 6 +- activerecord/test/models/face.rb | 1 + activerecord/test/models/interest.rb | 1 + activerecord/test/models/man.rb | 2 + activerecord/test/schema/schema.rb | 4 + 9 files changed, 139 insertions(+), 32 deletions(-) diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb index 67e18d692d..9678300003 100644 --- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb @@ -13,6 +13,7 @@ module ActiveRecord @updated = true end + set_inverse_instance(record, @owner) loaded record end @@ -22,19 +23,37 @@ module ActiveRecord end private + + # NOTE - for now, we're only supporting inverse setting from belongs_to back onto + # has_one associations. + def we_can_set_the_inverse_on_this?(record) + @reflection.has_inverse? && @reflection.polymorphic_inverse_of(record.class).macro == :has_one + end + + def set_inverse_instance(record, instance) + return if record.nil? || !we_can_set_the_inverse_on_this?(record) + inverse_relationship = @reflection.polymorphic_inverse_of(record.class) + unless inverse_relationship.nil? + record.send(:"set_#{inverse_relationship.name}_target", instance) + end + end + def find_target return nil if association_class.nil? - if @reflection.options[:conditions] - association_class.find( - @owner[@reflection.primary_key_name], - :select => @reflection.options[:select], - :conditions => conditions, - :include => @reflection.options[:include] - ) - else - association_class.find(@owner[@reflection.primary_key_name], :select => @reflection.options[:select], :include => @reflection.options[:include]) - end + target = + if @reflection.options[:conditions] + association_class.find( + @owner[@reflection.primary_key_name], + :select => @reflection.options[:select], + :conditions => conditions, + :include => @reflection.options[:include] + ) + else + association_class.find(@owner[@reflection.primary_key_name], :select => @reflection.options[:select], :include => @reflection.options[:include]) + end + set_inverse_instance(target, @owner) if target + target end def foreign_key_present diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index db5d2b25ed..72f7df32c7 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -214,8 +214,10 @@ module ActiveRecord end def check_validity_of_inverse! - if has_inverse? && inverse_of.nil? - raise InverseOfAssociationNotFoundError.new(self) + unless options[:polymorphic] + if has_inverse? && inverse_of.nil? + raise InverseOfAssociationNotFoundError.new(self) + end end end @@ -242,6 +244,14 @@ module ActiveRecord end end + def polymorphic_inverse_of(associated_class) + if has_inverse? + associated_class.reflect_on_association(options[:inverse_of]) + else + nil + end + end + private def derive_class_name class_name = name.to_s.camelize diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index c3d0d61cec..2ab3aa141a 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -85,7 +85,7 @@ class InverseHasOneTests < ActiveRecord::TestCase fixtures :men, :faces def test_parent_instance_should_be_shared_with_child_on_find - m = Man.find(:first) + m = men(:gordon) f = m.face assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance" m.name = 'Bongo' @@ -96,7 +96,7 @@ class InverseHasOneTests < ActiveRecord::TestCase def test_parent_instance_should_be_shared_with_eager_loaded_child_on_find - m = Man.find(:first, :include => :face) + m = Man.find(:first, :conditions => {:name => 'Gordon'}, :include => :face) f = m.face assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance" m.name = 'Bongo' @@ -104,7 +104,7 @@ class InverseHasOneTests < ActiveRecord::TestCase f.man.name = 'Mungo' assert_equal m.name, f.man.name, "Name of man should be the same after changes to child-owned instance" - m = Man.find(:first, :include => :face, :order => 'faces.id') + m = Man.find(:first, :conditions => {:name => 'Gordon'}, :include => :face, :order => 'faces.id') f = m.face assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance" m.name = 'Bongo' @@ -114,7 +114,7 @@ class InverseHasOneTests < ActiveRecord::TestCase end def test_parent_instance_should_be_shared_with_newly_built_child - m = Man.find(:first) + m = men(:gordon) f = m.build_face(:description => 'haunted') assert_not_nil f.man assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance" @@ -125,7 +125,7 @@ class InverseHasOneTests < ActiveRecord::TestCase end def test_parent_instance_should_be_shared_with_newly_created_child - m = Man.find(:first) + m = men(:gordon) f = m.create_face(:description => 'haunted') assert_not_nil f.man assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance" @@ -224,7 +224,7 @@ class InverseHasManyTests < ActiveRecord::TestCase fixtures :men, :interests def test_parent_instance_should_be_shared_with_every_child_on_find - m = Man.find(:first) + m = men(:gordon) is = m.interests is.each do |i| assert_equal m.name, i.man.name, "Name of man should be the same before changes to parent instance" @@ -236,7 +236,7 @@ class InverseHasManyTests < ActiveRecord::TestCase end def test_parent_instance_should_be_shared_with_eager_loaded_children - m = Man.find(:first, :include => :interests) + m = Man.find(:first, :conditions => {:name => 'Gordon'}, :include => :interests) is = m.interests is.each do |i| assert_equal m.name, i.man.name, "Name of man should be the same before changes to parent instance" @@ -246,7 +246,7 @@ class InverseHasManyTests < ActiveRecord::TestCase assert_equal m.name, i.man.name, "Name of man should be the same after changes to child-owned instance" end - m = Man.find(:first, :include => :interests, :order => 'interests.id') + m = Man.find(:first, :conditions => {:name => 'Gordon'}, :include => :interests, :order => 'interests.id') is = m.interests is.each do |i| assert_equal m.name, i.man.name, "Name of man should be the same before changes to parent instance" @@ -255,11 +255,10 @@ class InverseHasManyTests < ActiveRecord::TestCase i.man.name = 'Mungo' assert_equal m.name, i.man.name, "Name of man should be the same after changes to child-owned instance" end - end def test_parent_instance_should_be_shared_with_newly_built_child - m = Man.find(:first) + m = men(:gordon) i = m.interests.build(:topic => 'Industrial Revolution Re-enactment') assert_not_nil i.man assert_equal m.name, i.man.name, "Name of man should be the same before changes to parent instance" @@ -282,7 +281,7 @@ class InverseHasManyTests < ActiveRecord::TestCase end def test_parent_instance_should_be_shared_with_newly_created_child - m = Man.find(:first) + m = men(:gordon) i = m.interests.create(:topic => 'Industrial Revolution Re-enactment') assert_not_nil i.man assert_equal m.name, i.man.name, "Name of man should be the same before changes to parent instance" @@ -316,7 +315,7 @@ class InverseHasManyTests < ActiveRecord::TestCase end def test_parent_instance_should_be_shared_with_poked_in_child - m = Man.find(:first) + m = men(:gordon) i = Interest.create(:topic => 'Industrial Revolution Re-enactment') m.interests << i assert_not_nil i.man @@ -360,7 +359,7 @@ class InverseBelongsToTests < ActiveRecord::TestCase fixtures :men, :faces, :interests def test_child_instance_should_be_shared_with_parent_on_find - f = Face.find(:first) + f = faces(:trusting) m = f.man assert_equal f.description, m.face.description, "Description of face should be the same before changes to child instance" f.description = 'gormless' @@ -370,7 +369,7 @@ class InverseBelongsToTests < ActiveRecord::TestCase end def test_eager_loaded_child_instance_should_be_shared_with_parent_on_find - f = Face.find(:first, :include => :man) + f = Face.find(:first, :include => :man, :conditions => {:description => 'trusting'}) m = f.man assert_equal f.description, m.face.description, "Description of face should be the same before changes to child instance" f.description = 'gormless' @@ -378,8 +377,7 @@ class InverseBelongsToTests < ActiveRecord::TestCase m.face.description = 'pleasing' assert_equal f.description, m.face.description, "Description of face should be the same after changes to parent-owned instance" - - f = Face.find(:first, :include => :man, :order => 'men.id') + f = Face.find(:first, :include => :man, :order => 'men.id', :conditions => {:description => 'trusting'}) m = f.man assert_equal f.description, m.face.description, "Description of face should be the same before changes to child instance" f.description = 'gormless' @@ -389,7 +387,7 @@ class InverseBelongsToTests < ActiveRecord::TestCase end def test_child_instance_should_be_shared_with_newly_built_parent - f = Face.find(:first) + f = faces(:trusting) m = f.build_man(:name => 'Charles') assert_not_nil m.face assert_equal f.description, m.face.description, "Description of face should be the same before changes to child instance" @@ -400,7 +398,7 @@ class InverseBelongsToTests < ActiveRecord::TestCase end def test_child_instance_should_be_shared_with_newly_created_parent - f = Face.find(:first) + f = faces(:trusting) m = f.create_man(:name => 'Charles') assert_not_nil m.face assert_equal f.description, m.face.description, "Description of face should be the same before changes to child instance" @@ -411,7 +409,7 @@ class InverseBelongsToTests < ActiveRecord::TestCase end def test_should_not_try_to_set_inverse_instances_when_the_inverse_is_a_has_many - i = Interest.find(:first) + i = interests(:trainspotting) m = i.man assert_not_nil m.interests iz = m.interests.detect {|iz| iz.id == i.id} @@ -452,6 +450,70 @@ class InverseBelongsToTests < ActiveRecord::TestCase end end +class InversePolymorphicBelongsToTests < ActiveRecord::TestCase + fixtures :men, :faces, :interests + + def test_child_instance_should_be_shared_with_parent_on_find + f = Face.find(:first, :conditions => {:description => 'confused'}) + m = f.polymorphic_man + assert_equal f.description, m.polymorphic_face.description, "Description of face should be the same before changes to child instance" + f.description = 'gormless' + assert_equal f.description, m.polymorphic_face.description, "Description of face should be the same after changes to child instance" + m.polymorphic_face.description = 'pleasing' + assert_equal f.description, m.polymorphic_face.description, "Description of face should be the same after changes to parent-owned instance" + end + + def test_eager_loaded_child_instance_should_be_shared_with_parent_on_find + f = Face.find(:first, :conditions => {:description => 'confused'}, :include => :man) + m = f.polymorphic_man + assert_equal f.description, m.polymorphic_face.description, "Description of face should be the same before changes to child instance" + f.description = 'gormless' + assert_equal f.description, m.polymorphic_face.description, "Description of face should be the same after changes to child instance" + m.polymorphic_face.description = 'pleasing' + assert_equal f.description, m.polymorphic_face.description, "Description of face should be the same after changes to parent-owned instance" + + f = Face.find(:first, :conditions => {:description => 'confused'}, :include => :man, :order => 'men.id') + m = f.polymorphic_man + assert_equal f.description, m.polymorphic_face.description, "Description of face should be the same before changes to child instance" + f.description = 'gormless' + assert_equal f.description, m.polymorphic_face.description, "Description of face should be the same after changes to child instance" + m.polymorphic_face.description = 'pleasing' + assert_equal f.description, m.polymorphic_face.description, "Description of face should be the same after changes to parent-owned instance" + end + + def test_child_instance_should_be_shared_with_replaced_parent + face = faces(:confused) + old_man = face.polymorphic_man + new_man = Man.new + + assert_not_nil face.polymorphic_man + face.polymorphic_man.replace(new_man) + + assert_equal face.description, new_man.polymorphic_face.description, "Description of face should be the same before changes to parent instance" + face.description = 'Bongo' + assert_equal face.description, new_man.polymorphic_face.description, "Description of face should be the same after changes to parent instance" + new_man.polymorphic_face.description = 'Mungo' + assert_equal face.description, new_man.polymorphic_face.description, "Description of face should be the same after changes to replaced-parent-owned instance" + end + + def test_should_not_try_to_set_inverse_instances_when_the_inverse_is_a_has_many + i = interests(:llama_wrangling) + m = i.polymorphic_man + assert_not_nil m.polymorphic_interests + iz = m.polymorphic_interests.detect {|iz| iz.id == i.id} + assert_not_nil iz + assert_equal i.topic, iz.topic, "Interest topics should be the same before changes to child" + i.topic = 'Eating cheese with a spoon' + assert_not_equal i.topic, iz.topic, "Interest topics should not be the same after changes to child" + iz.topic = 'Cow tipping' + assert_not_equal i.topic, iz.topic, "Interest topics should not be the same after changes to parent-owned instance" + end + + def test_trying_to_use_inverses_that_dont_exist_should_raise_an_error + assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { Face.find(:first).horrible_man } + end +end + # NOTE - these tests might not be meaningful, ripped as they were from the parental_control plugin # which would guess the inverse rather than look for an explicit configuration option. class InverseMultipleHasManyInversesForSameModel < ActiveRecord::TestCase diff --git a/activerecord/test/fixtures/faces.yml b/activerecord/test/fixtures/faces.yml index 1dd2907cf7..c8e4a34484 100644 --- a/activerecord/test/fixtures/faces.yml +++ b/activerecord/test/fixtures/faces.yml @@ -5,3 +5,7 @@ trusting: weather_beaten: description: weather beaten man: steve + +confused: + description: confused + polymorphic_man: gordon (Man) diff --git a/activerecord/test/fixtures/interests.yml b/activerecord/test/fixtures/interests.yml index ec71890ab6..9200a19d5a 100644 --- a/activerecord/test/fixtures/interests.yml +++ b/activerecord/test/fixtures/interests.yml @@ -23,7 +23,11 @@ woodsmanship: zine: going_out man: steve -survial: +survival: topic: Survival zine: going_out man: steve + +llama_wrangling: + topic: Llama Wrangling + polymorphic_man: gordon (Man) diff --git a/activerecord/test/models/face.rb b/activerecord/test/models/face.rb index 1540dbf741..3e2bdc0307 100644 --- a/activerecord/test/models/face.rb +++ b/activerecord/test/models/face.rb @@ -1,5 +1,6 @@ class Face < ActiveRecord::Base belongs_to :man, :inverse_of => :face + belongs_to :polymorphic_man, :polymorphic => true, :inverse_of => :polymorphic_face # This is a "broken" inverse_of for the purposes of testing belongs_to :horrible_man, :class_name => 'Man', :inverse_of => :horrible_face end diff --git a/activerecord/test/models/interest.rb b/activerecord/test/models/interest.rb index d8291d00cc..d5d9226204 100644 --- a/activerecord/test/models/interest.rb +++ b/activerecord/test/models/interest.rb @@ -1,4 +1,5 @@ class Interest < ActiveRecord::Base belongs_to :man, :inverse_of => :interests + belongs_to :polymorphic_man, :polymorphic => true, :inverse_of => :polymorphic_interests belongs_to :zine, :inverse_of => :interests end diff --git a/activerecord/test/models/man.rb b/activerecord/test/models/man.rb index f40bc9d0fc..4bff92dc98 100644 --- a/activerecord/test/models/man.rb +++ b/activerecord/test/models/man.rb @@ -1,6 +1,8 @@ class Man < ActiveRecord::Base has_one :face, :inverse_of => :man + has_one :polymorphic_face, :class_name => 'Face', :as => :polymorphic_man, :inverse_of => :polymorphic_man has_many :interests, :inverse_of => :man + has_many :polymorphic_interests, :class_name => 'Interest', :as => :polymorphic_man, :inverse_of => :polymorphic_man # These are "broken" inverse_of associations for the purposes of testing has_one :dirty_face, :class_name => 'Face', :inverse_of => :dirty_man has_many :secret_interests, :class_name => 'Interest', :inverse_of => :secret_man diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 0dd9da4c11..1ec36e7832 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -520,11 +520,15 @@ ActiveRecord::Schema.define do create_table :faces, :force => true do |t| t.string :description t.integer :man_id + t.integer :polymorphic_man_id + t.string :polymorphic_man_type end create_table :interests, :force => true do |t| t.string :topic t.integer :man_id + t.integer :polymorphic_man_id + t.string :polymorphic_man_type t.integer :zine_id end -- cgit v1.2.3 From 6a74ee7f4deea4a44520d3fcc9120e0bb848823f Mon Sep 17 00:00:00 2001 From: Murray Steele Date: Thu, 17 Dec 2009 12:19:10 +0000 Subject: Provide a slightly more robust we_can_set_the_inverse_on_this? method for polymorphic belongs_to associations. [#3520 state:resolved] Also add a new test for polymorphic belongs_to that test direct accessor assignment, not just .replace assignment. Signed-off-by: Eloy Duran --- .../associations/belongs_to_polymorphic_association.rb | 9 +++++++-- .../cases/associations/inverse_associations_test.rb | 17 ++++++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb index 9678300003..f6edd6383c 100644 --- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb @@ -27,7 +27,12 @@ module ActiveRecord # NOTE - for now, we're only supporting inverse setting from belongs_to back onto # has_one associations. def we_can_set_the_inverse_on_this?(record) - @reflection.has_inverse? && @reflection.polymorphic_inverse_of(record.class).macro == :has_one + if @reflection.has_inverse? + inverse_association = @reflection.polymorphic_inverse_of(record.class) + inverse_association && inverse_association.macro == :has_one + else + false + end end def set_inverse_instance(record, instance) @@ -52,7 +57,7 @@ module ActiveRecord else association_class.find(@owner[@reflection.primary_key_name], :select => @reflection.options[:select], :include => @reflection.options[:include]) end - set_inverse_instance(target, @owner) if target + set_inverse_instance(target, @owner) target end diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index 2ab3aa141a..0696f06e5b 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -481,7 +481,22 @@ class InversePolymorphicBelongsToTests < ActiveRecord::TestCase assert_equal f.description, m.polymorphic_face.description, "Description of face should be the same after changes to parent-owned instance" end - def test_child_instance_should_be_shared_with_replaced_parent + def test_child_instance_should_be_shared_with_replaced_via_accessor_parent + face = faces(:confused) + old_man = face.polymorphic_man + new_man = Man.new + + assert_not_nil face.polymorphic_man + face.polymorphic_man = new_man + + assert_equal face.description, new_man.polymorphic_face.description, "Description of face should be the same before changes to parent instance" + face.description = 'Bongo' + assert_equal face.description, new_man.polymorphic_face.description, "Description of face should be the same after changes to parent instance" + new_man.polymorphic_face.description = 'Mungo' + assert_equal face.description, new_man.polymorphic_face.description, "Description of face should be the same after changes to replaced-parent-owned instance" + end + + def test_child_instance_should_be_shared_with_replaced_via_method_parent face = faces(:confused) old_man = face.polymorphic_man new_man = Man.new -- cgit v1.2.3 From ff508640e28914da2b546f6a8c9f215bab201b61 Mon Sep 17 00:00:00 2001 From: Murray Steele Date: Mon, 28 Dec 2009 14:13:33 +0100 Subject: Make polymorphic_inverse_of in Reflection throw an InverseOfAssociationNotFoundError if the supplied class doesn't have the appropriate association. [#3520 state:resolved] Signed-off-by: Eloy Duran --- activerecord/lib/active_record/associations.rb | 4 ++-- activerecord/lib/active_record/reflection.rb | 10 +++++----- .../cases/associations/inverse_associations_test.rb | 17 +++++++++++++++-- activerecord/test/models/face.rb | 3 ++- 4 files changed, 24 insertions(+), 10 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index c23c9f63f1..ff8c63bc91 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -3,8 +3,8 @@ require 'active_support/core_ext/enumerable' module ActiveRecord class InverseOfAssociationNotFoundError < ActiveRecordError #:nodoc: - def initialize(reflection) - super("Could not find the inverse association for #{reflection.name} (#{reflection.options[:inverse_of].inspect} in #{reflection.class_name})") + def initialize(reflection, associated_class = nil) + super("Could not find the inverse association for #{reflection.name} (#{reflection.options[:inverse_of].inspect} in #{associated_class.nil? ? reflection.class_name : associated_class.name})") end end diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 72f7df32c7..b751c9ad68 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -239,16 +239,16 @@ module ActiveRecord def inverse_of if has_inverse? @inverse_of ||= klass.reflect_on_association(options[:inverse_of]) - else - nil end end def polymorphic_inverse_of(associated_class) if has_inverse? - associated_class.reflect_on_association(options[:inverse_of]) - else - nil + if inverse_relationship = associated_class.reflect_on_association(options[:inverse_of]) + inverse_relationship + else + raise InverseOfAssociationNotFoundError.new(self, associated_class) + end end end diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index 0696f06e5b..457c4da9bf 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -524,8 +524,21 @@ class InversePolymorphicBelongsToTests < ActiveRecord::TestCase assert_not_equal i.topic, iz.topic, "Interest topics should not be the same after changes to parent-owned instance" end - def test_trying_to_use_inverses_that_dont_exist_should_raise_an_error - assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { Face.find(:first).horrible_man } + def test_trying_to_access_inverses_that_dont_exist_shouldnt_raise_an_error + # Ideally this would, if only for symmetry's sake with other association types + assert_nothing_raised(ActiveRecord::InverseOfAssociationNotFoundError) { Face.find(:first).horrible_polymorphic_man } + end + + def test_trying_to_set_polymorphic_inverses_that_dont_exist_at_all_should_raise_an_error + # fails because no class has the correct inverse_of for horrible_polymorphic_man + assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { Face.find(:first).horrible_polymorphic_man = Man.first } + end + + def test_trying_to_set_polymorphic_inverses_that_dont_exist_on_the_instance_being_set_should_raise_an_error + # passes because Man does have the correct inverse_of + assert_nothing_raised(ActiveRecord::InverseOfAssociationNotFoundError) { Face.find(:first).polymorphic_man = Man.first } + # fails because Interest does have the correct inverse_of + assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { Face.find(:first).polymorphic_man = Interest.first } end end diff --git a/activerecord/test/models/face.rb b/activerecord/test/models/face.rb index 3e2bdc0307..edb75d333f 100644 --- a/activerecord/test/models/face.rb +++ b/activerecord/test/models/face.rb @@ -1,6 +1,7 @@ class Face < ActiveRecord::Base belongs_to :man, :inverse_of => :face belongs_to :polymorphic_man, :polymorphic => true, :inverse_of => :polymorphic_face - # This is a "broken" inverse_of for the purposes of testing + # These is a "broken" inverse_of for the purposes of testing belongs_to :horrible_man, :class_name => 'Man', :inverse_of => :horrible_face + belongs_to :horrible_polymorphic_man, :polymorphic => true, :inverse_of => :horrible_polymorphic_face end -- cgit v1.2.3 From 9c771a9608f54ebdfcb6fca819c83038489ce50d Mon Sep 17 00:00:00 2001 From: Eloy Duran Date: Tue, 15 Dec 2009 16:40:02 +0100 Subject: Make sure to not add autosave callbacks multiple times. [#3575 state:resolved] This makes sure that, in a HABTM association, only one join record is craeted. --- .../lib/active_record/autosave_association.rb | 40 ++++++++++++++-------- .../lib/active_record/nested_attributes.rb | 4 +-- .../test/cases/autosave_association_test.rb | 29 ++++++++++++++++ activerecord/test/cases/nested_attributes_test.rb | 9 +++++ 4 files changed, 65 insertions(+), 17 deletions(-) diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index c0d8904bc8..44c668b619 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -155,6 +155,13 @@ module ActiveRecord # Adds a validate and save callback for the association as specified by # the +reflection+. + # + # For performance reasons, we don't check whether to validate at runtime, + # but instead only define the method and callback when needed. However, + # this can change, for instance, when using nested attributes. Since we + # don't want the callbacks to get defined multiple times, there are + # guards that check if the save or validation methods have already been + # defined before actually defining them. def add_autosave_association_callbacks(reflection) save_method = "autosave_associated_records_for_#{reflection.name}" validation_method = "validate_associated_records_for_#{reflection.name}" @@ -162,28 +169,33 @@ module ActiveRecord case reflection.macro when :has_many, :has_and_belongs_to_many - before_save :before_save_collection_association + unless method_defined?(save_method) + before_save :before_save_collection_association - define_method(save_method) { save_collection_association(reflection) } - # Doesn't use after_save as that would save associations added in after_create/after_update twice - after_create save_method - after_update save_method + define_method(save_method) { save_collection_association(reflection) } + # Doesn't use after_save as that would save associations added in after_create/after_update twice + after_create save_method + after_update save_method + end - if force_validation || (reflection.macro == :has_many && reflection.options[:validate] != false) + if !method_defined?(validation_method) && + (force_validation || (reflection.macro == :has_many && reflection.options[:validate] != false)) define_method(validation_method) { validate_collection_association(reflection) } validate validation_method end else - case reflection.macro - when :has_one - define_method(save_method) { save_has_one_association(reflection) } - after_save save_method - when :belongs_to - define_method(save_method) { save_belongs_to_association(reflection) } - before_save save_method + unless method_defined?(save_method) + case reflection.macro + when :has_one + define_method(save_method) { save_has_one_association(reflection) } + after_save save_method + when :belongs_to + define_method(save_method) { save_belongs_to_association(reflection) } + before_save save_method + end end - if force_validation + if !method_defined?(validation_method) && force_validation define_method(validation_method) { validate_single_association(reflection) } validate validation_method end diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index ca3110a374..386f0e7ca7 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -235,7 +235,7 @@ module ActiveRecord end reflection.options[:autosave] = true - + add_autosave_association_callbacks(reflection) self.nested_attributes_options[association_name.to_sym] = options if options[:reject_if] == :all_blank @@ -250,8 +250,6 @@ module ActiveRecord assign_nested_attributes_for_#{type}_association(:#{association_name}, attributes) end }, __FILE__, __LINE__ - - add_autosave_association_callbacks(reflection) else raise ArgumentError, "No association found for name `#{association_name}'. Has it been defined yet?" end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 9164701601..803e5b25b1 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -31,11 +31,40 @@ class TestAutosaveAssociationsInGeneral < ActiveRecord::TestCase assert base.valid_keys_for_has_and_belongs_to_many_association.include?(:autosave) end + def test_should_not_add_the_same_callbacks_multiple_times_for_has_one + assert_no_difference_when_adding_callbacks_twice_for Pirate, :ship + end + + def test_should_not_add_the_same_callbacks_multiple_times_for_belongs_to + assert_no_difference_when_adding_callbacks_twice_for Ship, :pirate + end + + def test_should_not_add_the_same_callbacks_multiple_times_for_has_many + assert_no_difference_when_adding_callbacks_twice_for Pirate, :birds + end + + def test_should_not_add_the_same_callbacks_multiple_times_for_has_and_belongs_to_many + assert_no_difference_when_adding_callbacks_twice_for Pirate, :parrots + end + private def base ActiveRecord::Base end + + def assert_no_difference_when_adding_callbacks_twice_for(model, association_name) + reflection = model.reflect_on_association(association_name) + assert_no_difference "callbacks_for_model(#{model.name}).length" do + model.send(:add_autosave_association_callbacks, reflection) + end + end + + def callbacks_for_model(model) + model.instance_variables.grep(/_callbacks$/).map do |ivar| + model.instance_variable_get(ivar) + end.flatten + end end class TestDefaultAutosaveAssociationOnAHasOneAssociation < ActiveRecord::TestCase diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index 53fd168e1b..5367907fb7 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -371,6 +371,15 @@ module NestedAttributesOnACollectionAssociationTests assert_respond_to @pirate, association_setter end + def test_should_save_only_one_association_on_create + pirate = Pirate.create!({ + :catchphrase => 'Arr', + association_getter => { 'foo' => { :name => 'Grace OMalley' } } + }) + + assert_equal 1, pirate.reload.send(@association_name).count + end + def test_should_take_a_hash_with_string_keys_and_assign_the_attributes_to_the_associated_models @alternate_params[association_getter].stringify_keys! @pirate.update_attributes @alternate_params -- cgit v1.2.3 From 91e28aae8649c503e81d66ad6829403ccc2c6571 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 29 Dec 2009 00:07:46 +0530 Subject: Add Model.having and Relation#having --- activerecord/CHANGELOG | 4 ++++ activerecord/lib/active_record/associations.rb | 6 ++++-- .../associations/association_collection.rb | 2 +- activerecord/lib/active_record/base.rb | 17 +++-------------- activerecord/lib/active_record/calculations.rb | 2 +- activerecord/lib/active_record/relation.rb | 12 ++++++++++++ 6 files changed, 25 insertions(+), 18 deletions(-) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 5af66c3519..28ae2262e2 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,9 @@ *Edge* +* Add Model.having and Relation#having. [Pratik Naik] + + Developer.group("salary").having("sum(salary) > 10000").select("salary") + * Add Relation#count. [Pratik Naik] legends = People.where("age > 100") diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index c23c9f63f1..7242ebf387 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1715,7 +1715,8 @@ module ActiveRecord relation = relation.joins(construct_join(options[:joins], scope)). select(column_aliases(join_dependency)). - group(construct_group(options[:group], options[:having], scope)). + group(options[:group] || (scope && scope[:group])). + having(options[:having] || (scope && scope[:having])). order(construct_order(options[:order], scope)). where(construct_conditions(options[:conditions], scope)). from((scope && scope[:from]) || options[:from]) @@ -1759,7 +1760,8 @@ module ActiveRecord relation = relation.joins(construct_join(options[:joins], scope)). where(construct_conditions(options[:conditions], scope)). - group(construct_group(options[:group], options[:having], scope)). + group(options[:group] || (scope && scope[:group])). + having(options[:having] || (scope && scope[:having])). order(construct_order(options[:order], scope)). limit(construct_limit(options[:limit], scope)). offset(construct_limit(options[:offset], scope)). diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 8bc64a3a93..56b2a90138 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -21,7 +21,7 @@ module ActiveRecord construct_sql end - delegate :group, :order, :limit, :joins, :where, :preload, :eager_load, :from, :lock, :readonly, :to => :scoped + delegate :group, :order, :limit, :joins, :where, :preload, :eager_load, :from, :lock, :readonly, :having, :to => :scoped def select(select = nil, &block) if block_given? diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 3135234706..767109474d 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -652,7 +652,7 @@ module ActiveRecord #:nodoc: end end - delegate :select, :group, :order, :limit, :joins, :where, :preload, :eager_load, :from, :lock, :readonly, :to => :scoped + delegate :select, :group, :order, :limit, :joins, :where, :preload, :eager_load, :from, :lock, :readonly, :having, :to => :scoped # A convenience wrapper for find(:first, *args). You can pass in all the # same arguments to this method as you can to find(:first). @@ -1566,7 +1566,8 @@ module ActiveRecord #:nodoc: joins(construct_join(options[:joins], scope)). where(construct_conditions(options[:conditions], scope)). select(options[:select] || (scope && scope[:select]) || default_select(options[:joins] || (scope && scope[:joins]))). - group(construct_group(options[:group], options[:having], scope)). + group(options[:group] || (scope && scope[:group])). + having(options[:having] || (scope && scope[:having])). order(construct_order(options[:order], scope)). limit(construct_limit(options[:limit], scope)). offset(construct_offset(options[:offset], scope)). @@ -1611,18 +1612,6 @@ module ActiveRecord #:nodoc: end end - def construct_group(group, having, scope) - sql = '' - if group - sql << group.to_s - sql << " HAVING #{sanitize_sql_for_conditions(having)}" if having - elsif scope && (scoped_group = scope[:group]) - sql << scoped_group.to_s - sql << " HAVING #{sanitize_sql_for_conditions(scope[:having])}" if scope[:having] - end - sql - end - def construct_order(order, scope) orders = [] diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb index fcba23dc0d..59811c40a8 100644 --- a/activerecord/lib/active_record/calculations.rb +++ b/activerecord/lib/active_record/calculations.rb @@ -194,7 +194,7 @@ module ActiveRecord options[:select] << ", #{group_field} AS #{group_alias}" - relation = relation.select(options[:select]).group(construct_group(options[:group], options[:having], nil)) + relation = relation.select(options[:select]).group(options[:group]).having(options[:having]) calculated_data = connection.select_all(relation.to_sql) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 291e97ff83..c7a74b7763 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -55,6 +55,18 @@ module ActiveRecord from.present? ? create_new_relation(@relation.from(from)) : create_new_relation end + def having(*args) + return create_new_relation if args.blank? + + if [String, Hash, Array].include?(args.first.class) + havings = @klass.send(:merge_conditions, args.size > 1 ? Array.wrap(args) : args.first) + else + havings = args.first + end + + create_new_relation(@relation.having(havings)) + end + def group(groups) groups.present? ? create_new_relation(@relation.group(groups)) : create_new_relation end -- cgit v1.2.3 From 4691cc43ee184414e2366340c3227e6fdf2aba7d Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 28 Dec 2009 11:40:23 -0800 Subject: Fix ruby version test --- .../test/initializer/check_ruby_version_test.rb | 49 ++++------------------ 1 file changed, 9 insertions(+), 40 deletions(-) diff --git a/railties/test/initializer/check_ruby_version_test.rb b/railties/test/initializer/check_ruby_version_test.rb index 0691caad9d..a2c07ece75 100644 --- a/railties/test/initializer/check_ruby_version_test.rb +++ b/railties/test/initializer/check_ruby_version_test.rb @@ -1,7 +1,7 @@ require "isolation/abstract_unit" module InitializerTests - class PathsTest < Test::Unit::TestCase + class CheckRubyVersionTest < Test::Unit::TestCase include ActiveSupport::Testing::Isolation def setup @@ -9,52 +9,21 @@ module InitializerTests boot_rails end - test "rails does not initialize with ruby version 1.8.1" do - assert_rails_does_not_boot "1.8.1" - end - - test "rails does not initialize with ruby version 1.8.2" do - assert_rails_does_not_boot "1.8.2" - end - - test "rails does not initialize with ruby version 1.8.3" do - assert_rails_does_not_boot "1.8.3" - end - - test "rails does not initialize with ruby version 1.8.4" do - assert_rails_does_not_boot "1.8.4" - end - - test "rails does not initializes with ruby version 1.8.5" do - assert_rails_does_not_boot "1.8.5" - end - - test "rails does not initialize with ruby version 1.8.6" do - assert_rails_does_not_boot "1.8.6" - end - - test "rails initializes with ruby version 1.8.7" do - assert_rails_boots "1.8.7" - end - - test "rails initializes with the current version of Ruby" do - assert_rails_boots - end - - def set_ruby_version(version) - $-w = nil - Object.const_set(:RUBY_VERSION, version.freeze) + test "rails initializes with ruby 1.8.7 or later" do + if RUBY_VERSION < '1.8.7' + assert_rails_does_not_boot + else + assert_rails_boots + end end - def assert_rails_boots(version = nil) - set_ruby_version(version) if version + def assert_rails_boots assert_nothing_raised "It appears that rails does not boot" do require "rails" end end - def assert_rails_does_not_boot(version) - set_ruby_version(version) + def assert_rails_does_not_boot $stderr = File.open("/dev/null", "w") assert_raises(SystemExit) do require "rails" -- cgit v1.2.3 From f95fcdd3af95c4819322e00100831f12f95a8e62 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 28 Dec 2009 11:40:34 -0800 Subject: Fix copy/pasted test class name --- railties/test/initializer/path_test.rb | 196 +++++++++++++++++---------------- 1 file changed, 99 insertions(+), 97 deletions(-) diff --git a/railties/test/initializer/path_test.rb b/railties/test/initializer/path_test.rb index fa66ebcd83..3bbf9617a0 100644 --- a/railties/test/initializer/path_test.rb +++ b/railties/test/initializer/path_test.rb @@ -1,101 +1,103 @@ require "isolation/abstract_unit" -class PathsTest < Test::Unit::TestCase - include ActiveSupport::Testing::Isolation - - def setup - build_app - boot_rails - require "rails" - add_to_config <<-RUBY - config.root = "#{app_path}" - config.frameworks = [:action_controller, :action_view, :action_mailer, :active_record] - config.after_initialize do - ActionController::Base.session_store = nil - end - RUBY - require "#{app_path}/config/environment" - @paths = Rails.application.config.paths - end - - def root(*path) - app_path(*path).to_s - end - - def assert_path(paths, *dir) - assert_equal [root(*dir)], paths.paths - end - - def assert_in_load_path(*path) - assert $:.any? { |p| File.expand_path(p) == root(*path) }, "Load path does not include '#{root(*path)}'. They are:\n-----\n #{$:.join("\n")}\n-----" - end - - def assert_not_in_load_path(*path) - assert !$:.any? { |p| File.expand_path(p) == root(*path) }, "Load path includes '#{root(*path)}'. They are:\n-----\n #{$:.join("\n")}\n-----" - end - - test "booting up Rails yields a valid paths object" do - assert_path @paths.app, "app" - assert_path @paths.app.metals, "app", "metal" - assert_path @paths.app.models, "app", "models" - assert_path @paths.app.helpers, "app", "helpers" - assert_path @paths.app.services, "app", "services" - assert_path @paths.lib, "lib" - assert_path @paths.vendor, "vendor" - assert_path @paths.vendor.plugins, "vendor", "plugins" - assert_path @paths.tmp, "tmp" - assert_path @paths.tmp.cache, "tmp", "cache" - assert_path @paths.config, "config" - assert_path @paths.config.locales, "config", "locales" - assert_path @paths.config.environments, "config", "environments" - - assert_equal root("app", "controllers"), @paths.app.controllers.to_a.first - assert_equal Pathname.new(File.dirname(__FILE__)).join("..", "..", "builtin", "rails_info").expand_path, - Pathname.new(@paths.app.controllers.to_a[1]).expand_path - end +module InitializerTests + class PathTest < Test::Unit::TestCase + include ActiveSupport::Testing::Isolation + + def setup + build_app + boot_rails + require "rails" + add_to_config <<-RUBY + config.root = "#{app_path}" + config.frameworks = [:action_controller, :action_view, :action_mailer, :active_record] + config.after_initialize do + ActionController::Base.session_store = nil + end + RUBY + require "#{app_path}/config/environment" + @paths = Rails.application.config.paths + end + + def root(*path) + app_path(*path).to_s + end + + def assert_path(paths, *dir) + assert_equal [root(*dir)], paths.paths + end + + def assert_in_load_path(*path) + assert $:.any? { |p| File.expand_path(p) == root(*path) }, "Load path does not include '#{root(*path)}'. They are:\n-----\n #{$:.join("\n")}\n-----" + end + + def assert_not_in_load_path(*path) + assert !$:.any? { |p| File.expand_path(p) == root(*path) }, "Load path includes '#{root(*path)}'. They are:\n-----\n #{$:.join("\n")}\n-----" + end + + test "booting up Rails yields a valid paths object" do + assert_path @paths.app, "app" + assert_path @paths.app.metals, "app", "metal" + assert_path @paths.app.models, "app", "models" + assert_path @paths.app.helpers, "app", "helpers" + assert_path @paths.app.services, "app", "services" + assert_path @paths.lib, "lib" + assert_path @paths.vendor, "vendor" + assert_path @paths.vendor.plugins, "vendor", "plugins" + assert_path @paths.tmp, "tmp" + assert_path @paths.tmp.cache, "tmp", "cache" + assert_path @paths.config, "config" + assert_path @paths.config.locales, "config", "locales" + assert_path @paths.config.environments, "config", "environments" + + assert_equal root("app", "controllers"), @paths.app.controllers.to_a.first + assert_equal Pathname.new(File.dirname(__FILE__)).join("..", "..", "builtin", "rails_info").expand_path, + Pathname.new(@paths.app.controllers.to_a[1]).expand_path + end + + test "booting up Rails yields a list of paths that are eager" do + assert @paths.app.models.eager_load? + assert @paths.app.controllers.eager_load? + assert @paths.app.helpers.eager_load? + assert @paths.app.metals.eager_load? + end + + test "environments has a glob equal to the current environment" do + assert_equal "#{RAILS_ENV}.rb", @paths.config.environments.glob + end + + test "load path includes each of the paths in config.paths as long as the directories exist" do + assert_in_load_path "app" + assert_in_load_path "app", "controllers" + assert_in_load_path "app", "models" + assert_in_load_path "app", "helpers" + assert_in_load_path "lib" + assert_in_load_path "vendor" + + assert_not_in_load_path "app", "views" + assert_not_in_load_path "app", "metal" + assert_not_in_load_path "app", "services" + assert_not_in_load_path "config" + assert_not_in_load_path "config", "locales" + assert_not_in_load_path "config", "environments" + assert_not_in_load_path "tmp" + assert_not_in_load_path "tmp", "cache" + end + + test "controller paths include builtin in development mode" do + RAILS_ENV.replace "development" + assert Rails::Configuration.new.paths.app.controllers.paths.any? { |p| p =~ /builtin/ } + end + + test "controller paths does not have builtin_directories in test mode" do + RAILS_ENV.replace "test" + assert !Rails::Configuration.new.paths.app.controllers.paths.any? { |p| p =~ /builtin/ } + end + + test "controller paths does not have builtin_directories in production mode" do + RAILS_ENV.replace "production" + assert !Rails::Configuration.new.paths.app.controllers.paths.any? { |p| p =~ /builtin/ } + end - test "booting up Rails yields a list of paths that are eager" do - assert @paths.app.models.eager_load? - assert @paths.app.controllers.eager_load? - assert @paths.app.helpers.eager_load? - assert @paths.app.metals.eager_load? end - - test "environments has a glob equal to the current environment" do - assert_equal "#{RAILS_ENV}.rb", @paths.config.environments.glob - end - - test "load path includes each of the paths in config.paths as long as the directories exist" do - assert_in_load_path "app" - assert_in_load_path "app", "controllers" - assert_in_load_path "app", "models" - assert_in_load_path "app", "helpers" - assert_in_load_path "lib" - assert_in_load_path "vendor" - - assert_not_in_load_path "app", "views" - assert_not_in_load_path "app", "metal" - assert_not_in_load_path "app", "services" - assert_not_in_load_path "config" - assert_not_in_load_path "config", "locales" - assert_not_in_load_path "config", "environments" - assert_not_in_load_path "tmp" - assert_not_in_load_path "tmp", "cache" - end - - test "controller paths include builtin in development mode" do - RAILS_ENV.replace "development" - assert Rails::Configuration.new.paths.app.controllers.paths.any? { |p| p =~ /builtin/ } - end - - test "controller paths does not have builtin_directories in test mode" do - RAILS_ENV.replace "test" - assert !Rails::Configuration.new.paths.app.controllers.paths.any? { |p| p =~ /builtin/ } - end - - test "controller paths does not have builtin_directories in production mode" do - RAILS_ENV.replace "production" - assert !Rails::Configuration.new.paths.app.controllers.paths.any? { |p| p =~ /builtin/ } - end - -end \ No newline at end of file +end -- cgit v1.2.3 From 07b615fb897017d7acfaafa88606bc88be30f6e4 Mon Sep 17 00:00:00 2001 From: Michael Siebert Date: Mon, 28 Dec 2009 19:02:01 +0100 Subject: Add an :update_only option to accepts_nested_attributes_for for to-one associations. [#2563 state:resolved] Signed-off-by: Eloy Duran --- .../lib/active_record/nested_attributes.rb | 26 +++++++++++++++--- activerecord/test/cases/nested_attributes_test.rb | 31 ++++++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 386f0e7ca7..5143e804d1 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -212,6 +212,11 @@ module ActiveRecord # nested attributes array exceeds the specified limit, NestedAttributes::TooManyRecords # exception is raised. If omitted, any number associations can be processed. # Note that the :limit option is only applicable to one-to-many associations. + # [:update_only] + # Allows to specify that the an existing record can only be updated. + # A new record in only created when there is no existing record. This + # option only works for on-to-one associations and is ignored for + # collection associations. This option is off by default. # # Examples: # # creates avatar_attributes= @@ -221,9 +226,9 @@ module ActiveRecord # # creates avatar_attributes= and posts_attributes= # accepts_nested_attributes_for :avatar, :posts, :allow_destroy => true def accepts_nested_attributes_for(*attr_names) - options = { :allow_destroy => false } + options = { :allow_destroy => false, :update_only => false } options.update(attr_names.extract_options!) - options.assert_valid_keys(:allow_destroy, :reject_if, :limit) + options.assert_valid_keys(:allow_destroy, :reject_if, :limit, :update_only) attr_names.each do |association_name| if reflection = reflect_on_association(association_name) @@ -288,6 +293,13 @@ module ActiveRecord # record’s id, then the existing record will be modified. Otherwise a new # record will be built. # + # If update_only is true, a new record is only created when no object exists, + # otherwise it will be updated + # + # If update_only is false and the given attributes include an :id + # that matches the existing record’s id, then the existing record will be + # modified. Otherwise a new record will be built. + # # If the given attributes include a matching :id attribute _and_ a # :_destroy key set to a truthy value, then the existing record # will be marked for destruction. @@ -295,7 +307,15 @@ module ActiveRecord options = self.nested_attributes_options[association_name] attributes = attributes.with_indifferent_access - if attributes['id'].blank? + if options[:update_only] + if existing_record = send(association_name) + assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy]) + else + unless reject_new_record?(association_name, attributes) + send("build_#{association_name}", attributes.except(*UNASSIGNABLE_KEYS)) + end + end + elsif attributes['id'].blank? unless reject_new_record?(association_name, attributes) method = "build_#{association_name}" if respond_to?(method) diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index 5367907fb7..5e4fc2b8d9 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -245,6 +245,37 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase def test_should_automatically_enable_autosave_on_the_association assert Pirate.reflect_on_association(:ship).options[:autosave] end + + def test_should_accept_update_only_option + Pirate.accepts_nested_attributes_for :ship, :update_only => true + @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :name => 'Mayflower' }) + + Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } + end + + def test_should_create_new_model_when_nothing_is_there_and_update_only_is_true + Pirate.accepts_nested_attributes_for :ship, :update_only => true + @ship.delete + + assert_difference('Ship.count', 1) do + @pirate.reload.update_attribute(:ship_attributes, { :name => 'Mayflower' }) + end + + Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } + end + + + def test_should_update_existing_when_update_only_is_true_and_no_id_is_given + Pirate.accepts_nested_attributes_for :ship, :update_only => true + + assert_no_difference('Ship.count') do + @pirate.reload.update_attributes(:ship_attributes => { :name => 'Mayflower' }) + end + + assert_equal 'Mayflower', @ship.reload.name + + Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } + end end class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase -- cgit v1.2.3 From c23fbd0d475612fe9cd493bd08c8da2f8d7e6f03 Mon Sep 17 00:00:00 2001 From: Eloy Duran Date: Mon, 28 Dec 2009 21:08:20 +0100 Subject: Refactored previous changes to nested attributes. --- .../lib/active_record/nested_attributes.rb | 50 ++++++++-------------- activerecord/test/cases/nested_attributes_test.rb | 41 +++++++++++------- activerecord/test/models/pirate.rb | 2 + activerecord/test/models/ship.rb | 2 + 4 files changed, 48 insertions(+), 47 deletions(-) diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 5143e804d1..ff3a51d5c0 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -213,9 +213,9 @@ module ActiveRecord # exception is raised. If omitted, any number associations can be processed. # Note that the :limit option is only applicable to one-to-many associations. # [:update_only] - # Allows to specify that the an existing record can only be updated. - # A new record in only created when there is no existing record. This - # option only works for on-to-one associations and is ignored for + # Allows you to specify that an existing record may only be updated. + # A new record may only be created when there is no existing record. + # This option only works for one-to-one associations and is ignored for # collection associations. This option is off by default. # # Examples: @@ -248,7 +248,7 @@ module ActiveRecord end # def pirate_attributes=(attributes) - # assign_nested_attributes_for_one_to_one_association(:pirate, attributes, false) + # assign_nested_attributes_for_one_to_one_association(:pirate, attributes) # end class_eval %{ def #{association_name}_attributes=(attributes) @@ -289,43 +289,29 @@ module ActiveRecord # Assigns the given attributes to the association. # - # If the given attributes include an :id that matches the existing - # record’s id, then the existing record will be modified. Otherwise a new - # record will be built. - # - # If update_only is true, a new record is only created when no object exists, - # otherwise it will be updated - # # If update_only is false and the given attributes include an :id # that matches the existing record’s id, then the existing record will be - # modified. Otherwise a new record will be built. + # modified. If update_only is true, a new record is only created when no + # object exists. Otherwise a new record will be built. # - # If the given attributes include a matching :id attribute _and_ a - # :_destroy key set to a truthy value, then the existing record - # will be marked for destruction. + # If the given attributes include a matching :id attribute, or + # update_only is true, and a :_destroy key set to a truthy value, + # then the existing record will be marked for destruction. def assign_nested_attributes_for_one_to_one_association(association_name, attributes) options = self.nested_attributes_options[association_name] attributes = attributes.with_indifferent_access + check_existing_record = (options[:update_only] || !attributes['id'].blank?) - if options[:update_only] - if existing_record = send(association_name) - assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy]) + if check_existing_record && (record = send(association_name)) && + (options[:update_only] || record.id.to_s == attributes['id'].to_s) + assign_to_or_mark_for_destruction(record, attributes, options[:allow_destroy]) + elsif !reject_new_record?(association_name, attributes) + method = "build_#{association_name}" + if respond_to?(method) + send(method, attributes.except(*UNASSIGNABLE_KEYS)) else - unless reject_new_record?(association_name, attributes) - send("build_#{association_name}", attributes.except(*UNASSIGNABLE_KEYS)) - end - end - elsif attributes['id'].blank? - unless reject_new_record?(association_name, attributes) - method = "build_#{association_name}" - if respond_to?(method) - send(method, attributes.except(*UNASSIGNABLE_KEYS)) - else - raise ArgumentError, "Cannot build association #{association_name}. Are you trying to build a polymorphic one-to-one association?" - end + raise ArgumentError, "Cannot build association #{association_name}. Are you trying to build a polymorphic one-to-one association?" end - elsif (existing_record = send(association_name)) && existing_record.id.to_s == attributes['id'].to_s - assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy]) end end diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index 5e4fc2b8d9..60c5bad225 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -247,34 +247,24 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase end def test_should_accept_update_only_option - Pirate.accepts_nested_attributes_for :ship, :update_only => true - @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :name => 'Mayflower' }) - - Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } + @pirate.update_attribute(: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 - Pirate.accepts_nested_attributes_for :ship, :update_only => true @ship.delete - assert_difference('Ship.count', 1) do - @pirate.reload.update_attribute(:ship_attributes, { :name => 'Mayflower' }) + @pirate.reload.update_attribute(:update_only_ship_attributes, { :name => 'Mayflower' }) end - - Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } end - def test_should_update_existing_when_update_only_is_true_and_no_id_is_given - Pirate.accepts_nested_attributes_for :ship, :update_only => true + @ship.delete + @ship = @pirate.create_update_only_ship(:name => 'Nights Dirty Lightning') assert_no_difference('Ship.count') do - @pirate.reload.update_attributes(:ship_attributes => { :name => 'Mayflower' }) + @pirate.update_attributes(:update_only_ship_attributes => { :name => 'Mayflower' }) end - assert_equal 'Mayflower', @ship.reload.name - - Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } end end @@ -393,6 +383,27 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase def test_should_automatically_enable_autosave_on_the_association assert Ship.reflect_on_association(:pirate).options[:autosave] end + + def test_should_accept_update_only_option + @ship.update_attribute(:update_only_pirate_attributes, { :id => @pirate.ship.id, :catchphrase => 'Arr' }) + end + + 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' }) + end + end + + def test_should_update_existing_when_update_only_is_true_and_no_id_is_given + @pirate.delete + @pirate = @ship.create_update_only_pirate(:catchphrase => 'Aye') + + assert_no_difference('Pirate.count') do + @ship.update_attributes(:update_only_pirate_attributes => { :catchphrase => 'Arr' }) + end + assert_equal 'Arr', @pirate.reload.catchphrase + end end module NestedAttributesOnACollectionAssociationTests diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb index f2c05dd48f..88c1634717 100644 --- a/activerecord/test/models/pirate.rb +++ b/activerecord/test/models/pirate.rb @@ -19,6 +19,7 @@ class Pirate < ActiveRecord::Base # These both have :autosave enabled because accepts_nested_attributes_for is used on them. has_one :ship + has_one :update_only_ship, :class_name => 'Ship' has_one :non_validated_ship, :class_name => 'Ship' has_many :birds has_many :birds_with_method_callbacks, :class_name => "Bird", @@ -35,6 +36,7 @@ class Pirate < ActiveRecord::Base accepts_nested_attributes_for :parrots, :birds, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } + accepts_nested_attributes_for :update_only_ship, :update_only => true accepts_nested_attributes_for :parrots_with_method_callbacks, :parrots_with_proc_callbacks, :birds_with_method_callbacks, :birds_with_proc_callbacks, :allow_destroy => true accepts_nested_attributes_for :birds_with_reject_all_blank, :reject_if => :all_blank diff --git a/activerecord/test/models/ship.rb b/activerecord/test/models/ship.rb index 06759d64b8..a96e38ab41 100644 --- a/activerecord/test/models/ship.rb +++ b/activerecord/test/models/ship.rb @@ -2,9 +2,11 @@ class Ship < ActiveRecord::Base self.record_timestamps = false belongs_to :pirate + belongs_to :update_only_pirate, :class_name => 'Pirate' has_many :parts, :class_name => 'ShipPart', :autosave => true accepts_nested_attributes_for :pirate, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } + accepts_nested_attributes_for :update_only_pirate, :update_only => true validates_presence_of :name end -- cgit v1.2.3 From ea7b5ff99e44aac0fc643e02dc4d046ab99ecdc7 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 28 Dec 2009 12:12:53 -0800 Subject: Use present rather than any --- activerecord/lib/active_record/relation.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index c7a74b7763..cb743358b2 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -25,7 +25,7 @@ module ActiveRecord select(r.send(:select_clauses).join(', ')). eager_load(r.eager_load_associations). preload(r.associations_to_preload). - from(r.send(:sources).any? ? r.send(:from_clauses) : nil) + from(r.send(:sources).present? ? r.send(:from_clauses) : nil) end alias :& :merge @@ -158,7 +158,7 @@ module ActiveRecord :conditions => where_clause, :limit => @relation.taken, :offset => @relation.skipped, - :from => (@relation.send(:from_clauses) if @relation.send(:sources).any?) + :from => (@relation.send(:from_clauses) if @relation.send(:sources).present?) }, ActiveRecord::Associations::ClassMethods::JoinDependency.new(@klass, @eager_load_associations, nil)) end -- cgit v1.2.3 From 1ebfd999ec79810cedae6b7d67631088b2d8bf90 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 29 Dec 2009 01:53:15 +0530 Subject: Add ruby-debug to Gemfile for Ruby < 1.9 --- Gemfile | 1 + 1 file changed, 1 insertion(+) diff --git a/Gemfile b/Gemfile index 4ffa9e00e8..015c6ab918 100644 --- a/Gemfile +++ b/Gemfile @@ -1,5 +1,6 @@ gem "rake", ">= 0.8.7" gem "mocha", ">= 0.9.8" +gem "ruby-debug", ">= 0.10.3" if RUBY_VERSION < '1.9' gem "rails", "3.0.pre", :path => "railties" %w(activesupport activemodel actionpack actionmailer activerecord activeresource).each do |lib| -- cgit v1.2.3 From a76af2309ac362ae1f38d24fd4746139deca3417 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 28 Dec 2009 12:28:37 -0800 Subject: rake smoke: toplevel task to quickly smoke-test all projects --- Rakefile | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Rakefile b/Rakefile index cda01d7d57..5ae09160f0 100644 --- a/Rakefile +++ b/Rakefile @@ -24,8 +24,15 @@ task :default => %w(test test:isolated) end end -spec = eval(File.read('rails.gemspec')) +desc "Smoke-test all projects" +task :smoke do + (PROJECTS - %w(activerecord)).each do |project| + system %(cd #{project} && #{env} #{$0} test:isolated) + end + system %(cd activerecord && #{env} #{$0} sqlite3:isolated_test) +end +spec = eval(File.read('rails.gemspec')) Rake::GemPackageTask.new(spec) do |pkg| pkg.gem_spec = spec end -- cgit v1.2.3 From d927265abda1797e86ba6c724f483f94d6b9f51c Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 28 Dec 2009 13:05:36 -0800 Subject: Fix pg test --- activerecord/test/cases/associations/inverse_associations_test.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index 457c4da9bf..1d7604f52b 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -434,7 +434,8 @@ class InverseBelongsToTests < ActiveRecord::TestCase end def test_child_instance_should_be_shared_with_replaced_via_method_parent - f = Face.find(:first) + f = faces(:trusting) + assert_not_nil f.man m = Man.new(:name => 'Charles') f.man.replace(m) assert_not_nil m.face -- cgit v1.2.3 From 1b91f534ce91ff5d0a4d39d96a8f19b58022d403 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 28 Dec 2009 14:06:22 -0800 Subject: Fix uniqueness validation: with_exclusive_scope is not public --- activerecord/lib/active_record/validations/uniqueness.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/validations/uniqueness.rb b/activerecord/lib/active_record/validations/uniqueness.rb index b3a34501dc..ffbe1b5c40 100644 --- a/activerecord/lib/active_record/validations/uniqueness.rb +++ b/activerecord/lib/active_record/validations/uniqueness.rb @@ -22,7 +22,7 @@ module ActiveRecord params << record.send(:id) end - finder_class.with_exclusive_scope do + finder_class.send(:with_exclusive_scope) do if finder_class.exists?([sql, *params]) record.errors.add(attribute, :taken, :default => options[:message], :value => value) end -- cgit v1.2.3 From 949c8c0d0e92e6e7855dc4decc10eb4c658e0ede Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 28 Dec 2009 14:07:23 -0800 Subject: Don't publicize with_scope for tests since it may shadow public misuse --- activerecord/test/cases/associations/eager_test.rb | 8 +- activerecord/test/cases/base_test.rb | 18 +-- activerecord/test/cases/calculations_test.rb | 2 +- activerecord/test/cases/finder_test.rb | 4 +- activerecord/test/cases/helper.rb | 5 - activerecord/test/cases/locking_test.rb | 2 +- activerecord/test/cases/method_scoping_test.rb | 174 ++++++++++----------- activerecord/test/cases/readonly_test.rb | 8 +- .../validations/uniqueness_validation_test.rb | 2 +- activerecord/test/cases/validations_test.rb | 6 +- 10 files changed, 112 insertions(+), 117 deletions(-) diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index 7fa5557b96..ffa6d45948 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -462,7 +462,7 @@ class EagerAssociationTest < ActiveRecord::TestCase def test_eager_with_has_many_and_limit_and_scoped_conditions_on_the_eagers posts = nil - Post.with_scope(:find => { + Post.send(:with_scope, :find => { :include => :comments, :conditions => "comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment'" }) do @@ -470,7 +470,7 @@ class EagerAssociationTest < ActiveRecord::TestCase assert_equal 2, posts.size end - Post.with_scope(:find => { + Post.send(:with_scope, :find => { :include => [ :comments, :author ], :conditions => "authors.name = 'David' AND (comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment')" }) do @@ -480,7 +480,7 @@ class EagerAssociationTest < ActiveRecord::TestCase end def test_eager_with_has_many_and_limit_and_scoped_and_explicit_conditions_on_the_eagers - Post.with_scope(:find => { :conditions => "1=1" }) do + Post.send(:with_scope, :find => { :conditions => "1=1" }) do posts = authors(:david).posts.find(:all, :include => :comments, :conditions => "comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment'", @@ -499,7 +499,7 @@ class EagerAssociationTest < ActiveRecord::TestCase def test_eager_with_scoped_order_using_association_limiting_without_explicit_scope posts_with_explicit_order = Post.find(:all, :conditions => 'comments.id is not null', :include => :comments, :order => 'posts.id DESC', :limit => 2) - posts_with_scoped_order = Post.with_scope(:find => {:order => 'posts.id DESC'}) do + posts_with_scoped_order = Post.send(:with_scope, :find => {:order => 'posts.id DESC'}) do Post.find(:all, :conditions => 'comments.id is not null', :include => :comments, :limit => 2) end assert_equal posts_with_explicit_order, posts_with_scoped_order diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index b51c9f0cb3..ebb717812d 100755 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1825,7 +1825,7 @@ class BasicsTest < ActiveRecord::TestCase end def test_scoped_find_conditions - scoped_developers = Developer.with_scope(:find => { :conditions => 'salary > 90000' }) do + scoped_developers = Developer.send(:with_scope, :find => { :conditions => 'salary > 90000' }) do Developer.find(:all, :conditions => 'id < 5') end assert !scoped_developers.include?(developers(:david)) # David's salary is less than 90,000 @@ -1833,7 +1833,7 @@ class BasicsTest < ActiveRecord::TestCase end def test_scoped_find_limit_offset - scoped_developers = Developer.with_scope(:find => { :limit => 3, :offset => 2 }) do + scoped_developers = Developer.send(:with_scope, :find => { :limit => 3, :offset => 2 }) do Developer.find(:all, :order => 'id') end assert !scoped_developers.include?(developers(:david)) @@ -1847,17 +1847,17 @@ class BasicsTest < ActiveRecord::TestCase def test_scoped_find_order # Test order in scope - scoped_developers = Developer.with_scope(:find => { :limit => 1, :order => 'salary DESC' }) do + scoped_developers = Developer.send(:with_scope, :find => { :limit => 1, :order => 'salary DESC' }) do Developer.find(:all) end assert_equal 'Jamis', scoped_developers.first.name assert scoped_developers.include?(developers(:jamis)) # Test scope without order and order in find - scoped_developers = Developer.with_scope(:find => { :limit => 1 }) do + scoped_developers = Developer.send(:with_scope, :find => { :limit => 1 }) do Developer.find(:all, :order => 'salary DESC') end # Test scope order + find order, find has priority - scoped_developers = Developer.with_scope(:find => { :limit => 3, :order => 'id DESC' }) do + scoped_developers = Developer.send(:with_scope, :find => { :limit => 3, :order => 'id DESC' }) do Developer.find(:all, :order => 'salary ASC') end assert scoped_developers.include?(developers(:poor_jamis)) @@ -1869,7 +1869,7 @@ class BasicsTest < ActiveRecord::TestCase end def test_scoped_find_limit_offset_including_has_many_association - topics = Topic.with_scope(:find => {:limit => 1, :offset => 1, :include => :replies}) do + topics = Topic.send(:with_scope, :find => {:limit => 1, :offset => 1, :include => :replies}) do Topic.find(:all, :order => "topics.id") end assert_equal 1, topics.size @@ -1877,7 +1877,7 @@ class BasicsTest < ActiveRecord::TestCase end def test_scoped_find_order_including_has_many_association - developers = Developer.with_scope(:find => { :order => 'developers.salary DESC', :include => :projects }) do + developers = Developer.send(:with_scope, :find => { :order => 'developers.salary DESC', :include => :projects }) do Developer.find(:all) end assert developers.size >= 2 @@ -1887,7 +1887,7 @@ class BasicsTest < ActiveRecord::TestCase end def test_scoped_find_with_group_and_having - developers = Developer.with_scope(:find => { :group => 'developers.salary', :having => "SUM(salary) > 10000", :select => "SUM(salary) as salary" }) do + developers = Developer.send(:with_scope, :find => { :group => 'developers.salary', :having => "SUM(salary) > 10000", :select => "SUM(salary) as salary" }) do Developer.find(:all) end assert_equal 3, developers.size @@ -1933,7 +1933,7 @@ class BasicsTest < ActiveRecord::TestCase end def test_find_scoped_ordered_last - last_developer = Developer.with_scope(:find => { :order => 'developers.salary ASC' }) do + last_developer = Developer.send(:with_scope, :find => { :order => 'developers.salary ASC' }) do Developer.find(:last) end assert_equal last_developer, Developer.find(:all, :order => 'developers.salary ASC').last diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 004f4d0ea6..7e4c4a0913 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -42,7 +42,7 @@ class CalculationsTest < ActiveRecord::TestCase end def test_should_get_maximum_of_field_with_scoped_include - Account.with_scope :find => { :include => :firm, :conditions => "companies.name != 'Summit'" } do + Account.send :with_scope, :find => { :include => :firm, :conditions => "companies.name != 'Summit'" } do assert_equal 50, Account.maximum(:credit_limit) end end diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index 87a9630978..d2451f24c1 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -120,7 +120,7 @@ class FinderTest < ActiveRecord::TestCase end def test_exists_with_scoped_include - Developer.with_scope(:find => { :include => :projects, :order => "projects.name" }) do + Developer.send(:with_scope, :find => { :include => :projects, :order => "projects.name" }) do assert Developer.exists? end end @@ -1022,7 +1022,7 @@ class FinderTest < ActiveRecord::TestCase def test_finder_with_scoped_from all_topics = Topic.find(:all) - Topic.with_scope(:find => { :from => 'fake_topics' }) do + Topic.send(:with_scope, :find => { :from => 'fake_topics' }) do assert_equal all_topics, Topic.from('topics').to_a end end diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index 243c05e665..479970b2fa 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -47,11 +47,6 @@ ActiveRecord::Base.connection.class.class_eval do alias_method_chain :execute, :query_record end -# Make with_scope public for tests -class << ActiveRecord::Base - public :with_scope, :with_exclusive_scope -end - unless ENV['FIXTURE_DEBUG'] module ActiveRecord::TestFixtures::ClassMethods def try_to_load_dependency_with_silence(*args) diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb index a64c01292f..dfaecf35cf 100644 --- a/activerecord/test/cases/locking_test.rb +++ b/activerecord/test/cases/locking_test.rb @@ -225,7 +225,7 @@ unless current_adapter?(:SybaseAdapter, :OpenBaseAdapter) def test_sane_find_with_scoped_lock assert_nothing_raised do Person.transaction do - Person.with_scope(:find => { :lock => true }) do + Person.send(:with_scope, :find => { :lock => true }) do Person.find 1 end end diff --git a/activerecord/test/cases/method_scoping_test.rb b/activerecord/test/cases/method_scoping_test.rb index eb4ce0e774..cfc6f8772c 100644 --- a/activerecord/test/cases/method_scoping_test.rb +++ b/activerecord/test/cases/method_scoping_test.rb @@ -10,19 +10,19 @@ class MethodScopingTest < ActiveRecord::TestCase fixtures :authors, :developers, :projects, :comments, :posts, :developers_projects def test_set_conditions - Developer.with_scope(:find => { :conditions => 'just a test...' }) do + Developer.send(:with_scope, :find => { :conditions => 'just a test...' }) do assert_equal 'just a test...', Developer.send(:current_scoped_methods)[:find][:conditions] end end def test_scoped_find - Developer.with_scope(:find => { :conditions => "name = 'David'" }) do + Developer.send(:with_scope, :find => { :conditions => "name = 'David'" }) do assert_nothing_raised { Developer.find(1) } end end def test_scoped_find_first - Developer.with_scope(:find => { :conditions => "salary = 100000" }) do + Developer.send(:with_scope, :find => { :conditions => "salary = 100000" }) do assert_equal Developer.find(10), Developer.find(:first, :order => 'name') end end @@ -30,7 +30,7 @@ class MethodScopingTest < ActiveRecord::TestCase def test_scoped_find_last highest_salary = Developer.find(:first, :order => "salary DESC") - Developer.with_scope(:find => { :order => "salary" }) do + Developer.send(:with_scope, :find => { :order => "salary" }) do assert_equal highest_salary, Developer.last end end @@ -39,38 +39,38 @@ class MethodScopingTest < ActiveRecord::TestCase lowest_salary = Developer.find(:first, :order => "salary ASC") highest_salary = Developer.find(:first, :order => "salary DESC") - Developer.with_scope(:find => { :order => "salary" }) do + Developer.send(:with_scope, :find => { :order => "salary" }) do assert_equal highest_salary, Developer.last assert_equal lowest_salary, Developer.first end end def test_scoped_find_combines_conditions - Developer.with_scope(:find => { :conditions => "salary = 9000" }) do + Developer.send(:with_scope, :find => { :conditions => "salary = 9000" }) do assert_equal developers(:poor_jamis), Developer.find(:first, :conditions => "name = 'Jamis'") end end def test_scoped_find_sanitizes_conditions - Developer.with_scope(:find => { :conditions => ['salary = ?', 9000] }) do + Developer.send(:with_scope, :find => { :conditions => ['salary = ?', 9000] }) do assert_equal developers(:poor_jamis), Developer.find(:first) end end def test_scoped_find_combines_and_sanitizes_conditions - Developer.with_scope(:find => { :conditions => ['salary = ?', 9000] }) do + Developer.send(:with_scope, :find => { :conditions => ['salary = ?', 9000] }) do assert_equal developers(:poor_jamis), Developer.find(:first, :conditions => ['name = ?', 'Jamis']) end end def test_scoped_find_all - Developer.with_scope(:find => { :conditions => "name = 'David'" }) do + Developer.send(:with_scope, :find => { :conditions => "name = 'David'" }) do assert_equal [developers(:david)], Developer.find(:all) end end def test_scoped_find_select - Developer.with_scope(:find => { :select => "id, name" }) do + Developer.send(:with_scope, :find => { :select => "id, name" }) do developer = Developer.find(:first, :conditions => "name = 'David'") assert_equal "David", developer.name assert !developer.has_attribute?(:salary) @@ -78,7 +78,7 @@ class MethodScopingTest < ActiveRecord::TestCase end def test_options_select_replaces_scope_select - Developer.with_scope(:find => { :select => "id, name" }) do + Developer.send(:with_scope, :find => { :select => "id, name" }) do developer = Developer.find(:first, :select => 'id, salary', :conditions => "name = 'David'") assert_equal 80000, developer.salary assert !developer.has_attribute?(:name) @@ -86,11 +86,11 @@ class MethodScopingTest < ActiveRecord::TestCase end def test_scoped_count - Developer.with_scope(:find => { :conditions => "name = 'David'" }) do + Developer.send(:with_scope, :find => { :conditions => "name = 'David'" }) do assert_equal 1, Developer.count end - Developer.with_scope(:find => { :conditions => 'salary = 100000' }) do + Developer.send(:with_scope, :find => { :conditions => 'salary = 100000' }) do assert_equal 8, Developer.count assert_equal 1, Developer.count(:conditions => "name LIKE 'fixture_1%'") end @@ -98,7 +98,7 @@ class MethodScopingTest < ActiveRecord::TestCase def test_scoped_find_include # with the include, will retrieve only developers for the given project - scoped_developers = Developer.with_scope(:find => { :include => :projects }) do + scoped_developers = Developer.send(:with_scope, :find => { :include => :projects }) do Developer.find(:all, :conditions => 'projects.id = 2') end assert scoped_developers.include?(developers(:david)) @@ -107,7 +107,7 @@ class MethodScopingTest < ActiveRecord::TestCase end def test_scoped_find_joins - scoped_developers = Developer.with_scope(:find => { :joins => 'JOIN developers_projects ON id = developer_id' } ) do + scoped_developers = Developer.send(:with_scope, :find => { :joins => 'JOIN developers_projects ON id = developer_id' } ) do Developer.find(:all, :conditions => 'developers_projects.project_id = 2') end assert scoped_developers.include?(developers(:david)) @@ -117,7 +117,7 @@ class MethodScopingTest < ActiveRecord::TestCase end def test_scoped_find_using_new_style_joins - scoped_developers = Developer.with_scope(:find => { :joins => :projects }) do + scoped_developers = Developer.send(:with_scope, :find => { :joins => :projects }) do Developer.find(:all, :conditions => 'projects.id = 2') end assert scoped_developers.include?(developers(:david)) @@ -127,7 +127,7 @@ class MethodScopingTest < ActiveRecord::TestCase end def test_scoped_find_merges_old_style_joins - scoped_authors = Author.with_scope(:find => { :joins => 'INNER JOIN posts ON authors.id = posts.author_id ' }) do + scoped_authors = Author.send(:with_scope, :find => { :joins => 'INNER JOIN posts ON authors.id = posts.author_id ' }) do Author.find(:all, :select => 'DISTINCT authors.*', :joins => 'INNER JOIN comments ON posts.id = comments.post_id', :conditions => 'comments.id = 1') end assert scoped_authors.include?(authors(:david)) @@ -137,7 +137,7 @@ class MethodScopingTest < ActiveRecord::TestCase end def test_scoped_find_merges_new_style_joins - scoped_authors = Author.with_scope(:find => { :joins => :posts }) do + scoped_authors = Author.send(:with_scope, :find => { :joins => :posts }) do Author.find(:all, :select => 'DISTINCT authors.*', :joins => :comments, :conditions => 'comments.id = 1') end assert scoped_authors.include?(authors(:david)) @@ -147,7 +147,7 @@ class MethodScopingTest < ActiveRecord::TestCase end def test_scoped_find_merges_new_and_old_style_joins - scoped_authors = Author.with_scope(:find => { :joins => :posts }) do + scoped_authors = Author.send(:with_scope, :find => { :joins => :posts }) do Author.find(:all, :select => 'DISTINCT authors.*', :joins => 'JOIN comments ON posts.id = comments.post_id', :conditions => 'comments.id = 1') end assert scoped_authors.include?(authors(:david)) @@ -157,7 +157,7 @@ class MethodScopingTest < ActiveRecord::TestCase end def test_scoped_find_merges_string_array_style_and_string_style_joins - scoped_authors = Author.with_scope(:find => { :joins => ["INNER JOIN posts ON posts.author_id = authors.id"]}) do + scoped_authors = Author.send(:with_scope, :find => { :joins => ["INNER JOIN posts ON posts.author_id = authors.id"]}) do Author.find(:all, :select => 'DISTINCT authors.*', :joins => 'INNER JOIN comments ON posts.id = comments.post_id', :conditions => 'comments.id = 1') end assert scoped_authors.include?(authors(:david)) @@ -167,7 +167,7 @@ class MethodScopingTest < ActiveRecord::TestCase end def test_scoped_find_merges_string_array_style_and_hash_style_joins - scoped_authors = Author.with_scope(:find => { :joins => :posts}) do + scoped_authors = Author.send(:with_scope, :find => { :joins => :posts}) do Author.find(:all, :select => 'DISTINCT authors.*', :joins => ['INNER JOIN comments ON posts.id = comments.post_id'], :conditions => 'comments.id = 1') end assert scoped_authors.include?(authors(:david)) @@ -177,7 +177,7 @@ class MethodScopingTest < ActiveRecord::TestCase end def test_scoped_find_merges_joins_and_eliminates_duplicate_string_joins - scoped_authors = Author.with_scope(:find => { :joins => 'INNER JOIN posts ON posts.author_id = authors.id'}) do + scoped_authors = Author.send(:with_scope, :find => { :joins => 'INNER JOIN posts ON posts.author_id = authors.id'}) do Author.find(:all, :select => 'DISTINCT authors.*', :joins => ["INNER JOIN posts ON posts.author_id = authors.id", "INNER JOIN comments ON posts.id = comments.post_id"], :conditions => 'comments.id = 1') end assert scoped_authors.include?(authors(:david)) @@ -187,7 +187,7 @@ class MethodScopingTest < ActiveRecord::TestCase end def test_scoped_find_strips_spaces_from_string_joins_and_eliminates_duplicate_string_joins - scoped_authors = Author.with_scope(:find => { :joins => ' INNER JOIN posts ON posts.author_id = authors.id '}) do + scoped_authors = Author.send(:with_scope, :find => { :joins => ' INNER JOIN posts ON posts.author_id = authors.id '}) do Author.find(:all, :select => 'DISTINCT authors.*', :joins => ['INNER JOIN posts ON posts.author_id = authors.id'], :conditions => 'posts.id = 1') end assert scoped_authors.include?(authors(:david)) @@ -198,7 +198,7 @@ class MethodScopingTest < ActiveRecord::TestCase def test_scoped_count_include # with the include, will retrieve only developers for the given project - Developer.with_scope(:find => { :include => :projects }) do + Developer.send(:with_scope, :find => { :include => :projects }) do assert_equal 1, Developer.count(:conditions => 'projects.id = 2') end end @@ -206,7 +206,7 @@ class MethodScopingTest < ActiveRecord::TestCase def test_scoped_create new_comment = nil - VerySpecialComment.with_scope(:create => { :post_id => 1 }) do + VerySpecialComment.send(:with_scope, :create => { :post_id => 1 }) do assert_equal({ :post_id => 1 }, VerySpecialComment.send(:current_scoped_methods)[:create]) new_comment = VerySpecialComment.create :body => "Wonderful world" end @@ -216,14 +216,14 @@ class MethodScopingTest < ActiveRecord::TestCase def test_immutable_scope options = { :conditions => "name = 'David'" } - Developer.with_scope(:find => options) do + Developer.send(:with_scope, :find => options) do assert_equal %w(David), Developer.find(:all).map { |d| d.name } options[:conditions] = "name != 'David'" assert_equal %w(David), Developer.find(:all).map { |d| d.name } end scope = { :find => { :conditions => "name = 'David'" }} - Developer.with_scope(scope) do + Developer.send(:with_scope, scope) do assert_equal %w(David), Developer.find(:all).map { |d| d.name } scope[:find][:conditions] = "name != 'David'" assert_equal %w(David), Developer.find(:all).map { |d| d.name } @@ -232,7 +232,7 @@ class MethodScopingTest < ActiveRecord::TestCase def test_scoped_with_duck_typing scoping = Struct.new(:method_scoping).new(:find => { :conditions => ["name = ?", 'David'] }) - Developer.with_scope(scoping) do + Developer.send(:with_scope, scoping) do assert_equal %w(David), Developer.find(:all).map { |d| d.name } end end @@ -241,7 +241,7 @@ class MethodScopingTest < ActiveRecord::TestCase scoped_methods = Developer.instance_eval('current_scoped_methods') begin - Developer.with_scope(:find => { :conditions => "name = 'Jamis'" }) do + Developer.send(:with_scope, :find => { :conditions => "name = 'Jamis'" }) do raise "an exception" end rescue @@ -254,8 +254,8 @@ class NestedScopingTest < ActiveRecord::TestCase fixtures :authors, :developers, :projects, :comments, :posts def test_merge_options - Developer.with_scope(:find => { :conditions => 'salary = 80000' }) do - Developer.with_scope(:find => { :limit => 10 }) do + Developer.send(:with_scope, :find => { :conditions => 'salary = 80000' }) do + Developer.send(:with_scope, :find => { :limit => 10 }) do merged_option = Developer.instance_eval('current_scoped_methods')[:find] assert_equal({ :conditions => 'salary = 80000', :limit => 10 }, merged_option) end @@ -263,8 +263,8 @@ class NestedScopingTest < ActiveRecord::TestCase end def test_merge_inner_scope_has_priority - Developer.with_scope(:find => { :limit => 5 }) do - Developer.with_scope(:find => { :limit => 10 }) do + Developer.send(:with_scope, :find => { :limit => 5 }) do + Developer.send(:with_scope, :find => { :limit => 10 }) do merged_option = Developer.instance_eval('current_scoped_methods')[:find] assert_equal({ :limit => 10 }, merged_option) end @@ -272,8 +272,8 @@ class NestedScopingTest < ActiveRecord::TestCase end def test_replace_options - Developer.with_scope(:find => { :conditions => "name = 'David'" }) do - Developer.with_exclusive_scope(:find => { :conditions => "name = 'Jamis'" }) do + Developer.send(:with_scope, :find => { :conditions => "name = 'David'" }) do + Developer.send(:with_exclusive_scope, :find => { :conditions => "name = 'Jamis'" }) do assert_equal({:find => { :conditions => "name = 'Jamis'" }}, Developer.instance_eval('current_scoped_methods')) assert_equal({:find => { :conditions => "name = 'Jamis'" }}, Developer.send(:scoped_methods)[-1]) end @@ -281,21 +281,21 @@ class NestedScopingTest < ActiveRecord::TestCase end def test_append_conditions - Developer.with_scope(:find => { :conditions => "name = 'David'" }) do - Developer.with_scope(:find => { :conditions => 'salary = 80000' }) do + Developer.send(:with_scope, :find => { :conditions => "name = 'David'" }) do + Developer.send(:with_scope, :find => { :conditions => 'salary = 80000' }) do appended_condition = Developer.instance_eval('current_scoped_methods')[:find][:conditions] assert_equal("(name = 'David') AND (salary = 80000)", appended_condition) assert_equal(1, Developer.count) end - Developer.with_scope(:find => { :conditions => "name = 'Maiha'" }) do + Developer.send(:with_scope, :find => { :conditions => "name = 'Maiha'" }) do assert_equal(0, Developer.count) end end end def test_merge_and_append_options - Developer.with_scope(:find => { :conditions => 'salary = 80000', :limit => 10 }) do - Developer.with_scope(:find => { :conditions => "name = 'David'" }) do + Developer.send(:with_scope, :find => { :conditions => 'salary = 80000', :limit => 10 }) do + Developer.send(:with_scope, :find => { :conditions => "name = 'David'" }) do merged_option = Developer.instance_eval('current_scoped_methods')[:find] assert_equal({ :conditions => "(salary = 80000) AND (name = 'David')", :limit => 10 }, merged_option) end @@ -303,8 +303,8 @@ class NestedScopingTest < ActiveRecord::TestCase end def test_nested_scoped_find - Developer.with_scope(:find => { :conditions => "name = 'Jamis'" }) do - Developer.with_exclusive_scope(:find => { :conditions => "name = 'David'" }) do + Developer.send(:with_scope, :find => { :conditions => "name = 'Jamis'" }) do + Developer.send(:with_exclusive_scope, :find => { :conditions => "name = 'David'" }) do assert_nothing_raised { Developer.find(1) } assert_equal('David', Developer.find(:first).name) end @@ -313,8 +313,8 @@ class NestedScopingTest < ActiveRecord::TestCase end def test_nested_scoped_find_include - Developer.with_scope(:find => { :include => :projects }) do - Developer.with_scope(:find => { :conditions => "projects.id = 2" }) do + Developer.send(:with_scope, :find => { :include => :projects }) do + Developer.send(:with_scope, :find => { :conditions => "projects.id = 2" }) do assert_nothing_raised { Developer.find(1) } assert_equal('David', Developer.find(:first).name) end @@ -323,24 +323,24 @@ class NestedScopingTest < ActiveRecord::TestCase def test_nested_scoped_find_merged_include # :include's remain unique and don't "double up" when merging - Developer.with_scope(:find => { :include => :projects, :conditions => "projects.id = 2" }) do - Developer.with_scope(:find => { :include => :projects }) do + Developer.send(:with_scope, :find => { :include => :projects, :conditions => "projects.id = 2" }) do + Developer.send(:with_scope, :find => { :include => :projects }) do assert_equal 1, Developer.instance_eval('current_scoped_methods')[:find][:include].length assert_equal('David', Developer.find(:first).name) end end # the nested scope doesn't remove the first :include - Developer.with_scope(:find => { :include => :projects, :conditions => "projects.id = 2" }) do - Developer.with_scope(:find => { :include => [] }) do + Developer.send(:with_scope, :find => { :include => :projects, :conditions => "projects.id = 2" }) do + Developer.send(:with_scope, :find => { :include => [] }) do assert_equal 1, Developer.instance_eval('current_scoped_methods')[:find][:include].length assert_equal('David', Developer.find(:first).name) end end # mixing array and symbol include's will merge correctly - Developer.with_scope(:find => { :include => [:projects], :conditions => "projects.id = 2" }) do - Developer.with_scope(:find => { :include => :projects }) do + Developer.send(:with_scope, :find => { :include => [:projects], :conditions => "projects.id = 2" }) do + Developer.send(:with_scope, :find => { :include => :projects }) do assert_equal 1, Developer.instance_eval('current_scoped_methods')[:find][:include].length assert_equal('David', Developer.find(:first).name) end @@ -348,21 +348,21 @@ class NestedScopingTest < ActiveRecord::TestCase end def test_nested_scoped_find_replace_include - Developer.with_scope(:find => { :include => :projects }) do - Developer.with_exclusive_scope(:find => { :include => [] }) do + Developer.send(:with_scope, :find => { :include => :projects }) do + Developer.send(:with_exclusive_scope, :find => { :include => [] }) do assert_equal 0, Developer.instance_eval('current_scoped_methods')[:find][:include].length end end end def test_three_level_nested_exclusive_scoped_find - Developer.with_scope(:find => { :conditions => "name = 'Jamis'" }) do + Developer.send(:with_scope, :find => { :conditions => "name = 'Jamis'" }) do assert_equal('Jamis', Developer.find(:first).name) - Developer.with_exclusive_scope(:find => { :conditions => "name = 'David'" }) do + Developer.send(:with_exclusive_scope, :find => { :conditions => "name = 'David'" }) do assert_equal('David', Developer.find(:first).name) - Developer.with_exclusive_scope(:find => { :conditions => "name = 'Maiha'" }) do + Developer.send(:with_exclusive_scope, :find => { :conditions => "name = 'Maiha'" }) do assert_equal(nil, Developer.find(:first)) end @@ -377,8 +377,8 @@ class NestedScopingTest < ActiveRecord::TestCase def test_merged_scoped_find poor_jamis = developers(:poor_jamis) - Developer.with_scope(:find => { :conditions => "salary < 100000" }) do - Developer.with_scope(:find => { :offset => 1, :order => 'id asc' }) do + Developer.send(:with_scope, :find => { :conditions => "salary < 100000" }) do + Developer.send(:with_scope, :find => { :offset => 1, :order => 'id asc' }) do # Oracle adapter does not generated space after asc therefore trailing space removed from regex assert_sql /ORDER BY id asc/ do assert_equal(poor_jamis, Developer.find(:first, :order => 'id asc')) @@ -388,16 +388,16 @@ class NestedScopingTest < ActiveRecord::TestCase end def test_merged_scoped_find_sanitizes_conditions - Developer.with_scope(:find => { :conditions => ["name = ?", 'David'] }) do - Developer.with_scope(:find => { :conditions => ['salary = ?', 9000] }) do + Developer.send(:with_scope, :find => { :conditions => ["name = ?", 'David'] }) do + Developer.send(:with_scope, :find => { :conditions => ['salary = ?', 9000] }) do assert_raise(ActiveRecord::RecordNotFound) { developers(:poor_jamis) } end end end def test_nested_scoped_find_combines_and_sanitizes_conditions - Developer.with_scope(:find => { :conditions => ["name = ?", 'David'] }) do - Developer.with_exclusive_scope(:find => { :conditions => ['salary = ?', 9000] }) do + Developer.send(:with_scope, :find => { :conditions => ["name = ?", 'David'] }) do + Developer.send(:with_exclusive_scope, :find => { :conditions => ['salary = ?', 9000] }) do assert_equal developers(:poor_jamis), Developer.find(:first) assert_equal developers(:poor_jamis), Developer.find(:first, :conditions => ['name = ?', 'Jamis']) end @@ -405,8 +405,8 @@ class NestedScopingTest < ActiveRecord::TestCase end def test_merged_scoped_find_combines_and_sanitizes_conditions - Developer.with_scope(:find => { :conditions => ["name = ?", 'David'] }) do - Developer.with_scope(:find => { :conditions => ['salary > ?', 9000] }) do + Developer.send(:with_scope, :find => { :conditions => ["name = ?", 'David'] }) do + Developer.send(:with_scope, :find => { :conditions => ['salary > ?', 9000] }) do assert_equal %w(David), Developer.find(:all).map { |d| d.name } end end @@ -414,8 +414,8 @@ class NestedScopingTest < ActiveRecord::TestCase def test_nested_scoped_create comment = nil - Comment.with_scope(:create => { :post_id => 1}) do - Comment.with_scope(:create => { :post_id => 2}) do + Comment.send(:with_scope, :create => { :post_id => 1}) do + Comment.send(:with_scope, :create => { :post_id => 2}) do assert_equal({ :post_id => 2 }, Comment.send(:current_scoped_methods)[:create]) comment = Comment.create :body => "Hey guys, nested scopes are broken. Please fix!" end @@ -425,8 +425,8 @@ class NestedScopingTest < ActiveRecord::TestCase def test_nested_exclusive_scope_for_create comment = nil - Comment.with_scope(:create => { :body => "Hey guys, nested scopes are broken. Please fix!" }) do - Comment.with_exclusive_scope(:create => { :post_id => 1 }) do + Comment.send(:with_scope, :create => { :body => "Hey guys, nested scopes are broken. Please fix!" }) do + Comment.send(:with_exclusive_scope, :create => { :post_id => 1 }) do assert_equal({ :post_id => 1 }, Comment.send(:current_scoped_methods)[:create]) comment = Comment.create :body => "Hey guys" end @@ -437,8 +437,8 @@ class NestedScopingTest < ActiveRecord::TestCase def test_merged_scoped_find_on_blank_conditions [nil, " ", [], {}].each do |blank| - Developer.with_scope(:find => {:conditions => blank}) do - Developer.with_scope(:find => {:conditions => blank}) do + Developer.send(:with_scope, :find => {:conditions => blank}) do + Developer.send(:with_scope, :find => {:conditions => blank}) do assert_nothing_raised { Developer.find(:first) } end end @@ -447,8 +447,8 @@ class NestedScopingTest < ActiveRecord::TestCase def test_merged_scoped_find_on_blank_bind_conditions [ [""], ["",{}] ].each do |blank| - Developer.with_scope(:find => {:conditions => blank}) do - Developer.with_scope(:find => {:conditions => blank}) do + Developer.send(:with_scope, :find => {:conditions => blank}) do + Developer.send(:with_scope, :find => {:conditions => blank}) do assert_nothing_raised { Developer.find(:first) } end end @@ -458,8 +458,8 @@ class NestedScopingTest < ActiveRecord::TestCase def test_immutable_nested_scope options1 = { :conditions => "name = 'Jamis'" } options2 = { :conditions => "name = 'David'" } - Developer.with_scope(:find => options1) do - Developer.with_exclusive_scope(:find => options2) do + Developer.send(:with_scope, :find => options1) do + Developer.send(:with_exclusive_scope, :find => options2) do assert_equal %w(David), Developer.find(:all).map { |d| d.name } options1[:conditions] = options2[:conditions] = nil assert_equal %w(David), Developer.find(:all).map { |d| d.name } @@ -470,8 +470,8 @@ class NestedScopingTest < ActiveRecord::TestCase def test_immutable_merged_scope options1 = { :conditions => "name = 'Jamis'" } options2 = { :conditions => "salary > 10000" } - Developer.with_scope(:find => options1) do - Developer.with_scope(:find => options2) do + Developer.send(:with_scope, :find => options1) do + Developer.send(:with_scope, :find => options2) do assert_equal %w(Jamis), Developer.find(:all).map { |d| d.name } options1[:conditions] = options2[:conditions] = nil assert_equal %w(Jamis), Developer.find(:all).map { |d| d.name } @@ -480,10 +480,10 @@ class NestedScopingTest < ActiveRecord::TestCase end def test_ensure_that_method_scoping_is_correctly_restored - Developer.with_scope(:find => { :conditions => "name = 'David'" }) do + Developer.send(:with_scope, :find => { :conditions => "name = 'David'" }) do scoped_methods = Developer.instance_eval('current_scoped_methods') begin - Developer.with_scope(:find => { :conditions => "name = 'Maiha'" }) do + Developer.send(:with_scope, :find => { :conditions => "name = 'Maiha'" }) do raise "an exception" end rescue @@ -493,8 +493,8 @@ class NestedScopingTest < ActiveRecord::TestCase end def test_nested_scoped_find_merges_old_style_joins - scoped_authors = Author.with_scope(:find => { :joins => 'INNER JOIN posts ON authors.id = posts.author_id' }) do - Author.with_scope(:find => { :joins => 'INNER JOIN comments ON posts.id = comments.post_id' }) do + scoped_authors = Author.send(:with_scope, :find => { :joins => 'INNER JOIN posts ON authors.id = posts.author_id' }) do + Author.send(:with_scope, :find => { :joins => 'INNER JOIN comments ON posts.id = comments.post_id' }) do Author.find(:all, :select => 'DISTINCT authors.*', :conditions => 'comments.id = 1') end end @@ -505,8 +505,8 @@ class NestedScopingTest < ActiveRecord::TestCase end def test_nested_scoped_find_merges_new_style_joins - scoped_authors = Author.with_scope(:find => { :joins => :posts }) do - Author.with_scope(:find => { :joins => :comments }) do + scoped_authors = Author.send(:with_scope, :find => { :joins => :posts }) do + Author.send(:with_scope, :find => { :joins => :comments }) do Author.find(:all, :select => 'DISTINCT authors.*', :conditions => 'comments.id = 1') end end @@ -517,8 +517,8 @@ class NestedScopingTest < ActiveRecord::TestCase end def test_nested_scoped_find_merges_new_and_old_style_joins - scoped_authors = Author.with_scope(:find => { :joins => :posts }) do - Author.with_scope(:find => { :joins => 'INNER JOIN comments ON posts.id = comments.post_id' }) do + scoped_authors = Author.send(:with_scope, :find => { :joins => :posts }) do + Author.send(:with_scope, :find => { :joins => 'INNER JOIN comments ON posts.id = comments.post_id' }) do Author.find(:all, :select => 'DISTINCT authors.*', :joins => '', :conditions => 'comments.id = 1') end end @@ -552,7 +552,7 @@ class HasManyScopingTest< ActiveRecord::TestCase end def test_nested_scope - Comment.with_scope(:find => { :conditions => '1=1' }) do + Comment.send(:with_scope, :find => { :conditions => '1=1' }) do assert_equal 'a comment...', @welcome.comments.what_are_you end end @@ -577,7 +577,7 @@ class HasAndBelongsToManyScopingTest< ActiveRecord::TestCase end def test_nested_scope - Category.with_scope(:find => { :conditions => '1=1' }) do + Category.send(:with_scope, :find => { :conditions => '1=1' }) do assert_equal 'a comment...', @welcome.comments.what_are_you end end @@ -633,7 +633,7 @@ class DefaultScopingTest < ActiveRecord::TestCase def test_nested_scope expected = Developer.find(:all, :order => 'name DESC').collect { |dev| dev.salary } - received = DeveloperOrderedBySalary.with_scope(:find => { :order => 'name DESC'}) do + received = DeveloperOrderedBySalary.send(:with_scope, :find => { :order => 'name DESC'}) do DeveloperOrderedBySalary.find(:all).collect { |dev| dev.salary } end assert_equal expected, received @@ -647,7 +647,7 @@ class DefaultScopingTest < ActiveRecord::TestCase def test_nested_exclusive_scope expected = Developer.find(:all, :limit => 100).collect { |dev| dev.salary } - received = DeveloperOrderedBySalary.with_exclusive_scope(:find => { :limit => 100 }) do + received = DeveloperOrderedBySalary.send(:with_exclusive_scope, :find => { :limit => 100 }) do DeveloperOrderedBySalary.find(:all).collect { |dev| dev.salary } end assert_equal expected, received diff --git a/activerecord/test/cases/readonly_test.rb b/activerecord/test/cases/readonly_test.rb index d2ef4fd6d2..98011f40a4 100644 --- a/activerecord/test/cases/readonly_test.rb +++ b/activerecord/test/cases/readonly_test.rb @@ -72,13 +72,13 @@ class ReadOnlyTest < ActiveRecord::TestCase end def test_readonly_scoping - Post.with_scope(:find => { :conditions => '1=1' }) do + Post.send(:with_scope, :find => { :conditions => '1=1' }) do assert !Post.find(1).readonly? assert Post.readonly(true).find(1).readonly? assert !Post.readonly(false).find(1).readonly? end - Post.with_scope(:find => { :joins => ' ' }) do + Post.send(:with_scope, :find => { :joins => ' ' }) do assert !Post.find(1).readonly? assert Post.readonly.find(1).readonly? assert !Post.readonly(false).find(1).readonly? @@ -87,14 +87,14 @@ class ReadOnlyTest < ActiveRecord::TestCase # Oracle barfs on this because the join includes unqualified and # conflicting column names unless current_adapter?(:OracleAdapter) - Post.with_scope(:find => { :joins => ', developers' }) do + Post.send(:with_scope, :find => { :joins => ', developers' }) do assert Post.find(1).readonly? assert Post.readonly.find(1).readonly? assert !Post.readonly(false).find(1).readonly? end end - Post.with_scope(:find => { :readonly => true }) do + Post.send(:with_scope, :find => { :readonly => true }) do assert Post.find(1).readonly? assert Post.readonly.find(1).readonly? assert !Post.readonly(false).find(1).readonly? diff --git a/activerecord/test/cases/validations/uniqueness_validation_test.rb b/activerecord/test/cases/validations/uniqueness_validation_test.rb index 17ba4e2f8a..db633339f3 100644 --- a/activerecord/test/cases/validations/uniqueness_validation_test.rb +++ b/activerecord/test/cases/validations/uniqueness_validation_test.rb @@ -213,7 +213,7 @@ class UniquenessValidationTest < ActiveRecord::TestCase def test_validates_uniqueness_inside_with_scope Topic.validates_uniqueness_of(:title) - Topic.with_scope(:find => { :conditions => { :author_name => "David" } }) do + Topic.send(:with_scope, :find => { :conditions => { :author_name => "David" } }) do t1 = Topic.new("title" => "I'm unique!", "author_name" => "Mary") assert t1.save t2 = Topic.new("title" => "I'm unique!", "author_name" => "David") diff --git a/activerecord/test/cases/validations_test.rb b/activerecord/test/cases/validations_test.rb index 130231c622..7462d944e0 100644 --- a/activerecord/test/cases/validations_test.rb +++ b/activerecord/test/cases/validations_test.rb @@ -98,14 +98,14 @@ class ValidationsTest < ActiveRecord::TestCase end def test_scoped_create_without_attributes - Reply.with_scope(:create => {}) do + Reply.send(:with_scope, :create => {}) do assert_raise(ActiveRecord::RecordInvalid) { Reply.create! } end end def test_create_with_exceptions_using_scope_for_protected_attributes assert_nothing_raised do - ProtectedPerson.with_scope( :create => { :first_name => "Mary" } ) do + ProtectedPerson.send(:with_scope, :create => { :first_name => "Mary" } ) do person = ProtectedPerson.create! :addon => "Addon" assert_equal person.first_name, "Mary", "scope should ignore attr_protected" end @@ -114,7 +114,7 @@ class ValidationsTest < ActiveRecord::TestCase def test_create_with_exceptions_using_scope_and_empty_attributes assert_nothing_raised do - ProtectedPerson.with_scope( :create => { :first_name => "Mary" } ) do + ProtectedPerson.send(:with_scope, :create => { :first_name => "Mary" } ) do person = ProtectedPerson.create! assert_equal person.first_name, "Mary", "should be ok when no attributes are passed to create!" end -- cgit v1.2.3 From 08633bae5e4f05e913ec5d5d2483bfd6c07c7375 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 29 Dec 2009 04:28:43 +0530 Subject: Migrate all the calculation methods to Relation --- activerecord/lib/active_record/associations.rb | 11 +- .../associations/association_collection.rb | 2 +- activerecord/lib/active_record/base.rb | 4 + activerecord/lib/active_record/calculations.rb | 200 ++++++--------------- activerecord/lib/active_record/relation.rb | 7 +- .../lib/active_record/relational_calculations.rb | 147 +++++++++++++-- .../associations/inner_join_association_test.rb | 2 + activerecord/test/cases/calculations_test.rb | 18 +- 8 files changed, 213 insertions(+), 178 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 0b248a6c55..f0bad6c3ba 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1466,11 +1466,10 @@ module ActiveRecord end def find_with_associations(options = {}, join_dependency = nil) - catch :invalid_query do - join_dependency ||= JoinDependency.new(self, merge_includes(scope(:find, :include), options[:include]), options[:joins]) - rows = select_all_rows(options, join_dependency) - return join_dependency.instantiate(rows) - end + join_dependency ||= JoinDependency.new(self, merge_includes(scope(:find, :include), options[:include]), options[:joins]) + rows = select_all_rows(options, join_dependency) + join_dependency.instantiate(rows) + rescue ThrowResult [] end @@ -1733,7 +1732,7 @@ module ActiveRecord def construct_arel_limited_ids_condition(options, join_dependency) if (ids_array = select_limited_ids_array(options, join_dependency)).empty? - throw :invalid_query + raise ThrowResult else Arel::Predicates::In.new( Arel::SqlLiteral.new("#{connection.quote_table_name table_name}.#{primary_key}"), diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 56b2a90138..b2b3a9789c 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -177,7 +177,7 @@ module ActiveRecord if @reflection.options[:counter_sql] @reflection.klass.count_by_sql(@counter_sql) else - column_name, options = @reflection.klass.send(:construct_count_options_from_args, *args) + column_name, options = @reflection.klass.scoped.send(:construct_count_options_from_args, *args) if @reflection.options[:uniq] # This is needed because 'SELECT count(DISTINCT *)..' is not valid SQL. column_name = "#{@reflection.quoted_table_name}.#{@reflection.klass.primary_key}" if column_name == :all diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 767109474d..53f0a920a3 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -69,6 +69,10 @@ module ActiveRecord #:nodoc: class StatementInvalid < ActiveRecordError end + # Raised when SQL statement is invalid and the application gets a blank result. + class ThrowResult < ActiveRecordError + end + # Parent class for all specific exceptions which wrap database driver exceptions # provides access to the original exception also. class WrappedDatabaseException < StatementInvalid diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb index 59811c40a8..d51d9f2159 100644 --- a/activerecord/lib/active_record/calculations.rb +++ b/activerecord/lib/active_record/calculations.rb @@ -44,7 +44,26 @@ module ActiveRecord # # Note: Person.count(:all) will not work because it will use :all as the condition. Use Person.count instead. def count(*args) - calculate(:count, *construct_count_options_from_args(*args)) + case args.size + when 0 + construct_calculation_arel.count + when 1 + if args[0].is_a?(Hash) + options = args[0] + distinct = options.has_key?(:distinct) ? options.delete(:distinct) : false + construct_calculation_arel(options).count(options[:select], :distinct => distinct) + else + construct_calculation_arel.count(args[0]) + end + when 2 + column_name, options = args + distinct = options.has_key?(:distinct) ? options.delete(:distinct) : false + construct_calculation_arel(options).count(column_name, :distinct => distinct) + else + raise ArgumentError, "Unexpected parameters passed to count(): #{args.inspect}" + end + rescue ThrowResult + 0 end # Calculates the average value on a given column. The value is returned as @@ -122,168 +141,63 @@ module ActiveRecord # Person.minimum(:age, :having => 'min(age) > 17', :group => :last_name) # Selects the minimum age for any family without any minors # Person.sum("2 * age") def calculate(operation, column_name, options = {}) - validate_calculation_options(operation, options) - operation = operation.to_s.downcase - - scope = scope(:find) + construct_calculation_arel(options).calculate(operation, column_name, options.slice(:distinct)) + rescue ThrowResult + 0 + end - merged_includes = merge_includes(scope ? scope[:include] : [], options[:include]) + private + def validate_calculation_options(options = {}) + options.assert_valid_keys(CALCULATIONS_OPTIONS) + end - if operation == "count" - if merged_includes.any? - distinct = true - column_name = options[:select] || primary_key - end + def construct_calculation_arel(options = {}) + validate_calculation_options(options) + options = options.except(:distinct) - distinct = nil if column_name.to_s =~ /\s*DISTINCT\s+/i - distinct ||= options[:distinct] - else - distinct = nil - end + scope = scope(:find) + includes = merge_includes(scope ? scope[:include] : [], options[:include]) - catch :invalid_query do - relation = if merged_includes.any? - join_dependency = ActiveRecord::Associations::ClassMethods::JoinDependency.new(self, merged_includes, construct_join(options[:joins], scope)) - construct_finder_arel_with_included_associations(options, join_dependency) + if includes.any? + join_dependency = ActiveRecord::Associations::ClassMethods::JoinDependency.new(self, includes, construct_join(options[:joins], scope)) + construct_calculation_arel_with_included_associations(options, join_dependency) else - relation = arel_table(options[:from]). + arel_table. joins(construct_join(options[:joins], scope)). + from((scope && scope[:from]) || options[:from]). where(construct_conditions(options[:conditions], scope)). order(options[:order]). limit(options[:limit]). - offset(options[:offset]) - end - if options[:group] - return execute_grouped_calculation(operation, column_name, options, relation) - else - return execute_simple_calculation(operation, column_name, options.merge(:distinct => distinct), relation) + offset(options[:offset]). + group(options[:group]). + having(options[:having]). + select(options[:select] || (scope && scope[:select]) || default_select(options[:joins] || (scope && scope[:joins]))) end end - 0 - end - - def execute_simple_calculation(operation, column_name, options, relation) #:nodoc: - column = if column_names.include?(column_name.to_s) - Arel::Attribute.new(arel_table(options[:from] || table_name), - options[:select] || column_name) - else - Arel::SqlLiteral.new(options[:select] || - (column_name == :all ? "*" : column_name.to_s)) - end - - relation = relation.select(operation == 'count' ? column.count(options[:distinct]) : column.send(operation)) - - type_cast_calculated_value(connection.select_value(relation.to_sql), column_for(column_name), operation) - end - def execute_grouped_calculation(operation, column_name, options, relation) #:nodoc: - group_attr = options[:group].to_s - association = reflect_on_association(group_attr.to_sym) - associated = association && association.macro == :belongs_to # only count belongs_to associations - group_field = associated ? association.primary_key_name : group_attr - group_alias = column_alias_for(group_field) - group_column = column_for group_field + def construct_calculation_arel_with_included_associations(options, join_dependency) + scope = scope(:find) - options[:group] = connection.adapter_name == 'FrontBase' ? group_alias : group_field + relation = arel_table - aggregate_alias = column_alias_for(operation, column_name) - - options[:select] = (operation == 'count' && column_name == :all) ? - "COUNT(*) AS count_all" : - Arel::Attribute.new(arel_table, column_name).send(operation).as(aggregate_alias).to_sql - - options[:select] << ", #{group_field} AS #{group_alias}" - - relation = relation.select(options[:select]).group(options[:group]).having(options[:having]) - - calculated_data = connection.select_all(relation.to_sql) - - if association - key_ids = calculated_data.collect { |row| row[group_alias] } - key_records = association.klass.base_class.find(key_ids) - key_records = key_records.inject({}) { |hsh, r| hsh.merge(r.id => r) } - end - - calculated_data.inject(ActiveSupport::OrderedHash.new) do |all, row| - key = type_cast_calculated_value(row[group_alias], group_column) - key = key_records[key] if associated - value = row[aggregate_alias] - all[key] = type_cast_calculated_value(value, column_for(column_name), operation) - all - end - end - - protected - def construct_count_options_from_args(*args) - options = {} - column_name = :all - - # We need to handle - # count() - # count(:column_name=:all) - # count(options={}) - # count(column_name=:all, options={}) - # selects specified by scopes - case args.size - when 0 - column_name = scope(:find)[:select] if scope(:find) - when 1 - if args[0].is_a?(Hash) - column_name = scope(:find)[:select] if scope(:find) - options = args[0] - else - column_name = args[0] - end - when 2 - column_name, options = args - else - raise ArgumentError, "Unexpected parameters passed to count(): #{args.inspect}" + for association in join_dependency.join_associations + relation = association.join_relation(relation) end - [column_name || :all, options] - end - - private - def validate_calculation_options(operation, options = {}) - options.assert_valid_keys(CALCULATIONS_OPTIONS) - end + relation = relation.joins(construct_join(options[:joins], scope)). + select(column_aliases(join_dependency)). + group(options[:group]). + having(options[:having]). + order(options[:order]). + where(construct_conditions(options[:conditions], scope)). + from((scope && scope[:from]) || options[:from]) - # Converts the given keys to the value that the database adapter returns as - # a usable column name: - # - # column_alias_for("users.id") # => "users_id" - # column_alias_for("sum(id)") # => "sum_id" - # column_alias_for("count(distinct users.id)") # => "count_distinct_users_id" - # column_alias_for("count(*)") # => "count_all" - # column_alias_for("count", "id") # => "count_id" - def column_alias_for(*keys) - table_name = keys.join(' ') - table_name.downcase! - table_name.gsub!(/\*/, 'all') - table_name.gsub!(/\W+/, ' ') - table_name.strip! - table_name.gsub!(/ +/, '_') + relation = relation.where(construct_arel_limited_ids_condition(options, join_dependency)) if !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit]) + relation = relation.limit(construct_limit(options[:limit], scope)) if using_limitable_reflections?(join_dependency.reflections) - connection.table_alias_for(table_name) + relation end - def column_for(field) - field_name = field.to_s.split('.').last - columns.detect { |c| c.name.to_s == field_name } - end - - def type_cast_calculated_value(value, column, operation = nil) - case operation - when 'count' then value.to_i - when 'sum' then type_cast_using_column(value || '0', column) - when 'average' then value && (value.is_a?(Fixnum) ? value.to_f : value).to_d - else type_cast_using_column(value, column) - end - end - - def type_cast_using_column(value, column) - column ? column.type_cast(value) : value - end end end end diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index cb743358b2..e495aa80db 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -149,8 +149,8 @@ module ActiveRecord return @records if loaded? @records = if @eager_load_associations.any? - catch :invalid_query do - return @klass.send(:find_with_associations, { + begin + @klass.send(:find_with_associations, { :select => @relation.send(:select_clauses).join(', '), :joins => @relation.joins(relation), :group => @relation.send(:group_clauses).join(', '), @@ -161,8 +161,9 @@ module ActiveRecord :from => (@relation.send(:from_clauses) if @relation.send(:sources).present?) }, ActiveRecord::Associations::ClassMethods::JoinDependency.new(@klass, @eager_load_associations, nil)) + rescue ThrowResult + [] end - [] else @klass.find_by_sql(@relation.to_sql) end diff --git a/activerecord/lib/active_record/relational_calculations.rb b/activerecord/lib/active_record/relational_calculations.rb index 10eb992167..d77624c7bf 100644 --- a/activerecord/lib/active_record/relational_calculations.rb +++ b/activerecord/lib/active_record/relational_calculations.rb @@ -2,39 +2,119 @@ module ActiveRecord module RelationalCalculations def count(*args) - column_name, options = construct_count_options_from_args(*args) - distinct = options[:distinct] ? true : false + calculate(:count, *construct_count_options_from_args(*args)) + end + + def average(column_name) + calculate(:average, column_name) + end + + def minimum(column_name) + calculate(:minimum, column_name) + end + + def maximum(column_name) + calculate(:maximum, column_name) + end + + def sum(column_name) + calculate(:sum, column_name) + end + + def calculate(operation, column_name, options = {}) + operation = operation.to_s.downcase + + if operation == "count" + joins = @relation.joins(relation) + if joins.present? && joins =~ /LEFT OUTER/i + distinct = true + column_name = @klass.primary_key if column_name == :all + end + distinct = nil if column_name.to_s =~ /\s*DISTINCT\s+/i + distinct ||= options[:distinct] + else + distinct = nil + end + + distinct = options[:distinct] || distinct + column_name = :all if column_name.blank? && operation == "count" + + if @relation.send(:groupings).any? + return execute_grouped_calculation(operation, column_name) + else + return execute_simple_calculation(operation, column_name, distinct) + end + rescue ThrowResult + 0 + end + + private + + def execute_simple_calculation(operation, column_name, distinct) #:nodoc: column = if @klass.column_names.include?(column_name.to_s) - Arel::Attribute.new(@relation.table, column_name) + Arel::Attribute.new(@klass.arel_table, column_name) else Arel::SqlLiteral.new(column_name == :all ? "*" : column_name.to_s) end - relation = select(column.count(distinct)) - @klass.connection.select_value(relation.to_sql).to_i + relation = select(operation == 'count' ? column.count(distinct) : column.send(operation)) + type_cast_calculated_value(@klass.connection.select_value(relation.to_sql), column_for(column_name), operation) end - private + def execute_grouped_calculation(operation, column_name) #:nodoc: + group_attr = @relation.send(:groupings).first.value + association = @klass.reflect_on_association(group_attr.to_sym) + associated = association && association.macro == :belongs_to # only count belongs_to associations + group_field = associated ? association.primary_key_name : group_attr + group_alias = column_alias_for(group_field) + group_column = column_for(group_field) + + group = @klass.connection.adapter_name == 'FrontBase' ? group_alias : group_field + + aggregate_alias = column_alias_for(operation, column_name) + + select_statement = if operation == 'count' && column_name == :all + "COUNT(*) AS count_all" + else + Arel::Attribute.new(@klass.arel_table, column_name).send(operation).as(aggregate_alias).to_sql + end + + select_statement << ", #{group_field} AS #{group_alias}" + + relation = select(select_statement).group(group) + + calculated_data = @klass.connection.select_all(relation.to_sql) + + if association + key_ids = calculated_data.collect { |row| row[group_alias] } + key_records = association.klass.base_class.find(key_ids) + key_records = key_records.inject({}) { |hsh, r| hsh.merge(r.id => r) } + end + + calculated_data.inject(ActiveSupport::OrderedHash.new) do |all, row| + key = type_cast_calculated_value(row[group_alias], group_column) + key = key_records[key] if associated + value = row[aggregate_alias] + all[key] = type_cast_calculated_value(value, column_for(column_name), operation) + all + end + end def construct_count_options_from_args(*args) options = {} column_name = :all - # We need to handle - # count() - # count(:column_name=:all) - # count(options={}) - # count(column_name=:all, options={}) - # selects specified by scopes - + # Handles count(), count(:column), count(:distinct => true), count(:column, :distinct => true) # TODO : relation.projections only works when .select() was last in the chain. Fix it! case args.size when 0 - column_name = @relation.send(:select_clauses).join(', ') if @relation.respond_to?(:projections) && @relation.projections.present? + select = @relation.send(:select_clauses).join(', ') if @relation.respond_to?(:projections) && @relation.projections.present? + column_name = select if select !~ /(,|\*)/ when 1 if args[0].is_a?(Hash) - column_name = @relation.send(:select_clauses).join(', ') if @relation.respond_to?(:projections) && @relation.projections.present? + select = @relation.send(:select_clauses).join(', ') if @relation.respond_to?(:projections) && @relation.projections.present? + column_name = select if select !~ /(,|\*)/ options = args[0] else column_name = args[0] @@ -48,5 +128,42 @@ module ActiveRecord [column_name || :all, options] end + # Converts the given keys to the value that the database adapter returns as + # a usable column name: + # + # column_alias_for("users.id") # => "users_id" + # column_alias_for("sum(id)") # => "sum_id" + # column_alias_for("count(distinct users.id)") # => "count_distinct_users_id" + # column_alias_for("count(*)") # => "count_all" + # column_alias_for("count", "id") # => "count_id" + def column_alias_for(*keys) + table_name = keys.join(' ') + table_name.downcase! + table_name.gsub!(/\*/, 'all') + table_name.gsub!(/\W+/, ' ') + table_name.strip! + table_name.gsub!(/ +/, '_') + + @klass.connection.table_alias_for(table_name) + end + + def column_for(field) + field_name = field.to_s.split('.').last + @klass.columns.detect { |c| c.name.to_s == field_name } + end + + def type_cast_calculated_value(value, column, operation = nil) + case operation + when 'count' then value.to_i + when 'sum' then type_cast_using_column(value || '0', column) + when 'average' then value && (value.is_a?(Fixnum) ? value.to_f : value).to_d + else type_cast_using_column(value, column) + end + end + + def type_cast_using_column(value, column) + column ? column.type_cast(value) : value + end + end end diff --git a/activerecord/test/cases/associations/inner_join_association_test.rb b/activerecord/test/cases/associations/inner_join_association_test.rb index 18a1cd3cd0..0315604106 100644 --- a/activerecord/test/cases/associations/inner_join_association_test.rb +++ b/activerecord/test/cases/associations/inner_join_association_test.rb @@ -81,6 +81,8 @@ class InnerJoinAssociationTest < ActiveRecord::TestCase end def test_calculate_honors_implicit_inner_joins + Author.calculate(:count, 'authors.id', :joins => :posts) + return real_count = Author.scoped.to_a.sum{|a| a.posts.count } assert_equal real_count, Author.calculate(:count, 'authors.id', :joins => :posts), "plain inner join count should match the number of referenced posts records" end diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 7e4c4a0913..bd2d471fc7 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -29,8 +29,8 @@ class CalculationsTest < ActiveRecord::TestCase end def test_type_cast_calculated_value_should_convert_db_averages_of_fixnum_class_to_decimal - assert_equal 0, NumericData.send(:type_cast_calculated_value, 0, nil, 'avg') - assert_equal 53.0, NumericData.send(:type_cast_calculated_value, 53, nil, 'avg') + assert_equal 0, NumericData.scoped.send(:type_cast_calculated_value, 0, nil, 'avg') + assert_equal 53.0, NumericData.scoped.send(:type_cast_calculated_value, 53, nil, 'avg') end def test_should_get_maximum_of_field @@ -248,17 +248,15 @@ class CalculationsTest < ActiveRecord::TestCase def test_should_reject_invalid_options assert_nothing_raised do - [:count, :sum].each do |func| - # empty options are valid - Company.send(:validate_calculation_options, func) - # these options are valid for all calculations - [:select, :conditions, :joins, :order, :group, :having, :distinct].each do |opt| - Company.send(:validate_calculation_options, func, opt => true) - end + # empty options are valid + Company.send(:validate_calculation_options) + # these options are valid for all calculations + [:select, :conditions, :joins, :order, :group, :having, :distinct].each do |opt| + Company.send(:validate_calculation_options, opt => true) end # :include is only valid on :count - Company.send(:validate_calculation_options, :count, :include => true) + Company.send(:validate_calculation_options, :include => true) end assert_raise(ArgumentError) { Company.send(:validate_calculation_options, :sum, :foo => :bar) } -- cgit v1.2.3 From 078ea0dfbdfa3267da13e88536dc73aa477a162c Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 29 Dec 2009 04:33:37 +0530 Subject: Oops. Remove debug information inside a test from the previous commit --- activerecord/test/cases/associations/inner_join_association_test.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/activerecord/test/cases/associations/inner_join_association_test.rb b/activerecord/test/cases/associations/inner_join_association_test.rb index 0315604106..18a1cd3cd0 100644 --- a/activerecord/test/cases/associations/inner_join_association_test.rb +++ b/activerecord/test/cases/associations/inner_join_association_test.rb @@ -81,8 +81,6 @@ class InnerJoinAssociationTest < ActiveRecord::TestCase end def test_calculate_honors_implicit_inner_joins - Author.calculate(:count, 'authors.id', :joins => :posts) - return real_count = Author.scoped.to_a.sum{|a| a.posts.count } assert_equal real_count, Author.calculate(:count, 'authors.id', :joins => :posts), "plain inner join count should match the number of referenced posts records" end -- cgit v1.2.3