Skip to content

Fix broken todo GraphQL API filtering when filtering by type

Allison Browne requested to merge graphql-bug-fix-get-todo-by-type into master

What does this MR do?

Graphql todo fetching was not filtering properly by a list of target_type. The TodoFinder support for an array of types was broken. The finder would always return all types.

The spec was working because there was no data that should have been filtered out by the given example in both the graphql resolver and the TodoFinder. Adding an Design todo to the example caused it to fail:

Failures:

  1) TodosFinder#execute #execute filtering returns correct todos when filtered by a group
     Failure/Error: expect(todos).to match_array([todo1, todo2])

       expected collection contained:  [#<Todo id: 1, user_id: 1, project_id: 1, target_id: 1, target_type: "Issue", author_id: 2, action: 1...ated_at: "2020-06-18 11:08:19", note_id: nil, commit_id: nil, group_id: 1, resolved_by_action: nil>]
       actual collection contained:    [#<Todo id: 1, user_id: 1, project_id: 1, target_id: 1, target_type: "Issue", author_id: 2, action: 1...ated_at: "2020-06-18 11:08:19", note_id: nil, commit_id: nil, group_id: 1, resolved_by_action: nil>]
       the extra elements were:        [#<Todo id: 3, user_id: 1, project_id: 3, target_id: 1, target_type: "DesignManagement::Design", auth...ated_at: "2020-06-18 11:08:19", note_id: nil, commit_id: nil, group_id: 1, resolved_by_action: nil>]

The todo finder now properly supports passing either a single target_type or an array.

See example query:

{
  currentUser {
    todos(type: [MERGEREQUEST, ISSUE]) {
      nodes {
        id,
        targetType
      }
    }
  }
}

Explain plans

For a user with 62141 todos:

explain SELECT "todos".* 
FROM "todos" 
WHERE "todos"."user_id" = 2503633 
AND ("todos"."state" IN ('pending')) 
AND "todos"."target_type" = 'Issue'
ORDER BY "todos"."id" DESC 
LIMIT 100

Time: 0.558 ms
  - planning: 0.242 ms
  - execution: 0.316 ms
    - I/O read: 0.000 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 103 (~824.00 KiB) from the buffer pool
  - reads: 0 from the OS file cache, including disk I/O
  - dirtied: 0
  - writes: 0

mit  (cost=0.43..212.04 rows=100 width=106)
  ->  Index Scan Backward using index_todos_on_user_id_and_id_pending on todos  (cost=0.43..17303.61 rows=8177 width=106)
        Index Cond: (user_id = 2503633)
        Filter: ((target_type)::text = ''::text)

 Limit  (cost=0.43..355.30 rows=100 width=106) (actual time=0.039..0.277 rows=100 loops=1)
   Buffers: shared hit=103
   ->  Index Scan using index_todos_on_user_id_and_id_pending on public.todos  (cost=0.43..17303.61 rows=4876 width=106) (actual time=0.037..0.261 rows=100 loops=1)
         Index Cond: (todos.user_id = 2503633)
         Filter: ((todos.target_type)::text = 'Issue'::text)
         Rows Removed by Filter: 0
         Buffers: shared hit=103

Screenshots

description screenshot
todo ui Screen_Shot_2020-06-18_at_9.05.43_AM
qraphql explorer Screen_Shot_2020-06-18_at_9.08.07_AM

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] 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
Edited by Allison Browne

Merge request reports