Skip to content
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

Fixes Model.limit(n).delete_all generates incorrect query #200

Merged
merged 2 commits into from
Dec 4, 2023
Merged
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
38 changes: 38 additions & 0 deletions lib/activerecord-multi-tenant/delete_operations.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# frozen_string_literal: true

module Arel
module ActiveRecordRelationExtension
# Overrides the delete_all method to include tenant scoping
def delete_all
# 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?

tenant_key = MultiTenant.partition_key(MultiTenant.current_tenant_class)
tenant_id = MultiTenant.current_tenant_id
arel = eager_loading? ? apply_join_dependency.arel : build_arel
arel.source.left = table

if tenant_id && klass.column_names.include?(tenant_key)
# Check if the tenant key is present in the model's column names
tenant_condition = table[tenant_key].eq(tenant_id)
# Add the tenant condition to the arel query if it is not already present
unless arel.constraints.any? { |node| node.to_sql.include?(tenant_condition.to_sql) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tenant_condition.to_sql can be hoisted out of any? loop

Copy link
Collaborator

@serprex serprex Dec 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Granted, that will cause an unnecessary to_sql call when arel.constraints.empty?

arel = arel.where(tenant_condition)

Check warning on line 20 in lib/activerecord-multi-tenant/delete_operations.rb

View check run for this annotation

Codecov / codecov/patch

lib/activerecord-multi-tenant/delete_operations.rb#L20

Added line #L20 was not covered by tests
end
end

subquery = arel.clone
subquery.projections.clear
subquery = subquery.project(table[primary_key])
in_condition = Arel::Nodes::In.new(table[primary_key], subquery.ast)
stmt = Arel::DeleteManager.new.from(table)
stmt.wheres = [in_condition]

# Execute the delete statement using the connection and return the result
klass.connection.delete(stmt, "#{klass} Delete All").tap { reset }
end
end
end

# Patch ActiveRecord::Relation with the extension module
ActiveRecord::Relation.prepend(Arel::ActiveRecordRelationExtension)
2 changes: 1 addition & 1 deletion lib/activerecord-multi-tenant/multi_tenant.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def self.tenant_klass_defined?(tenant_name, options = {})
end

def self.partition_key(tenant_name)
"#{tenant_name}_id"
"#{tenant_name.to_s.underscore}_id"
end

# rubocop:disable Style/ClassVars
Expand Down
1 change: 1 addition & 0 deletions lib/activerecord_multi_tenant.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@
require_relative 'activerecord-multi-tenant/table_node'
require_relative 'activerecord-multi-tenant/version'
require_relative 'activerecord-multi-tenant/habtm'
require_relative 'activerecord-multi-tenant/delete_operations'
27 changes: 27 additions & 0 deletions spec/activerecord-multi-tenant/query_rewriter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,39 @@
let!(:manager1) { Manager.create(name: 'Manager 1', project: project1, account: account) }
let!(:manager2) { Manager.create(name: 'Manager 2', project: project2, account: account) }

before(:each) do
@queries = []
ActiveSupport::Notifications.subscribe('sql.active_record') do |_name, _started, _finished, _unique_id, payload|
@queries << payload[:sql]
end
end

after(:each) do
ActiveSupport::Notifications.unsubscribe('sql.active_record')
end

it 'delete_all the records' do
expected_query = <<-SQL.strip
DELETE FROM "projects" WHERE "projects"."id" IN
(SELECT "projects"."id" FROM "projects"
INNER JOIN "managers" ON "managers"."project_id" = "projects"."id"
and "managers"."account_id" = :account_id
WHERE "projects"."account_id" = :account_id
)
AND "projects"."account_id" = :account_id
SQL

expect do
MultiTenant.with(account) do
Project.joins(:manager).delete_all
end
end.to change { Project.count }.from(3).to(1)

@queries.each do |actual_query|
next unless actual_query.include?('DELETE FROM ')

expect(format_sql(actual_query)).to eq(format_sql(expected_query.gsub(':account_id', account.id.to_s)))
end
end

it 'delete_all the records without a current tenant' do
Expand Down