<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/engineering/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Work on this issue](https://contributors.gitlab.com/manage-issue?action=work&projectId=278964&issueIid=635535) - [Close this issue](https://contributors.gitlab.com/manage-issue?action=close&projectId=278964&issueIid=635535) </details> <!--IssueSummary end--> ### 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. 1. Enable the performance bar and run this in GraphiQL (`/-/graphql-explorer`): ```graphql query { group(fullPath: "<group>") { labels(includeAncestorGroups: true, includeDescendantGroups: true) { nodes { id title } } } } ``` 1. 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 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/264415, 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 16456d8a707a. ### 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 <details> <summary>Measurement</summary> - 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`. </details> ### 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: ```diff --- 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. 1. Add an N+1 example to `spec/requests/api/graphql/group/labels_query_spec.rb` with `includeDescendantGroups: true` and labels on separate subgroups. 1. 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](https://handbook.gitlab.com/handbook/engineering/releases/patch-releases) to a version under [the maintenance policy](https://docs.gitlab.com/policy/maintenance/), please follow the steps on the [patch release runbook for GitLab engineers](https://gitlab.com/gitlab-org/release/docs/-/blob/master/general/patch/engineers.md). Refer to the [internal "Release Information" dashboard](https://dashboards.gitlab.net/d/delivery-release_info/delivery3a-release-information?orgId=1) 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](https://handbook.gitlab.com/handbook/engineering/releases/internal-releases/) for single-tenant SaaS instances, refer to the [internal release process for engineers](https://gitlab.com/gitlab-org/release/docs/-/blob/master/general/internal-releases/engineers.md?ref_type=heads).