Follow-up from "Preload Group plans in EpicsFinder"

In !5877 (merged), I wrote:

I think there are basically two things feeding into how I feel about this.

The first is that in this specific case, caching the plans would actually save us a query, because we generated all_plan_ids as a set, rather than as a subquery. You're right that this would complicate things elsewhere (and perhaps not save us any queries, but I'll get to that), so we don't need to do that here, but I'm curious about what you think in general.

The second problem I see is that we're hiding our use of the recursive query as a subquery in a bunch of places. This is good - we want to construct the best queries we can - but it seems that https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/helpers/groups_helper.rb#L53 means that every group page load will run a query like this on its own just to build the breadcrumbs.

Therefore, we might actually see some performance improvements by doing something like this:

class Namespace
  attr_writer :self_and_ancestors, :ancestors

  def self_and_ancestors
    return @self_and_ancestors if defined?(@self_and_ancestors)

    # current implementation
  end

  def ancestors
    return @ancestors if defined?(@ancestors)

    # current implementation
  end
end

class Groups::ApplicationController
  def group
    @group ||= find_routable!(Group, params[:group_id] || params[:id]).tap do |group|
      group.self_and_ancestors = group.self_and_ancestors.to_a
      group.ancestors = group.self_and_ancestors - [group]
    end
  end
end

This would mean that we wouldn't need any extra queries to get the plans for the current namespace in the case where we're showing the breadcrumbs. We couldn't use that here, because we're doing the calculation for multiple groups, but in the case we have elsewhere, it might be worthwhile.

However, that might be controversial 🙂 It relies on quite a lot of current knowledge, that may not apply in the future. But I think it would prevent us needing any queries to find the current group's plans, in the common case. So I guess the question is, how much do we want to remove those queries?

@yorickpeterse replied:

@smcgivern Long term I feel that we should start passing in relations more, instead of having models query them themselves. In this case that means having to explicitly pass the plans to a Group, meaning you can control how to query those. Hanami does this (http://hanamirb.org/guides/1.2/associations/overview/ / http://hanamirb.org/guides/1.2/associations/has-many/), and from the outside it sounds like a really good idea.

Unfortunately, this is pretty different from how you usually do things in Rails, so I'm not sure exactly what this would look like. This is further complicated by a lot of methods being used in many different places (usually indirectly), making it hard to incrementally refactor things. Our policy system doesn't make things much easier either, where often such code is triggered 15 call frames deep.

tl;dr: I agree we need something better, but everything I can think of requires a lot more work (in the order of months), as any such refactor is likely to touch many different parts of the application.