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

[julik]

Let's alter this comment because it is a bit misleading. A Rack response body may permit "materialization" into an Array of strings using to_ary - this is in fact in the Rack spec, here https://github.com/rack/rack/blob/main/SPEC.rdoc#the-body- So in this case, checking for respond_to?(:to_ary) and calling it is sufficient. For each and call we might need more special cases, but in this instance it is enough to state that some Rack bodies can be "expanded" using to_ary. It doesn't matter whether such a body comes from Rails or any other callee. It is also important to note that if we did use to_ary we are not supposed to call close - i.e. we need to stick to the Rack semantics very carefully here, and the comments should reflect that.

[julik]

No need for any extra checkes except for the responding to to_ary. An Array will respond to to_ary and return self:

[2] pry(main)> ar = []
=> []
[3] pry(main)> ar.object_id
=> 320
[4] pry(main)> ar.to_ary.object_id
=> 320

A Body being an Enumerator is another check that should not be done, as an object can perfectly be a Rack response body and respond to either each or call, but not be an Enumerator.

[julik]

Most likely you want to call to_ary here, actually - because an Array of strings may be persistable, and replacing body with the "materialized" array will allow response_may_be_persisted? to size it correctly by a sum of bytesizes of the array elements. So first call to_ary to materialize, then check for whether the materialized body is too big.

[julik]

This replacement won't be needed if you materialize into an Array earlier