Skip to content

Raise error if transaction block is exited in dev/test envs

Peter Leitzen requested to merge pl-raise-exit-transation-warning into master

What does this MR do and why?

This MR turns the following Rails deprecation warning into an error on development/test:

DEPRECATION WARNING: Using `return`, `break` or `throw` to exit a transaction block is
deprecated without replacement. If the `throw` came from
`Timeout.timeout(duration)`, pass an exception class as a second
argument so it doesn't use `throw` to abort its block. This results
in the transaction being committed, but in the next release of Rails

It also enables a 🆕 👮 Rails/TransactionExitStatement and fixes all offenses (6) and instances not caught by this cop.

The generated TODOs are empty but it's still worth to put this cop in "grace period" to maintain master stability.

See #410086 (closed).

The fixes

According to https://docs.rubocop.org/rubocop-rails/cops_rails.html#railstransactionexitstatement

As alternatives, it would be more intuitive to explicitly raise an error when rollback is desired, and to use next when commit is desired.

In this MR we use next instead of break and return where appropriate.

How to verify locally

bin/rspec --order defined spec/services/projects/create_service_spec.rb -e "tracks for imported project"

Before

Passes with deprecation warning

Projects::CreateService#execute
  import data
DEPRECATION WARNING: Using `return`, `break` or `throw` to exit a transaction block is
deprecated without replacement. If the `throw` came from
`Timeout.timeout(duration)`, pass an exception class as a second
argument so it doesn't use `throw` to abort its block. This results
in the transaction being committed, but in the next release of Rails
it will rollback.
 (called from public_send at /home/peter/devel/gitlab/gdk/gitlab/lib/gitlab/database/load_balancing/connection_proxy.rb:127)
    tracks for imported project

After

Fails with mysterious error.

Failures:

  1) Projects::CreateService#execute import data tracks for imported project
     Failure/Error:
       expect(Gitlab::Tracking).to have_received(tracking_method) # rubocop:disable RSpec/ExpectGitlabTracking
         .with(category, action, **kwargs).at_least(:once)

       (Gitlab::Tracking).event("Projects::CreateService", "import_project", {:user=>#<User id:314 @user1>})
           expected: at least 1 time with arguments: ("Projects::CreateService", "import_project", {:user=>#<User id:314 @user1>})
           received: 0 times
     # ./spec/support/helpers/snowplow_helpers.rb:63:in `expect_snowplow_event'
     # ./spec/services/projects/create_service_spec.rb:477:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:419:in `block (3 levels) in <top (required)>'
     # ./spec/support/sidekiq_middleware.rb:18:in `with_sidekiq_server_middleware'
     # ./spec/spec_helper.rb:411:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:407:in `block (3 levels) in <top (required)>'
     # ./lib/gitlab/application_context.rb:61:in `with_raw_context'
     # ./spec/spec_helper.rb:407:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:242:in `block (2 levels) in <top (required)>'
     # ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
     # ./spec/support/database/prevent_cross_joins.rb:108:in `block (3 levels) in <main>'
     # ./spec/support/database/prevent_cross_joins.rb:62:in `with_cross_joins_prevented'
     # ./spec/support/database/prevent_cross_joins.rb:108:in `block (2 levels) in <main>'

Finished in 7.71 seconds (files took 11.1 seconds to load)
1 example, 1 failure

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 Peter Leitzen

Merge request reports