Return 0 for projectCount when no associated projects are found
What does this MR do?
This MR fixes the logic in RunnerType#project_count
so that it returns:
- number of projects if the runner is associated with projects
- 0 if the runner is a project type runner but not associated with any projects (this is the part that was broken)
- nil if the runner is not associated with any projects
Test run on old code
expected {"jobCount"=>0, "projectCount"=>nil} to match (a hash including {"jobCount" => 0, "projectCount" => 0})
Diff:
@@ -1,2 +1,3 @@
-(a hash including {"jobCount" => 0, "projectCount" => 0})
+"jobCount" => 0,
+"projectCount" => nil,
0) Query.runner(id) for multiple runners requesting project and job counts retrieves expected fields
Failure/Error:
expect(runner2_data).to match a_hash_including(
'jobCount' => 0,
'projectCount' => 0)
expected {"jobCount"=>0, "projectCount"=>nil} to match (a hash including {"jobCount" => 0, "projectCount" => 0})
Diff:
@@ -1,2 +1,3 @@
-(a hash including {"jobCount" => 0, "projectCount" => 0})
+"jobCount" => 0,
+"projectCount" => nil,
# ./spec/requests/api/graphql/ci/runner_spec.rb:157:in `block (4 levels) in <top (required)>'
# ./spec/spec_helper.rb:387:in `block (3 levels) in <top (required)>'
# ./spec/support/sidekiq_middleware.rb:9:in `with_sidekiq_server_middleware'
# ./spec/spec_helper.rb:378:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:374:in `block (3 levels) in <top (required)>'
# ./lib/gitlab/application_context.rb:31:in `with_raw_context'
# ./spec/spec_helper.rb:374:in `block (2 levels) in <top (required)>'
Database query plans
There was a change in the project_count
query.
Old query plan
Query plan: https://gitlab.slack.com/archives/CLJMDRD8C/p1625206122382500
SELECT
"ci_runner_projects"."runner_id",
COUNT(*) AS count
FROM
"ci_runner_projects"
WHERE
"ci_runner_projects"."runner_id" IN (38, 37, 24, 2)
GROUP BY
"ci_runner_projects"."runner_id"
Aggregate (cost=0.43..7.89 rows=6 width=12) (actual time=10.254..10.255 rows=0 loops=1)
Group Key: ci_runner_projects.runner_id
Buffers: shared hit=12 read=3
I/O Timings: read=10.157 write=0.000
-> Index Only Scan using index_ci_runner_projects_on_runner_id on public.ci_runner_projects (cost=0.43..7.80 rows=6 width=4) (actual time=10.250..10.251 rows=0 loops=1)
Index Cond: (ci_runner_projects.runner_id = ANY ('{38,37,24,2}'::integer[]))
Heap Fetches: 0
Buffers: shared hit=12 read=3
I/O Timings: read=10.157 write=0.000
New query plan
Query plan: https://gitlab.slack.com/archives/CLJMDRD8C/p1625206131385100
SELECT
"ci_runners"."id",
COUNT(ci_runner_projects.id) AS count
FROM
"ci_runners"
LEFT OUTER JOIN "ci_runner_projects" ON "ci_runner_projects"."runner_id" = "ci_runners"."id"
WHERE
"ci_runners"."runner_type" = 3
AND "ci_runners"."id" IN (38, 37, 24, 2)
GROUP BY
"ci_runners"."id"
Aggregate (cost=0.85..18.53 rows=2 width=12) (actual time=0.045..0.046 rows=0 loops=1)
Group Key: ci_runners.id
Buffers: shared hit=15
I/O Timings: read=0.000 write=0.000
-> Nested Loop Left Join (cost=0.85..18.50 rows=2 width=8) (actual time=0.044..0.044 rows=0 loops=1)
Buffers: shared hit=15
I/O Timings: read=0.000 write=0.000
-> Index Scan using ci_runners_pkey on public.ci_runners (cost=0.43..11.59 rows=2 width=4) (actual time=0.043..0.043 rows=0 loops=1)
Index Cond: (ci_runners.id = ANY ('{38,37,24,2}'::integer[]))
Filter: (ci_runners.runner_type = 3)
Rows Removed by Filter: 0
Buffers: shared hit=15
I/O Timings: read=0.000 write=0.000
-> Index Scan using index_ci_runner_projects_on_runner_id on public.ci_runner_projects (cost=0.43..3.45 rows=1 width=8) (actual time=0.000..0.000 rows=0 loops=0)
Index Cond: (ci_runner_projects.runner_id = ci_runners.id)
I/O Timings: read=0.000 write=0.000
Does this MR meet the acceptance criteria?
Conformity
-
I have included changelog trailers, or none are needed. (Does this MR need a changelog?) -
I have added/updated documentation, or it's not needed. (Is documentation required?) -
I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?) -
I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) -
I have self-reviewed this MR per code review guidelines. -
This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines) -
I have followed the style guides. -
This change is backwards compatible across updates, or this does not apply.
Availability and Testing
-
I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.) -
I have tested this MR in all supported browsers, or it's not needed. -
I have informed the Infrastructure department of a default or new setting change per definition of done, or it's not needed.
Security
Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Closes #334956 (closed)