From 45a5c79561c7b222126d458f93778d5721ea850a Mon Sep 17 00:00:00 2001
From: Malcolm Locke <mlocke@gitlab.com>
Date: Mon, 4 Mar 2024 15:02:18 +1300
Subject: [PATCH 1/4] Fix GraphQL pipeline findings pagination
---
ee/app/finders/security/findings_finder.rb | 20 +--
ee/app/graphql/ee/types/ci/pipeline_type.rb | 1 +
...eline_security_report_findings_resolver.rb | 40 +++++-
ee/lib/api/vulnerability_findings.rb | 20 ++-
.../pipelines/user_sees_security_tab_spec.rb | 11 +-
.../pipeline/security_report_findings_spec.rb | 85 ++++++++++++-
.../findings_finder_shared_examples.rb | 117 ------------------
7 files changed, 149 insertions(+), 145 deletions(-)
diff --git a/ee/app/finders/security/findings_finder.rb b/ee/app/finders/security/findings_finder.rb
index 3e9a1b93d7021f..709328ad02e3de 100644
--- a/ee/app/finders/security/findings_finder.rb
+++ b/ee/app/finders/security/findings_finder.rb
@@ -11,8 +11,7 @@
# confidence: Array<String>
# report_type: Array<String>
# scope: String
-# page: Int
-# per_page: Int
+# limit: Int
module Security
class FindingsFinder
@@ -23,8 +22,7 @@ class FindingsFinder
:next_page, :prev_page, :page, :empty?, :without_count, to: :relation
end
- DEFAULT_PAGE = 1
- DEFAULT_PER_PAGE = 20
+ DEFAULT_LIMIT = 20
def initialize(pipeline, params: {})
@pipeline = pipeline
@@ -32,7 +30,7 @@ def initialize(pipeline, params: {})
end
def execute
- return ResultSet.new(Security::Finding.none.page(DEFAULT_PAGE), []) unless has_security_findings?
+ return ResultSet.new(Security::Finding.none, []) unless has_security_findings?
ResultSet.new(security_findings, findings)
end
@@ -122,7 +120,7 @@ def load_security_findings
.then { |relation| by_scanner_external_ids(relation) }
.then { |relation| by_state(relation) }
.then { |relation| by_include_dismissed(relation) }
- .limit((per_page * page) + 1)
+ .limit(limit + 1)
from_sql = <<~SQL.squish
"security_scans",
@@ -142,16 +140,10 @@ def load_security_findings
.merge(::Security::Scan.latest_successful)
.ordered(params[:sort])
.then { |relation| by_report_types(relation) }
- .page(page)
- .per(per_page)
end
- def per_page
- @per_page ||= params[:per_page] || DEFAULT_PER_PAGE
- end
-
- def page
- @page ||= params[:page] || DEFAULT_PAGE
+ def limit
+ @limit ||= params[:limit] || DEFAULT_LIMIT
end
def include_dismissed?
diff --git a/ee/app/graphql/ee/types/ci/pipeline_type.rb b/ee/app/graphql/ee/types/ci/pipeline_type.rb
index 266a3d0454d844..db3e1613abb15d 100644
--- a/ee/app/graphql/ee/types/ci/pipeline_type.rb
+++ b/ee/app/graphql/ee/types/ci/pipeline_type.rb
@@ -25,6 +25,7 @@ module PipelineType
::Types::PipelineSecurityReportFindingType.connection_type,
null: true,
description: 'Vulnerability findings reported on the pipeline. By default all the states except dismissed are included in the response.',
+ connection_extension: ::Gitlab::Graphql::Extensions::ExternallyPaginatedArrayExtension,
resolver: ::Resolvers::PipelineSecurityReportFindingsResolver
# rubocop:enable Layout/LineLength
diff --git a/ee/app/graphql/resolvers/pipeline_security_report_findings_resolver.rb b/ee/app/graphql/resolvers/pipeline_security_report_findings_resolver.rb
index bdcd995cf38701..8ebef219e0d8c4 100644
--- a/ee/app/graphql/resolvers/pipeline_security_report_findings_resolver.rb
+++ b/ee/app/graphql/resolvers/pipeline_security_report_findings_resolver.rb
@@ -28,8 +28,11 @@ class PipelineSecurityReportFindingsResolver < BaseResolver
description: 'List vulnerability findings by sort order.'
def resolve(**args)
- pure_finder = ::Security::PureFindingsFinder.new(pipeline, params: args)
- return pure_finder.execute if pure_finder.available?
+ pure_finder = ::Security::PureFindingsFinder.new(
+ pipeline, params: args.merge(limit: limit(args))
+ )
+
+ return offset_pagination(pure_finder.execute) if pure_finder.available?
# This finder class has been deprecated and will be removed by
# https://gitlab.com/gitlab-org/gitlab/-/issues/334488.
@@ -38,5 +41,38 @@ def resolve(**args)
rescue ::Security::PipelineVulnerabilitiesFinder::ParseError
[]
end
+
+ private
+
+ def limit(args)
+ first = args[:first]
+ last = args[:last]
+ after = decode(args[:after])
+ before = decode(args[:before])
+
+ first = context.schema.default_max_page_size if !first && !last
+
+ if first && last
+ raise(Gitlab::Graphql::Errors::ArgumentError, "Can only provide either `first` or `last`, not both")
+ end
+
+ if after
+ first + after
+ elsif before
+ before - 1
+ else
+ first || last
+ end
+ end
+
+ # The :before and :after cursor arguments when used with keyset
+ # pagination are JSON structures Base64 encoded.
+ #
+ # When using offset pagination they are just an integer representing
+ # the offset of the desired page. The framework still encodes them
+ # with Base64 though, so we need to decode them here to get an int.
+ def decode(value)
+ GraphQL::Schema::Base64Encoder.decode(value).to_i if value
+ end
end
end
diff --git a/ee/lib/api/vulnerability_findings.rb b/ee/lib/api/vulnerability_findings.rb
index f22214011dfe27..d37397ecbbc461 100644
--- a/ee/lib/api/vulnerability_findings.rb
+++ b/ee/lib/api/vulnerability_findings.rb
@@ -28,22 +28,36 @@ def vulnerability_findings
end
def with_pure_finder
- pure_finder = Security::PureFindingsFinder.new(pipeline, params: declared_params)
+ pure_finder = Security::PureFindingsFinder.new(pipeline, params: finder_params)
return unless pure_finder.available?
findings = pure_finder.execute
.tap { |findings| findings.each(&:remediations) } # initiates Batchloader
- paginate(findings.without_count, without_count: true)
+ paginate(findings.page(page).per(per_page).without_count, without_count: true)
end
def with_adaptive_finder
result = Security::FindingsFinder.new(pipeline, params: declared_params).execute
- paginate(result, without_count: true)
+ paginate(result.page(page).per(per_page), without_count: true)
result.findings
end
+
+ def page
+ params[:page] || 1
+ end
+
+ def per_page
+ params[:per_page] || 20
+ end
+
+ def finder_params
+ declared_params.merge(
+ limit: page * per_page
+ )
+ end
end
before do
diff --git a/ee/spec/features/projects/pipelines/user_sees_security_tab_spec.rb b/ee/spec/features/projects/pipelines/user_sees_security_tab_spec.rb
index d379fc9e0b655a..ce0e6b03c47cf3 100644
--- a/ee/spec/features/projects/pipelines/user_sees_security_tab_spec.rb
+++ b/ee/spec/features/projects/pipelines/user_sees_security_tab_spec.rb
@@ -9,6 +9,13 @@
create(:ee_ci_pipeline, :success, :with_sast_report, project: project)
end
+ let!(:vulnerabilities_scanner) do
+ create(
+ :vulnerabilities_scanner,
+ project: project
+ )
+ end
+
let!(:security_scan) do
create(
:security_scan,
@@ -26,6 +33,7 @@
:security_finding,
21, # rubocop:disable RSpec/FactoryBot/ExcessiveCreateList -- see note above
:with_finding_data,
+ scanner: vulnerabilities_scanner,
scan: security_scan,
deduplicated: true
)
@@ -36,9 +44,6 @@
security_dashboard: true,
sast: true
)
- stub_feature_flags(
- pipeline_security_dashboard_graphql: false
- )
end
it "shows the pipeline security findings" do
diff --git a/ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb b/ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb
index f029572a17649e..e9f9ad965f9ed9 100644
--- a/ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb
+++ b/ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb
@@ -9,7 +9,7 @@
let_it_be(:project) { create(:project, :repository, :public) }
let_it_be(:pipeline) { create(:ci_pipeline, :success, project: project) }
let_it_be(:user) { create(:user) }
- let_it_be(:query) do
+ let(:query) do
%(
query {
project(fullPath: "#{project.full_path}") {
@@ -44,11 +44,21 @@
let(:security_report_findings) { subject.dig('project', 'pipeline', 'securityReportFindings', 'nodes') }
before_all do
- create(:ci_build, :success, name: 'dast_job', pipeline: pipeline, project: project) do |job|
- create(:ee_ci_job_artifact, :dast_large_scanned_resources_field, job: job, project: project)
- end
- create(:ci_build, :success, name: 'sast_job', pipeline: pipeline, project: project) do |job|
- create(:ee_ci_job_artifact, :sast, job: job, project: project)
+ dast_job = create(:ci_build, :success, name: 'dast_job', pipeline: pipeline, project: project)
+ dast_artifact = create(:ee_ci_job_artifact, :dast_large_scanned_resources_field, job: dast_job, project: project)
+
+ sast_job = create(:ci_build, :success, name: 'sast_job', pipeline: pipeline, project: project)
+ sast_artifact = create(:ee_ci_job_artifact, :sast, job: sast_job, project: project)
+
+ # StoreGroupedScansService acquires a Gitlab::Instrumentation::ExclusiveLock
+ # We have checks in place to raise errors if these locks are acquired
+ # within transactions. Because our tests all run inside a database
+ # cleaner transaction these checks need to be skipped here.
+ Gitlab::ExclusiveLease.skipping_transaction_check do
+ # This causes the job artifacts to be ingested and security_findings
+ # records to be created with the correct associations to scanners
+ # and the pipeline.
+ Security::StoreGroupedScansService.new([dast_artifact, sast_artifact]).execute
end
end
@@ -85,6 +95,69 @@
expect(security_report_finding['solution']).not_to be_nil
expect(security_report_finding['description']).not_to be_nil
end
+
+ describe 'pagination' do
+ let(:query) do
+ %(
+ query {
+ project(fullPath: "#{project.full_path}") {
+ pipeline(iid: "#{pipeline.iid}") {
+ securityReportFindings(#{pagination_arguments}, reportType: ["sast", "dast"]) {
+ nodes {
+ title
+ reportType
+ uuid
+ }
+ }
+ }
+ }
+ }
+ )
+ end
+
+ let(:actual_uuids) { security_report_findings.map { |finding| finding.fetch('uuid') } }
+
+ context 'with only :first argument' do
+ let(:pagination_arguments) { "first: 5" }
+
+ it 'returns the first 5 findings' do
+ expected_uuids = Security::Finding.all.order(severity: :desc, id: :asc).limit(5).map(&:uuid)
+ expect(actual_uuids).to eq(expected_uuids)
+ end
+ end
+
+ context 'with :first and :after arguments' do
+ let(:pagination_arguments) { "first: 5,after: \"#{encode('5')}\"" }
+
+ it 'returns the 5th to 10th findings' do
+ expected_uuids = Security::Finding.all.order(severity: :desc, id: :asc).offset(5).limit(5).map(&:uuid)
+ expect(actual_uuids).to eq(expected_uuids)
+ end
+ end
+
+ context 'with :last and :before arguments' do
+ let(:pagination_arguments) { "last: 10,before: \"#{encode('21')}\"" }
+
+ it 'returns the 10th to 20th findings' do
+ expected_uuids = Security::Finding.all.order(severity: :desc, id: :asc).offset(10).limit(10).map(&:uuid)
+ expect(actual_uuids).to eq(expected_uuids)
+ end
+ end
+
+ context 'with :first and :last arguments' do
+ let(:pagination_arguments) { "last: 10,first: 10,before: \"#{encode('21')}\"" }
+
+ it 'returns an error' do
+ expect(security_report_findings).to be_blank
+ expect(graphql_errors).to be_present
+ end
+ end
+
+ # The before and after pagination cursors are base64 encoded
+ def encode(value)
+ GraphQL::Schema::Base64Encoder.encode(value.to_s)
+ end
+ end
end
context 'when user is not a member of the project' do
diff --git a/ee/spec/support/shared_examples/finders/security/findings_finder_shared_examples.rb b/ee/spec/support/shared_examples/finders/security/findings_finder_shared_examples.rb
index f25a85673ef48d..37aaf96be01b72 100644
--- a/ee/spec/support/shared_examples/finders/security/findings_finder_shared_examples.rb
+++ b/ee/spec/support/shared_examples/finders/security/findings_finder_shared_examples.rb
@@ -15,8 +15,6 @@
let(:confidence_levels) { nil }
let(:report_types) { nil }
let(:scope) { nil }
- let(:page) { nil }
- let(:per_page) { nil }
let(:scanner) { nil }
let(:state) { nil }
let(:sort) { nil }
@@ -27,8 +25,6 @@
confidence: confidence_levels,
report_type: report_types,
scope: scope,
- page: page,
- per_page: per_page,
scanner: scanner,
state: state,
sort: sort
@@ -99,103 +95,6 @@
end
end
- describe '#current_page' do
- subject { finder_result.current_page }
-
- context 'when the page is not provided' do
- it { is_expected.to be(1) }
- end
-
- context 'when the page is provided' do
- let(:page) { 2 }
-
- it { is_expected.to be(2) }
- end
- end
-
- describe '#limit_value' do
- subject { finder_result.limit_value }
-
- context 'when the per_page is not provided' do
- it { is_expected.to be(20) }
- end
-
- context 'when the per_page is provided' do
- let(:per_page) { 100 }
-
- it { is_expected.to be(100) }
- end
- end
-
- describe '#total_pages' do
- subject { finder_result.total_pages }
-
- context 'when the per_page is not provided' do
- it { is_expected.to be(1) }
- end
-
- context 'when the per_page is provided' do
- let(:per_page) { 3 }
-
- it { is_expected.to be(3) }
- end
- end
-
- describe '#total_count' do
- subject { finder_result.total_count }
-
- context 'when the scope is not provided' do
- let(:finding_to_dismiss) { Security::Finding.first }
-
- before do
- vulnerability_finding = create(:vulnerabilities_finding, uuid: finding_to_dismiss.uuid)
-
- create(:vulnerability, state: :dismissed, findings: [vulnerability_finding])
- end
-
- it { is_expected.to be(7) }
- end
-
- context 'when the scope is provided as `all`' do
- let(:scope) { 'all' }
-
- it { is_expected.to be(8) }
- end
- end
-
- describe '#next_page' do
- subject { finder_result.next_page }
-
- context 'when the page is not provided' do
- # Limit per_page to force pagination on smaller dataset
- let(:per_page) { 2 }
-
- it { is_expected.to be(2) }
- end
-
- context 'when the page is provided' do
- let(:page) { 2 }
-
- it { is_expected.to be_nil }
- end
- end
-
- describe '#prev_page' do
- subject { finder_result.prev_page }
-
- context 'when the page is not provided' do
- it { is_expected.to be_nil }
- end
-
- context 'when the page is provided' do
- let(:page) { 2 }
- # Limit per_page to force pagination on smaller dataset
- let(:per_page) { 2 }
-
- it { is_expected.to be(1) }
- end
- end
-
describe '#findings' do
subject { findings.map(&:uuid) }
@@ -216,22 +115,6 @@
it { is_expected.to match_array([uuid]) }
end
- context 'when the page is provided' do
- let(:page) { 2 }
- # Limit per_page to force pagination on smaller dataset
- let(:per_page) { 2 }
- let(:expected_uuids) { Security::Finding.deduplicated.ordered.pluck(:uuid)[2..3] }
-
- it { is_expected.to match_array(expected_uuids) }
- end
-
- context 'when the per_page is provided' do
- let(:per_page) { 1 }
- let(:expected_uuids) { [Security::Finding.deduplicated.ordered.pick(:uuid)] }
-
- it { is_expected.to match_array(expected_uuids) }
- end
-
context 'when the `severity_levels` is provided' do
let(:severity_levels) { [:medium] }
let(:expected_uuids) { Security::Finding.where(severity: 'medium').pluck(:uuid) }
--
GitLab
From 979b0c796bffa149b0b71fbf4f3e454098ef4fb4 Mon Sep 17 00:00:00 2001
From: Malcolm Locke <mlocke@gitlab.com>
Date: Fri, 8 Mar 2024 15:40:37 +1300
Subject: [PATCH 2/4] Fix vuln findings pagination tests
These tests are checking for the presence of the `X-Total` pagination
header but we do not actually want these on this endoint.
The endpoint uses pagination without counts which does not produce this
header.
Other changes on this MR have removed the headers and highlighted these
outdated tests, so we're removing them here.
---
.../api/vulnerability_findings_spec.rb | 22 +++++++++----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/ee/spec/requests/api/vulnerability_findings_spec.rb b/ee/spec/requests/api/vulnerability_findings_spec.rb
index 0dcefc055afbc2..5f8781e109e2f3 100644
--- a/ee/spec/requests/api/vulnerability_findings_spec.rb
+++ b/ee/spec/requests/api/vulnerability_findings_spec.rb
@@ -63,14 +63,14 @@
it 'returns all non-dismissed vulnerabilities', :aggregate_failures do
# all findings except one that was dismissed
- finding_count = (sast_report.findings.count + ds_report.findings.count - 1).to_s
+ finding_count = (sast_report.findings.count + ds_report.findings.count - 1)
get api(project_vulnerability_findings_path, user), params: pagination
expect(response).to have_gitlab_http_status(:ok)
- expect(response).to include_pagination_headers
+ expect(response).to include_limited_pagination_headers
expect(response).to match_response_schema('vulnerabilities/finding_list', dir: 'ee')
- expect(response.headers['X-Total']).to eq finding_count
+ expect(json_response.count).to eq finding_count
expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[dependency_scanning sast]
end
@@ -109,13 +109,13 @@
describe 'filtering' do
it 'returns vulnerabilities with sast report_type', :aggregate_failures do
- finding_count = (sast_report.findings.count - 1).to_s # all SAST findings except one that was dismissed
+ finding_count = (sast_report.findings.count - 1) # all SAST findings except one that was dismissed
get api(project_vulnerability_findings_path, user), params: { report_type: 'sast' }
expect(response).to have_gitlab_http_status(:ok)
- expect(response.headers['X-Total']).to eq finding_count
+ expect(json_response.count).to eq finding_count
expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[sast]
@@ -125,13 +125,13 @@
end
it 'returns vulnerabilities with dependency_scanning report_type', :aggregate_failures do
- finding_count = ds_report.findings.count.to_s
+ finding_count = ds_report.findings.count
get api(project_vulnerability_findings_path, user), params: { report_type: 'dependency_scanning' }
expect(response).to have_gitlab_http_status(:ok)
- expect(response.headers['X-Total']).to eq finding_count
+ expect(json_response.count).to eq finding_count
expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[dependency_scanning]
@@ -147,13 +147,13 @@
end
it 'returns dismissed vulnerabilities with `all` scope', :aggregate_failures do
- finding_count = (sast_report.findings.count + ds_report.findings.count).to_s
+ finding_count = (sast_report.findings.count + ds_report.findings.count)
get api(project_vulnerability_findings_path, user), params: { scope: 'all' }.merge(pagination) # rubocop:disable Performance/CollectionLiteralInLoop
expect(response).to have_gitlab_http_status(:ok)
- expect(response.headers['X-Total']).to eq finding_count
+ expect(json_response.count).to eq finding_count
end
it 'returns vulnerabilities with low severity', :aggregate_failures do
@@ -186,13 +186,13 @@
context 'when pipeline_id is supplied' do
it 'returns vulnerabilities from supplied pipeline', :aggregate_failures do
- finding_count = (sast_report.findings.count + ds_report.findings.count - 1).to_s
+ finding_count = (sast_report.findings.count + ds_report.findings.count - 1)
get api(project_vulnerability_findings_path, user), params: { pipeline_id: pipeline.id }.merge(pagination)
expect(response).to have_gitlab_http_status(:ok)
- expect(response.headers['X-Total']).to eq finding_count
+ expect(json_response.count).to eq finding_count
end
context 'pipeline has no reports' do
--
GitLab
From 77a2459a2d753fbdfa81d8fdafc9f796ef89ab31 Mon Sep 17 00:00:00 2001
From: Malcolm Locke <mlocke@gitlab.com>
Date: Thu, 14 Mar 2024 13:10:08 +1300
Subject: [PATCH 3/4] Apply reviewer feedback fixes
---
ee/app/graphql/ee/types/ci/pipeline_type.rb | 4 ++++
.../projects/pipelines/user_sees_security_tab_spec.rb | 2 +-
.../graphql/project/pipeline/security_report_findings_spec.rb | 2 +-
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/ee/app/graphql/ee/types/ci/pipeline_type.rb b/ee/app/graphql/ee/types/ci/pipeline_type.rb
index db3e1613abb15d..b389030b70f407 100644
--- a/ee/app/graphql/ee/types/ci/pipeline_type.rb
+++ b/ee/app/graphql/ee/types/ci/pipeline_type.rb
@@ -25,6 +25,10 @@ module PipelineType
::Types::PipelineSecurityReportFindingType.connection_type,
null: true,
description: 'Vulnerability findings reported on the pipeline. By default all the states except dismissed are included in the response.',
+ # Although we're not paginating an Array here we're using this
+ # connection extension because it leaves the pagination
+ # arguments available for the resolver. Otherwise they are
+ # removed by the framework.
connection_extension: ::Gitlab::Graphql::Extensions::ExternallyPaginatedArrayExtension,
resolver: ::Resolvers::PipelineSecurityReportFindingsResolver
# rubocop:enable Layout/LineLength
diff --git a/ee/spec/features/projects/pipelines/user_sees_security_tab_spec.rb b/ee/spec/features/projects/pipelines/user_sees_security_tab_spec.rb
index ce0e6b03c47cf3..82d0eb40bac07a 100644
--- a/ee/spec/features/projects/pipelines/user_sees_security_tab_spec.rb
+++ b/ee/spec/features/projects/pipelines/user_sees_security_tab_spec.rb
@@ -9,7 +9,7 @@
create(:ee_ci_pipeline, :success, :with_sast_report, project: project)
end
- let!(:vulnerabilities_scanner) do
+ let(:vulnerabilities_scanner) do
create(
:vulnerabilities_scanner,
project: project
diff --git a/ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb b/ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb
index e9f9ad965f9ed9..03be02ccf9d6d6 100644
--- a/ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb
+++ b/ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb
@@ -129,7 +129,7 @@
context 'with :first and :after arguments' do
let(:pagination_arguments) { "first: 5,after: \"#{encode('5')}\"" }
- it 'returns the 5th to 10th findings' do
+ it 'returns the 6th to 10th findings' do
expected_uuids = Security::Finding.all.order(severity: :desc, id: :asc).offset(5).limit(5).map(&:uuid)
expect(actual_uuids).to eq(expected_uuids)
end
--
GitLab
From 62a313c85a861de0c24e26dd385d583b647b4721 Mon Sep 17 00:00:00 2001
From: Malcolm Locke <mlocke@gitlab.com>
Date: Thu, 14 Mar 2024 16:40:49 +1300
Subject: [PATCH 4/4] Add more robust handling of pagination args
---
...eline_security_report_findings_resolver.rb | 36 +++++++++++++++----
.../pipeline/security_report_findings_spec.rb | 27 ++++++++++++++
2 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/ee/app/graphql/resolvers/pipeline_security_report_findings_resolver.rb b/ee/app/graphql/resolvers/pipeline_security_report_findings_resolver.rb
index 8ebef219e0d8c4..3815c70e4b0caf 100644
--- a/ee/app/graphql/resolvers/pipeline_security_report_findings_resolver.rb
+++ b/ee/app/graphql/resolvers/pipeline_security_report_findings_resolver.rb
@@ -50,19 +50,43 @@ def limit(args)
after = decode(args[:after])
before = decode(args[:before])
- first = context.schema.default_max_page_size if !first && !last
+ validate_pagination_args!(first: first, last: last, after: after, before: before)
- if first && last
- raise(Gitlab::Graphql::Errors::ArgumentError, "Can only provide either `first` or `last`, not both")
- end
+ page_size = first || last || context.schema.default_max_page_size
if after
- first + after
+ after + page_size
elsif before
before - 1
else
- first || last
+ page_size
+ end
+ end
+
+ def validate_pagination_args!(first:, last:, after:, before:)
+ if first && last
+ raise(Gitlab::Graphql::Errors::ArgumentError, "Can only provide either `first` or `last`, not both")
end
+
+ if after && before
+ raise(Gitlab::Graphql::Errors::ArgumentError, "Can only provide either `first` or `last`, not both")
+ end
+
+ if first && before
+ raise(
+ Gitlab::Graphql::Errors::ArgumentError,
+ "Behaviour of `first` and `before` in combination is undefined. " \
+ "Use either `first` + `after` or `last` + `before`."
+ )
+ end
+
+ return unless last && after
+
+ raise(
+ Gitlab::Graphql::Errors::ArgumentError,
+ "Behaviour of `last` and `after` in combination is undefined. " \
+ "Use either `first` + `after` or `last` + `before`."
+ )
end
# The :before and :after cursor arguments when used with keyset
diff --git a/ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb b/ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb
index 03be02ccf9d6d6..c0192b0ca54b95 100644
--- a/ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb
+++ b/ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb
@@ -153,6 +153,33 @@
end
end
+ context 'with :after and :before arguments' do
+ let(:pagination_arguments) { "after: \"#{encode('20')}\",before: \"#{encode('20')}\"" }
+
+ it 'returns an error' do
+ expect(security_report_findings).to be_blank
+ expect(graphql_errors).to be_present
+ end
+ end
+
+ context 'with :last and :after arguments' do
+ let(:pagination_arguments) { "last: 5,after: \"#{encode('5')}\"" }
+
+ it 'returns an error' do
+ expect(security_report_findings).to be_blank
+ expect(graphql_errors).to be_present
+ end
+ end
+
+ context 'with :first and :before arguments' do
+ let(:pagination_arguments) { "first: 5,before: \"#{encode('10')}\"" }
+
+ it 'returns an error' do
+ expect(security_report_findings).to be_blank
+ expect(graphql_errors).to be_present
+ end
+ end
+
# The before and after pagination cursors are base64 encoded
def encode(value)
GraphQL::Schema::Base64Encoder.encode(value.to_s)
--
GitLab