Skip to content

Fix unintended side effects of delete_all and update_all with multi-tenancy for non-multi-tenant models #269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/activerecord-multi-tenant/relation_extension.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ module Arel
module ActiveRecordRelationExtension
# Overrides the delete_all method to include tenant scoping
def delete_all
model = MultiTenant.multi_tenant_model_for_table(table_name)

# Call the original delete_all method if the current tenant is identified by an ID
return super if MultiTenant.current_tenant_is_id? || MultiTenant.current_tenant.nil?
return super if model.nil? || MultiTenant.current_tenant_is_id? || MultiTenant.current_tenant.nil?

stmt = Arel::DeleteManager.new.from(table)
stmt.wheres = [generate_in_condition_subquery]
Expand All @@ -16,8 +18,10 @@ def delete_all

# Overrides the update_all method to include tenant scoping
def update_all(updates)
model = MultiTenant.multi_tenant_model_for_table(table_name)

# Call the original update_all method if the current tenant is identified by an ID
return super if MultiTenant.current_tenant_is_id? || MultiTenant.current_tenant.nil?
return super if model.nil? || MultiTenant.current_tenant_is_id? || MultiTenant.current_tenant.nil?

stmt = Arel::UpdateManager.new
stmt.table(table)
Expand Down
32 changes: 32 additions & 0 deletions spec/activerecord-multi-tenant/query_rewriter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,38 @@
end
end

context 'when using non-multi-tenant model' do
let!(:account1) { Account.create!(name: 'Test Account') }
let!(:account2) { Account.create!(name: 'Test Account2') }
let!(:unscoped1) { UnscopedModelWithAccount.create!(name: 'Model 1', account: account1) }
let!(:unscoped2) { UnscopedModelWithAccount.create!(name: 'Model 2', account: account2) }

it 'updates records without tenant condition' do
expect do
MultiTenant.with(account1) do
UnscopedModelWithAccount.update_all(name: 'Updated Name')
end
end.to change { unscoped1.reload.name }.from('Model 1').to('Updated Name')
.and change { unscoped2.reload.name }.from('Model 2').to('Updated Name')

update_query = @queries.find { |q| q.include?('UPDATE "unscoped_model_with_accounts"') }
expect(update_query).to be_present
expect(update_query).not_to include('account_id')
end

it 'deletes records without tenant condition' do
expect do
MultiTenant.with(account1) do
UnscopedModelWithAccount.delete_all
end
end.to change { UnscopedModelWithAccount.count }.from(2).to(0)

delete_query = @queries.find { |q| q.include?('DELETE FROM "unscoped_model_with_accounts"') }
expect(delete_query).to be_present
expect(delete_query).not_to include('account_id')
end
end

context 'when update without arel' do
it 'can call method' do
expect do
Expand Down
9 changes: 9 additions & 0 deletions spec/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@
t.column :name, :string
end

create_table :unscoped_model_with_accounts, force: true do |t|
t.references :account, :integer
t.column :name, :string
end

create_table :aliased_tasks, force: true, partition_key: :account_id do |t|
t.column :account_id, :integer
t.column :name, :string
Expand Down Expand Up @@ -208,6 +213,10 @@ class UnscopedModel < ActiveRecord::Base
validates_uniqueness_of :name
end

class UnscopedModelWithAccount < ActiveRecord::Base
belongs_to :account
end

class AliasedTask < ActiveRecord::Base
multi_tenant :account
belongs_to :project_alias, class_name: 'Project'
Expand Down