Merge branch 'sy-alert-assignee-n-1-again' into 'master'

Improve performance of querying alert assignees - attempt 2

See merge request gitlab-org/gitlab!34228

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 {
+    nodes {
       username
-  },
+    }
+  }
 }

app/graphql/resolvers/alert_management/alert_resolver.rb

@@ -3,6 +3,8 @@
 module Resolvers
   module AlertManagement
     class AlertResolver < BaseResolver
+      include LooksAhead
+
       argument :iid, GraphQL::STRING_TYPE,
                 required: false,
                 description: 'IID of the alert. For example, "1"'
@@ -22,11 +24,17 @@ module Resolvers
 
       type Types::AlertManagement::AlertType, null: true
 
-      def resolve(**args)
+      def resolve_with_lookahead(**args)
         parent = object.respond_to?(:sync) ? object.sync : object
         return ::AlertManagement::Alert.none if parent.nil?
 
-        ::AlertManagement::AlertsFinder.new(context[:current_user], parent, args).execute
+        apply_lookahead(::AlertManagement::AlertsFinder.new(context[:current_user], parent, args).execute)
+      end
+
+      def preloads
+        {
+          assignees: [:assignees]
+        }
       end
     end
   end

app/graphql/types/alert_management/alert_type.rb

@@ -85,7 +85,7 @@ module Types
             description: 'Timestamp the alert was last updated'
 
       field :assignees,
-            [Types::UserType],
+            Types::UserType.connection_type,
             null: true,
             description: 'Assignees of the alert'
 

app/graphql/types/project_type.rb

@@ -218,6 +218,7 @@ module Types
           Types::AlertManagement::AlertType.connection_type,
           null: true,
           description: 'Alert Management alerts of the project',
+          extras: [:lookahead],
           resolver: Resolvers::AlertManagement::AlertResolver
 
     field :alert_management_alert,

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": [
-
+                {
+                  "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": "LIST",
-                "name": null,
-                "ofType": {
-                  "kind": "NON_NULL",
-                  "name": null,
-                  "ofType": {
                 "kind": "OBJECT",
-                    "name": "User",
+                "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 |

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

@@ -8,7 +8,7 @@
     "startedAt": "2020-04-17T23:18:14.996Z",
     "endedAt": "2020-04-17T23:18:14.996Z",
     "status": "TRIGGERED",
-      "assignees": []
+    "assignees": { "nodes": [] }
   },
   {
     "iid": "1527543",
@@ -18,7 +18,7 @@
     "startedAt": "2020-04-17T23:18:14.996Z",
     "endedAt": "2020-04-17T23:18:14.996Z",
     "status": "ACKNOWLEDGED",
-      "assignees": [{"username": "root"}]
+    "assignees": { "nodes": [{ "username": "root" }] }
   },
   {
     "iid": "1527544",
@@ -28,6 +28,6 @@
     "startedAt": "2020-04-17T23:18:14.996Z",
     "endedAt": "2020-04-17T23:18:14.996Z",
     "status": "RESOLVED",
-      "assignees": [{"username": "root"}]
+    "assignees": { "nodes": [{ "username": "root" }] }
   }
-  ]
+]

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 {
+        iid
+        assignees {
+          nodes {
+            username
+          }
+        }
+      }
+    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

@@ -15,7 +15,7 @@ describe 'getting Alert Management Alerts' do
   let(:fields) do
     <<~QUERY
       nodes {
-        #{all_graphql_fields_for('AlertManagementAlert'.classify)}
+        #{all_graphql_fields_for('AlertManagementAlert', excluded: ['assignees'])}
       }
     QUERY
   end
@@ -75,8 +75,6 @@ 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(second_alert).to include(
           'iid' => resolved_alert.iid.to_s,
           'issueIid' => nil,
@@ -137,18 +135,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