Commit b8ccd78e authored by Sarah Yasonik's avatar Sarah Yasonik Committed by Clement Ho

Improve performance of querying alert assignees

Querying AlertManagementAlerts with the assignee field
through the GraphQL API would result in a N+1 query.

This fixes the query by using BatchLoader::Graphql to
group the assignee queries at the end of multiplex query,
only once all alerts have been processed. The accomodate
authorizations, this logic circumvents our GraphQL auth
layer, asserting the same authorization only after
the BatchLoader has completed its work.
parent 20c7ec4c
......@@ -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;
......
......@@ -6,8 +6,10 @@ fragment AlertListItem on AlertManagementAlert {
startedAt
endedAt
eventCount
issueIid,
issueIid
assignees {
username
},
nodes {
username
}
}
}
# 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
......@@ -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
......@@ -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
......@@ -145,7 +145,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
......
......@@ -398,20 +398,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
......@@ -52,7 +52,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 |
......
# 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
......@@ -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: [] },
},
],
},
......
[
{
"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" }] }
}
]
# 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
# 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
......@@ -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
# 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
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment