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(proj/get_segment): fix n+1 queries for rules conditions #445

Merged
merged 2 commits into from
Nov 1, 2021

Conversation

gagantrivedi
Copy link
Member

Remove two level deep rules prefetching since we don't support that
from the UI.
Add code to prefetch conditions of the child rule as well

Remove two level deep rules prefetching since we don't support that
from the UI.
Add code to prefetch conditions of the child rule as well
Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gagantrivedi I made a small tweak to the test and determined that your solution still wasn't quite right. I've added the required arguments back in and the tests are now passing.

Interestingly, if we remove the "rules__rules__conditions" argument we end up with 106 queries based on this (hyper unrealistic) test.

@matthewelwell matthewelwell removed their assignment Oct 26, 2021
@gagantrivedi
Copy link
Member Author

gagantrivedi commented Oct 26, 2021

@gagantrivedi I made a small tweak to the test and determined that your solution still wasn't quite right. I've added the required arguments back in and the tests are now passing.

Interestingly, if we remove the "rules__rules__conditions" argument we end up with 106 queries based on this (hyper unrealistic) test.

It actually should work since to fetch rules__conditions you need rules ? also I don't think we need "rules__rules__rules"

@gagantrivedi gagantrivedi merged commit c9f7310 into main Nov 1, 2021
@gagantrivedi gagantrivedi deleted the fix/github-420/n+1 branch November 1, 2021 15:45
@dabeeeenster dabeeeenster linked an issue Nov 2, 2021 that may be closed by this pull request
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.

n+1 ORM issue for environments with large number of segments
2 participants