Skip to content

Data loaded twice from DB in a situation with many-to-many permissions table #637

Open
driehle opened this Issue Jun 2, 2012 · 11 comments

6 participants

@driehle
driehle commented Jun 2, 2012

Hey there,

I'm having a little touble to get CanCan behaving the way I expect it to ;) I have the following basic setup, using Devise for authentication and CanCan for authorization:

class User < ActiveRecord::Base
  has_many :user_roles
  has_many :roles, :through => :user_roles
  has_many :my_model_permissions
  has_many :my_models, :through => :my_model_permissions
end

class MyModel < ActiveRecord::Base
  # normal relation to all users via their permissions
  has_many :my_model_permissions
  has_many :users, :through => :my_model_permissions

  # special relation to owners, needed for CanCan!
  has_many :my_model_owner_permissions, :class_name => "MyModelPermission", :conditions => "my_model_owner_permissions_my_models_join.variant = 'owner'"
  has_many :owners, :through => :my_model_owner_permissions, :class_name => "User", :source => :user
end

class MyModelPermission < ActiveRecord::Base
  belongs_to :user
  belongs_to :my_model

  validates_inclusion_of :variant, :in => %w{owner write read}
end

So as you can see there are objects (MyModel) that users can create (then they are an owner) and share with other users (they can gain read or read+write permissions). Now I want the following abilities:

  • Owners can do everything with their objects
  • Users with read+write permissions can do everything except of destroying the object
  • Users with read permissions can only view the object
  • Owners can manage the permissions for their objects

The first three things seemd to be pretty easy, I solved them like this:

      can :create, MyModel
      can :manage, MyModel, :my_model_permissions => {:user_id => user.id, :variant => "owner"}
      can :manage, MyModel, :my_model_permissions => {:user_id => user.id, :variant => "write"}
      cannot :destroy, MyModel, :my_model_permissions => {:user_id => user.id, :variant => "write"}
      can :read, MyModel, :my_model_permissions => {:user_id => user.id, :variant => "read"}

The last thing was a little hard to solve, I needed to add an owner relation to MyModel, otherwise something with the table joins didn't seem to work out. After adding the owner-relation (see above) I solved it this way:

      # only owner can manage permissions and invitations
      can :manage, MyModelPermission, :my_model => {:owners => {:id => user.id}}
      can :manage, MyModelInvitation, :my_model => {:owners => {:id => user.id}}

The MyModelController is set up like this:

class MyModelsController < ApplicationController
  before_filter :authenticate_user!
  load_and_authorize_resource :through => :current_user
  [...]
end

Now that setup kinna works, but not a 100%. When I call /my_models.json all MyModels exist two times in the output.
Whats going wrong here?

Is there something wrong with my whole setup?

According to the console it's executing the following SQL commands:

  User Load (0.0ms)  SELECT "users".* FROM "users" WHERE "users"."id" = 2 LIMIT 1
  MyModel Load (1.0ms)  SELECT "my_models".* FROM "my_models" INNER JOIN "my_model_permissions" ON "my_models"."id" = "my_model_permissions"."my_model_id" WHERE "my_model_permissions"."user_id" = 2
  Role Load (1.0ms)  SELECT "roles".* FROM "roles" INNER JOIN "user_roles" ON "roles"."id" = "user_roles"."role_id" WHERE "user_roles"."user_id" = 2 AND "roles"."name" = 'Administrator' LIMIT 1
  MyModel Load (1.0ms)  SELECT "my_models".* FROM "my_models" INNER JOIN "my_model_permissions" "my_model_permissions_my_models" ON "my_model_permissions_my_models"."my_model_id" = "my_models"."id" INNER JOIN "my_model_permissions" ON "my_models"."id" = "my_model_permissions"."my_model_id" WHERE "my_model_permissions"."user_id" = 2 AND (("my_model_permissions"."user_id" = 2 AND "my_model_permissions"."variant" = 'read') OR (("my_model_permissions"."user_id" = 2 AND "my_model_permissions"."variant" = 'write') OR ("my_model_permissions"."user_id" = 2 AND "my_model_permissions"."variant" = 'owner')))

Help appreciated ;)

@mikepack
Collaborator
mikepack commented Jun 3, 2012

@driehle Can you please provide the index action on your MyModelsController? It looks like the last query of the four is the one doing the permission checking. The last query also includes a "my_model_permissions_my_models" table and the INNER JOIN syntax is invalid (ie INNER JOIN "my_model_permissions" "my_model_permissions_my_models" ON).

@driehle
driehle commented Jun 3, 2012

@mikepack There is nothing special about the index action, it's more or less empty:

