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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

a5-stable
Copy link

@a5-stable a5-stable commented Feb 14, 2025

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
Author

@microsoft-github-policy-service agree

@a5-stable 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
@a5-stable a5-stable force-pushed the prepend-multi-tenant-model-only branch from 29a8d96 to dbf398a Compare February 27, 2025 12:08
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