Skip to content

Commit 07558ff

Browse files
committed
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 rails#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.
1 parent ef747e9 commit 07558ff

File tree

3 files changed

+14
-1
lines changed

3 files changed

+14
-1
lines changed

actionpack/CHANGELOG.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,10 @@
1+
* Deleting an item from the Middleware stack will raise if the item is not found
2+
3+
Previously, calling `config.middleware.delete(ItemNotInMiddleware)` would fail silently.
4+
Now it will raise, same as `config.middleware.move(0, ItemNotInMiddleware)` does.
5+
6+
*Alex Ghiculescu*
7+
18
* OpenSSL constants are now used for Digest computations.
29

310
*Dirkjan Bussink*

actionpack/lib/action_dispatch/middleware/stack.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ def swap(target, *args, &block)
130130
ruby2_keywords(:swap)
131131

132132
def delete(target)
133-
middlewares.delete_if { |m| m.name == target.name }
133+
middlewares.reject! { |m| m.name == target.name } || (raise "No such middleware to delete: #{target.inspect}")
134134
end
135135

136136
def move(target, source)

actionpack/test/dispatch/middleware_stack_test.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,12 @@ def test_delete_works
105105
end
106106
end
107107

108+
test "delete requires the middleware to be in the stack" do
109+
assert_raises RuntimeError do
110+
@stack.delete(BazMiddleware)
111+
end
112+
end
113+
108114
test "move preserves the arguments of the moved middleware" do
109115
@stack.use BazMiddleware, true, foo: "bar"
110116
@stack.move_before(FooMiddleware, BazMiddleware)

0 commit comments

Comments
 (0)