Skip to content

Disallow disable_ddl_transaction! with only validate_foreign_key

João Alexandre Cunha requested to merge 388967-update-docs-to-suggest-disabling-transactions-for-constraint-validation into master

What does this MR do?

In migrations to validate FKs, it's useless to add disable_ddl_transaction!, PostgreSQL actually adds an implicit transaction for single statements, as discussed here: !109824 (comment 1251147922)

To educate about this behaviour and to avoid future bike-shedding and possible extra review cycles, we're adding this cop to simply choose one of the options (add or not add disable_ddl_transaction!). In this case - not to add disable_ddl_transaction! when the only method being called is validate_foreign_key.

The cop logic is:

  1. Detects disable_ddl_transaction! on the main code node.
  2. For each def node, which is one of [:up, :down, :change], detects if validate_foreign_key was used the only method used.
  3. If 1 and 2 are true, then it raises the offence, asking the developer to remove disable_ddl_transaction!.

Related issues

Closes #388967 (closed)

Test

New offence detected on an existing migration: https://gitlab.com/gitlab-org/gitlab/-/jobs/3685865694

Then fixed on the following commit.

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 João Alexandre Cunha

Merge request reports