diff --git a/CHANGELOG.md b/CHANGELOG.md index 24cecc0c..a63cdaf5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,9 @@ ## 3.0.2 * [#590](https://github.com/CanCanCommunity/cancancan/pull/590): Fix Rule#inspect when rule is created through a SQL array. ([@frostblooded][]) -* [#592](https://github.com/CanCanCommunity/cancancan/pull/592): Prevent normalization of through polymorphic associations.([@eloyesp][]) +* [#592](https://github.com/CanCanCommunity/cancancan/pull/592): Prevent normalization of through polymorphic associations. ([@eloyesp][]) +* [#600](https://github.com/CanCanCommunity/cancancan/pull/600): Switch back to eagerly loading associations for ActiveRecord 5.0.x and 5.1.x. 5.2+ will still not automatically eager load. ([@ghiculescu][]) +* [#600](https://github.com/CanCanCommunity/cancancan/pull/600): Fix regression when using accessibl_by with default_scopes that set an order. ([@ghiculescu][]) ## 3.0.1 @@ -650,3 +652,4 @@ Please read the [guide on migrating from CanCanCan 2.x to 3.0](https://github.co [@kaspernj]: https://github.com/kaspernj [@frostblooded]: https://github.com/frostblooded [@eloyesp]: https://github.com/eloyesp +[@ghiculescu]: https://github.com/ghiculescu diff --git a/lib/cancan/model_adapters/active_record_5_adapter.rb b/lib/cancan/model_adapters/active_record_5_adapter.rb index cac1ac4b..687fbfcc 100644 --- a/lib/cancan/model_adapters/active_record_5_adapter.rb +++ b/lib/cancan/model_adapters/active_record_5_adapter.rb @@ -22,9 +22,46 @@ def self.matches_condition?(subject, name, value) private def build_relation(*where_conditions) - relation = @model_class.where(*where_conditions) - relation = relation.left_joins(joins).distinct if joins.present? - relation + relation = @model_class.all + return relation unless where_conditions.any?(&:present?) || joins.present? + + relation = relation.where(*where_conditions) + relation = add_joins_to_relation(relation) + extract_ids_and_requery(relation) + end + + def extract_ids_and_requery(relation) + need_to_extract_ids = relation.values[:distinct].present? || relation.values[:order].present? + + # we need to extract IDs if the query has a builtin `distinct` or `order`. an example of when + # would be a `default_scope` that adds either. this is because otherwise you'll call + # SELECT DISTINCT @model_class.* ORDER BY ___, but there's no guarantee that the columns you + # order by will be included in the SELECT. a previously attempted solution was to add + # SELECTs for all the ORDER depends on (see https://github.com/CanCanCommunity/cancancan/pull/600) + # but this breaks COUNT queries. the simplest general purpose solution is to get IDs for all + # records that match the cancan criteria, then do *another* query that re-adds the default scope. + # this is what we do here. + # the main downside is some queries cancan generates will now look a bit uglier. + if need_to_extract_ids + @model_class.where(id: relation.reorder(nil).pluck(:id).uniq) + else + # if we don't already have a `distinct` (relation.values[:distinct].blank?) + # but we have added a join, we need to add our own `distinct`. + relation = relation.distinct if joins.present? + relation + end + end + + def add_joins_to_relation(relation) + return relation unless joins.present? + + # AR#left_joins doesn't play nicely in AR 5.0 and 5.1 + # see https://github.com/CanCanCommunity/cancancan/pull/600#issuecomment-524672268 + if self.class.version_greater_or_equal?('5.2') + relation.left_joins(joins) + else + relation.includes(joins).references(joins) + end end # Rails 4.2 deprecates `sanitize_sql_hash_for_conditions` diff --git a/spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb b/spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb index 28e2d836..b1c62057 100644 --- a/spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb +++ b/spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb @@ -87,7 +87,7 @@ class Editor < ActiveRecord::Base end end - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.2.0') describe 'selecting custom columns' do it 'extracts custom columns correctly' do posts = Post.accessible_by(ability).select('title as mytitle') diff --git a/spec/cancan/model_adapters/accessible_by_integration_spec.rb b/spec/cancan/model_adapters/accessible_by_integration_spec.rb index f5a2ae93..181b76c7 100644 --- a/spec/cancan/model_adapters/accessible_by_integration_spec.rb +++ b/spec/cancan/model_adapters/accessible_by_integration_spec.rb @@ -89,7 +89,7 @@ class Editor < ActiveRecord::Base end end - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.2.0') describe 'selecting custom columns' do it 'extracts custom columns correctly' do posts = Post.accessible_by(ability).select('title as mytitle') diff --git a/spec/cancan/model_adapters/active_record_5_adapter_with_default_scopes_spec.rb b/spec/cancan/model_adapters/active_record_5_adapter_with_default_scopes_spec.rb new file mode 100644 index 00000000..ad9f1e95 --- /dev/null +++ b/spec/cancan/model_adapters/active_record_5_adapter_with_default_scopes_spec.rb @@ -0,0 +1,156 @@ +# frozen_string_literal: true + +require 'spec_helper' + +if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + describe CanCan::ModelAdapters::ActiveRecord5Adapter do + context 'with postgresql' do + before :each do + connect_db + ActiveRecord::Migration.verbose = false + + ActiveRecord::Schema.define do + create_table(:blog_authors) do |t| + t.string :name, null: false + t.timestamps null: false + end + + create_table(:blog_posts) do |t| + t.references :blog_author + t.string :title, null: false + t.timestamps null: false + end + + create_table(:blog_post_comments) do |t| + t.references :blog_post + t.string :body, null: false + t.timestamps null: false + end + end + + unless defined?(BlogAuthor) + class BlogAuthor < ActiveRecord::Base + has_many :blog_posts + has_many :blog_post_comments, through: :blog_posts + end + end + + unless defined?(BlogPost) + class BlogPost < ActiveRecord::Base + has_many :blog_post_comments + belongs_to :blog_author + + default_scope -> { order(:title) } + end + end + + unless defined?(BlogPostComment) + class BlogPostComment < ActiveRecord::Base + belongs_to :blog_post + + default_scope -> { order(created_at: :desc) } + end + end + end + + subject(:ability) { Ability.new(nil) } + + let(:alex) { BlogAuthor.create!(name: 'Alex') } + let(:josh) { BlogAuthor.create!(name: 'Josh') } + + let(:p1) { josh.blog_posts.create!(title: 'p1') } + let(:p2) { alex.blog_posts.create!(title: 'p2') } + + let(:p1c1) { p1.blog_post_comments.create!(body: 'p1c1', created_at: Time.new(2019, 8, 25, 1)) } + let(:p1c2) { p1.blog_post_comments.create!(body: 'p1c2', created_at: Time.new(2019, 8, 25, 2)) } + + let(:p2c1) { p2.blog_post_comments.create!(body: 'p2c1', created_at: Time.new(2019, 8, 25, 3)) } + let(:p2c2) { p2.blog_post_comments.create!(body: 'p2c2', created_at: Time.new(2019, 8, 25, 4)) } + + context 'when default scope sets an order, and abilities dont have extra checks' do + before do + ability.can :read, BlogPost + ability.can :read, BlogPostComment + end + + it 'can get accessible records' do + expected_bodies_in_order = [p2c2, p2c1, p1c2, p1c1].map(&:body) + accessible = BlogPostComment.accessible_by(ability) + expect(accessible.map(&:body)).to eq(expected_bodies_in_order) + expect(accessible.count).to eq(4) + end + + it 'can get accessible records from a has_many' do + expected_bodies_in_order = [p2c2, p2c1].map(&:body) + accessible = p2.blog_post_comments.accessible_by(ability) + expect(accessible.map(&:body)).to eq(expected_bodies_in_order) + expect(accessible.count).to eq(2) + end + + it 'can get accessible records from a has_many - other post' do + expected_bodies_in_order = [p1c2, p1c1].map(&:body) + accessible = p1.blog_post_comments.accessible_by(ability) + expect(accessible.map(&:body)).to eq(expected_bodies_in_order) + expect(accessible.count).to eq(2) + end + + it 'can get accessible records from a has_many :through' do + expected_bodies_in_order = [p2c2, p2c1].map(&:body) + accessible = alex.blog_post_comments.accessible_by(ability) + expect(accessible.map(&:body)).to eq(expected_bodies_in_order) + expect(accessible.count).to eq(2) + end + + it 'can get accessible records from a has_many :through - other user' do + expected_bodies_in_order = [p1c2, p1c1].map(&:body) + accessible = josh.blog_post_comments.accessible_by(ability) + expect(accessible.map(&:body)).to eq(expected_bodies_in_order) + expect(accessible.count).to eq(2) + end + end + + context 'when default scope sets an order, and abilities have extra checks' do + before do + ability.can :read, BlogPost + # this is the only change vs. the above context -- in this context we can *only* see alex's posts + ability.can :read, BlogPostComment, blog_post: { blog_author_id: alex.id } + end + + it 'can get accessible records' do + expected_bodies_in_order = [p2c2, p2c1].map(&:body) + accessible = BlogPostComment.accessible_by(ability) + expect(accessible.map(&:body)).to eq(expected_bodies_in_order) + expect(accessible.count).to eq(2) + end + + it 'can get accessible records from a has_many' do + expected_bodies_in_order = [p2c2, p2c1].map(&:body) + accessible = p2.blog_post_comments.accessible_by(ability) + expect(accessible.map(&:body)).to eq(expected_bodies_in_order) + expect(accessible.count).to eq(2) + end + + it 'can get accessible records from a has_many - none returned' do + expected_bodies_in_order = [] + accessible = p1.blog_post_comments.accessible_by(ability) + expect(accessible.map(&:body)).to eq(expected_bodies_in_order) + expect(accessible.count).to eq(0) + end + + it 'can get accessible records from a has_many :through' do + expected_bodies_in_order = [p2c2, p2c1].map(&:body) + accessible = alex.blog_post_comments.accessible_by(ability) + expect(accessible.map(&:body)).to eq(expected_bodies_in_order) + expect(accessible.count).to eq(2) + end + + it 'can get accessible records from a has_many :through - none returned' do + expected_bodies_in_order = [] + accessible = josh.blog_post_comments.accessible_by(ability) + expect(accessible.map(&:body)).to eq(expected_bodies_in_order) + expect(accessible.count).to eq(0) + end + end + end + end +end diff --git a/spec/cancan/model_adapters/active_record_adapter_spec.rb b/spec/cancan/model_adapters/active_record_adapter_spec.rb index e5691044..f28f5391 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_adapter_spec.rb @@ -194,6 +194,7 @@ class User < ActiveRecord::Base Article.create!(published: true, category: category) Article.create!(published: true, category: category) expect(Category.accessible_by(@ability)).to match_array([category]) + expect(Category.accessible_by(@ability).count).to eq(1) end it 'only reads comments for visible categories through articles' do @@ -529,7 +530,8 @@ class Transaction < ActiveRecord::Base ability.can :read, Article, mentioned_users: { name: u1.name } ability.can :read, Article, mentions: { user: { name: u2.name } } expect(Article.accessible_by(ability)).to match_array([a1, a2]) - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + + if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.2.0') expect(ability.model_adapter(Article, :read)).to generate_sql(%( SELECT DISTINCT "articles".* FROM "articles" diff --git a/spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb b/spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb index ad556066..5f478f32 100644 --- a/spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb +++ b/spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb @@ -51,7 +51,7 @@ class House < ActiveRecord::Base expect(houses).to match_array [@house2, @house1] end - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.2.0') it 'generates the correct query' do expect(ability.model_adapter(House, :read)) .to generate_sql("SELECT DISTINCT \"houses\".* diff --git a/spec/changelog_spec.rb b/spec/changelog_spec.rb index 468447fd..1220e643 100644 --- a/spec/changelog_spec.rb +++ b/spec/changelog_spec.rb @@ -11,6 +11,8 @@ expect(changelog.end_with?("\n")).to be true end + # if this test fails it means you probably haven't added a link + # for your GH username at the bottom of the CHANGELOG it 'has link definitions for all implicit links' do implicit_link_names = changelog.scan(/\[([^\]]+)\]\[\]/).flatten.uniq implicit_link_names.each do |name|