Add a rules compressor for performance optimization

CHANGELOG.rdoc

@@ -1,5 +1,6 @@
 Unreleased
 
+* Compress irrelevant rules before generating a query to optimize performances (coorasse)
 * Remove ruby 2.2 from Travis and add ruby 2.5.1 (coorasse)
 
 2.2.0 (Apr 13th, 2018)

lib/cancan.rb

@@ -8,6 +8,7 @@
 
 require 'cancan/model_adapters/abstract_adapter'
 require 'cancan/model_adapters/default_adapter'
+require 'cancan/rules_compressor'
 
 if defined? ActiveRecord
   require 'cancan/model_adapters/active_record_adapter'

lib/cancan/controller_additions.rb

@@ -384,6 +384,8 @@ def cannot?(*args)
   end
 end
 
-ActiveSupport.on_load(:action_controller) do
-  include CanCan::ControllerAdditions
+if defined? ActiveSupport
+  ActiveSupport.on_load(:action_controller) do
+    include CanCan::ControllerAdditions
+  end
 end

lib/cancan/model_adapters/active_record_adapter.rb

@@ -1,4 +1,5 @@
 require_relative 'can_can/model_adapters/active_record_adapter/joins.rb'
+require 'cancan/rules_compressor'
 module CanCan
   module ModelAdapters
     module ActiveRecordAdapter
@@ -20,16 +21,17 @@ module ActiveRecordAdapter
       #   query(:manage, User).conditions # => "not (self_managed = 't') AND ((manager_id = 1) OR (id = 1))"
       #
       def conditions
-        if @rules.size == 1 && @rules.first.base_behavior
+        compressed_rules = RulesCompressor.new(@rules.reverse).rules_collapsed.reverse
+        if compressed_rules.size == 1 && compressed_rules.first.base_behavior
           # Return the conditions directly if there's just one definition
-          tableized_conditions(@rules.first.conditions).dup
+          tableized_conditions(compressed_rules.first.conditions).dup
         else
-          extract_multiple_conditions
+          extract_multiple_conditions(compressed_rules)
         end
       end
 
-      def extract_multiple_conditions
-        @rules.reverse.inject(false_sql) do |sql, rule|
+      def extract_multiple_conditions(rules)
+        rules.reverse.inject(false_sql) do |sql, rule|
           merge_conditions(sql, tableized_conditions(rule.conditions).dup, rule.base_behavior)
         end
       end

lib/cancan/rule.rb

@@ -24,6 +24,18 @@ def initialize(base_behavior, action, subject, conditions, block)
       @block = block
     end
 
+    def can_rule?
+      base_behavior
+    end
+
+    def cannot_catch_all?
+      !can_rule? && catch_all?
+    end
+
+    def catch_all?
+      [nil, false, [], {}, '', ' '].include? @conditions
+    end
+
     # Matches both the subject and action, not necessarily the conditions
     def relevant?(action, subject)
       subject = subject.values.first if subject.class == Hash

lib/cancan/rules_compressor.rb

@@ -0,0 +1,20 @@
+require_relative 'conditions_matcher.rb'
+module CanCan
+  class RulesCompressor
+    attr_reader :initial_rules, :rules_collapsed
+
+    def initialize(rules)
+      @initial_rules = rules
+      @rules_collapsed = compress(@initial_rules)
+    end
+
+    def compress(array)
+      idx = array.rindex(&:catch_all?)
+      return array unless idx
+      value = array[idx]
+      array[idx..-1]
+        .drop_while { |n| n.base_behavior == value.base_behavior }
+        .tap { |a| a.unshift(value) unless value.cannot_catch_all? }
+    end
+  end
+end

spec/cancan/model_adapters/active_record_adapter_spec.rb

@@ -249,14 +249,17 @@ class User < ActiveRecord::Base
     it 'returns SQL for single `can` definition in front of default `cannot` condition' do
       @ability.cannot :read, Article
       @ability.can :read, Article, published: false, secret: true
-      expect(@ability.model_adapter(Article, :read).conditions)
-        .to orderlessly_match(%("#{@article_table}"."published" = 'f' AND "#{@article_table}"."secret" = 't'))
+      expect(@ability.model_adapter(Article, :read)).to generate_sql(%(
+SELECT "articles".*
+FROM "articles"
+WHERE "articles"."published" = 'f' AND "articles"."secret" = 't'))
     end
 
     it 'returns true condition for single `can` definition in front of default `can` condition' do
       @ability.can :read, Article
       @ability.can :read, Article, published: false, secret: true
-      expect(@ability.model_adapter(Article, :read).conditions).to eq("'t'='t'")
+      expect(@ability.model_adapter(Article, :read).conditions).to eq({})
+      expect(@ability.model_adapter(Article, :read)).to generate_sql(%(SELECT "articles".* FROM "articles"))
     end
 
     it 'returns `false condition` for single `cannot` definition in front of default `cannot` condition' do
@@ -279,10 +282,11 @@ class User < ActiveRecord::Base
       @ability.cannot :update, Article, secret: true
       expect(@ability.model_adapter(Article, :update).conditions)
         .to eq(%[not ("#{@article_table}"."secret" = 't') ] +
-               %[AND (("#{@article_table}"."published" = 't') ] +
-               %[OR ("#{@article_table}"."id" = 1))])
+                 %[AND (("#{@article_table}"."published" = 't') ] +
+                 %[OR ("#{@article_table}"."id" = 1))])
       expect(@ability.model_adapter(Article, :manage).conditions).to eq(id: 1)
