Skip to content

Danger comment when using ActiveRecord bulk update methods

Drew Blessing requested to merge dblessing_danger_comment_bulk_db_actions into master

What does this MR do and why?

Related to Danger comment for usage of update_all/destroy_all (#418818 - closed) and FCL from Incident 16042 - [Manage::Auth] FCL for Incident 16042 (gitlab-com&2191 - closed)

Adds a new Danger plugin that will comment on a MR diff for any usage of .update, .delete, .update_all or .destroy_all. The comment's intention is to indicate to the author that this usage requires the full SQL query and query plan to be pasted in the MR description as well as to have a database review of the query. These bulk updates have the potential to either perform poorly, or to destroy data, if the wrong or unexpected conditions are present in the resulting database query. Similarly, using this methods is incompatible with CTEs and may cause bad/unexpected queries.

Why not Rubocop?

I considered a new cop, but cops are most useful for style considerations that should be fixed 100% of the time. In this case, it's not that we shouldn't use update_all or destroy_all. Just that we want some extra care around such usage. I felt Danger was a great tool for this case.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Drew Blessing

Merge request reports