From 20d09a83db160e40db8e526becf66f59ad65f9a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrkan=20=C4=B0ndibay?= Date: Mon, 4 Dec 2023 08:43:36 +0300 Subject: [PATCH] Fixes Model.limit(n).delete_all & Model.limit(n).update_all generates incorrect query (#200) * Adds initial implementation but still failing * Users/amit909sin/issue 195 (#219) * Fix the tenant scoping for delete_all --------- Co-authored-by: amit909singh --- .../delete_operations.rb | 38 +++++++++++++++++++ lib/activerecord-multi-tenant/multi_tenant.rb | 2 +- lib/activerecord_multi_tenant.rb | 1 + .../query_rewriter_spec.rb | 27 +++++++++++++ 4 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 lib/activerecord-multi-tenant/delete_operations.rb diff --git a/lib/activerecord-multi-tenant/delete_operations.rb b/lib/activerecord-multi-tenant/delete_operations.rb new file mode 100644 index 00000000..b2bfbf3d --- /dev/null +++ b/lib/activerecord-multi-tenant/delete_operations.rb @@ -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) } + arel = arel.where(tenant_condition) + 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) diff --git a/lib/activerecord-multi-tenant/multi_tenant.rb b/lib/activerecord-multi-tenant/multi_tenant.rb index 91abba8d..e03008dc 100644 --- a/lib/activerecord-multi-tenant/multi_tenant.rb +++ b/lib/activerecord-multi-tenant/multi_tenant.rb @@ -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 diff --git a/lib/activerecord_multi_tenant.rb b/lib/activerecord_multi_tenant.rb index 1b9b606d..09a29a0c 100644 --- a/lib/activerecord_multi_tenant.rb +++ b/lib/activerecord_multi_tenant.rb @@ -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' diff --git a/spec/activerecord-multi-tenant/query_rewriter_spec.rb b/spec/activerecord-multi-tenant/query_rewriter_spec.rb index 5bc280cb..2b6410e3 100644 --- a/spec/activerecord-multi-tenant/query_rewriter_spec.rb +++ b/spec/activerecord-multi-tenant/query_rewriter_spec.rb @@ -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