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

Include test cases for joins #168

Merged

Conversation

jainharsheet77
Copy link

No description provided.

@serprex
Copy link
Collaborator

serprex commented Nov 17, 2022

Could you provide context on what these tests are covering which was previously not covered?

@jainharsheet77
Copy link
Author

jainharsheet77 commented Nov 17, 2022

[Appears to be a BUG] I encountered an issue wherein tenant clause does not get appended to the query generated for joins. It happens in case of query being run inside MultiTenant.wihout and for models which have associations with an extra where condition. Happening in Rails 6.1

MultiTenant.without do
  Comment.joins(:task)
end

This should produce query with tenant scoped in it.

Produces:-
SELECT "comments".* FROM "comments" INNER JOIN "tasks" ON "tasks"."id" = "comments"."commentable_id" AND "comments"."commentable_type" = 'Task'

Should produce:-
SELECT "comments".* FROM "comments" INNER JOIN "tasks" ON "tasks"."id" = "comments"."commentable_id" AND "comments"."commentable_type" = 'Task' AND "comments"."account_id" = "tasks"."account_id"

Whereas this tenant clause is present in case of other models.
Screenshot 2022-11-17 at 12 49 24 PM

@serprex
Copy link
Collaborator

serprex commented Nov 17, 2022

I'm going to need some help understanding this

This is without:

def self.without(&block)
return block.call if self.current_tenant.nil?
old_tenant = self.current_tenant
begin
self.current_tenant = nil
return block.call
ensure
self.current_tenant = old_tenant
end
end

Are these non-multitenant associations which are failing to generate a condition? If this is an existing bug, why are the tests in this PR passing?

@jainharsheet77
Copy link
Author

jainharsheet77 commented Nov 17, 2022

These are multi_tenant associations present in gem's schema.rb here

In case of the schema provided in the gem. For Comment model when joined with Task. The query should have
"comments"."account_id" = "tasks"."account_id" condition which it does not have.

Are the tests passing for the PR in case of rails_6_1 too. Cause it is failing in the docker setup in my local.
Screenshot 2022-11-17 at 9 29 00 PM

@serprex
Copy link
Collaborator

serprex commented Nov 17, 2022

I'm confused because GitHub Actions doesn't seem to be running CI on this PR for some reason

@jainharsheet77 jainharsheet77 force-pushed the scoping_issue_when_tenant_is_not_set branch from f4e38d7 to dfeeb08 Compare November 18, 2022 05:17
@jainharsheet77 jainharsheet77 force-pushed the scoping_issue_when_tenant_is_not_set branch from dfeeb08 to 202a614 Compare November 18, 2022 05:34
@jainharsheet77
Copy link
Author

Tests are failing for rails_6_1 and above

@serprex
Copy link
Collaborator

serprex commented Nov 18, 2022

In CI it's only failing for 5.2, & in that case the SQL looks right only it's relying on join condition

(realizing now that this may be because your latest commit fixes the issue)

@jainharsheet77
Copy link
Author

In case of 5.2 the test is failing because when tenant is set in scope, the query generated has both tenant scoping condition
table_a.tenant_id = <val> AND table_b.tenant_id = <val> AND table_a.tenant_id = table_b.tenant_id. But in other versions of active_records, the condition is table_a.tenant = <val> AND table_b.tenant = <val> and table_a.tenant_id = table_b.tenant_id is absent.

Should push a fix soon.

@mintuhouse
Copy link
Collaborator

mintuhouse commented Nov 20, 2022

@serprex Issue happens only on older versions of rails 5.2 i.e., 5.2.3 (It is green on latest 5.2.8)
Can drop the 5.2.3 support and say we support latest patch of minor version 5.2 like we do for 6.0, 6.1, 7.0?

@serprex
Copy link
Collaborator

serprex commented Nov 20, 2022

I'm fine with removing 5.2.3 from CI

harsheetjain added 2 commits November 20, 2022 17:31
Description rails is further adding a children node
@jainharsheet77 jainharsheet77 force-pushed the scoping_issue_when_tenant_is_not_set branch from 2b44c90 to 8104621 Compare November 20, 2022 12:01
@mintuhouse mintuhouse requested a review from serprex November 20, 2022 21:11
@serprex serprex merged commit 66a6df2 into citusdata:master Nov 20, 2022
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.

3 participants