class MyModelsController < ApplicationController
  before_filter :authenticate_user!
  load_and_authorize_resource :through => :current_user

  def index
    respond_to do |format|
      format.html # index.html.erb
      format.json { render json: @my_models.to_json().html_safe }
    end
  end

  [... other CRUD actions ...]
end

And additional question: Can the :manage for MyModelPermission and MyModelInvitation be done without the need for the addional :owner relation?

@mikepack
Collaborator
mikepack commented Jun 5, 2012

Can Not Reproduce. A couple preliminary questions:

  • What version of CanCan?
  • Are you sure you don't have duplicate records in the database (and hence the duplication you're seeing)?

As far as multiple MyModel queries go, even if it's grabbing the collection twice, it wouldn't be merging the two results. You can see from line 160 of controller_resource.rb that it would overwrite the previously assigned @my_models variable for the second call. This leads me to believe that either you have duplicate records in the database, your JSON serialization technique exposes duplicate records, the SQL generated produced duplicate records, or a variety of other forms of duplication exist. Incorrect SQL, of course, would be an issue with CanCan.

Also, is this really the query that gets generated and output or did you fiddle with it for the sake of the bug report? It appears to be invalid.   
`MyModel Load (1.0ms)  SELECT "my_models".* FROM "my_models" INNER JOIN "my_model_permissions" "my_model_permissions_my_models" ON "my_model_permissions_my_models"."my_model_id" = "my_models"."id" INNER JOIN "my_model_permissions" ON "my_models"."id" = "my_model_permissions"."my_model_id" WHERE "my_model_permissions"."user_id" = 2 AND (("my_model_permissions"."user_id" = 2 AND "my_model_permissions"."variant" = 'read') OR (("my_model_permissions"."user_id" = 2 AND "my_model_permissions"."variant" = 'write') OR ("my_model_permissions"."user_id" = 2 AND "my_model_permissions"."variant" = 'owner')))`
@mhenrixon

@mikepack I can confirm this is indeed happening and I believe it's something to do with upgrading to rails 3.2.5. After upgrading from rails 3.2.3 to 3.2.5 it seems cancan is calling every can? :action, @instance multiple times. Our view performance in views using can? is 5-6 times slower now.

@mhenrixon

It's not rails 3.2.5 it's something else, I am investigating right now.

@driehle
driehle commented Jun 5, 2012

I'm using rails 3.2.2 and cancan 1.6.7 if that helps...

@driehle
driehle commented Jun 19, 2012

Any updates on this?

@DavidMikeSimon

I am seeing similar behavior on my app (Rails 2.3.12, cancan 1.6.8 and also tried with 1.6.7 with same results) which definitely stems from the same core problem: that oddly-named and unused inner join.

In my case, I have School, Student, and a join model SchoolStudent. A user should be allowed to view the complete school history for any student if that student is associated with the user's school:

can :read, SchoolStudent, :student => { :school_students => { :school_id => my_school_ids } }

So to see all the schools that student_x has attended:

student_x.school_students.accessible_by(current_ability)

And I get back duplicate entries just like driehle. I'm also missing some rows from the result. :-(

Both problems stem from the query being incorrect; it only returns records for the given student and the current school, not all the records for the given student. This is plainly visible in the query:

SELECT `school_students`.* FROM `school_students`
INNER JOIN `students` ON `students`.id = `school_students`.student_id
INNER JOIN `school_students` school_students_students ON school_students_students.student_id = students.id 
WHERE ((`school_students`.`student_id`=1161 AND `school_students`.`school_id` IN (3)))

For the query to be correct, the condition after the AND needs to refer to school_students_students rather than school_students. If I make that correction to the query manually, it fixes both the duplicate-rows problem and the missing-rows problem.

@jonsgreen
Collaborator

I spent a bit of time looking into this and though I have no easy answers or solutions I did uncover that there is a common misunderstanding about how the ActiveRecord API for nested where conditions are supposed to work. There does seem to be a change with Rails 3.2.6 that now underscores this because previously incorrect multi-level hashes now create broken SQL.

Here are some useful threads to explain how conditions like

:student => { :school_students => { :school_id => my_school_ids } }

were never really supposed to work:

http://coderwall.com/p/7u6-rg
rails/rails#6718

@xhoy
xhoy commented Apr 10, 2014

Dear submitter, Since cancan/raynB hasn't been active for more than 6 months and no body else then ryam himself has commit permissions the cancan project is on a stand still.
Since cancan has several issues including missing support for rails 4 cancan is moving forward to cancancan. More details on: #994

If your feel that your pull request or bug is still applicable (and hasn't been merged in to cancan) it would be really appreciated if you would resubmit it to cancancan (https://github.com/cancancommunity/cancancan)

We hope to see you on the other side!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.