Skip to content

Always preload if using proc with multifetch cache#44616

Merged
byroot merged 1 commit intorails:mainfrom
ghiculescu:preload-all-if-proc
Aug 9, 2022
Merged

Always preload if using proc with multifetch cache#44616
byroot merged 1 commit intorails:mainfrom
ghiculescu:preload-all-if-proc

Conversation

@ghiculescu
Copy link
Copy Markdown
Member

@ghiculescu ghiculescu commented Mar 4, 2022

#31250 optimised rendering a collection with cached: true, but also when using a proc: cached: proc {}. The problem is the proc may reference associations that should be preloaded to avoid n+1s in generating the cache key.

For example:

# controller
@posts = Post.preload(:author)

# view
render partial: "test/partial", collection: @post, cached: proc { |post| [post, post.author] }

Since #31250, this will perform n+1 queries on post.author. preload(:author) is never executed.

To fix this, this PR will always preload the collection if there's a proc given for cached. cached: true will still not preload unless it renders, as it did in #31250.

@rails-bot rails-bot bot added the actionview label Mar 4, 2022
@ghiculescu ghiculescu force-pushed the preload-all-if-proc branch from 05204db to 78bcb43 Compare March 4, 2022 17:56
@ghiculescu ghiculescu marked this pull request as draft May 7, 2022 09:07
@ghiculescu ghiculescu force-pushed the preload-all-if-proc branch from 78bcb43 to dc90451 Compare May 7, 2022 12:18
rails#31250 optimised rendering a collection with `cached: true`, but also with `cached: proc {}`. The problem is the proc may reference associations that should be preloaded to avoid n+1s in generating the cache key.

For example:

```ruby
@posts = Post.preload(:author)

@view.render partial: "test/partial", collection: @post, cached: proc { |post| [post, post.author] }
```

This will n+1 on `post.author`.

To fix this, this PR will always preload the collection if there's a proc given for `cached`. `cached: true` will still not preload unless it renders, as it did in rails#31250.
@ghiculescu ghiculescu force-pushed the preload-all-if-proc branch from dc90451 to 3f1f4c7 Compare May 7, 2022 12:19
@ghiculescu ghiculescu marked this pull request as ready for review May 7, 2022 12:19
@byroot byroot merged commit 8994f09 into rails:main Aug 9, 2022
@ghiculescu ghiculescu deleted the preload-all-if-proc branch August 9, 2022 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants