Skip to content

Blacklist the use of destroy_all

Yorick Peterse requested to merge blacklist-destroy-all into master

What does this MR do?

This merge request adds a RuboCop cop that blacklists the use of .destroy_all, except for existing offences.

destroy_all is similar to dependent: :destroy, and blacklisted for the same reason: it loads every row into memory, then issues a DELETE for every individual row. This in turn can be a disaster for both performance and availability. This lead to us blacklisting dependent: :destroy in the past, and with this MR we're applying the same pattern to destroy_all.

Sometimes a model may use a set of hooks to remove non DB data. Such logic should be moved outside of a model as this approach won't work for bulk removals. This is already documented in https://docs.gitlab.com/ee/development/foreign_keys.html#dependent-removals.

By adding this cop, database engineers (or other reviewers for that matter) no longer have to remind MR authors to avoid the use of destroy_all.

EE MR: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6914

Does this MR meet the acceptance criteria?

Edited by Yorick Peterse

Merge request reports