From 07558ff57c32ac96ec75d744e304944c36282b1b Mon Sep 17 00:00:00 2001 From: Alex Ghiculescu Date: Wed, 30 Jun 2021 12:36:16 -0500 Subject: [PATCH] Deleting an item from the Middleware stack should raise if the item is not found Currently if you call any `move` operation on the middleware stack with a middleware item that doesn't exist, [it will raise](https://github.com/rails/rails/blob/53d54694e9a423642ba9c984207097e9522db26f/actionpack/test/dispatch/middleware_stack_test.rb#L102). But the same doesn't happen if you call `delete` with a non-existant item. This makes it hard to debug issues like https://github.com/rails/rails/pull/42652 as the `delete` call fails silently. I think `delete` should raise same as `move` does, and this PR implements that. This would be a breaking change if someone has code calling `delete` on a non-existant middleware item, the fix would be to just remove that code since it's not doing anything. --- actionpack/CHANGELOG.md | 7 +++++++ actionpack/lib/action_dispatch/middleware/stack.rb | 2 +- actionpack/test/dispatch/middleware_stack_test.rb | 6 ++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 795f8164c2a2b..1bc5ab1c3e39b 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,10 @@ +* Deleting an item from the Middleware stack will raise if the item is not found + + Previously, calling `config.middleware.delete(ItemNotInMiddleware)` would fail silently. + Now it will raise, same as `config.middleware.move(0, ItemNotInMiddleware)` does. + + *Alex Ghiculescu* + * OpenSSL constants are now used for Digest computations. *Dirkjan Bussink* diff --git a/actionpack/lib/action_dispatch/middleware/stack.rb b/actionpack/lib/action_dispatch/middleware/stack.rb index 2e1f21d5cc28c..3a29745e55ba3 100644 --- a/actionpack/lib/action_dispatch/middleware/stack.rb +++ b/actionpack/lib/action_dispatch/middleware/stack.rb @@ -130,7 +130,7 @@ def swap(target, *args, &block) ruby2_keywords(:swap) def delete(target) - middlewares.delete_if { |m| m.name == target.name } + middlewares.reject! { |m| m.name == target.name } || (raise "No such middleware to delete: #{target.inspect}") end def move(target, source) diff --git a/actionpack/test/dispatch/middleware_stack_test.rb b/actionpack/test/dispatch/middleware_stack_test.rb index 52c40511d94ca..15e6c0d286c9e 100644 --- a/actionpack/test/dispatch/middleware_stack_test.rb +++ b/actionpack/test/dispatch/middleware_stack_test.rb @@ -105,6 +105,12 @@ def test_delete_works end end + test "delete requires the middleware to be in the stack" do + assert_raises RuntimeError do + @stack.delete(BazMiddleware) + end + end + test "move preserves the arguments of the moved middleware" do @stack.use BazMiddleware, true, foo: "bar" @stack.move_before(FooMiddleware, BazMiddleware)