Permalink
Browse files

fixing active record adapter behavior and improving specs for it

  • Loading branch information...
1 parent af9e77a commit cc30e838c04ef4ae0376af467aec4182b5517fa7 @ryanb committed Dec 30, 2010
View
@@ -2,7 +2,9 @@ source "http://rubygems.org"
case ENV["MODEL_ADAPTER"]
when nil, "active_record"
+ gem "sqlite3-ruby", :require => "sqlite3"
gem "activerecord", :require => "active_record"
+ gem "with_model"
when "data_mapper"
gem "dm-core", "~> 1.0.2"
when "mongoid"
@@ -19,14 +19,26 @@ class ActiveRecordAdapter < AbstractAdapter
def conditions
if @rules.size == 1 && @rules.first.base_behavior
# Return the conditions directly if there's just one definition
- @rules.first.tableized_conditions.dup
+ tableized_conditions(@rules.first.conditions).dup
else
@rules.reverse.inject(false_sql) do |sql, rule|
- merge_conditions(sql, rule.tableized_conditions.dup, rule.base_behavior)
+ merge_conditions(sql, tableized_conditions(rule.conditions).dup, rule.base_behavior)
end
end
end
+ def tableized_conditions(conditions)
+ return conditions unless conditions.kind_of? Hash
+ conditions.inject({}) do |result_hash, (name, value)|
+ if value.kind_of? Hash
+ name = @model_class.reflect_on_association(name).table_name
+ value = tableized_conditions(value)
+ end
+ result_hash[name] = value
+ result_hash
+ end
+ end
+
# Returns the associations used in conditions for the :joins option of a search.
# See ActiveRecordAdditions#accessible_by for use in Active Record.
def joins
View
@@ -3,7 +3,7 @@ module CanCan
# it holds the information about a "can" call made on Ability and provides
# helpful methods to determine permission checking and conditions hash generation.
class Rule # :nodoc:
- attr_reader :base_behavior, :actions
+ attr_reader :base_behavior, :actions, :conditions
attr_writer :expanded_actions
# The first argument when initializing is the base_behavior which is a true/false
@@ -41,18 +41,6 @@ def matches_conditions?(action, subject, extra_args)
end
end
- def tableized_conditions(conditions = @conditions)
- return conditions unless conditions.kind_of? Hash
- conditions.inject({}) do |result_hash, (name, value)|
- if value.kind_of? Hash
- name = name.to_s.tableize.to_sym
- value = tableized_conditions(value)
- end
- result_hash[name] = value
- result_hash
- end
- end
-
def only_block?
conditions_empty? && !@block.nil?
end
@@ -1,77 +1,175 @@
if ENV["MODEL_ADAPTER"].nil? || ENV["MODEL_ADAPTER"] == "active_record"
require "spec_helper"
+ ActiveRecord::Base.establish_connection(:adapter => "sqlite3", :database => ":memory:")
@fillman

fillman Feb 22, 2012

Ryan, why do you use ActiveRecrod connection when you use supermodel gem. You could define all those models in spec_helper like you did for Project and Category? Or there's some magic hidden?

