From b4b289c3ac7c873f230f8064feaa6b0c6f01ac30 Mon Sep 17 00:00:00 2001 From: Albert Song Date: Wed, 24 Feb 2021 10:32:10 +0800 Subject: [PATCH] Use `relation.and` instead of `relation.merge` whenever possible. `relation.merge` intends to mimics Hash#merge, (i.e. later condition on the same column replaces the previous one) see rails/rails#39250 & rails/rails#40944 Using relation#merge would cause suprising result when you try to limit the resource collection through nesting while the foreign_key also has condition in ability rule scope. --- lib/cancan.rb | 1 + .../model_adapters/active_record_5_adapter.rb | 2 +- .../active_record_61_adapter.rb | 34 +++++++++++++++++++ .../active_record_adapter_spec.rb | 7 +++- 4 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 lib/cancan/model_adapters/active_record_61_adapter.rb diff --git a/lib/cancan.rb b/lib/cancan.rb index 80d7a24dc..427da4187 100644 --- a/lib/cancan.rb +++ b/lib/cancan.rb @@ -21,4 +21,5 @@ require 'cancan/model_adapters/active_record_adapter' require 'cancan/model_adapters/active_record_4_adapter' require 'cancan/model_adapters/active_record_5_adapter' + require 'cancan/model_adapters/active_record_61_adapter' end diff --git a/lib/cancan/model_adapters/active_record_5_adapter.rb b/lib/cancan/model_adapters/active_record_5_adapter.rb index 072ed7f50..2dd219589 100644 --- a/lib/cancan/model_adapters/active_record_5_adapter.rb +++ b/lib/cancan/model_adapters/active_record_5_adapter.rb @@ -6,7 +6,7 @@ class ActiveRecord5Adapter < ActiveRecord4Adapter AbstractAdapter.inherited(self) def self.for_class?(model_class) - version_greater_or_equal?('5.0.0') && model_class <= ActiveRecord::Base + version_greater_or_equal?('5.0.0') && version_lower?('6.1.0') && model_class <= ActiveRecord::Base end # rails 5 is capable of using strings in enum diff --git a/lib/cancan/model_adapters/active_record_61_adapter.rb b/lib/cancan/model_adapters/active_record_61_adapter.rb new file mode 100644 index 000000000..bcec1bb6c --- /dev/null +++ b/lib/cancan/model_adapters/active_record_61_adapter.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module CanCan + module ModelAdapters + class ActiveRecord61Adapter < ActiveRecord5Adapter + AbstractAdapter.inherited(self) + + def self.for_class?(model_class) + version_greater_or_equal?('6.1.0') && model_class <= ActiveRecord::Base + end + + # rails 6.1 introduced `relation.and` + # which is more suitable for intersecting two relations then `relation.merge` + def database_records + return super unless override_scope + + if @model_class.all.send(:structurally_incompatible_values_for, override_scope).empty? + @model_class.and(override_scope) + else + # wrap both side in subqeruy to satisfy structural compatibility requirements + wrap_model_subquery(@model_class.all).and(wrap_model_subquery(override_scope)) + end + end + + private + + def wrap_model_subquery(scope) + real_model_class = @model_class.all.klass + + real_model_class.where(real_model_class.primary_key => scope) + 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 df2a019e2..6b1b39379 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_adapter_spec.rb @@ -134,7 +134,12 @@ class User < ActiveRecord::Base end it 'is for only active record classes' do - if ActiveRecord.version > Gem::Version.new('5') + if ActiveRecord.version > Gem::Version.new('6.1') + expect(CanCan::ModelAdapters::ActiveRecord61Adapter).to_not be_for_class(Object) + expect(CanCan::ModelAdapters::ActiveRecord61Adapter).to be_for_class(Article) + expect(CanCan::ModelAdapters::AbstractAdapter.adapter_class(Article)) + .to eq(CanCan::ModelAdapters::ActiveRecord61Adapter) + elsif ActiveRecord.version > Gem::Version.new('5') expect(CanCan::ModelAdapters::ActiveRecord5Adapter).to_not be_for_class(Object) expect(CanCan::ModelAdapters::ActiveRecord5Adapter).to be_for_class(Article) expect(CanCan::ModelAdapters::AbstractAdapter.adapter_class(Article))