Skip to content

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

Open
B6 (a5-stable) wants to merge 1 commit into
citusdata:masterfrom
a5-stable:prepend-multi-tenant-model-only
Open

Fix unintended side effects of delete_all and update_all with multi-tenancy for non-multi-tenant models#269
B6 (a5-stable) wants to merge 1 commit into
citusdata:masterfrom
a5-stable:prepend-multi-tenant-model-only

Conversation

@a5-stable

@a5-stable B6 (a5-stable) commented Feb 14, 2025

Copy link
Copy Markdown

Motivation / Background

The Arel::ActiveRecordRelationExtension module is currently prepended directly to ActiveRecord::Relation, which can lead to unintended side effects for models that do not support multi-tenancy.

e.g.

class PageView < ActiveRecord::Base
  multi_tenant :customer
  ...
end

class AnotherPageView < ActiveRecord::Base
  ...
end

# OK - works as expected
MultiTenant.with(customer) do
   PageView.all.delete_all
end
#=> DELETE FROM "page_views" WHERE "page_views"."id" IN (SELECT "page_views"."id" FROM "page_views" WHERE "page_views"."customer_id" = '1') AND "page_views"."customer_id" = '1'

But when running the same block for another model that doesn't support multi-tenancy:

MultiTenant.with(customer) do
  AnotherPageView.all.delete_all
end

# Current Query
#=> DELETE FROM "another_page_views" WHERE "another_page_views"."id" IN (SELECT "another_page_views"."id" FROM "another_page_views" WHERE "another_page_views"."customer_id" = '1') AND "another_page_views"."customer_id" = '1'

# Ideal Query; The query should not add an additional customer_id condition since AnotherPageView does not support multi-tenancy.
#=> DELETE FROM "another_page_views" WHERE "another_page_views"."id"

This happens because the module is being prepended to all relations, even those that shouldn't be influenced by multi-tenancy.

Solution

To fix this, we modified the code to call the original delete_all method when the model does not support multi-tenancy.

@a5-stable

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@a5-stable
B6 (a5-stable) force-pushed the prepend-multi-tenant-model-only branch 2 times, most recently from 5c1cfff to 29a8d96 Compare February 18, 2025 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant