Merge branch 'sy-fix-assignee-n-plus-one' into 'master'

Improve performance of querying alert assignees

See merge request gitlab-org/gitlab!33656

app/assets/javascripts/alert_management/components/alert_management_list.vue

@@ -282,7 +282,9 @@ export default {
     },
     getAssignees(assignees) {
       // TODO: Update to show list of assignee(s) after https://gitlab.com/gitlab-org/gitlab/-/issues/218405
-      return assignees?.length > 0 ? assignees[0]?.username : s__('AlertManagement|Unassigned');
+      return assignees.nodes?.length > 0
+        ? assignees.nodes[0]?.username
+        : s__('AlertManagement|Unassigned');
     },
     handlePageChange(page) {
       const { startCursor, endCursor } = this.alerts.pageInfo;

app/assets/javascripts/alert_management/graphql/fragments/list_item.fragment.graphql

@@ -6,8 +6,10 @@ fragment AlertListItem on AlertManagementAlert {
   startedAt
   endedAt
   eventCount
-  issueIid,
+  issueIid
   assignees {
-    username
-  },
+    nodes {
+      username
+    }
+  }
 }

app/graphql/resolvers/alert_management/alerts/assignees_resolver.rb

+# frozen_string_literal: true
+
+module Resolvers
+  module AlertManagement
+    module Alerts
+      class AssigneesResolver < BaseResolver
+        type Types::UserType, null: true
+
+        # With lazy-loaded assignees, authorizations should be avoided
+        # in earlier phases to take advantage of batching. See
+        # AssigneeLoader for authorization steps.
+        # https://gitlab.com/gitlab-org/gitlab/-/issues/217767
+        def self.skip_authorizations?
+          true
+        end
+
+        def resolve(**args)
+          return [] unless Feature.enabled?(:alert_assignee)
+
+          ::Gitlab::Graphql::Loaders::AlertManagement::Alerts::AssigneesLoader
+            .new(object.id, user_authorization_filter)
+            .find
+        end
+
+        private
+
+        def user_authorization_filter
+          proc do |users|
+            users.select { |user| Ability.allowed?(context[:current_user], :read_user, user) }
+          end
+        end
+      end
+    end
+  end
+end

app/graphql/types/alert_management/alert_type.rb

@@ -85,15 +85,10 @@ module Types
             description: 'Timestamp the alert was last updated'
 
       field :assignees,
-            [Types::UserType],
+            Types::UserType.connection_type,
             null: true,
-            description: 'Assignees of the alert'
-
-      def assignees
-        return User.none unless Feature.enabled?(:alert_assignee, object.project)
-
-        object.assignees
-      end
+            description: 'Assignees of the alert',
+            resolver: ::Resolvers::AlertManagement::Alerts::AssigneesResolver
     end
   end
 end

app/models/alert_management/alert_assignee.rb

@@ -7,5 +7,7 @@ module AlertManagement
 
     validates :alert, presence: true
     validates :assignee, presence: true, uniqueness: { scope: :alert_id }
+
+    scope :for_alert_ids, -> (ids) { includes(:assignee).where(alert_id: ids) }
   end
 end

doc/api/graphql/reference/gitlab_schema.graphql

