Skip to content

Avoid N+1 queries when loading notes for indexing in Elasticsearch

Dylan Griffith requested to merge 324745-preload-notes-data-for-es-indexing into master

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

Availability and Testing

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)

Edited by Dylan Griffith

Merge request reports