Fix #400 Add Rails/TransactionRequiresNew cop #414

the-wendell · 2021-01-05T08:49:12Z

Replace this text with a summary of the changes in your PR.
The more detailed you are, the better.

Before submitting the PR make sure the following are checked:

Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.
Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.
If this is a new cop, consider making a corresponding update to the Rails Style Guide.

koic · 2021-01-05T09:14:44Z

config/default.yml

@@ -695,6 +694,11 @@ Rails/TimeZone:
    - strict
    - flexible

+Rails/TransactionRequiresNew:
+  Description: 'When using a transaction requires that the argument `requires_new: true` is passed.'
+  Enabled: true


As far as I know, it's not a common rule. I think it should be Enable: false by default.

koic · 2021-01-05T09:15:53Z

config/default.yml

+Rails/TransactionRequiresNew:
+  Description: 'When using a transaction requires that the argument `requires_new: true` is passed.'
+  Enabled: true
+  VersionAdded: '2.10'


I think this auto-correction will be false because the behavior will change.

SafeAutoCorrect: false

koic · 2021-01-05T09:18:15Z

lib/rubocop/cop/rails/transaction_requires_new.rb

+          (block
+            (send _ :transaction ...)
+            (args) {_})
+        PATTERN


Can you replace RESTRICT_ON_SEND = %i[transaction].freeze?

koic · 2021-01-05T09:19:18Z

spec/rubocop/cop/rails/transaction_requires_new_spec.rb

+
+      # does NOT auto-correct when arguments are present
+      expect_correction(<<~RUBY)
+        User.transaction(some_option: true) do


koic · 2021-01-05T09:19:40Z

spec/rubocop/cop/rails/transaction_requires_new_spec.rb

+
+      # Does NOT auto-correct when arugments are present
+      expect_correction(<<~RUBY)
+        ActiveRecord::Base.transaction(some_option: false) do


I think it can be auto-corrected for this offense.

Yes, I suppose it can be. I imagine that it will be safe enough to simply add requires_new: true as the last argument in a transaction.

koic · 2021-01-05T09:23:39Z

docs/modules/ROOT/pages/cops_rails.adoc

+end
+
+# good
+ActiveRecord::Base.transaction(requires_new: true) do


Is it okay not to specify joinable: false together?

My understanding is that they work independently. requires_new: true is for custom tranaction (transaction do ... end) and joinable: false is for the transactions created by ActiveRecord methods such as update!

Anyways, I will do some research and testing on this. I have read many blogs/articles about it so far but none of them seem super clear on this topic. I will try to figure it out and come up with some good documentation and examples. I might even write a medium article 😂

koic · 2021-01-05T09:25:13Z

lib/rubocop/cop/rails/transaction_requires_new.rb

+          return unless transaction?(node) && !requires_new_argument_passed?(node)
+
+          if arguments_present?(node)
+            add_offense(node)


Perhaps it is possible to add_offense whenever it will be auto-correction :-)

koic · 2021-01-05T09:44:54Z

I have never used transaction(requires_new: true) of Active Record and have not been able to read and survey https://makandracards.com/makandra/42885-nested-activerecord-transaction-pitfalls.
(I remember a little about what I experienced at J2EE a long time ago :-)

I think that the following investigations are probably needed for cop documentation and implementation.

If there is an option that conflicts with transaction(requires_new: true), this cop will not offense for it when the conflict option is specified
About the background to use with joinable: false
Trade-offs when using transaction(requires_new: true). I think there is a reason why transaction(requires_new: true) is not default in Rails.

There may still be others...

Anyway, I think it is difficult for users to understand and use it if the information for using this cop is not documented. Story of transactions is complicated 😅

koic · 2021-01-05T09:48:52Z

CHANGELOG.md

@@ -4,6 +4,7 @@

 ### Changes