@@ -172,7 +172,27 @@ type AlertManagementAlert {
   """
   Assignees of the alert
   """
-  assignees: [User!]
+  assignees(
+    """
+    Returns the elements in the list that come after the specified cursor.
+    """
+    after: String
+
+    """
+    Returns the elements in the list that come before the specified cursor.
+    """
+    before: String
+
+    """
+    Returns the first _n_ elements from the list.
+    """
+    first: Int
+
+    """
+    Returns the last _n_ elements from the list.
+    """
+    last: Int
+  ): UserConnection
 
   """
   Timestamp the alert was created

doc/api/graphql/reference/gitlab_schema.json

@@ -486,20 +486,51 @@
               "name": "assignees",
               "description": "Assignees of the alert",
               "args": [
-
-              ],
-              "type": {
-                "kind": "LIST",
-                "name": null,
-                "ofType": {
-                  "kind": "NON_NULL",
-                  "name": null,
-                  "ofType": {
-                    "kind": "OBJECT",
-                    "name": "User",
+                {
+                  "name": "after",
+                  "description": "Returns the elements in the list that come after the specified cursor.",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "String",
                     "ofType": null
-                  }
+                  },
+                  "defaultValue": null
+                },
+                {
+                  "name": "before",
+                  "description": "Returns the elements in the list that come before the specified cursor.",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "String",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                },
+                {
+                  "name": "first",
+                  "description": "Returns the first _n_ elements from the list.",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "Int",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                },
+                {
+                  "name": "last",
+                  "description": "Returns the last _n_ elements from the list.",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "Int",
+                    "ofType": null
+                  },
+                  "defaultValue": null
                 }
+              ],
+              "type": {
+                "kind": "OBJECT",
+                "name": "UserConnection",
+                "ofType": null
               },
               "isDeprecated": false,
               "deprecationReason": null

doc/api/graphql/reference/index.md

@@ -61,7 +61,6 @@ Describes an alert from the project's Alert Management
 
 | Name  | Type  | Description |
 | ---   |  ---- | ----------  |
-| `assignees` | User! => Array | Assignees of the alert |
 | `createdAt` | Time | Timestamp the alert was created |
 | `description` | String | Description of the alert |
 | `details` | JSON | Alert details |

lib/gitlab/graphql/loaders/alert_management/alerts/assignees_loader.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Loaders
+      module AlertManagement
+        module Alerts
+          # Batches requests for AlertManagement::Alert#assignees
+          # to avoid N+1 queries
+          class AssigneesLoader
+            # @param alert_id [Integer]
+            # @param authorization_filter [Proc] Filter to be applied
+            #                             to output assignees
+            def initialize(alert_id, authorization_filter)
+              @alert_id = alert_id
+              @authorization_filter = authorization_filter
+            end
+
+            # Returns BatchLoader::GraphQL which evaluates
+            # to a Gitlab::Graphql::FilterableArray of User objects
+            def find
+              BatchLoader::GraphQL.for(alert_id).batch(default_value: default_value) do |alert_ids, loader|
+                load_assignees(loader, alert_ids)
+              end
+            end
+
+            private
+
+            attr_reader :alert_id, :authorization_filter
+
+            def default_value
+              Gitlab::Graphql::FilterableArray.new(authorization_filter)
+            end
+
+            def load_assignees(loader, alert_ids)
+              ::AlertManagement::AlertAssignee
+                .for_alert_ids(alert_ids)
+                .each { |alert_assignee| add_assignee_for_alert(loader, alert_assignee) }
+            end
+
+            def add_assignee_for_alert(loader, alert_assignee)
+              # loader optionally accepts a block, allowing
+              # access to the current expected output, allowing
+              # us to collect assignees
+              loader.call(alert_assignee.alert_id) { |assignees| assignees << alert_assignee.assignee }
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/frontend/alert_management/components/alert_management_list_spec.js

@@ -262,7 +262,7 @@ describe('AlertManagementList', () => {
         findAssignees()
           .at(1)
           .text(),
-      ).toBe(mockAlerts[1].assignees[0].username);
+      ).toBe(mockAlerts[1].assignees.nodes[0].username);
     });
 
     it('navigates to the detail page when alert row is clicked', () => {
@@ -291,6 +291,7 @@ describe('AlertManagementList', () => {
                   startedAt: '2020-03-17T23:18:14.996Z',
                   endedAt: '2020-04-17T23:18:14.996Z',
                   severity: 'high',
+                  assignees: { nodes: [] },
                 },
               ],
             },

spec/frontend/alert_management/mocks/alerts.json

 [
-    {
-      "iid": "1527542",
-      "title": "SyntaxError: Invalid or unexpected token",
-      "severity": "CRITICAL",
-      "eventCount": 7,
-      "createdAt": "2020-04-17T23:18:14.996Z",
-      "startedAt": "2020-04-17T23:18:14.996Z",
-      "endedAt": "2020-04-17T23:18:14.996Z",
-      "status": "TRIGGERED",
-      "assignees": []
-    },
-    {
-      "iid": "1527543",
-      "title": "Some other alert Some other alert Some other alert Some other alert Some other alert Some other alert",
-      "severity": "MEDIUM",
-      "eventCount": 1,
-      "startedAt": "2020-04-17T23:18:14.996Z",
-      "endedAt": "2020-04-17T23:18:14.996Z",
-      "status": "ACKNOWLEDGED",
-      "assignees": [{"username": "root"}]
-    },
-    {
-      "iid": "1527544",
-      "title": "SyntaxError: Invalid or unexpected token",
-      "severity": "LOW",
-      "eventCount": 4,
-      "startedAt": "2020-04-17T23:18:14.996Z",
-      "endedAt": "2020-04-17T23:18:14.996Z",
-      "status": "RESOLVED",
-      "assignees": [{"username": "root"}]
-    }
-  ]
+  {
+    "iid": "1527542",
+    "title": "SyntaxError: Invalid or unexpected token",
+    "severity": "CRITICAL",
+    "eventCount": 7,
+    "createdAt": "2020-04-17T23:18:14.996Z",
+    "startedAt": "2020-04-17T23:18:14.996Z",
+    "endedAt": "2020-04-17T23:18:14.996Z",
+    "status": "TRIGGERED",
+    "assignees": { "nodes": [] }
+  },
+  {
+    "iid": "1527543",
+    "title": "Some other alert Some other alert Some other alert Some other alert Some other alert Some other alert",
+    "severity": "MEDIUM",
+    "eventCount": 1,
+    "startedAt": "2020-04-17T23:18:14.996Z",
+    "endedAt": "2020-04-17T23:18:14.996Z",
+    "status": "ACKNOWLEDGED",
+    "assignees": { "nodes": [{ "username": "root" }] }
+  },
+  {
+    "iid": "1527544",
+    "title": "SyntaxError: Invalid or unexpected token",
+    "severity": "LOW",
+    "eventCount": 4,
+    "startedAt": "2020-04-17T23:18:14.996Z",
+    "endedAt": "2020-04-17T23:18:14.996Z",
+    "status": "RESOLVED",
+    "assignees": { "nodes": [{ "username": "root" }] }
+  }
+]

spec/graphql/resolvers/alert_management/alerts/assignees_resolver_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Resolvers::AlertManagement::Alerts::AssigneesResolver do
+  include GraphqlHelpers
+
+  describe '#resolve' do
+    let_it_be(:current_user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:alert) { create(:alert_management_alert, :all_fields, project: project) }
+    let_it_be(:another_alert) { create(:alert_management_alert, :all_fields, project: project) }
+
+    it 'resolves for a single alert' do
+      result = batch_sync(max_queries: 2) { resolve_assignees(alert) }
+
+      expect(result).to match_array(alert.assignees)
+    end
+
+    it 'resolves for multiple alerts' do
+      result = batch_sync(max_queries: 2) { [resolve_assignees(alert), resolve_assignees(another_alert)] }
+
+      expect(result).to match_array([alert.assignees, another_alert.assignees])
+    end
+
+    private
+
+    def resolve_assignees(alert, args = {}, context = { current_user: current_user })
+      resolve(described_class, obj: alert, args: args, ctx: context)
+    end
+  end
+end

spec/lib/gitlab/graphql/loaders/alert_management/alerts/assignees_loader_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Graphql::Loaders::AlertManagement::Alerts::AssigneesLoader do
+  describe '#find' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:alert1) { create(:alert_management_alert, project: project, assignees: [user]) }
+    let_it_be(:alert2) { create(:alert_management_alert, :all_fields, project: project) }
+    let_it_be(:alert3) { create(:alert_management_alert, project: project) }
+    let(:filter) { proc {} }
+
+    subject do
+      [
+        described_class.new(alert1.id, filter).find,
+        described_class.new(alert2.id, filter).find,
+        described_class.new(alert3.id, filter).find
+      ].map(&:sync)
+    end
+
+    it 'only queries once for alert assignees' do
+      # One query for alert_assignees, one query for users
+      expect { subject }.not_to exceed_query_limit(2)
+    end
+
+    it 'returns appropriate assignees for alerts' do
+      expect(subject).to eq [alert1.assignees.to_a, alert2.assignees.to_a, alert3.assignees.to_a]
+    end
+
+    context 'with a filter' do
+      let(:filter) { proc { |users| users.select { |u| u == user } } }
+
+      it 'limits assignees by the filter' do
+        expect(subject).to eq [alert1.assignees.to_a, alert2.assignees.to_a, alert3.assignees.to_a]
+        expect(subject.all?(Gitlab::Graphql::FilterableArray)).to be_truthy
+
+        filtered_result = subject.map { |assignees| assignees.filter_callback.call(assignees) }
+
+        expect(filtered_result).to eq [[user], [], []]
+      end
+    end
+  end
+end

spec/models/alert_management/alert_assignee_spec.rb

@@ -3,19 +3,37 @@
 require 'spec_helper'
 
 describe AlertManagement::AlertAssignee do
+  let_it_be(:user1) { create(:user) }
+  let_it_be(:user2) { create(:user) }
+  let_it_be(:alert1) { create(:alert_management_alert, assignees: [user1, user2]) }
+  let_it_be(:alert2) { create(:alert_management_alert, assignees: [user2]) }
+
   describe 'associations' do
     it { is_expected.to belong_to(:alert) }
     it { is_expected.to belong_to(:assignee) }
   end
 
   describe 'validations' do
-    let(:alert) { create(:alert_management_alert) }
-    let(:user) { create(:user) }
-
-    subject { alert.alert_assignees.build(assignee: user) }
+    subject { alert1.alert_assignees.build(assignee: user1) }
 
     it { is_expected.to validate_presence_of(:alert) }
     it { is_expected.to validate_presence_of(:assignee) }
     it { is_expected.to validate_uniqueness_of(:assignee).scoped_to(:alert_id) }
   end
+
+  describe 'scopes' do
+    describe '.for_alert_ids' do
+      let(:alert_ids) { alert1.id }
+
+      subject { described_class.for_alert_ids(alert_ids) }
+
+      it { is_expected.to contain_exactly(*alert1.reload.alert_assignees) }
+
+      context 'with multiple ids' do
+        let(:alert_ids) { [alert1.id, alert2.id] }
+
+        it { is_expected.to contain_exactly(*alert1.reload.alert_assignees, *alert2.reload.alert_assignees) }
+      end
+    end
+  end
 end

spec/requests/api/graphql/project/alert_management/alert/assignees_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe 'getting Alert Management Alert Assignees' do
+  include GraphqlHelpers
+
+  let_it_be(:project) { create(:project) }
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:first_alert) { create(:alert_management_alert, project: project, assignees: [current_user]) }
+  let_it_be(:second_alert) { create(:alert_management_alert, project: project) }
+
+  let(:params) { {} }
+
+  let(:fields) do
+    <<~QUERY
+      nodes {
+        #{all_graphql_fields_for('AlertManagementAlert')}
+      }
+    QUERY
+  end
+
+  let(:query) do
+    graphql_query_for(
+      'project',
+      { 'fullPath' => project.full_path },
+      query_graphql_field('alertManagementAlerts', params, fields)
+    )
+  end
+
+  let(:alerts) { graphql_data.dig('project', 'alertManagementAlerts', 'nodes') }
+  let(:assignees) { alerts.map { |alert| [alert['iid'], alert['assignees']['nodes']] }.to_h }
+  let(:first_assignees) { assignees[first_alert.iid.to_s] }
+  let(:second_assignees) { assignees[second_alert.iid.to_s] }
+
+  before do
+    project.add_developer(current_user)
+  end
+
+  it 'returns the correct assignees' do
+    post_graphql(query, current_user: current_user)
+
+    expect(first_assignees.length).to eq(1)
+    expect(first_assignees.first).to include('username' => current_user.username)
+    expect(second_assignees).to be_empty
+  end
+
+  it 'applies appropriate filters for non-visible users' do
+    allow(Ability).to receive(:allowed?).and_call_original
+    allow(Ability).to receive(:allowed?).with(current_user, :read_user, current_user).and_return(false)
+
+    post_graphql(query, current_user: current_user)
+
+    expect(first_assignees).to be_empty
+    expect(second_assignees).to be_empty
+  end
+
+  it 'avoids N+1 queries' do
+    base_count = ActiveRecord::QueryRecorder.new do
+      post_graphql(query, current_user: current_user)
+    end
+
+    # An N+1 would mean a new alert would increase the query count
+    third_alert = create(:alert_management_alert, project: project, assignees: [current_user])
+
+    expect { post_graphql(query, current_user: current_user) }.not_to exceed_query_limit(base_count)
+
+    third_assignees = assignees[third_alert.iid.to_s]
+
+    expect(third_assignees.length).to eq(1)
+    expect(third_assignees.first).to include('username' => current_user.username)
+  end
+
+  context 'with alert_assignee flag disabled' do
+    before do
+      stub_feature_flags(alert_assignee: false)
+    end
+
+    it 'excludes assignees' do
+      post_graphql(query, current_user: current_user)
+
+      expect(first_assignees).to be_empty
+      expect(second_assignees).to be_empty
+    end
+  end
+end

spec/requests/api/graphql/project/alert_management/alerts_spec.rb

@@ -75,7 +75,7 @@ describe 'getting Alert Management Alerts' do
           'updatedAt' => triggered_alert.updated_at.strftime('%Y-%m-%dT%H:%M:%SZ')
         )
 
-        expect(first_alert['assignees'].first).to include('username' => triggered_alert.assignees.first.username)
+        expect(first_alert['assignees']['nodes'].first).to include('username' => triggered_alert.assignees.first.username)
 
         expect(second_alert).to include(
           'iid' => resolved_alert.iid.to_s,
@@ -137,18 +137,5 @@ describe 'getting Alert Management Alerts' do
         end
       end
     end
-
-    context 'with alert_assignee flag disabled' do
-      before do
-        stub_feature_flags(alert_assignee: false)
-        project.add_developer(current_user)
-
-        post_graphql(query, current_user: current_user)
-      end
-
-      it 'excludes assignees' do
-        expect(alerts.first['assignees']).to be_empty
-      end
-    end
   end
 end