From 48a5f3c68ff70151132972fa5c2c1c5505d1bdc9 Mon Sep 17 00:00:00 2001 From: Alex Riedler Date: Mon, 27 Nov 2023 11:00:18 -0500 Subject: [PATCH 1/5] feat: disallow after-commit --- CHANGELOG.md | 4 ++++ Gemfile.lock | 2 +- .../cop/vendor/disallow_after_commit.rb | 23 +++++++++++++++++++ lib/rubocop/cop/vendor_cops.rb | 1 + lib/rubocop/vendor/version.rb | 2 +- manual/cops.md | 1 + manual/cops_vendor.md | 22 ++++++++++++------ .../cop/vendor/disallow_after_commit_spec.rb | 17 ++++++++++++++ 8 files changed, 63 insertions(+), 9 deletions(-) create mode 100644 lib/rubocop/cop/vendor/disallow_after_commit.rb create mode 100644 spec/rubocop/cop/vendor/disallow_after_commit_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index ebf7346..c59b91e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## 0.13.0 - 2023-11-27 +### Added +- Disallow `after_commit` as it has no transactional guarantee, switch to `before_commit`. + ## 0.12.1 - 2023-08-01 ### Fixed - Fixes for `ws-sdk` path injection rubocops diff --git a/Gemfile.lock b/Gemfile.lock index 3513e74..703ab31 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - rubocop-vendor (0.12.1) + rubocop-vendor (0.13.0) rubocop GEM diff --git a/lib/rubocop/cop/vendor/disallow_after_commit.rb b/lib/rubocop/cop/vendor/disallow_after_commit.rb new file mode 100644 index 0000000..74c39d0 --- /dev/null +++ b/lib/rubocop/cop/vendor/disallow_after_commit.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Vendor + class DisallowAfterCommit < Base + extend AutoCorrector + + NO_METH_MSG = "You should not use `%s` %s" + NO_METHS = { + after_commit: "generally you want `before_commit` as after_commit is non-transactional in guarentees of running", + }.freeze + + def on_send(node) + name = node.method_name + if NO_METHS.key?(name) + add_offense(node, message: format(NO_METH_MSG, old: name, desc: NO_METHS[name])) + end + end + end + end + end +end diff --git a/lib/rubocop/cop/vendor_cops.rb b/lib/rubocop/cop/vendor_cops.rb index 50df498..79b616a 100644 --- a/lib/rubocop/cop/vendor_cops.rb +++ b/lib/rubocop/cop/vendor_cops.rb @@ -4,6 +4,7 @@ module RuboCop end require_relative 'vendor/active_record_connection_execute' +require_relative 'vendor/disallow_after_commit' require_relative 'vendor/recursive_open_struct_gem' require_relative 'vendor/sidekiq_throttled_gem' require_relative 'vendor/recursive_open_struct_use' diff --git a/lib/rubocop/vendor/version.rb b/lib/rubocop/vendor/version.rb index fba8428..6a9cf0e 100644 --- a/lib/rubocop/vendor/version.rb +++ b/lib/rubocop/vendor/version.rb @@ -2,6 +2,6 @@ module RuboCop module Vendor - VERSION = '0.12.1' + VERSION = '0.13.0' end end diff --git a/manual/cops.md b/manual/cops.md index b363d39..7d6d75c 100644 --- a/manual/cops.md +++ b/manual/cops.md @@ -2,6 +2,7 @@ #### Department [Vendor](cops_vendor.md) * [Vendor/ActiveRecordConnectionExecute](cops_vendor.md#vendoractiverecordconnectionexecute) +* [Vendor/DisallowAfterCommit](cops_vendor.md#vendordisallowaftercommit) * [Vendor/RecursiveOpenStructGem](cops_vendor.md#vendorrecursiveopenstructgem) * [Vendor/RecursiveOpenStructUse](cops_vendor.md#vendorrecursiveopenstructuse) * [Vendor/RollbarInsideRescue](cops_vendor.md#vendorrollbarinsiderescue) diff --git a/manual/cops_vendor.md b/manual/cops_vendor.md index 49509f1..4a3c4ae 100644 --- a/manual/cops_vendor.md +++ b/manual/cops_vendor.md @@ -29,12 +29,20 @@ ApplicationRecord.connection.select_all('SELECT * FROM users') User.connection.select_all('SELECT * FROM users') ``` -## Vendor/RecursiveOpenStructGem +## Vendor/DisallowAfterCommit Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged --- | --- | --- | --- | --- Enabled | Yes | No | - | - +No documentation + +## Vendor/RecursiveOpenStructGem + +Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged +--- | --- | --- | --- | --- +Enabled | Yes | No | 0.1.0 | - + This cop flags uses of the recursive-open-struct gem. RecursiveOpenStruct inherits from OpenStruct, which is now officially discouraged to be used @@ -46,7 +54,7 @@ https://ruby-doc.org/stdlib-3.0.1/libdoc/ostruct/rdoc/OpenStruct.html#class-Open Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged --- | --- | --- | --- | --- -Enabled | Yes | No | - | - +Enabled | Yes | No | 0.1.0 | - This cop flags uses of RecursiveOpenStruct. RecursiveOpenStruct is a library used in the Wealthsimple ecosystem that is being phased out due to security issues. @@ -79,7 +87,7 @@ allow(test_double).to receive(:a).and_return('b') Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged --- | --- | --- | --- | --- -Enabled | Yes | No | - | - +Enabled | Yes | No | 0.1.0 | - This cop checks for Rollbar calls outside `rescue` blocks. @@ -205,7 +213,7 @@ Rollbar.error(exception, "Unable to sync account") Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged --- | --- | --- | --- | --- -Enabled | Yes | No | - | - +Enabled | Yes | No | 0.1.0 | - This cop flags uses of the sidekiq-throttled gem. @@ -220,7 +228,7 @@ https://wealthsimple.slack.com/archives/C19UB3HNZ/p1683721247371709 for more det Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged --- | --- | --- | --- | --- -Enabled | Yes | No | - | - +Enabled | Yes | No | 0.1.0 | - This cop flags uses of DryStruct without strict mode @@ -231,7 +239,7 @@ We want to enfore strict mode which will throw an error in that case. Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged --- | --- | --- | --- | --- -Enabled | Yes | No | - | - +Enabled | Yes | No | 0.12.0 | - This cop checks for `Ws::Service#get,patch,post,put,delete,...` usage where the array format is used, but it contains (probably not) intended slashes. @@ -251,7 +259,7 @@ Ws::AccountService.post(["test", "foo"]) Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged --- | --- | --- | --- | --- -Enabled | Yes | No | - | - +Enabled | Yes | No | 0.12.0 | - This cop checks for `Ws::Service#get,patch,post,put,delete,...` usage and suggests to use component based paths instead of using interpolated values that could be user input. diff --git a/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb b/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb new file mode 100644 index 0000000..909d225 --- /dev/null +++ b/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Vendor::DisallowAfterCommit, :config do + it 'registers an offense when using `after_commit` with no args' do + expect_offense(<<~RUBY) + after_commit + ^^^^^^^^^^^^ You should not use `after_commit` generally you want `before_commit` as after_commit is non-transactional in guarentees of running + RUBY + end + + it 'registers an offense when using `after_commit` with args' do + expect_offense(<<~RUBY) + after_commit :foo_bar + ^^^^^^^^^^^^^^^^^^^^^ You should not use `after_commit` generally you want `before_commit` as after_commit is non-transactional in guarentees of running + RUBY + end +end From 2e90cc1ddc505c2ac021fcdd1f3a6949c753a236 Mon Sep 17 00:00:00 2001 From: Alex Riedler Date: Mon, 27 Nov 2023 16:49:56 +0000 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Cassidy Scheffer --- lib/rubocop/cop/vendor/disallow_after_commit.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/rubocop/cop/vendor/disallow_after_commit.rb b/lib/rubocop/cop/vendor/disallow_after_commit.rb index 74c39d0..8ea76a6 100644 --- a/lib/rubocop/cop/vendor/disallow_after_commit.rb +++ b/lib/rubocop/cop/vendor/disallow_after_commit.rb @@ -6,9 +6,9 @@ module Vendor class DisallowAfterCommit < Base extend AutoCorrector - NO_METH_MSG = "You should not use `%s` %s" + NO_METH_MSG = "Do not not use `%s` %s" NO_METHS = { - after_commit: "generally you want `before_commit` as after_commit is non-transactional in guarentees of running", + after_commit: "use `before_commit` because after_commit is non-transactional an may not run", }.freeze def on_send(node) From 2ecc2297b1b82288629c9fd89ba48c7c5624a87c Mon Sep 17 00:00:00 2001 From: Alex Riedler Date: Mon, 27 Nov 2023 11:53:08 -0500 Subject: [PATCH 3/5] chore: bump tests --- spec/rubocop/cop/vendor/disallow_after_commit_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb b/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb index 909d225..52a2c86 100644 --- a/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb +++ b/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb @@ -4,14 +4,14 @@ it 'registers an offense when using `after_commit` with no args' do expect_offense(<<~RUBY) after_commit - ^^^^^^^^^^^^ You should not use `after_commit` generally you want `before_commit` as after_commit is non-transactional in guarentees of running + ^^^^^^^^^^^^ Do not not use `after_commit` use `before_commit` because after_commit is non-transactional an may not run RUBY end it 'registers an offense when using `after_commit` with args' do expect_offense(<<~RUBY) after_commit :foo_bar - ^^^^^^^^^^^^^^^^^^^^^ You should not use `after_commit` generally you want `before_commit` as after_commit is non-transactional in guarentees of running + ^^^^^^^^^^^^^^^^^^^^^ Do not not use `after_commit` use `before_commit` because after_commit is non-transactional an may not run RUBY end end From 86f2b636ebebacc3341f1559a5be414174e03e2f Mon Sep 17 00:00:00 2001 From: Alex Riedler Date: Wed, 29 Nov 2023 15:26:15 +0000 Subject: [PATCH 4/5] fix: switch to after_save since before_commit is not always allowed --- CHANGELOG.md | 2 +- lib/rubocop/cop/vendor/disallow_after_commit.rb | 2 +- spec/rubocop/cop/vendor/disallow_after_commit_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c59b91e..6e6c0b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## 0.13.0 - 2023-11-27 ### Added -- Disallow `after_commit` as it has no transactional guarantee, switch to `before_commit`. +- Disallow `after_commit` as it has no transactional guarantee, switch to `after_save`. ## 0.12.1 - 2023-08-01 ### Fixed diff --git a/lib/rubocop/cop/vendor/disallow_after_commit.rb b/lib/rubocop/cop/vendor/disallow_after_commit.rb index 8ea76a6..8778b8c 100644 --- a/lib/rubocop/cop/vendor/disallow_after_commit.rb +++ b/lib/rubocop/cop/vendor/disallow_after_commit.rb @@ -8,7 +8,7 @@ class DisallowAfterCommit < Base NO_METH_MSG = "Do not not use `%s` %s" NO_METHS = { - after_commit: "use `before_commit` because after_commit is non-transactional an may not run", + after_commit: "use `after_save` because `after_commit` is non-transactional an may not run", }.freeze def on_send(node) diff --git a/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb b/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb index 52a2c86..4d46cc7 100644 --- a/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb +++ b/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb @@ -4,14 +4,14 @@ it 'registers an offense when using `after_commit` with no args' do expect_offense(<<~RUBY) after_commit - ^^^^^^^^^^^^ Do not not use `after_commit` use `before_commit` because after_commit is non-transactional an may not run + ^^^^^^^^^^^^ Do not not use `after_commit` use `after_save` because after_commit is non-transactional an may not run RUBY end it 'registers an offense when using `after_commit` with args' do expect_offense(<<~RUBY) after_commit :foo_bar - ^^^^^^^^^^^^^^^^^^^^^ Do not not use `after_commit` use `before_commit` because after_commit is non-transactional an may not run + ^^^^^^^^^^^^^^^^^^^^^ Do not not use `after_commit` use `after_save` because after_commit is non-transactional an may not run RUBY end end From bc8f9ed246b3d3befefc67c136c28b211b17b397 Mon Sep 17 00:00:00 2001 From: Alex Riedler Date: Wed, 29 Nov 2023 10:27:05 -0500 Subject: [PATCH 5/5] chore: fix test suite --- spec/rubocop/cop/vendor/disallow_after_commit_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb b/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb index 4d46cc7..582766c 100644 --- a/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb +++ b/spec/rubocop/cop/vendor/disallow_after_commit_spec.rb @@ -4,14 +4,14 @@ it 'registers an offense when using `after_commit` with no args' do expect_offense(<<~RUBY) after_commit - ^^^^^^^^^^^^ Do not not use `after_commit` use `after_save` because after_commit is non-transactional an may not run + ^^^^^^^^^^^^ Do not not use `after_commit` use `after_save` because `after_commit` is non-transactional an may not run RUBY end it 'registers an offense when using `after_commit` with args' do expect_offense(<<~RUBY) after_commit :foo_bar - ^^^^^^^^^^^^^^^^^^^^^ Do not not use `after_commit` use `after_save` because after_commit is non-transactional an may not run + ^^^^^^^^^^^^^^^^^^^^^ Do not not use `after_commit` use `after_save` because `after_commit` is non-transactional an may not run RUBY end end