Preload system_note_metadata in the GraphQL notes resolver

Marc Shawrequested to merge
629326-mr-notes-graphql-n-plus-one into master

What does this MR do and why?

Preloads system_note_metadata in the GraphQL notes resolver. NotePolicy checks system_note_visible_for? on every note it authorizes, which reads that association, so each system note on a page cost one extra query. It is now one batched query per page. Adds a request spec that fails without the preload.

References

Screenshots or screen recordings

Not applicable, backend-only change.

How to set up and validate locally

  1. Check out the branch.
  2. Run bundle exec rspec spec/requests/api/graphql/project/merge_request/diff_notes_spec.rb.
  3. Optionally revert the change in app/graphql/resolvers/noteable/notes_resolver.rb and rerun to see the new spec fail with one extra system_note_metadata query per system note.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist.

Detailed context for AI agents

Change

Resolvers::Noteable::NotesResolver#unconditional_includes in app/graphql/resolvers/noteable/notes_resolver.rb now preloads :system_note_metadata alongside the existing [:author, :project, :note_metadata]. One-line change.

Root cause of the N+1

NotePolicy has condition(:is_visible) { @subject.system_note_visible_for?(@user) }, evaluated for read_note type authorization on every note the resolver returns. Note#system_note_visible_for? delegates to Note#system_note_viewable_by?, which reads the has_one :system_note_metadata association (app/models/note.rb). The resolver never preloaded it, so each system note triggered SELECT system_note_metadata ... WHERE note_id = ? LIMIT 1. The systemNoteMetadata and systemNoteIconName fields hit the same association when selected.

Why unconditional rather than lookahead-conditional

The policy check runs regardless of which fields the client selects, so a lookahead-conditional preload keyed on systemNoteMetadata / systemNoteIconName would not remove the per-note query.

Query cost impact

Adds one WHERE note_id IN (...) preload per notes page and removes one query per system note. For a page of 100 activity notes that is roughly 100 queries down to 1.

New spec

spec/requests/api/graphql/project/merge_request/diff_notes_spec.rb, context "system notes on a merge request", example "avoids N+1 queries when resolving system notes". It mirrors the external client's field selection:

notes(filter: ONLY_ACTIVITY) {
  nodes {
    id createdAt updatedAt body internal system resolvable resolved resolvedAt
    author { id username name }
    resolvedBy { id username name }
  }
}

It records with ActiveRecord::QueryRecorder.new(skip_cached: false), takes a control run, adds 3 system notes with system_note_metadata, and asserts issue_same_number_of_queries_as(control). Before the fix it failed with "Expected 59 queries, but got 62"; the 3 extra queries were system_note_metadata lookups, one per added note.

Where the operation comes from

No .graphql file in gitlab-org/gitlab defines getProjectsMergeRequestNotes. It is the third-party exporter cluttrdev/gitlab-exporter. Query shape:

projects(ids:) {
  nodes {
    mergeRequests(updatedAfter:, updatedBefore:) {
      nodes {
        notes(filter: ONLY_ACTIVITY) { nodes { ... } }
      }
    }
  }
}

No first arguments, so default page sizes apply: 100 merge requests and 100 notes per page.

Explicitly out of scope

This MR addresses only the per-note N+1 within a single notes connection. The larger cost in the exporter operation is per merge request, not per note. A local throwaway spec mirroring the exporter's exact query measured roughly 23 queries per merge request node, because the notes connection is resolved independently for each MR:

  • NotesFinder, including MergeRequest#related_notes, which loads merge_request_diff and runs a commit_shas query per MR
  • Preloaders::Projects::NotesPreloader (namespace, author status, max member access preloads)
  • Notes::RenderService Banzai rendering: the merge request reference parser loads merge_requests, projects, namespaces, routes and project_features per MR, even though this client never selects bodyHtml
  • the GraphQL authz boundary preloader

Cross-MR batching of nested connections is not supported by the current resolver design and needs a separate design decision. A smaller follow-up candidate: make the Notes::RenderService call in NotesPreloader conditional on bodyHtml / bodyFirstLineHtml being selected.

Other field costs observed but not addressed

Not used by this client, not touched here:

  • lastEditedBy triggers one users lookup per note via Note#edited? -> updated_by
  • discussion triggers find_discussion per discussion note

Verification

  • spec/requests/api/graphql/project/merge_request/diff_notes_spec.rb: 17 examples, 0 failures
  • spec/requests/api/graphql/project/issue/notes_spec.rb and ee/spec/requests/api/graphql/group/epic/notes_spec.rb: 16 examples, 0 failures
  • rubocop clean on both changed files

Blast radius

The resolver is shared: Types::Notes::NoteableInterface#notes (merge requests, issues, snippets, designs, epics) and Types::WorkItems::Widgets::NotesType. The preload applies to all of them.

No database schema change, no feature flag, no user-facing behaviour change. Commit carries Changelog: performance.

Merge request reports