Skip to content

Remove an N+1 call rendering projects search results and fix false positive tests

Dylan Griffith requested to merge 34457-remove-n-plus-1-search into master

What does this MR do?

Initially the intent of this MR was to remove an N+1 call rendering projects search results (additional context from #34457 (closed))

But when reviewing @lulalala noticed that the assertion in the test was doing nothing. As such this led to the following:

  1. The test was not running sidekiq and thus nothing was indexed and thus no results were being found in search and thus the N+1 query wasn't asserting anything meaningful
  2. Adding sidekiq_inlint globally in these tests meant I could remove a bunch of specific mentions of sidekiq_might_not_need_inline
  3. A new assertion was added to prevent regressions so that we always verify we see results
  4. This new assertion showed other tests finding no results for different reasons
  5. The milestones search didn't match anything due to milestones not being named as expected so we just search * instead
  6. The merge requests search didn't match anything due to an existing bug now described at #103458 (closed) but I didn't want to fix this now so I updated the test to keep the merge requests on the same project
  7. Adding a collection of merge requests to the same project using factories forced me expand the merge requests factory to avoid uniqueness constraint on source_branch
  8. Debugging what the heck is the difference between refresh_index and the sidekiq led me to realise the refresh_index is an Elasticsearch specific thing that has to do with ensuring that all the previous writes are refreshed and can be served up in subsequent searches.

Screenshots

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

#34457 (closed)

Edited by 🤖 GitLab Bot 🤖

Merge request reports

Loading