Skip to content

G
gitlab-ce
Commit 57f33ff7 authored by charlie ablett's avatar charlie ablett
Browse files
Options

Merge branch 'issue_207898' into 'master' 

Display epic issues with null relative position

See merge request gitlab-org/gitlab!33105
parents b6bc1122 461f9781
Hide whitespace changes
Inline Side-by-side
Showing with 27 additions and 202 deletions
ee/app/graphql/resolvers/epic_issues_resolver.rb
View file @ 57f33ff7
......@@ -6,20 +6,8 @@ module Resolvers
alias_method :epic, :object
# When using EpicIssuesResolver then epic's issues are authorized when
# rendering lazy-loaded issues, we explicitly ignore any inherited
# type_authorizations to avoid executing any authorization checks in earlier
# phase
def self.skip_authorizations?
true
end
def resolve(**args)
filter = proc do |issues|
Ability.issues_readable_by_user(issues, context[:current_user])
end
Gitlab::Graphql::Loaders::BatchEpicIssuesLoader.new(epic.id, filter).find
epic.issues_readable_by(context[:current_user], preload: { project: [:namespace, :project_feature] })
end
end
end
ee/app/graphql/types/epic_type.rb
View file @ 57f33ff7
......@@ -121,7 +121,7 @@ module Types
field :issues,
Types::EpicIssueType.connection_type,
null: true,
complexity: 2,
complexity: 5,
description: 'A list of issues associated with the epic',
resolver: Resolvers::EpicIssuesResolver
......
ee/changelogs/unreleased/issue_207898.yml 0 → 100644
View file @ 57f33ff7
---
title: Display epic issues with null relative position
merge_request: 33105
author:
type: fixed
ee/lib/gitlab/graphql/loaders/batch_epic_issues_loader.rb deleted 100644 → 0
View file @ b6bc1122
# frozen_string_literal: true
module Gitlab
module Graphql
module Loaders
class BatchEpicIssuesLoader
# this will assure that no more than 100 queries will be done to fetch issues
MAX_LOADED_ISSUES = 100_000
BATCH_SIZE = 1_000
def initialize(model_id, authorization_filter)
@model_id = model_id
@authorization_filter = authorization_filter
end
def find
BatchLoader::GraphQL.for(@model_id).batch(default_value: []) do |ids, loader|
load_issues(loader, ids)
end
end
private
def load_issues(loader, ids)
issues = ::EpicIssue.related_issues_for_batches(ids)
issues.each_batch(of: BATCH_SIZE, column: 'relative_position') do |batch, idx|
process_batch(loader, batch, idx)
end
end
def process_batch(loader, batch, idx)
Epic.related_issues(preload: { project: [:namespace, :project_feature] })
.merge(batch.except(:select)).each do |issue|
ensure_limit_not_exceeded!(idx)
loader.call(issue.epic_id) do |memo|
unless memo.is_a?(Gitlab::Graphql::FilterableArray)
# memo is an empty array by default
memo = Gitlab::Graphql::FilterableArray.new(@authorization_filter)
end
memo << issue
end
end
end
def ensure_limit_not_exceeded!(current_index)
if current_index * BATCH_SIZE > MAX_LOADED_ISSUES
raise Gitlab::Graphql::Errors::ArgumentError, 'Too many epic issues requested.'
end
end
end
end
end
end
ee/spec/graphql/resolvers/epic_issues_resolver_spec.rb
View file @ 57f33ff7
......@@ -7,17 +7,19 @@ RSpec.describe Resolvers::EpicIssuesResolver do
let_it_be(:current_user) { create(:user) }
let_it_be(:user) { create(:user) }
let_it_be(:group) { create(:group) }
let_it_be(:group) { create(:group, :public) }
let_it_be(:project1) { create(:project, :public, group: group) }
let_it_be(:project2) { create(:project, :private, group: group) }
let_it_be(:epic1) { create(:epic, group: group) }
let_it_be(:epic2) { create(:epic, group: group) }
let_it_be(:issue1) { create(:issue, project: project1) }
let_it_be(:issue2) { create(:issue, project: project1) }
let_it_be(:issue2) { create(:issue, project: project1, confidential: true) }
let_it_be(:issue3) { create(:issue, project: project2) }
let_it_be(:issue4) { create(:issue, project: project2) }
let_it_be(:epic_issue1) { create(:epic_issue, epic: epic1, issue: issue1, relative_position: 3) }
let_it_be(:epic_issue2) { create(:epic_issue, epic: epic1, issue: issue2, relative_position: 2) }
let_it_be(:epic_issue3) { create(:epic_issue, epic: epic2, issue: issue3, relative_position: 1) }
let_it_be(:epic_issue4) { create(:epic_issue, epic: epic2, issue: issue4, relative_position: nil) }
before do
group.add_developer(current_user)
......@@ -26,17 +28,21 @@ RSpec.describe Resolvers::EpicIssuesResolver do
describe '#resolve' do
it 'finds all epic issues' do
result = batch_sync(max_queries: 6) { resolve_epic_issues(epic1) }
result = [resolve_epic_issues(epic1), resolve_epic_issues(epic2)]
expect(result).to contain_exactly(issue1, issue2)
expect(result).to contain_exactly([issue2, issue1], [issue3, issue4])
end
it 'can batch-resolve epic issues from different epics' do
result = batch_sync(max_queries: 6) do
[resolve_epic_issues(epic1), resolve_epic_issues(epic2)]
end
it 'finds only epic issues that user can read' do
guest = create(:user)
expect(result).to contain_exactly([issue2, issue1], [issue3])
result =
[
resolve_epic_issues(epic1, {}, { current_user: guest }),
resolve_epic_issues(epic2, {}, { current_user: guest })
]
expect(result).to contain_exactly([], [issue1])
end
end
......
ee/spec/lib/gitlab/graphql/loaders/batch_epic_issues_loader_spec.rb deleted 100644 → 0
View file @ b6bc1122
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::Graphql::Loaders::BatchEpicIssuesLoader do
describe '#find' do
let_it_be(:user) { create(:user) }
let_it_be(:group) { create(:group) }
let_it_be(:project1) { create(:project, :public, group: group) }
let_it_be(:project2) { create(:project, :private, group: group) }
let_it_be(:epic1) { create(:epic, group: group) }
let_it_be(:epic2) { create(:epic, group: group) }
let_it_be(:issue1) { create(:issue, project: project1) }
let_it_be(:issue2) { create(:issue, project: project2) }
let_it_be(:issue3) { create(:issue, project: project2) }
let_it_be(:epic_issue1) { create(:epic_issue, epic: epic1, issue: issue1, relative_position: 1000) }
let_it_be(:epic_issue2) { create(:epic_issue, epic: epic2, issue: issue2, relative_position: 99999) }
let_it_be(:epic_issue3) { create(:epic_issue, epic: epic2, issue: issue3, relative_position: 1) }
let(:filter) { proc {} }
subject do
[
described_class.new(epic1.id, filter).find,
described_class.new(epic2.id, filter).find
].map(&:sync)
end
it 'only queries once for epic issues' do
# 6 queries are done: 2 queries in EachBatch and then getting issues
# and getting projects, project_features and groups for these issues
expect { subject }.not_to exceed_query_limit(6)
end
it 'returns all epic issues ordered by relative position' do
expect(subject).to eq [[issue1], [issue3, issue2]]
end
it 'returns an instance of FilterableArray' do
expect(subject.all?(Gitlab::Graphql::FilterableArray)).to be_truthy
end
it 'raises an error if too many issues are loaded' do
stub_const('Gitlab::Graphql::Loaders::BatchEpicIssuesLoader::MAX_LOADED_ISSUES', 2)
expect { subject }.to raise_error Gitlab::Graphql::Errors::ArgumentError, 'Too many epic issues requested.'
end
end
end
ee/spec/requests/api/graphql/group/epic/epic_issues_spec.rb
View file @ 57f33ff7
......@@ -157,7 +157,10 @@ RSpec.describe 'Getting issues for an epic' do
expect(result[epic2.iid]).to eq [issue2.to_global_id.to_s]
end
it 'avoids N+1 queries' do
# TODO remove the pending state of this spec when
# we have an efficient way of preloading data on GraphQL.
# For more information check: https://gitlab.com/gitlab-org/gitlab/-/issues/207898
xit 'avoids N+1 queries' do
control_count = ActiveRecord::QueryRecorder.new do
post_graphql(epic_query(iid: epic.iid), current_user: user)
end.count
......
lib/gitlab/graphql/authorize/instrumentation.rb
View file @ 57f33ff7
......@@ -9,16 +9,12 @@ module Gitlab
def instrument(_type, field)
service = AuthorizeFieldService.new(field)
if service.authorizations? && !resolver_skips_authorizations?(field)
if service.authorizations?
field.redefine { resolve(service.authorized_resolve) }
else
field
end
end
def resolver_skips_authorizations?(field)
field.metadata[:resolver].try(:skip_authorizations?)
end
end
end
end
......
lib/gitlab/graphql/filterable_array.rb deleted 100644 → 0
View file @ b6bc1122
# frozen_string_literal: true
module Gitlab
module Graphql
class FilterableArray < Array
attr_reader :filter_callback
def initialize(filter_callback, *args)
super(args)
@filter_callback = filter_callback
end
end
end
end
lib/gitlab/graphql/pagination/connections.rb
View file @ 57f33ff7
......@@ -9,10 +9,6 @@ module Gitlab
ActiveRecord::Relation,
Gitlab::Graphql::Pagination::Keyset::Connection)
schema.connections.add(
Gitlab::Graphql::FilterableArray,
Gitlab::Graphql::Pagination::FilterableArrayConnection)
schema.connections.add(
Gitlab::Graphql::ExternallyPaginatedArray,
Gitlab::Graphql::Pagination::ExternallyPaginatedArrayConnection)
......
lib/gitlab/graphql/pagination/filterable_array_connection.rb deleted 100644 → 0
View file @ b6bc1122
# frozen_string_literal: true
module Gitlab
module Graphql
module Pagination
# FilterableArrayConnection is useful especially for lazy-loaded values.
# It allows us to call a callback only on the slice of array being
# rendered in the "after loaded" phase. For example we can check
# permissions only on a small subset of items.
class FilterableArrayConnection < GraphQL::Pagination::ArrayConnection
def nodes
@nodes ||= items.filter_callback.call(super)
end
end
end
end
end
spec/graphql/gitlab_schema_spec.rb
View file @ 57f33ff7
......@@ -46,12 +46,6 @@ describe GitlabSchema do
expect(connection).to eq(Gitlab::Graphql::Pagination::ExternallyPaginatedArrayConnection)
end
it 'paginates FilterableArray using `Pagination::FilterableArrayConnection`' do
connection = connections[Gitlab::Graphql::FilterableArray]
expect(connection).to eq(Gitlab::Graphql::Pagination::FilterableArrayConnection)
end
describe '.execute' do
context 'for different types of users' do
context 'when no context' do
......
spec/lib/gitlab/graphql/pagination/filterable_array_connection_spec.rb deleted 100644 → 0
View file @ b6bc1122
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Graphql::Pagination::FilterableArrayConnection do
let(:callback) { proc { |nodes| nodes } }
let(:all_nodes) { Gitlab::Graphql::FilterableArray.new(callback, 1, 2, 3, 4, 5) }
let(:arguments) { {} }
subject(:connection) do
described_class.new(all_nodes, { max_page_size: 3 }.merge(arguments))
end
describe '#nodes' do
let(:paged_nodes) { subject.nodes }
it_behaves_like 'connection with paged nodes' do
let(:paged_nodes_size) { 3 }
end
context 'when callback filters some nodes' do
let(:callback) { proc { |nodes| nodes[1..-1] } }
it 'does not return filtered elements' do
expect(subject.nodes).to contain_exactly(all_nodes[1], all_nodes[2])
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7