Handle ActionDispatch::Response::RackBody as body

[skatkov]

With our new rails 7.1 project, idempotent requests don't seem to be working. They are not even saved.

As it turns out, Idempo middleware receives an instance of ActionDispatch::Response::RackBody as a body. And since it's not Array, we're not saving that body.

I'm not sure exactly why another project based on rails 7.0 keeps working. Because rails started returning ActionDispatch::Response::RackBody pre 7.0 version. I assume, that this could be due to the way how middleware is being inserted (how deep in a stack).

Here is what we do in our application.rb:

config.middleware.insert_after ActionDispatch::Executor, Idempo, backend: Idempo::ActiveRecordBackend.new

And here is an output of rails middleware:

use Rack::Sendfile
use ActionDispatch::Static
use ActionDispatch::Executor
use Idempo
use ActionDispatch::ServerTiming
use ActiveSupport::Cache::Strategy::LocalCache::Middleware
use Rack::Runtime
use Rack::MethodOverride
use ActionDispatch::RequestId
use ActionDispatch::RemoteIp
use Rails::Rack::Logger
use ActionDispatch::ShowExceptions
use WebConsole::Middleware
use ActionDispatch::DebugExceptions
use ActionDispatch::ActionableExceptions
use ActionDispatch::Reloader
use ActionDispatch::Callbacks
use ActiveRecord::Migration::CheckPending
use ActionDispatch::Cookies
use ActionDispatch::Session::CookieStore
use ActionDispatch::Flash
use ActionDispatch::ContentSecurityPolicy::Middleware
use ActionDispatch::PermissionsPolicy::Middleware
use Rack::Head
use Rack::ConditionalGet
use Rack::ETag
use Rack::TempfileReaper
use ActionDispatch::Static
run Banksvc::Application.routes

But that is just a wild guess.

As a fix, I suggest trying converting body to array — this proved effective in our project.

[franzliedke]

Whew, I got quite scared when I read the gem is not working for you with Rails 7.1. In our project (also Rails 7.1), it does.

We insert the middleware directly in the base controller (a barely-known Rails feature), can you try that out to get closer to the root cause? (You hinted middleware order may play a role.)

class MyRailsController
  use Idempo, backend: Idempo::ActiveRecordBackend.new, other: options
end

[julik]

I am inclined to merge this with a few changes (only relying on to_ary if we are, in fact, under Rack 3 or greater). to_ary will materialize the entire response into memory and the body can then be handled as an array of strings, in the +- usual manner, but there have been cases with various libraries implementing to_ary on Rack response bodies incorrectly. @franzliedke you have any objections to this?

[franzliedke]

No big objections, haven't peeked into the insides of Rack here. Is there any way we can add some regression tests around this?

[skatkov]

@julik > only relying on to_ary if we are, in fact, under Rack 3 or greater

I'll make a change.

[skatkov]

@julik Did couple of adjustments.

• Body will be converted to array only for rack 3.
• Ensured that we keep testing rack v2 as well

[julik]

Few tweaks still needed. Also maybe a good idea to note around rack_response_body.close that this will not be applied to Arrays resulting from to_ary

[skatkov]

@julik thanks for such a thorough review! I have implemented requested changes.

[julik]

Awesome. One little optional bit and we're good to go

[julik]

nit: since you already do respond_to? you don't need try() you can call to_ary` directly