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

Avoid re-fetching associations that have already been preloaded #901

Merged
merged 10 commits into from
Oct 24, 2022

Conversation

matthewmcgarvey
Copy link
Member

@matthewmcgarvey matthewmcgarvey commented Oct 21, 2022

This updates the class-level preload functions to check if the association has already been loaded before redoing the process. Now you don't have to worry about making unnecessary SQL queries when you don't have to.

TODO Support has many through passing multiple records.

Honestly, has many through has been the most difficult part of the codebase by far and this is no different. The problem is when the intermediary association is loaded but not the one in the has many through. Since we pass that we want the has many through association preloaded in the query that loads the intermediary, if it is loaded that call to preload is completely ignored.

The fix is to rethink how we make that query. Instead of loading the intermediaries, just set up fancy joins to only query for the has many through association. But how do we do that? 🤷

Also, this preload stuff is very broken for a has many through with more than one intermediary (a through list with more than 2 items)

I figure that this is good enough to include without that handling for the has many through. This stuff should be mostly invisible to end-users, anyways.

user = UserQuery.find(123)
user = UserQuery.preload_subscription(user) # <--- Query happens here to load the subscription
user = UserQuery.preload_subscription(user) # <--- User already has subscription loaded, nothing happens
user = UserQuery.preload_subscription(user, force: true) # <--- Force query to refetch association
user = UserQuery.preload_subscription(user, &.renewal("monthly") # <--- The actual query we pass in doesn't matter, we still don't fetch the subscription again

@matthewmcgarvey matthewmcgarvey changed the title Avoid re-fetched associations that have already been preloaded Avoid re-fetching associations that have already been preloaded Oct 21, 2022
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

This was a bit tough to follow, but I didn't see anything glaring. This isn't a breaking change right?

@matthewmcgarvey
Copy link
Member Author

@jwoertink no, it should not be a breaking change. I added the force param but it has a default value and I maintained ordering of the overloads that take in an array

@matthewmcgarvey
Copy link
Member Author

I'll look over the code one more time and merge later today/tomorrow

@matthewmcgarvey matthewmcgarvey merged commit 285a902 into luckyframework:main Oct 24, 2022
@matthewmcgarvey matthewmcgarvey deleted the preload-reload branch October 24, 2022 22:12
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.

None yet

2 participants