+
describe CanCan::ModelAdapters::ActiveRecordAdapter do
+ with_model :article do
+ table do |t|
+ t.boolean "published"
+ t.boolean "secret"
+ end
+ model do
+ has_many :comments
+ end
+ end
+
+ with_model :comment do
+ table do |t|
+ t.boolean "spam"
+ t.integer "article_id"
+ end
+ model do
+ belongs_to :article
+ end
+ end
+
before(:each) do
- @model_class = Class.new(Project)
- stub(@model_class).scoped { :scoped_stub }
- @model_class.send(:include, CanCan::ActiveRecordAdditions)
+ Article.delete_all
+ Comment.delete_all
@ability = Object.new
@ability.extend(CanCan::Ability)
+ @article_table = Article.table_name
+ @comment_table = Comment.table_name
end
- it "should call where('true=false') when no ability is defined so no records are found" do
- stub(@model_class).joins { true } # just so it responds to .joins as well
- stub(@model_class).where('true=false').stub!.joins(nil) { :no_match }
- @model_class.accessible_by(@ability, :read).should == :no_match
+ it "should not fetch any records when no abilities are defined" do
+ Article.create!
+ Article.accessible_by(@ability).should be_empty
end
- it "should call where with matching ability conditions" do
- @ability.can :read, @model_class, :foo => {:bar => 1}
- stub(@model_class).joins { true } # just so it responds to .joins as well
- stub(@model_class).where(:foos => { :bar => 1 }).stub!.joins([:foo]) { :found_records }
- @model_class.accessible_by(@ability, :read).should == :found_records
+ it "should fetch all articles when one can read all" do
+ @ability.can :read, Article
+ article = Article.create!
+ Article.accessible_by(@ability).should == [article]
end
- it "should default to :read ability and use scoped when where isn't available" do
- @ability.can :read, @model_class, :foo => {:bar => 1}
- stub(@model_class).scoped(:conditions => {:foos => {:bar => 1}}, :joins => [:foo]) { :found_records }
- @model_class.accessible_by(@ability).should == :found_records
+ it "should fetch only the articles that are published" do
+ @ability.can :read, Article, :published => true
+ article1 = Article.create!(:published => true)
+ article2 = Article.create!(:published => false)
+ Article.accessible_by(@ability).should == [article1]
end
- it "should merge association joins and sanitize conditions" do
- @ability.can :read, @model_class, :foo => {:bar => 1}
- @ability.can :read, @model_class, :too => {:car => 1, :far => {:bar => 1}}
+ it "should fetch any articles which are published or secret" do
+ @ability.can :read, Article, :published => true
+ @ability.can :read, Article, :secret => true
+ article1 = Article.create!(:published => true, :secret => false)
+ article2 = Article.create!(:published => true, :secret => true)
+ article3 = Article.create!(:published => false, :secret => true)
+ article4 = Article.create!(:published => false, :secret => false)
+ Article.accessible_by(@ability).should == [article1, article2, article3]
+ end
- condition_variants = [
- '(toos.fars.bar=1 AND toos.car=1) OR (foos.bar=1)', # faked sql sanitizer is stupid ;-)
- '(toos.car=1 AND toos.fars.bar=1) OR (foos.bar=1)'
- ]
- joins_variants = [
- [:foo, {:too => [:far]}],
- [{:too => [:far]}, :foo]
- ]
+ it "should fetch only the articles that are published and not secret" do
+ @ability.can :read, Article, :published => true
+ @ability.cannot :read, Article, :secret => true
+ article1 = Article.create!(:published => true, :secret => false)
+ article2 = Article.create!(:published => true, :secret => true)
+ article3 = Article.create!(:published => false, :secret => true)
+ article4 = Article.create!(:published => false, :secret => false)
+ Article.accessible_by(@ability).should == [article1]
+ end
- condition_variants.each do |condition|
- joins_variants.each do |joins|
- stub(@model_class).scoped( :conditions => condition, :joins => joins ) { :found_records }
- end
- end
- # @ability.conditions(:read, @model_class).should == '(too.car=1 AND too.far.bar=1) OR (foo.bar=1)'
- # @ability.associations_hash(:read, @model_class).should == [{:too => [:far]}, :foo]
- @model_class.accessible_by(@ability).should == :found_records
+ it "should only read comments for articles which are published" do
+ @ability.can :read, Comment, :article => { :published => true }
+ comment1 = Comment.create!(:article => Article.create!(:published => true))
+ comment2 = Comment.create!(:article => Article.create!(:published => false))
+ Comment.accessible_by(@ability).should == [comment1]
end
- it "should allow to define sql conditions by not hash" do
- @ability.can :read, @model_class, :foo => 1
- @ability.can :read, @model_class, ['bar = ?', 1]
- stub(@model_class).scoped( :conditions => '(bar = 1) OR (foo=1)', :joins => nil ) { :found_records }
- stub(@model_class).scoped{|*args| args.inspect}
- @model_class.accessible_by(@ability).should == :found_records
+ it "should allow conditions in SQL and merge with hash conditions" do
+ @ability.can :read, Article, :published => true
+ @ability.can :read, Article, ["secret=?", true]
+ article1 = Article.create!(:published => true, :secret => false)
+ article4 = Article.create!(:published => false, :secret => false)
+ Article.accessible_by(@ability).should == [article1]
end
it "should not allow to fetch records when ability with just block present" do
- @ability.can :read, @model_class do false end
- lambda {
- @model_class.accessible_by(@ability)
- }.should raise_error(CanCan::Error)
+ @ability.can :read, Article do
+ false
+ end
+ lambda { Article.accessible_by(@ability) }.should raise_error(CanCan::Error)
+ end
+
+ it "should not allow to check ability on object against SQL conditions without block" do
+ @ability.can :read, Article, ["secret=?", true]
+ lambda { @ability.can? :read, Article.new }.should raise_error(CanCan::Error)
+ end
+
+ it "should have false conditions if no abilities match" do
+ @ability.model_adapter(Article, :read).conditions.should == "'t'='f'"
+ end
+
+ it "should return false conditions for cannot clause" do
+ @ability.cannot :read, Article
+ @ability.model_adapter(Article, :read).conditions.should == "'t'='f'"
+ end
+
+ it "should return SQL for single `can` definition in front of default `cannot` condition" do
+ @ability.cannot :read, Article
+ @ability.can :read, Article, :published => false, :secret => true
+ @ability.model_adapter(Article, :read).conditions.should orderlessly_match(%Q["#{@article_table}"."published" = 'f' AND "#{@article_table}"."secret" = 't'])
+ end
+
+ it "should return 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
+ @ability.model_adapter(Article, :read).conditions.should == "'t'='t'"
+ end
+
+ it "should return `false condition` for single `cannot` definition in front of default `cannot` condition" do
+ @ability.cannot :read, Article
+ @ability.cannot :read, Article, :published => false, :secret => true
+ @ability.model_adapter(Article, :read).conditions.should == "'t'='f'"
+ end
+
+ it "should return `not (sql)` for single `cannot` definition in front of default `can` condition" do
+ @ability.can :read, Article
+ @ability.cannot :read, Article, :published => false, :secret => true
+ @ability.model_adapter(Article, :read).conditions.should orderlessly_match(%Q["not (#{@article_table}"."published" = 'f' AND "#{@article_table}"."secret" = 't')])
+ end
+
+ it "should return appropriate sql conditions in complex case" do
+ @ability.can :read, Article
+ @ability.can :manage, Article, :id => 1
+ @ability.can :update, Article, :published => true
+ @ability.cannot :update, Article, :secret => true
+ @ability.model_adapter(Article, :update).conditions.should == %Q[not ("#{@article_table}"."secret" = 't') AND (("#{@article_table}"."published" = 't') OR ("#{@article_table}"."id" = 1))]
+ @ability.model_adapter(Article, :manage).conditions.should == {:id => 1}
+ @ability.model_adapter(Article, :read).conditions.should == "'t'='t'"
+ end
+
+ it "should not forget conditions when calling with SQL string" do
+ @ability.can :read, Article, :published => true
+ @ability.can :read, Article, ['secret=?', false]
+ adapter = @ability.model_adapter(Article, :read)
+ 2.times do
+ adapter.conditions.should == %Q[(secret='f') OR ("#{@article_table}"."published" = 't')]
+ end
+ end
+
+ it "should have nil joins if no rules" do
+ @ability.model_adapter(Article, :read).joins.should be_nil
+ end
+
+ it "should have nil joins if no nested hashes specified in conditions" do
+ @ability.can :read, Article, :published => false
+ @ability.can :read, Article, :secret => true
+ @ability.model_adapter(Article, :read).joins.should be_nil
+ end
+
+ it "should merge separate joins into a single array" do
+ @ability.can :read, Article, :project => { :blocked => false }
+ @ability.can :read, Article, :company => { :admin => true }
+ @ability.model_adapter(Article, :read).joins.inspect.should orderlessly_match([:company, :project].inspect)
end
- it "should not allow to check ability on object when nonhash sql ability definition without block present" do
- @ability.can :read, @model_class, ['bar = ?', 1]
- lambda {
- @ability.can? :read, @model_class.new
- }.should raise_error(CanCan::Error)
+ it "should merge same joins into a single array" do
+ @ability.can :read, Article, :project => { :blocked => false }
+ @ability.can :read, Article, :project => { :admin => true }
+ @ability.model_adapter(Article, :read).joins.should == [:project]
end
end
end
Oops, something went wrong.

2 comments on commit cc30e83

I did some digging on the issue # 204, and I think that the problem is with this commit, the merge_conditions(sql, tableized_conditions(rule.conditions).dup, rule.base_behavior) is not producing the result we want.

I'm sorry turns out I tested it wrong....I'll come up with a fix hopefully
EDIT: merge_conditions(sql, tableized_conditions(rule.conditions).dup, rule.base_behavior) is actually giving the correct individual parts of the SQL script to be performed. However, all combined, this part:

@rules.reverse.inject(false_sql) do |sql, rule|
    merge_conditions(sql, tableized_conditions(rule.conditions).dup, rule.base_behavior)
end

is messing up those parts and is always giving out (1=1) instead. I have a couple of hack fixes on top of my head, but I think I will need more time to think about an elegant fix.

Please sign in to comment.