citusdata
`Model.limit(n).delete_all` & `Model.limit(n).update_all` generates incorrect query
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
@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?
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 https://github.com/citusdata/activerecord-multi-tenant/issues/195#issuecomment-1566708170, 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
@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
@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: https://github.com/citusdata/activerecord-multi-tenant/pull/219
