Skip to content

Commit

Permalink
Deleting an item from the Middleware stack should raise if the item i…
Browse files Browse the repository at this point in the history
…s 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 #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.
  • Loading branch information
ghiculescu committed Jul 1, 2021
1 parent ef747e9 commit 07558ff
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 1 deletion.
7 changes: 7 additions & 0 deletions 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*
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_dispatch/middleware/stack.rb
Expand Up @@ -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)
Expand Down
6 changes: 6 additions & 0 deletions actionpack/test/dispatch/middleware_stack_test.rb
Expand Up @@ -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)
Expand Down

0 comments on commit 07558ff

Please sign in to comment.