Skip to content

Use use_includes: false where necessary

Manoj M J requested to merge mmj-tidy-up into master

What does this MR do and why?

where_full_path_in method has the following definition now:

   def where_full_path_in(paths, use_includes: true)
      return none if paths.empty?

      wheres = paths.map do |path|
        "(LOWER(routes.path) = LOWER(#{connection.quote(path)}))"
      end

      route =
        if use_includes
          includes(:route).references(:routes)
        else
          joins(:route)
        end

      route
        .where(wheres.join(' OR '))
        .allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/420046")
    end

use_includes: true is the default, and when used, the routes records are included to prevent N+1.

However, there are different areas in our codebase where routes are loaded, but that loading isn't necessary because the result of this method is just used to pluck IDs of projects or groups.

Right now, this isn't much of a problem because includes happens via LEFT OUTER JOIN, and we need to perform this JOIN anyway so as to filter out projects/groups based on the paths we supply to this method.

However, the definition of this method is going to be changed in the upcoming MR: !137886 (merged), where will be using preload instead of includes to prevent N+1.

And, when we use preload, it will fire an entirely separate query to load routes. So the question is, why make the app fire an extra query to preload routes, in areas where we are certain that preloading routes are not needed at all?

This MR makes that change in the areas where we are certain that including routes in the result isn't necessary. These calls are now made with use_includes: false, so that routes aren't included. And when we move to the new definition in !137886 (merged), an extra query won't be un-necessarily fired.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Manoj M J

Merge request reports