Add a new rubocop Database/DisableReferentialIntegrity
🔬 What does this MR do and why?
disable_referential_integrity
allows a developer to disable all triggers on all tables in the database. This can lead to data consistency problems as described in this PostgreSQL thread.
There are two problems with this method:
- Triggers are disabled for all tables even though we may only need to disable them for one table
- The disabling of triggers occurs in a separate database transaction from the query, which presents an opportunity for rollback problem.
This MR adds a new rubocop rule to prevent usage of this method.
While it seems this would be reasonable to prevent in all rails projects, I opted not to add this cop to https://gitlab.com/gitlab-org/ruby/gems/gitlab-styles since I'm not sure if there might be good reasons to use it in database-specific projects. I'm open to moving it to the gitlab-styles
gem if that makes more sense.
📹 Screenshots or screen recordings
N/A
🎤 How to set up and validate locally
You can remove the file listed in the rubocop TODO file and run rubocop:
bundle exec rubocop spec/lib/gitlab/background_migration/cleanup_orphaned_lfs_objects_projects_spec.rb
Inspecting 1 file
C
Offenses:
spec/lib/gitlab/background_migration/cleanup_orphaned_lfs_objects_projects_spec.rb:31:5: C: Database/DisableReferentialIntegrity: Do not use disable_referential_integrity, disable triggers in a safe
transaction instead. Follow the format:
BEGIN;
ALTER TABLE my_table DISABLE TRIGGER ALL;
-- execute query that requires disabled triggers
ALTER TABLE my_table ENABLE TRIGGER ALL;
COMMIT;
ActiveRecord::Base.connection.disable_referential_integrity do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1 file inspected, 1 offense detected
🛃 MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related: #358608 (closed)