Return 0 for projectCount when no associated projects are found (!65252) · Merge requests · GitLab.org / GitLab

Pedro Pombeiro (OOO from July 16th till Aug 7th) requested to merge pedropombeiro/334956-fix-nil-project-count into master Jul 01, 2021

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)

Edited Jul 02, 2021 by Pedro Pombeiro (OOO from July 16th till Aug 7th)

Return 0 for projectCount when no associated projects are found