Skip to content

Store mentions in after_save callback

Alexandru Croitor requested to merge trigger-store-mentions-in-after-save-callback into master

What does this MR do?

Attempt to ensure all Mentionables get their mentions saved to DB without worrying where/when/why is specific mentionable saved. Adding the functionality of storing mentions on after_save callback should hopefully ensure that.

We did start saving mentions to DB on production for everyone.

After running some checks I found out that we are missing on storing mentions for some epic notes. The logic behind the check is that if we are saving mentions for every new epic note and every updated epic note, then the number of epic notes that still have unsaved mentions, would be the same or decrease(if somebody edits an epic that was created before we deployed the code for saving mentions to production).

I also checked sentry for possible exceptions on saving data to epic_user_mentions table at DB level, but nothign suggested that https://sentry.gitlab.net/gitlab/gitlabcom/?query=user_mentions

Having that below are 2 queries that show that we are missing saving mentions for epic notes somewhere.

Feb 5, 7:11 PM: 10026 mention notes that need to be migrated

/chatops run explain SELECT “notes”.“id” FROM “notes” LEFT JOIN epic_user_mentions ON notes.id = epic_user_mentions.note_id WHERE note LIKE ‘%@%’ AND epic_user_mentions.epic_id IS NULL  AND notes.noteable_type = ‘Epic’

Merge Left Join  (cost=0.56..414.36 rows=1 width=4) (actual time=0.189..25.815 rows=10026 loops=1)
  Merge Cond: (notes.id = epic_user_mentions.note_id)
  Filter: (epic_user_mentions.epic_id IS NULL)
  Rows Removed by Filter: 1179
  Buffers: shared hit=7177
  ->  Index Only Scan using epic_mentions_temp_index on notes  (cost=0.29..345.59 rows=13549 width=4) (actual time=0.105..22.477 rows=11205 loops=1)
        Heap Fetches: 312
        Buffers: shared hit=6991
  ->  Index Scan using index_epic_user_mentions_on_note_id on epic_user_mentions  (cost=0.28..58.20 rows=2115 width=8) (actual time=0.083..1.093 rows=1184 loops=1)
        Buffers: shared hit=186
Planning time: 8.155 ms
Execution time: 26.418 ms

Feb 6, 10:57AM: 10038 mention notes that need to be migrated

/chatops run explain SELECT notes.id FROM notes LEFT JOIN epic_user_mentions ON notes.id = epic_user_mentions.note_id WHERE note LIKE ‘%@%’ AND epic_user_mentions.epic_id IS NULL  AND notes.noteable_type = ‘Epic’

Merge Left Join  (cost=0.56..429.89 rows=1 width=4) (actual time=2.742..49.734 rows=10038 loops=1)
  Merge Cond: (notes.id = epic_user_mentions.note_id)
  Filter: (epic_user_mentions.epic_id IS NULL)
  Rows Removed by Filter: 1210
  Buffers: shared hit=6968 read=297
  I/O Timings: read=22.317
  ->  Index Only Scan using epic_mentions_temp_index on notes  (cost=0.29..361.24 rows=13573 width=4) (actual time=1.579..42.280 rows=11248 loops=1)
        Heap Fetches: 397
        Buffers: shared hit=6801 read=278
        I/O Timings: read=17.932
  ->  Index Scan using index_epic_user_mentions_on_note_id on epic_user_mentions  (cost=0.28..59.30 rows=2171 width=8) (actual time=1.159..5.294 rows=1215 loops=1)
        Buffers: shared hit=167 read=19
        I/O Timings: read=4.385
Planning time: 7.774 ms
Execution time: 50.302 ms

Screenshots

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 Mayra Cabrera

Merge request reports