Model.limit(n).delete_all & Model.limit(n).update_all generates incorrect query

[mintuhouse]

Sharing the queries so it is clear what is happening

MultiTenant.with(Account.find(1)) { Project.limit(1).delete_all } generates a SQL query

DELETE FROM "projects" WHERE "projects"."id" IN (SELECT "projects"."id" FROM "projects" WHERE "projects"."account_id" = 1 AND "projects"."account_id" = 1 LIMIT 1)

As you can see subquery has account_id condition added correctly but once it returns ids, top level query doesnt have account_id condition.

Ideally it should have generated

DELETE FROM "projects" WHERE "projects"."id" IN (SELECT "projects"."id" FROM "projects" WHERE "projects"."account_id" = 1 LIMIT 1) AND "projects"."account_id" = 1

Environment
Rails 6.1
Ruby 3.1
Citus 10.2

[gurkanindibay]

@mintuhouse checking now

[gurkanindibay]

@mintuhouse I agree with including account_id in the outer query. However, removing account_id causes no problems because project_id is unique in this situation. Is there any functional impact on your application as a result of this?

[mintuhouse]

project_id need not be unique right? (I remember seeing tests to check for cases where same project_id in different accounts) (account_id, project_id) is the primary key Yes, it caused a functional issue

On Mon, 29 May 2023 at 1:20 PM, Gürkan İndibay ***@***.***> wrote: @mintuhouse <https://github.com/mintuhouse> I agree that it is better to add account_id in the outer query however, not adding account_id does not cause a problem since project_id is unique here. Does this cause a functional problem in your application? — Reply to this email directly, view it on GitHub <#195 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAF37HXOM5OGP2SJOOQI2LLXIRIMDANCNFSM6AAAAAAYKDO76M> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- *--Regards,* *Hasan Kumar* *E-mail: ***@***.*** ***@***.***> | **Ph: +91 7738776725 | Github <https://github.com/mintuhouse> | Twitter <http://twitter.com/mintuhouse> **| LinkedIn <http://in.linkedin.com/in/hasankumar>*

[gurkanindibay]

@mintuhouse I tried to implement but Arel::DeleteManager did not allow me to add tenant parameter into delete statement.
Here is the draft PR. Feel free to continue working and adding logic to find a solution
#200

[Amit909Singh]

@mintuhouse I tried to implement but Arel::DeleteManager did not allow me to add tenant parameter into delete statement. Here is the draft PR. Feel free to continue working and adding logic to find a solution #200

@gurkanindibay I am trying to fix the tenant scoping issue on your draft PR. Please review: #219

[Amit909Singh]

@gurkanindibay Please review PR