From f8c4d504d28a4596ee4253a9f29e5f075962dcfe Mon Sep 17 00:00:00 2001 From: junglefish Date: Tue, 9 Jul 2024 18:03:08 +0900 Subject: [PATCH] Fix false negatives for implicit render or rescue blocks --- ...r_action_controller_flash_before_render.md | 1 + .../action_controller_flash_before_render.rb | 10 +- ...ion_controller_flash_before_render_spec.rb | 179 +++++++++++++++++- 3 files changed, 184 insertions(+), 6 deletions(-) create mode 100644 changelog/fix_false_negatives_for_action_controller_flash_before_render.md diff --git a/changelog/fix_false_negatives_for_action_controller_flash_before_render.md b/changelog/fix_false_negatives_for_action_controller_flash_before_render.md new file mode 100644 index 0000000000..6d253ae144 --- /dev/null +++ b/changelog/fix_false_negatives_for_action_controller_flash_before_render.md @@ -0,0 +1 @@ +* [#1311](https://github.com/rubocop/rubocop-rails/pull/1311): Fix false negatives for `Rails/ActionControllerFlashBeforeRender` when using implicit render or rescue blocks. ([@tldn0718][]) diff --git a/lib/rubocop/cop/rails/action_controller_flash_before_render.rb b/lib/rubocop/cop/rails/action_controller_flash_before_render.rb index 806e92e455..bf4602cdab 100644 --- a/lib/rubocop/cop/rails/action_controller_flash_before_render.rb +++ b/lib/rubocop/cop/rails/action_controller_flash_before_render.rb @@ -72,13 +72,13 @@ def followed_by_render?(flash_node) if (node = context.each_ancestor(:if, :rescue).first) return false if use_redirect_to?(context) - context = node - elsif context.right_siblings.empty? - return true + context = node.rescue_type? ? node.parent : node end - context = context.right_siblings - context.compact.any? do |render_candidate| + siblings = context.right_siblings + return true if siblings.empty? + + siblings.compact.any? do |render_candidate| render?(render_candidate) end end diff --git a/spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb b/spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb index 4704d07a29..0c8f682e16 100644 --- a/spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb +++ b/spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb @@ -128,9 +128,36 @@ def create end end RUBY + + expect_offense(<<~RUBY) + class HomeController < #{parent_class} + def create + flash[:alert] = "msg" if condition + ^^^^^ Use `flash.now` before `render`. + end + end + RUBY + + expect_correction(<<~RUBY) + class HomeController < #{parent_class} + def create + flash.now[:alert] = "msg" if condition + end + end + RUBY end it 'does not register an offense when using `flash` before `redirect_to`' do + expect_no_offenses(<<~RUBY) + class HomeController < #{parent_class} + def create + flash[:alert] = "msg" if condition + + redirect_to :index + end + end + RUBY + expect_no_offenses(<<~RUBY) class HomeController < #{parent_class} def create @@ -145,6 +172,16 @@ def create end it 'does not register an offense when using `flash` before `redirect_back`' do + expect_no_offenses(<<~RUBY) + class HomeController < #{parent_class} + def create + flash[:alert] = "msg" if condition + + redirect_back fallback_location: root_path + end + end + RUBY + expect_no_offenses(<<~RUBY) class HomeController < #{parent_class} def create @@ -158,7 +195,7 @@ def create RUBY end - it 'registers an offense when using `flash` in multiline `if` branch before `render_to`' do + it 'registers an offense when using `flash` in multiline `if` branch before `render`' do expect_offense(<<~RUBY) class HomeController < #{parent_class} def create @@ -185,6 +222,29 @@ def create end end RUBY + + expect_offense(<<~RUBY) + class HomeController < #{parent_class} + def create + if condition + do_something + flash[:alert] = "msg" + ^^^^^ Use `flash.now` before `render`. + end + end + end + RUBY + + expect_correction(<<~RUBY) + class HomeController < #{parent_class} + def create + if condition + do_something + flash.now[:alert] = "msg" + end + end + end + RUBY end it 'does not register an offense when using `flash` in multiline `if` branch before `redirect_to`' do @@ -217,6 +277,81 @@ def create end end RUBY + + expect_no_offenses(<<~RUBY) + class HomeController < #{parent_class} + def create + if condition + flash[:alert] = "msg" + redirect_to :index + + return + end + end + end + RUBY + end + + it 'registers an offense when using `flash` in multiline `rescue` branch before `render`' do + expect_offense(<<~RUBY) + class HomeController < #{parent_class} + def create + begin + do_something + flash[:alert] = "msg in begin" + ^^^^^ Use `flash.now` before `render`. + rescue + flash[:alert] = "msg in rescue" + ^^^^^ Use `flash.now` before `render`. + end + + render :index + end + end + RUBY + + expect_correction(<<~RUBY) + class HomeController < #{parent_class} + def create + begin + do_something + flash.now[:alert] = "msg in begin" + rescue + flash.now[:alert] = "msg in rescue" + end + + render :index + end + end + RUBY + + expect_offense(<<~RUBY) + class HomeController < #{parent_class} + def create + begin + do_something + flash[:alert] = "msg in begin" + ^^^^^ Use `flash.now` before `render`. + rescue + flash[:alert] = "msg in rescue" + ^^^^^ Use `flash.now` before `render`. + end + end + end + RUBY + + expect_correction(<<~RUBY) + class HomeController < #{parent_class} + def create + begin + do_something + flash.now[:alert] = "msg in begin" + rescue + flash.now[:alert] = "msg in rescue" + end + end + end + RUBY end it 'does not register an offense when using `flash` in multiline `rescue` branch before `redirect_to`' do @@ -235,6 +370,48 @@ def create end RUBY end + + it 'does not register an offense when using `flash` before `redirect_to` in `rescue` branch' do + expect_no_offenses(<<~RUBY) + class HomeController < #{parent_class} + def create + begin + do_something + flash[:alert] = "msg in begin" + redirect_to :index + + return + rescue + flash[:alert] = "msg in rescue" + redirect_to :index + + return + end + + render :index + end + end + RUBY + + expect_no_offenses(<<~RUBY) + class HomeController < #{parent_class} + def create + begin + do_something + flash[:alert] = "msg in begin" + redirect_to :index + + return + rescue + flash[:alert] = "msg in rescue" + redirect_to :index + + return + end + end + end + RUBY + end end end