Avoid N+1 queries when loading notes for indexing in Elasticsearch
What does this MR do?
This MR does 2 things.
noteable.assignees
before indexing notes
(1) Preloading The main thing this is doing is to reduce N+1 queries in our Elasticsearch indexing process for notes. The code handles a batch of documents of different types and then calls #as_indexed_json
on the batch of documents one at a time to convert them into the payload for indexing in Elasticsearch. Since #as_indexed_json
will load related objects we need to ensure that we have preloaded those relations ahead of time. This code introduces a generic method .preload_indexing_data
on the different indexed document types to preload the data using includes
ActiveRecord method. We only implement this for the Note
type in this MR and will plan to do a similar thing for other document types in future.
noteable.assignees
when noteable
is a Commit
(2) Fix a bug with trying to preload Without this we receive an exception
undefined method preloaded_records for nil:NilClass
when trying to call
Note.includes(noteable: {assignees: []})
if any of the notes are on a Commit
. The reason this seems to happen
is due to a refactoring in Rails preloaders in
https://github.com/rails/rails/commit/2847653869ffc1ff5139c46e520c72e26618c199#diff-6d659f140c909c5a70850ec69eab0014834ffb371029ccf7554b6a421a1fb0ca
which expects the #run
method to return the preloader (you can see
other examples updated to return self
).
It is possible longer term that this may be made redundant
by some much larger refactoring happening to this code in
https://github.com/rails/rails/pull/41385 since it appears this no
longer relies on the return value of #run
. But that is not released
yet and we may end up with more problems to solve when we upgrade to
that.
Screenshots (strongly suggested)
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
- [-] Documentation (if required)
-
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Related to #324745 (closed)