N+1 queries authorizing subgroups in GraphQL group labels with includeDescendantGroups

Summary

Group.labels with includeDescendantGroups: true runs 2 queries per subgroup that owns a returned label. The queries come from GroupPolicy conditions that read each label group's root ancestor. EE::Preloaders::LabelsPreloader preloads root ancestors for project labels' groups but not for group labels' groups.

The search_labels MCP tool (version 0.2.0) hits this on every group call, because it always sets includeDescendantGroups: true.

Steps to reproduce

  1. In a GDK with an Ultimate license, create a group with two subgroups, each with one group label.

  2. Enable the performance bar and run this in GraphiQL (/-/graphql-explorer):

    query {
  group(fullPath: "<group>") {
    labels(includeAncestorGroups: true, includeDescendantGroups: true) {
      nodes { id title }
    }
  }
}

  3. Add a third subgroup with a label and rerun the query. The pg count goes up by 2.

Also reproduced by the 'an MCP tool that avoids N+1 queries' example in ee/spec/services/ee/mcp/tools/labels/search_service_spec.rb, added in !264415 (merged), which carries threshold: 4 for two extra subgroups. FOSS isn't affected: the FOSS-only example in spec/services/mcp/tools/labels/search_service_spec.rb has no threshold.

Example Project

Not applicable. Reproduced locally with factory data on master at 16456d8a.

What is the current bug behavior?

Each additional subgroup with a label adds 2 queries, from ee/app/policies/ee/group_policy.rb:

Query Condition
find_namespaces_by_id (root ancestor) user_banned_from_namespace → @subject.root_ancestor
saml_providers (cached) needs_new_sso_session → SsoEnforcer.access_restricted? → EE::Group#root_saml_provider → root_ancestor.saml_provider

What is the expected correct behavior?

The query count stays the same as subgroups are added.

Relevant logs and/or screenshots

Measurement
  • Service-level spec calling Mcp::Tools::Labels::SearchService (0.2.0) with a group full_path. Each call runs with a fresh request store, Rails cache and BatchLoader executor, as GraphqlHelpers#run_with_clean_state does.
  • Warm-up call, then a control with one label on the group, then two more labels, each on its own subgroup.
  • 20 queries for the control, 24 after, counting cached queries. Each query was attributed with QUERY_RECORDER_DEBUG=1.

Output of checks

Not reproduced on GitLab.com specifically. The code path is the same there.

Results of GitLab environment info

Not applicable. Reproduced in specs on master.

Results of GitLab application Check

Not applicable.

Possible fixes

Run the existing root-ancestor preload in EE::Preloaders::LabelsPreloader#preload_all over group labels' groups too:

--- a/ee/app/models/ee/preloaders/labels_preloader.rb
+++ b/ee/app/models/ee/preloaders/labels_preloader.rb
@@ -20,14 +20,14 @@ def preload_all
         ).call

-        # preloading the root ancestors for the project labels for the authorizations checks
-        project_groups = project_labels.filter_map(&:project).filter_map(&:group)
+        # preloading the root ancestors for the authorizations checks
+        groups = group_labels.filter_map(&:group) + project_labels.filter_map(&:project).filter_map(&:group)

-        return if project_groups.empty?
+        return if groups.empty?

-        ::Namespaces::Preloaders::GroupRootAncestorPreloader.new(project_groups).execute
+        ::Namespaces::Preloaders::GroupRootAncestorPreloader.new(groups).execute

         ActiveRecord::Associations::Preloader.new(
-          records: project_groups.map(&:root_ancestor),
+          records: groups.map(&:root_ancestor),
           associations: [:saml_provider]
         ).call

With this change, the EE MCP example passes without a threshold:, and spec/models/preloaders/labels_preloader_spec.rb, spec/graphql/resolvers/group_labels_resolver_spec.rb, spec/requests/api/graphql/group/labels_query_spec.rb plus the MCP service spec still pass.

Implementation plan (weight 1):

  1. Apply the change above.
  2. Add an N+1 example to spec/requests/api/graphql/group/labels_query_spec.rb with includeDescendantGroups: true and labels on separate subgroups.
  3. Delete ee/spec/services/ee/mcp/tools/labels/search_service_spec.rb. In spec/services/mcp/tools/labels/search_service_spec.rb, unwrap the N+1 example from its context 'when CE edition' block.

Patch release information for backports

If the bug fix needs to be backported in a patch release to a version under the maintenance policy, please follow the steps on the patch release runbook for GitLab engineers.

Refer to the internal "Release Information" dashboard for information about the next patch release, including the targeted versions, expected release date, and current status.

High-severity bug remediation

To remediate high-severity issues requiring an internal release for single-tenant SaaS instances, refer to the internal release process for engineers.

Edited by 🤖 GitLab Bot 🤖