diff options
19 files changed, 96 insertions, 32 deletions
diff --git a/activemodel/lib/active_model/validations.rb b/activemodel/lib/active_model/validations.rb index 92206450d2..cb1db1e5aa 100644 --- a/activemodel/lib/active_model/validations.rb +++ b/activemodel/lib/active_model/validations.rb @@ -226,7 +226,6 @@ module ActiveModel # Person.validators_on(:name) # # => [ # # #<ActiveModel::Validations::PresenceValidator:0x007fe604914e60 @attributes=[:name], @options={}>, - # # #<ActiveModel::Validations::InclusionValidator:0x007fe603bb8780 @attributes=[:age], @options={in:0..99}> # # ] def validators_on(*attributes) attributes.flat_map do |attribute| diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index e5312d7d7c..170084cf67 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,40 @@ +* Remove column restrictions for `count`, let the database raise if the SQL is + invalid. The previos behavior was untested and surprising for the user. + Fixes #5554. + + Example: + + User.select("name, username").count + # Before => SELECT count(*) FROM users + # After => ActiveRecord::StatementInvalid + + # you can still use `count(:all)` to perform a query unrelated to the + # selected columns + User.select("name, username").count(:all) # => SELECT count(*) FROM users + + *Yves Senn* + +* Rails now automatically detects inverse associations. If you do not set the + `:inverse_of` option on the association, then Active Record will guess the + inverse association based on heuristics. + + Note that automatic inverse detection only works on `has_many`, `has_one`, + and `belongs_to` associations. Extra options on the associations will + also prevent the association's inverse from being found automatically. + + The automatic guessing of the inverse association uses a heuristic based + on the name of the class, so it may not work for all associations, + especially the ones with non-standard names. + + You can turn off the automatic detection of inverse associations by setting + the `:inverse_of` option to `false` like so: + + class Taggable < ActiveRecord::Base + belongs_to :tag, inverse_of: false + end + + *John Wang* + * Fix `add_column` with `array` option when using PostgreSQL. Fixes #10432 *Adam Anderson* diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 3490057298..6fd4f3042c 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -586,9 +586,10 @@ module ActiveRecord # belongs_to :tag, inverse_of: :taggings # end # - # If you do not set the +:inverse_of+ record, the association will do its - # best to match itself up with the correct inverse. Automatic +:inverse_of+ - # detection only works on +has_many+, +has_one+, and +belongs_to+ associations. + # If you do not set the <tt>:inverse_of</tt> record, the association will + # do its best to match itself up with the correct inverse. Automatic + # inverse detection only works on <tt>has_many</tt>, <tt>has_one</tt>, and + # <tt>belongs_to</tt> associations. # # Extra options on the associations, as defined in the # <tt>AssociationReflection::INVALID_AUTOMATIC_INVERSE_OPTIONS</tt> constant, will @@ -599,10 +600,10 @@ module ActiveRecord # especially the ones with non-standard names. # # You can turn off the automatic detection of inverse associations by setting - # the +:automatic_inverse_of+ option to +false+ like so: + # the <tt>:inverse_of</tt> option to <tt>false</tt> like so: # # class Taggable < ActiveRecord::Base - # belongs_to :tag, automatic_inverse_of: false + # belongs_to :tag, inverse_of: false # end # # == Nested \Associations diff --git a/activerecord/lib/active_record/associations/builder/has_many.rb b/activerecord/lib/active_record/associations/builder/has_many.rb index 429def5455..0d1bdd21ee 100644 --- a/activerecord/lib/active_record/associations/builder/has_many.rb +++ b/activerecord/lib/active_record/associations/builder/has_many.rb @@ -5,7 +5,7 @@ module ActiveRecord::Associations::Builder end def valid_options - super + [:primary_key, :dependent, :as, :through, :source, :source_type, :inverse_of, :automatic_inverse_of, :counter_cache] + super + [:primary_key, :dependent, :as, :through, :source, :source_type, :inverse_of, :counter_cache] end def valid_dependent_options diff --git a/activerecord/lib/active_record/associations/builder/singular_association.rb b/activerecord/lib/active_record/associations/builder/singular_association.rb index 96ccbeb8a3..76e48e66e5 100644 --- a/activerecord/lib/active_record/associations/builder/singular_association.rb +++ b/activerecord/lib/active_record/associations/builder/singular_association.rb @@ -3,7 +3,7 @@ module ActiveRecord::Associations::Builder class SingularAssociation < Association #:nodoc: def valid_options - super + [:remote, :dependent, :counter_cache, :primary_key, :inverse_of, :automatic_inverse_of] + super + [:remote, :dependent, :counter_cache, :primary_key, :inverse_of] end def constructable? diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 8bdaeef924..0e8822d63f 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -230,6 +230,10 @@ module ActiveRecord # validates_presence_of :member # end # + # Note that if you do not specify the <tt>inverse_of</tt> option, then + # Active Record will try to automatically guess the inverse association + # based on heuristics. + # # For one-to-one nested associations, if you build the new (in-memory) # child object yourself before assignment, then this module will not # overwrite it, e.g.: diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 27aa20b6c0..2ba89b13b7 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -313,8 +313,7 @@ module ActiveRecord # and prevents this object from finding the inverse association # automatically in the future. def remove_automatic_inverse_of! - @automatic_inverse_of = nil - options[:automatic_inverse_of] = false + @automatic_inverse_of = false end def polymorphic_inverse_of(associated_class) @@ -449,15 +448,16 @@ module ActiveRecord # Checks to see if the reflection doesn't have any options that prevent # us from being able to guess the inverse automatically. First, the - # +automatic_inverse_of+ option cannot be set to false. Second, we must - # have +has_many+, +has_one+, +belongs_to+ associations. Third, we must - # not have options such as +:polymorphic+ or +:foreign_key+ which prevent us - # from correctly guessing the inverse association. + # <tt>inverse_of</tt> option cannot be set to false. Second, we must + # have <tt>has_many</tt>, <tt>has_one</tt>, <tt>belongs_to</tt> associations. + # Third, we must not have options such as <tt>:polymorphic</tt> or + # <tt>:foreign_key</tt> which prevent us from correctly guessing the + # inverse association. # # Anything with a scope can additionally ruin our attempt at finding an # inverse, so we exclude reflections with scopes. def can_find_inverse_of_automatically?(reflection) - reflection.options[:automatic_inverse_of] != false && + reflection.options[:inverse_of] != false && VALID_AUTOMATIC_INVERSE_MACROS.include?(reflection.macro) && !INVALID_AUTOMATIC_INVERSE_OPTIONS.any? { |opt| reflection.options[opt] } && !reflection.scope diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index d54479edbb..d37471e9ad 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -247,7 +247,7 @@ module ActiveRecord def empty? return @records.empty? if loaded? - c = count + c = count(:all) c.respond_to?(:zero?) ? c.zero? : c.empty? end diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index ccb48247b7..4becf3980d 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -207,15 +207,18 @@ module ActiveRecord end if operation == "count" - column_name ||= (select_for_count || :all) + if select_values.present? + column_name ||= select_values.join(", ") + else + column_name ||= :all + end unless arel.ast.grep(Arel::Nodes::OuterJoin).empty? distinct = true end column_name = primary_key if column_name == :all && distinct - - distinct = nil if column_name =~ /\s*DISTINCT\s+/i + distinct = nil if column_name =~ /\s*DISTINCT[\s(]+/i end if group_values.any? @@ -376,13 +379,6 @@ module ActiveRecord column ? column.type_cast(value) : value end - def select_for_count - if select_values.present? - select = select_values.join(", ") - select if select !~ /[,*]/ - end - end - def build_count_subquery(relation, column_name, distinct) column_alias = Arel.sql('count_column') subquery_alias = Arel.sql('subquery_for_count') diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index c8ac77984f..0f3f9aecfc 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -167,6 +167,15 @@ class CalculationsTest < ActiveRecord::TestCase assert_no_match(/OFFSET/, queries.first) end + def test_count_on_invalid_columns_raises + e = assert_raises(ActiveRecord::StatementInvalid) { + Account.select("credit_limit, firm_name").count + } + + assert_match "accounts", e.message + assert_match "credit_limit, firm_name", e.message + end + def test_should_group_by_summed_field_having_condition c = Account.group(:firm_id).having('sum(credit_limit) > 50').sum(:credit_limit) assert_nil c[1] diff --git a/activerecord/test/models/club.rb b/activerecord/test/models/club.rb index 7d7c205041..816c5e6937 100644 --- a/activerecord/test/models/club.rb +++ b/activerecord/test/models/club.rb @@ -1,6 +1,6 @@ class Club < ActiveRecord::Base has_one :membership - has_many :memberships, :automatic_inverse_of => false + has_many :memberships, :inverse_of => false has_many :members, :through => :memberships has_many :current_memberships has_one :sponsor diff --git a/activerecord/test/models/interest.rb b/activerecord/test/models/interest.rb index f772bb1c7f..d5d9226204 100644 --- a/activerecord/test/models/interest.rb +++ b/activerecord/test/models/interest.rb @@ -1,5 +1,5 @@ class Interest < ActiveRecord::Base - belongs_to :man, :inverse_of => :interests, :automatic_inverse_of => false + 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 49f002aa9a..4bff92dc98 100644 --- a/activerecord/test/models/man.rb +++ b/activerecord/test/models/man.rb @@ -1,7 +1,7 @@ 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, :automatic_inverse_of => false + 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 diff --git a/activerecord/test/models/member.rb b/activerecord/test/models/member.rb index b81304b8e0..cc47c7bc18 100644 --- a/activerecord/test/models/member.rb +++ b/activerecord/test/models/member.rb @@ -9,7 +9,7 @@ class Member < ActiveRecord::Base has_one :hairy_club, -> { where :clubs => {:name => "Moustache and Eyebrow Fancier Club"} }, :through => :membership, :source => :club has_one :sponsor, :as => :sponsorable has_one :sponsor_club, :through => :sponsor - has_one :member_detail, :automatic_inverse_of => false + has_one :member_detail, :inverse_of => false has_one :organization, :through => :member_detail belongs_to :member_type diff --git a/activerecord/test/models/member_detail.rb b/activerecord/test/models/member_detail.rb index a256c73c7e..9d253aa126 100644 --- a/activerecord/test/models/member_detail.rb +++ b/activerecord/test/models/member_detail.rb @@ -1,5 +1,5 @@ class MemberDetail < ActiveRecord::Base - belongs_to :member, :automatic_inverse_of => false + belongs_to :member, :inverse_of => false belongs_to :organization has_one :member_type, :through => :member diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 5a1c14683e..0f9399f4d6 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,8 @@ +* Fix `ActiveSupport::Dependencies::Loadable#load_dependency` calling + `#blame_file!` on Exceptions that do not have the Blamable mixin + + *Andrew Kreiling* + * Override `Time.at` to support the passing of Time-like values when called with a single argument. *Andrew White* diff --git a/activesupport/lib/active_support/dependencies.rb b/activesupport/lib/active_support/dependencies.rb index fff4c776a9..d38e4b0732 100644 --- a/activesupport/lib/active_support/dependencies.rb +++ b/activesupport/lib/active_support/dependencies.rb @@ -213,7 +213,7 @@ module ActiveSupport #:nodoc: yield end rescue Exception => exception # errors from loading file - exception.blame_file! file + exception.blame_file! file if exception.respond_to? :blame_file! raise end diff --git a/activesupport/test/dependencies/raises_exception_without_blame_file.rb b/activesupport/test/dependencies/raises_exception_without_blame_file.rb new file mode 100644 index 0000000000..4b2da6ff30 --- /dev/null +++ b/activesupport/test/dependencies/raises_exception_without_blame_file.rb @@ -0,0 +1,5 @@ +exception = Exception.new('I am not blamable!') +class << exception + undef_method(:blame_file!) +end +raise exception diff --git a/activesupport/test/dependencies_test.rb b/activesupport/test/dependencies_test.rb index 4b1426bb2e..68b6cc6e8c 100644 --- a/activesupport/test/dependencies_test.rb +++ b/activesupport/test/dependencies_test.rb @@ -76,6 +76,14 @@ class DependenciesTest < ActiveSupport::TestCase end end + def test_dependency_which_raises_doesnt_blindly_call_blame_file! + with_loading do + filename = 'dependencies/raises_exception_without_blame_file' + + assert_raises(Exception) { require_dependency filename } + end + end + def test_warnings_should_be_enabled_on_first_load with_loading 'dependencies' do old_warnings, ActiveSupport::Dependencies.warnings_on_first_load = ActiveSupport::Dependencies.warnings_on_first_load, true |