Delayed middleware delete does not allow move operations

While trying to fix #16433, we made the middleware deletions always
happen at the end. While this works for the case of deleting the
Rack::Runtime middleware, it makes operations like the following
misbehave.

```ruby
gem "bundler", "< 1.16"

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails"
end

require "action_controller/railtie"

class TestApp < Rails::Application
  config.root = __dir__
  secrets.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  middleware.insert_after ActionDispatch::Session::CookieStore, ::Rails::Rack::Logger, config.log_tags
  middleware.delete ::Rails::Rack::Logger
end

require "minitest/autorun"
require "rack/test"

class BugTest < Minitest::Test
  include Rack::Test::Methods

  def test_returns_success
    get "/"
    assert last_response.ok?
  end

  private
    def app
      Rails.application
    end
end
```

In the case ☝️  the ::Rails::Rack::Logger would be deleted instead of
moved, because the order of middleware stack building execution will be:

```ruby
[:insert, ActionDispatch::Session::CookieStore, [::Rails::Rack::Logger]]
[:delete, ::Rails::Rack::Logger, [config.log_tags]]
```

This is pretty surprising and hard to reason about behaviour, unless you
go spelunking into the Rails configuration code.

I have a few solutions in mind and all of them have their drawbacks.

1. Introduce a `Rails::Configuration::MiddlewareStackProxy#delete!` that
delays the deleted operations. This will make `#delete` to be executed
in order. The drawback here is backwards incompatible behavior and a new
public method.

2. Just revert to the old operations. This won't allow people to delete
the `Rack::Runtime` middleware.

3. Legitimize the middleware moving with the new `#move_after` and
`#move_before` methods. This does not breaks any backwards
compatibility, but includes 2 new methods to the middleware stack.

I have implemented `3.` in this pull request.

Happy holidays! 🎄

actionpack/lib/action_dispatch/middleware/stack.rb

@@ -122,6 +122,24 @@ module ActionDispatch
       middlewares.delete_if { |m| m.klass == target }
     end
 
+    def move(target, source)
+      source_index = assert_index(source, :before)
+      source_middleware = middlewares.delete_at(source_index)
+
+      target_index = assert_index(target, :before)
+      middlewares.insert(target_index, source_middleware)
+    end
+
+    alias_method :move_before, :move
+
+    def move_after(target, source)
+      source_index = assert_index(source, :after)
+      source_middleware = middlewares.delete_at(source_index)
+
+      target_index = assert_index(target, :after)
+      middlewares.insert(target_index + 1, source_middleware)
+    end
+
     def use(klass, *args, &block)
       middlewares.push(build_middleware(klass, args, block))
     end

actionpack/test/dispatch/middleware_stack_test.rb

@@ -93,6 +93,60 @@ class MiddlewareStackTest < ActiveSupport::TestCase
     assert_equal FooMiddleware, @stack[0].klass
   end
 
+  test "move moves middleware at the integer index" do
+    @stack.move(0, BarMiddleware)
+    assert_equal BarMiddleware, @stack[0].klass
+    assert_equal FooMiddleware, @stack[1].klass
+  end
+
+  test "move requires the moved middleware to be in the stack" do
+    assert_raises RuntimeError do
+      @stack.move(0, BazMiddleware)
+    end
+  end
+
+  test "move preserves the arguments of the moved middleware" do
+    @stack.use BazMiddleware, true, foo: "bar"
+    @stack.move_before(FooMiddleware, BazMiddleware)
+
+    assert_equal [true, foo: "bar"], @stack.first.args
+  end
+
+  test "move_before moves middleware before another middleware class" do
+    @stack.move_before(FooMiddleware, BarMiddleware)
+    assert_equal BarMiddleware, @stack[0].klass
+    assert_equal FooMiddleware, @stack[1].klass
+  end
+
+  test "move_after requires the moved middleware to be in the stack" do
+    assert_raises RuntimeError do
+      @stack.move_after(BarMiddleware, BazMiddleware)
+    end
+  end
+
+  test "move_after moves middleware after the integer index" do
+    @stack.insert_after(BarMiddleware, BazMiddleware)
+    @stack.move_after(0, BazMiddleware)
+    assert_equal FooMiddleware, @stack[0].klass
+    assert_equal BazMiddleware, @stack[1].klass
+    assert_equal BarMiddleware, @stack[2].klass
+  end
+
+  test "move_after moves middleware after another middleware class" do
+    @stack.insert_after(BarMiddleware, BazMiddleware)
+    @stack.move_after(BarMiddleware, FooMiddleware)
+    assert_equal BarMiddleware, @stack[0].klass
+    assert_equal FooMiddleware, @stack[1].klass
+    assert_equal BazMiddleware, @stack[2].klass
+  end
+
+  test "move_afters preserves the arguments of the moved middleware" do
+    @stack.use BazMiddleware, true, foo: "bar"
+    @stack.move_after(FooMiddleware, BazMiddleware)
+
+    assert_equal [true, foo: "bar"], @stack[1].args
+  end
+
   test "unshift adds a new middleware at the beginning of the stack" do
     @stack.unshift MiddlewareStackTest::BazMiddleware
     assert_equal BazMiddleware, @stack.first.klass

