Avoid N+1 queries when loading notes for indexing in Elasticsearch
What does this MR do?
This MR does 2 things.
(1) Preloading noteable.assignees before indexing notes
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.
(2) Fix a bug with trying to preload noteable.assignees when noteable is a Commit
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)