-      expect(@ability.model_adapter(Article, :read).conditions).to eq("'t'='t'")
+      expect(@ability.model_adapter(Article, :read).conditions).to eq({})
+      expect(@ability.model_adapter(Article, :read)).to generate_sql(%(SELECT "articles".* FROM "articles"))
     end
 
     it 'returns appropriate sql conditions in complex case with nested joins' do

spec/cancan/rule_compressor_spec.rb

@@ -0,0 +1,119 @@
+require 'spec_helper'
+
+describe CanCan::RulesCompressor do
+  before do
+    class Blog
+    end
+  end
+
+  def can(action, subject, args = nil)
+    CanCan::Rule.new(true, action, subject, args, nil)
+  end
+
+  def cannot(action, subject, args = nil)
+    CanCan::Rule.new(false, action, subject, args, nil)
+  end
+
+  context 'a "cannot catch_all" rule is in first position' do
+    let(:rules) do
+      [cannot(:read, Blog),
+       can(:read, Blog)]
+    end
+    it 'deletes it' do
+      expect(described_class.new(rules).rules_collapsed).to eq rules[1..-1]
+    end
+  end
+
+  context 'a "can catch all" rule is in last position' do
+    let(:rules) do
+      [cannot(:read, Blog, id: 2),
+       can(:read, Blog, id: 1),
+       can(:read, Blog)]
+    end
+
+    it 'deletes all previous rules' do
+      expect(described_class.new(rules).rules_collapsed).to eq [rules.last]
+    end
+  end
+
+  context 'a "can catch_all" rule is in front of others can rules' do
+    let(:rules) do
+      [can(:read, Blog, id: 1),
+       can(:read, Blog),
+       can(:read, Blog, id: 3),
+       can(:read, Blog, author: { id: 3 }),
+       cannot(:read, Blog, private: true)]
+    end
+
+    it 'deletes all previous rules and subsequent rules of the same type' do
+      expect(described_class.new(rules).rules_collapsed).to eq [rules[1], rules.last]
+    end
+  end
+
+  context 'a "cannot catch_all" rule is in front of others cannot rules' do
+    let(:rules) do
+      [can(:read, Blog, id: 1),
+       can(:read, Blog),
+       can(:read, Blog, id: 3),
+       cannot(:read, Blog),
+       cannot(:read, Blog, private: true),
+       can(:read, Blog, id: 3)]
+    end
+
+    it 'deletes all previous rules and subsequent rules of the same type' do
+      expect(described_class.new(rules).rules_collapsed).to eq [rules.last]
+    end
+  end
+
+  context 'a lot of rules' do
+    let(:rules) do
+      [
+        cannot(:read, Blog, id: 4),
+        can(:read, Blog, id: 1),
+        can(:read, Blog),
+        can(:read, Blog, id: 3),
+        cannot(:read, Blog),
+        cannot(:read, Blog, private: true),
+        can(:read, Blog, id: 3),
+        can(:read, Blog, id: 8),
+        cannot(:read, Blog, id: 5)
+      ]
+    end
+
+    it 'minimizes the rules' do
+      expect(described_class.new(rules).rules_collapsed).to eq rules.last(3)
+    end
+  end
+
+  # TODO: not supported yet
+  xcontext 'duplicate rules' do
+    let(:rules) do
+      [can(:read, Blog, id: 4),
+       can(:read, Blog, id: 1),
+       can(:read, Blog, id: 2),
+       can(:read, Blog, id: 2),
+       can(:read, Blog, id: 3),
+       can(:read, Blog, id: 2)]
+    end
+
+    it 'minimizes the rules, by removing duplicates' do
+      expect(described_class.new(rules).rules_collapsed).to eq [rules[0], rules[1], rules[2], rules[4]]
+    end
+  end
+
+  # TODO: not supported yet
+  xcontext 'merges rules' do
+    let(:rules) do
+      [can(:read, Blog, id: 4),
+       can(:read, Blog, id: 1),
+       can(:read, Blog, id: 2),
+       can(:read, Blog, id: 2),
+       can(:read, Blog, id: 3),
+       can(:read, Blog, id: 2)]
+    end
+
+    it 'minimizes the rules, by merging them' do
+      expect(described_class.new(rules).rules_collapsed).to eq [can(:read, Blog, id: [4, 1, 2, 3])]
+    end
+  end
+end

spec/spec_helper.rb

@@ -24,4 +24,15 @@
   config.expect_with :rspec do |c|
     c.syntax = :expect
   end
+
+  config.include SQLHelpers
+end
+
+RSpec::Matchers.define :generate_sql do |expected|
+  match do |actual|
+    normalized_sql(actual) == expected.gsub(/\s+/, ' ').strip
+  end
+  failure_message do |actual|
+    "Returned sql:\n#{normalized_sql(actual)}\ninstead of:\n#{expected.gsub(/\s+/, ' ').strip}"
+  end
 end

spec/support/sql_helpers.rb

@@ -0,0 +1,5 @@
+module SQLHelpers
+  def normalized_sql(adapter)
+    adapter.database_records.to_sql.strip.squeeze(' ')
+  end
+end