Skip to content

Avoid the use of Elasticsearch joins when searching for issues

Dylan Griffith requested to merge 273262-dont-use-joins-in-issue-global-search-new into master

What does this MR do?

This MR is part of a series of MRs which will allow us to move issues into a separate index in Elasticsearch &4697 (closed). In the past all of the documents were part of a single Elasticsearch index which allowed us to use joins when performing searches. This was relevant for permissions checks when searching for issues as you need to check the issues_access_level and visibility_level of the project to know if an issue is visible to a user. Some time ago we decided to change our architecture as using joins this way is quite flawed since it requires all documents to be in a single monolithic index which has several performance costs and Elasticsearch joins are generally discouraged and quite limited and there is usually a preference for de-normalized data for better performance &2054 .

Now that project permissions have been denormalized into the issue document (see !48436 (merged) and !47007 (merged) !46916 (merged)) we don't need to join to the parent project anymore to find the relevant issues.

Before performing each search we need to check first to confirm the migration (added in !47819 (merged)) has finished before we can use this no join search as the Elasticsearch migrations happen in the background and we have no guarantee when or even if they will finish on any given instance.

On it's own this MR is only a step towards moving the issues to a new index. That still needs to happen in #273264 (closed) but this MR should still offer some small performance improvements as the global searches are no longer using joins for issue searches.

This MR also includes 2 other related changes to specs

  1. Specs should assume the index is fully migrated (ie. mark all migrations as completed) from the start as this is how the application behaves for a newly created index. This was a change to spec setup for :elastic tagged specs but also meant a few changes to an existing spec that had the opposite assumption
  2. Add a new spec to validate visibility_level is updated on issues in Elasticsearch. This behaviour was added in !48436 (merged) but per !48436 (comment 455347997) we weren't able to integration test this until the searches actually used this new visibility_level field. As such we can add this integration test now with !48583 (merged)

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Test matrix

  • Public
    • Everyone with access
      • Admin: Yes
      • Member: Yes
      • Non member: Yes
      • Not logged in: Yes
    • Members only
      • Admin: Yes
      • Member: Yes
      • Non member: No
      • Not logged in: No
    • Disabled
      • Admin: No
      • Member: No
      • Non member: No
      • Not logged in: No
  • Private
    • Members only
      • Admin: Yes
      • Member: Yes
      • Non member: No
      • Not logged in: No
    • Disabled
      • Admin: No
      • Member: No
      • Non member: No
      • Not logged in: No
  • Internal
    • Everyone with access
      • Admin: Yes
      • Member: Yes
      • Non member: Yes
      • Not logged in: No
    • Members only
      • Admin: Yes
      • Member: Yes
      • Non member: No
      • Not logged in: No
    • Disabled
      • Admin: No
      • Member: No
      • Non member: No
      • Not logged in: No

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 #273262 (closed)

Edited by Dylan Griffith

Merge request reports