guides/source/configuring.md

@@ -317,6 +317,19 @@ Middlewares can also be completely swapped out and replaced with others:
 config.middleware.swap ActionController::Failsafe, Lifo::Failsafe
 ```
 
+Middlewares can be moved from one place to another:
+
+```ruby
+config.middleware.move_before ActionDispatch::Flash, Magical::Unicorns
+```
+
+This will move the `Magical::Unicorns` middleware before
+`ActionDispatch::Flash`. You can also move it after:
+
+```ruby
+config.middleware.move_after ActionDispatch::Flash, Magical::Unicorns
+```
+
 They can also be removed from the stack completely:
 
 ```ruby

railties/CHANGELOG.md

@@ -1,3 +1,21 @@
+*   Introduce middleware move operations
+
+    With this change, you no longer need to delete and reinsert a middleware to
+    move it from one place to another in the stack:
+
+    ```ruby
+    config.middleware.move_before ActionDispatch::Flash, Magical::Unicorns
+    ```
+
+    This will move the `Magical::Unicorns` middleware before
+    `ActionDispatch::Flash`. You can also move it after with:
+
+    ```ruby
+    config.middleware.move_after ActionDispatch::Flash, Magical::Unicorns
+    ```
+
+    *Genadi Samokovarov*
+
 *   Generators that inherit from NamedBase respect `--force` option
 
     *Josh Brody*

railties/lib/rails/configuration.rb

@@ -30,6 +30,15 @@ module Rails
     #
     #     config.middleware.swap ActionDispatch::Flash, Magical::Unicorns
     #
+    # Middlewares can be moved from one place to another:
+    #
+    #     config.middleware.move_before ActionDispatch::Flash, Magical::Unicorns
+    #
+    # This will move the <tt>Magical::Unicorns</tt> middleware before the
+    # <tt>ActionDispatch::Flash</tt>. You can also move it after:
+    #
+    #     config.middleware.move_after ActionDispatch::Flash, Magical::Unicorns
+    #
     # And finally they can also be removed from the stack completely:
     #
     #     config.middleware.delete ActionDispatch::Flash
@@ -66,6 +75,16 @@ module Rails
         @delete_operations << -> middleware { middleware.send(__method__, *args, &block) }
       end
 
+      def move_before(*args, &block)
+        @delete_operations << -> middleware { middleware.send(__method__, *args, &block) }
+      end
+
+      alias :move :move_before
+
+      def move_after(*args, &block)
+        @delete_operations << -> middleware { middleware.send(__method__, *args, &block) }
+      end
+
       def unshift(*args, &block)
         @operations << -> middleware { middleware.send(__method__, *args, &block) }
       end

railties/test/configuration/middleware_stack_proxy_test.rb

@@ -18,6 +18,11 @@ module Rails
         assert_playback :insert_before, :foo
       end
 
+      def test_playback_insert
+        @stack.insert :foo
+        assert_playback :insert_before, :foo
+      end
+
       def test_playback_insert_after
         @stack.insert_after :foo
         assert_playback :insert_after, :foo
@@ -38,6 +43,21 @@ module Rails
         assert_playback :delete, :foo
       end
 
+      def test_playback_move_before
+        @stack.move_before :foo
+        assert_playback :move_before, :foo
+      end
+
+      def test_playback_move
+        @stack.move :foo
+        assert_playback :move_before, :foo
+      end
+
+      def test_playback_move_after
+        @stack.move_after :foo
+        assert_playback :move_after, :foo
+      end
+
       def test_order
         @stack.swap :foo
         @stack.delete :foo