Skip to content

Comments

Refactor tenant enforcement to use Arel visitor pattern#283

Open
a5-stable wants to merge 1 commit intocitusdata:masterfrom
a5-stable:subquery/leave-query-to-rails
Open

Refactor tenant enforcement to use Arel visitor pattern#283
a5-stable wants to merge 1 commit intocitusdata:masterfrom
a5-stable:subquery/leave-query-to-rails

Conversation

@a5-stable
Copy link

Background

#223 attempted to fix an issue where delete_all and update_all with LIMIT would generate subqueries missing the tenant condition in the outer query.

However, this approach has some problems:

Cannot handle edge cases

The implementation cannot properly handle certain scenarios, as reported in #269, #282

Generates unnecessary subqueries

Subqueries are added even when they are not needed:

# e.g.)
Project.update_all(name: 'new')

# Expected
UPDATE "projects" SET "name" = 'new'
WHERE "projects"."account_id" = 1

# Actual
UPDATE "projects" SET "name" = 'new'
WHERE "projects"."id" IN (
  SELECT "projects"."id" FROM "projects"
  WHERE "projects"."account_id" = 1
) AND "projects"."account_id" = 1

What I did here

Instead of overriding update_all or delete_all methods, this PR patches prepare_update_statement in Arel::Visitors::ToSql.

This method decides whether a subquery is needed. By hooking here, we can add tenant conditions only when Rails actually creates a subquery, and let Rails handle all the subquery logic itself.

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