Skip to content

Add a new rubocop Database/DisableReferentialIntegrity

Steve Abrams requested to merge 358608-disable_referential_integrity-cop into master

🔬 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.

Related: #358608 (closed)

Edited by Steve Abrams

Merge request reports