+* [#410](https://github.com/rubocop-hq/rubocop-rails/pull/410): Add new`Rails/TransactionRequiresNew` cop. ([@the-wendell][])


This entry belongs to the ### New features category. Can you create category and move to it?

koic · 2021-01-05T09:52:53Z

config/default.yml

@@ -695,6 +694,11 @@ Rails/TimeZone:
    - strict
    - flexible

+Rails/TransactionRequiresNew:
+  Description: 'When using a transaction requires that the argument `requires_new: true` is passed.'


I think the reference will help users.

Reference: https://makandracards.com/makandra/42885-nested-activerecord-transaction-pitfalls

the-wendell · 2021-01-07T11:51:42Z

I have never used transaction(requires_new: true) of Active Record and have not been able to read and survey https://makandracards.com/makandra/42885-nested-activerecord-transaction-pitfalls.
(I remember a little about what I experienced at J2EE a long time ago :-)

I think that the following investigations are probably needed for cop documentation and implementation.
* If there is an option that conflicts with `transaction(requires_new: true)`, this cop will not offense for it when the conflict option is specified

* About the background to use with `joinable: false`

* Trade-offs when using `transaction(requires_new: true)`. I think there is a reason why `transaction(requires_new: true)` is not default in Rails.
There may still be others...

Anyway, I think it is difficult for users to understand and use it if the information for using this cop is not documented. Story of transactions is complicated sweat_smile

I will do some more research and come up with some examples to add to the documentation to make it more clear. As well, I will clarify the appropriate use of joinable: false. I think it might make sense to allow the cop to be configured for either requires_new: true or joinable: false or both.

Where should detailed documentation/explanations about the cop go? With the examples? (This is my first attempted contribution for any sort of rubocop rule)

koic · 2021-01-12T01:38:31Z

Where should detailed documentation/explanations about the cop go? With the examples?

Please describe it in the class comment of cop. The following is an example:
https://github.com/rubocop-hq/rubocop-rails/blob/v2.9.1/lib/rubocop/cop/rails/unique_validation_without_index.rb#L6-L15

stokarenko · 2021-03-06T19:52:22Z

@koic @the-wendell

There is an example right from a months of debugging and frustration:

class UserAssinment
  class << self
    def create_progress!
      Progress.transaction do
         Progress.create!
         raise 'API error'
      end
    end

    def assign!
      create_progress!
    rescue => e
      logger.warn e
    end
  end
end

UserAssinment.create_progress!
p Progress.count # 0 as expected

UserAssinment.assign!
p Progress.count # 0 as expected

UserAssinment.transaction { UserAssinment.assign! }
p Progress.count # 1 surprise!

The point is that only the root transaction is a real transaction, any nested one is just an illusion by AR defaults. So exception blows the nested transaction block, but going to be rescued upper the stack and never hit the root transaction. So - root transaction will never be rolled back and will commit the part of orphaned queries above the exception. Even worse, the behavior depends on the depth of nested transaction and may (and will) be blinking across refactorings and service classes which call another service classes.

stokarenko · 2021-03-06T22:58:05Z

lib/rubocop/cop/rails/transaction_requires_new.rb

+            add_offense(node)
+          else
+            add_offense(node) do |corrector|
+              corrector.replace(node, node.source.gsub('.transaction', '.transaction(requires_new: true)'))


transaction do may be called as own Model method and without a dot at the beginning. Cop is catching such calls while auto-correct is running into infinitive loop

Suggested change

corrector.replace(node, node.source.gsub('.transaction', '.transaction(requires_new: true)'))

corrector.replace(node, node.source.gsub('transaction', 'transaction(requires_new: true)'))

Anyway this gsub is wide enough and becomes to be even wider with suggestion above, affecting the whole block including comments, nested method calls like

myvar.transactional_user_actions(foo: :bar) # before myvar.transaction(requires_new: true)al_user_actions(foo: :bar) # after

etc

the-wendell added 2 commits January 5, 2021 09:48

Fix rubocop#400 Add Rails/TransactionRequiresNew cop

a595d33

Add link to CHANGELOG.md

a0eac67

koic reviewed Jan 5, 2021

View reviewed changes

stokarenko reviewed Mar 6, 2021

View reviewed changes

koic force-pushed the master branch from 45247fb to 6e31622 Compare January 9, 2022 20:57

the-wendell closed this Jun 8, 2022

pirj mentioned this pull request Feb 11, 2024

Add new Rails/PrivateTransactionOption cop #1236

Open

9 tasks

pirj mentioned this pull request Jul 26, 2024

Add "Nested Transactions" section rubocop/rails-style-guide#355

Open

		@@ -4,6 +4,7 @@

		### Changes

		* [#410](https://github.com/rubocop-hq/rubocop-rails/pull/410): Add new`Rails/TransactionRequiresNew` cop. ([@the-wendell][])

	corrector.replace(node, node.source.gsub('.transaction', '.transaction(requires_new: true)'))
	corrector.replace(node, node.source.gsub('transaction', 'transaction(requires_new: true)'))

Uh oh!

Fix #400 Add Rails/TransactionRequiresNew cop #414

Fix #400 Add Rails/TransactionRequiresNew cop #414

Uh oh!

Conversation

the-wendell commented Jan 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koic commented Jan 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

the-wendell commented Jan 7, 2021

Uh oh!

koic commented Jan 12, 2021

Uh oh!

stokarenko commented Mar 6, 2021

Uh oh!

stokarenko Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stokarenko Mar 6, 2021 •

edited

Loading