From c6e1f86fa61249e7275f312fbe2ac4010bc2d637 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Sun, 20 Oct 2013 18:41:30 -0600 Subject: [PATCH] Fix converting association options to a Relation Under Rails 3 when using an association matcher in conjunction with a submatcher such as #order, we have to take the options on the association (:conditions, :order, etc.) and convert them to an ActiveRecord::Relation object. This happens in ModelReflector. Unfortunately, converting an :includes option was broken. This is in part due to ModelReflector not having proper tests, but also, the method that does the conversion to Relation is rather complex and could be broken up so it can be tested better. In fact I noticed that there's a lot of stuff in ModelReflector that does stuff around a Reflection object, so it would be better to define a ModelReflection class that simply decorates a Reflection. That's what I've done and also added proper tests. --- NEWS.md | 3 + lib/shoulda/matchers/active_record.rb | 1 + .../association_matchers/model_reflection.rb | 80 ++++++ .../association_matchers/model_reflector.rb | 62 +---- .../model_reflection_spec.rb | 247 ++++++++++++++++++ spec/support/rails_versions.rb | 18 ++ 6 files changed, 357 insertions(+), 54 deletions(-) create mode 100644 lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb create mode 100644 spec/shoulda/matchers/active_record/association_matchers/model_reflection_spec.rb create mode 100644 spec/support/rails_versions.rb diff --git a/NEWS.md b/NEWS.md index 454ce66a..d79feaef 100644 --- a/NEWS.md +++ b/NEWS.md @@ -12,6 +12,9 @@ * The `ensure_inclusion_of` matcher now works with a decimal column. +* Fix association matchers on Rails 3 so they work when used in conjunction with + a submatcher such as #order and an association that has :include on it. + # v 2.4.0 * Fix a bug with the `validate_numericality_of` matcher that would not allow the diff --git a/lib/shoulda/matchers/active_record.rb b/lib/shoulda/matchers/active_record.rb index 51fb003f..6cd15d28 100644 --- a/lib/shoulda/matchers/active_record.rb +++ b/lib/shoulda/matchers/active_record.rb @@ -4,6 +4,7 @@ require 'shoulda/matchers/active_record/association_matchers/order_matcher' require 'shoulda/matchers/active_record/association_matchers/through_matcher' require 'shoulda/matchers/active_record/association_matchers/dependent_matcher' require 'shoulda/matchers/active_record/association_matchers/model_reflector' +require 'shoulda/matchers/active_record/association_matchers/model_reflection' require 'shoulda/matchers/active_record/association_matchers/option_verifier' require 'shoulda/matchers/active_record/have_db_column_matcher' require 'shoulda/matchers/active_record/have_db_index_matcher' diff --git a/lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb b/lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb new file mode 100644 index 00000000..b85b51b2 --- /dev/null +++ b/lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb @@ -0,0 +1,80 @@ +require 'delegate' + +module Shoulda + module Matchers + module ActiveRecord + module AssociationMatchers + class ModelReflection < SimpleDelegator + def initialize(reflection) + super(reflection) + @reflection = reflection + @subject = reflection.active_record + end + + def associated_class + reflection.klass + end + + def through? + reflection.options[:through] + end + + def join_table + join_table = + if reflection.respond_to?(:join_table) + reflection.join_table + else + reflection.options[:join_table] + end + + join_table.to_s + end + + def association_relation + if reflection.respond_to?(:scope) + convert_scope_to_relation(reflection.scope) + else + convert_options_to_relation(reflection.options) + end + end + + private + + attr_reader :reflection, :subject + + def convert_scope_to_relation(scope) + relation = associated_class.all + + if scope + # Source: AR::Associations::AssociationScope#eval_scope + relation.instance_exec(subject, &scope) + else + relation + end + end + + def convert_options_to_relation(options) + relation = associated_class.scoped + relation = extend_relation_with(relation, :where, options[:conditions]) + relation = extend_relation_with(relation, :includes, options[:include]) + relation = extend_relation_with(relation, :order, options[:order]) + relation = extend_relation_with(relation, :group, options[:group]) + relation = extend_relation_with(relation, :having, options[:having]) + relation = extend_relation_with(relation, :limit, options[:limit]) + relation = extend_relation_with(relation, :offset, options[:offset]) + relation = extend_relation_with(relation, :select, options[:select]) + relation + end + + def extend_relation_with(relation, method_name, value) + if value + relation.__send__(method_name, value) + else + relation + end + end + end + end + end + end +end diff --git a/lib/shoulda/matchers/active_record/association_matchers/model_reflector.rb b/lib/shoulda/matchers/active_record/association_matchers/model_reflector.rb index b426b4e1..a8a9cac0 100644 --- a/lib/shoulda/matchers/active_record/association_matchers/model_reflector.rb +++ b/lib/shoulda/matchers/active_record/association_matchers/model_reflector.rb @@ -3,6 +3,9 @@ module Shoulda # :nodoc: module ActiveRecord # :nodoc: module AssociationMatchers class ModelReflector + delegate :associated_class, :through?, :join_table, + :association_relation, to: :reflection + def initialize(subject, name) @subject = subject @name = name @@ -13,57 +16,17 @@ module Shoulda # :nodoc: end def reflect_on_association(name) - model_class.reflect_on_association(name) + reflection = model_class.reflect_on_association(name) + + if reflection + ModelReflection.new(reflection) + end end def model_class subject.class end - def associated_class - reflection.klass - end - - def through? - reflection.options[:through] - end - - def join_table - if reflection.respond_to? :join_table - reflection.join_table.to_s - else - reflection.options[:join_table].to_s - end - end - - def association_relation - if reflection.respond_to?(:scope) && reflection.scope - relation_from_scope(reflection.scope) - else - options = reflection.options - relation = RailsShim.clean_scope(reflection.klass) - if options[:conditions] - relation = relation.where(options[:conditions]) - end - if options[:include] - relation = relation.include(options[:include]) - end - if options[:order] - relation = relation.order(options[:order]) - end - if options[:group] - relation = relation.group(options[:group]) - end - if options[:having] - relation = relation.having(options[:having]) - end - if options[:limit] - relation = relation.limit(options[:limit]) - end - relation - end - end - def build_relation_with_clause(name, value) case name when :conditions then associated_class.where(value) @@ -82,15 +45,6 @@ module Shoulda # :nodoc: private - def relation_from_scope(scope) - # Source: AR::Associations::AssociationScope#eval_scope - if scope.is_a?(::Proc) - associated_class.all.instance_exec(subject, &scope) - else - scope - end - end - attr_reader :subject, :name end end diff --git a/spec/shoulda/matchers/active_record/association_matchers/model_reflection_spec.rb b/spec/shoulda/matchers/active_record/association_matchers/model_reflection_spec.rb new file mode 100644 index 00000000..c8dfba80 --- /dev/null +++ b/spec/shoulda/matchers/active_record/association_matchers/model_reflection_spec.rb @@ -0,0 +1,247 @@ +require 'spec_helper' + +describe Shoulda::Matchers::ActiveRecord::AssociationMatchers::ModelReflection do + it 'delegates other methods to the given Reflection object' do + define_model(:country) + person_model = define_model(:person, country_id: :integer) do + belongs_to :country + end + delegate_reflection = person_model.reflect_on_association(:country) + delegate_reflection.stubs(foo: 'bar') + reflection = described_class.new(delegate_reflection) + + expect(reflection.foo).to eq 'bar' + end + + describe '#associated_class' do + it 'returns the model that the association refers to' do + define_model(:country) + person_model = define_model(:person, country_id: :integer) do + belongs_to :country + end + delegate_reflection = person_model.reflect_on_association(:country) + reflection = described_class.new(delegate_reflection) + + expect(reflection.associated_class).to be Country + end + end + + describe '#through?' do + it 'returns true if the reflection is for a has_many :through association' do + define_model(:city, person_id: :integer) + define_model(:person, country_id: :integer) do + has_many :cities + end + country_model = define_model(:country) do + has_many :people + has_many :cities, through: :people + end + delegate_reflection = country_model.reflect_on_association(:cities) + reflection = described_class.new(delegate_reflection) + + expect(reflection).to be_through + end + + it 'returns false if not' do + define_model(:person, country_id: :integer) + country_model = define_model(:country) do + has_many :people + end + delegate_reflection = country_model.reflect_on_association(:people) + reflection = described_class.new(delegate_reflection) + + expect(reflection).not_to be_through + end + end + + describe '#join_table' do + context 'when the association was defined with a :join_table option' do + it 'returns the value of the option' do + define_model(:person, country_id: :integer) + country_model = define_model(:country) do + has_and_belongs_to_many :people, join_table: 'foos' + end + delegate_reflection = country_model.reflect_on_association(:people) + reflection = described_class.new(delegate_reflection) + + expect(reflection.join_table).to eq 'foos' + end + end + + context 'when the association was not defined with :join_table' do + it 'returns the default join_table that ActiveRecord generates' do + define_model(:person, country_id: :integer) + country_model = define_model(:country) do + has_and_belongs_to_many :people + end + delegate_reflection = country_model.reflect_on_association(:people) + reflection = described_class.new(delegate_reflection) + + expect(reflection.join_table).to eq 'countries_people' + end + end + end + + describe '#association_relation' do + if rails_4_x? + context 'when the reflection object has a #scope method' do + context 'when the scope is a block' do + it 'executes the block in the context of an empty scope' do + define_model(:country, mood: :string) + person_model = define_model(:person, country_id: :integer) do + belongs_to :country, -> { where(mood: 'nice') } + end + delegate_reflection = person_model.reflect_on_association(:country) + reflection = described_class.new(delegate_reflection) + + actual_sql = reflection.association_relation.to_sql + expected_sql = Country.where(mood: 'nice').to_sql + expect(actual_sql).to eq expected_sql + end + end + + context 'when the scope is nil' do + it 'returns an empty scope' do + define_model(:country) + person_model = define_model(:person, country_id: :integer) do + belongs_to :country + end + delegate_reflection = person_model.reflect_on_association(:country) + reflection = described_class.new(delegate_reflection) + + actual_sql = reflection.association_relation.to_sql + expected_sql = Country.all.to_sql + expect(actual_sql).to eq expected_sql + end + end + end + end + + if rails_3_x? + context 'when the reflection object does not have a #scope method' do + context 'when the reflection options contain :conditions' do + it 'creates an ActiveRecord::Relation from the conditions' do + define_model(:country, mood: :string) + person_model = define_model(:person, country_id: :integer) do + belongs_to :country, conditions: { mood: 'nice' } + end + delegate_reflection = person_model.reflect_on_association(:country) + reflection = described_class.new(delegate_reflection) + + actual_sql = reflection.association_relation.to_sql + expected_sql = Country.where(mood: 'nice').to_sql + expect(actual_sql).to eq expected_sql + end + end + + context 'when the reflection options contain :order' do + it 'creates an ActiveRecord::Relation from the order' do + define_model(:person, country_id: :integer, age: :integer) + country_model = define_model(:country) do + has_many :people, order: 'age' + end + delegate_reflection = country_model.reflect_on_association(:people) + reflection = described_class.new(delegate_reflection) + + actual_sql = reflection.association_relation.to_sql + expected_sql = Person.order('age').to_sql + expect(actual_sql).to eq expected_sql + end + end + + context 'when the reflection options contain :include' do + it 'creates an ActiveRecord::Relation from the include' do + define_model(:city, country_id: :integer) + define_model(:country) do + has_many :cities + end + person_model = define_model(:person, country_id: :integer) do + belongs_to :country, include: :cities + end + delegate_reflection = person_model.reflect_on_association(:country) + reflection = described_class.new(delegate_reflection) + + actual_includes = reflection.association_relation.includes_values + expected_includes = Country.includes(:cities).includes_values + expect(actual_includes).to eq expected_includes + end + end + + context 'when the reflection options contain :group' do + it 'creates an ActiveRecord::Relation from the group' do + country_model = define_model(:country, mood: :string) do + has_many :people, group: 'age' + end + define_model(:person, country_id: :integer, age: :integer) + delegate_reflection = country_model.reflect_on_association(:people) + reflection = described_class.new(delegate_reflection) + + actual_sql = reflection.association_relation.to_sql + expected_sql = Person.group('age').to_sql + expect(actual_sql).to eq expected_sql + end + end + + context 'when the reflection options contain :having' do + it 'creates an ActiveRecord::Relation from the having' do + country_model = define_model(:country) do + has_many :people, having: 'country_id > 1' + end + define_model(:person, country_id: :integer) + delegate_reflection = country_model.reflect_on_association(:people) + reflection = described_class.new(delegate_reflection) + + actual_sql = reflection.association_relation.to_sql + expected_sql = Person.having('country_id > 1').to_sql + expect(actual_sql).to eq expected_sql + end + end + + context 'when the reflection options contain :limit' do + it 'creates an ActiveRecord::Relation from the limit' do + country_model = define_model(:country) do + has_many :people, limit: 10 + end + define_model(:person, country_id: :integer) + delegate_reflection = country_model.reflect_on_association(:people) + reflection = described_class.new(delegate_reflection) + + actual_sql = reflection.association_relation.to_sql + expected_sql = Person.limit(10).to_sql + expect(actual_sql).to eq expected_sql + end + end + + context 'when the reflection options contain :offset' do + it 'creates an ActiveRecord::Relation from the offset' do + country_model = define_model(:country) do + has_many :people, offset: 5 + end + define_model(:person, country_id: :integer) + delegate_reflection = country_model.reflect_on_association(:people) + reflection = described_class.new(delegate_reflection) + + actual_sql = reflection.association_relation.to_sql + expected_sql = Person.offset(5).to_sql + expect(actual_sql).to eq expected_sql + end + end + + context 'when the reflection options contain :select' do + it 'creates an ActiveRecord::Relation from the select' do + country_model = define_model(:country) do + has_many :people, select: 'age' + end + define_model(:person, country_id: :integer, age: :integer) + delegate_reflection = country_model.reflect_on_association(:people) + reflection = described_class.new(delegate_reflection) + + actual_sql = reflection.association_relation.to_sql + expected_sql = Person.select('age').to_sql + expect(actual_sql).to eq expected_sql + end + end + end + end + end +end diff --git a/spec/support/rails_versions.rb b/spec/support/rails_versions.rb new file mode 100644 index 00000000..459ef598 --- /dev/null +++ b/spec/support/rails_versions.rb @@ -0,0 +1,18 @@ +module RailsVersions + def rails_version + Gem::Version.new(Rails::VERSION::STRING) + end + + def rails_3_x? + Gem::Requirement.new('~> 3.0').satisfied_by?(rails_version) + end + + def rails_4_x? + Gem::Requirement.new('~> 4.0').satisfied_by?(rails_version) + end +end + +RSpec.configure do |config| + config.include(RailsVersions) + config.extend(RailsVersions) +end