-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle ActionDispatch::Response::RackBody as body #22
base: main
Are you sure you want to change the base?
Conversation
6590400
to
d8cc3ff
Compare
@@ -55,6 +55,9 @@ def call(env) | |||
end | |||
|
|||
status, headers, body = @app.call(env) | |||
# `body` could be of type `ActionDispatch::Response::RackBody` and idempo will not even attempt to store it, | |||
# we're converting ActionDispatch::Response::RackBody to a storable array format. | |||
body = body.try(:to_ary) || body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why the behavior differs is likely because there was some tweaking of the rack body in Rails due to the changes in Rack 3.x. The issue with calling to_ary
explicitly is that it instantly "materializes" the entire Rack body into strings, and to_ary
comes from the Rack 3.x protocol (and has Semantics™ attached to it). There are implications to this (look for RackBody changes in Rails and the related PR discussions and https://github.com/appsignal/appsignal-ruby/pull/1037/files#diff-542053b5df2402727a27d180e5f6f63bf8a646b81d72fa0c013de4177a6becafR23 ).
So it is not as much as needing to "convert" something into something else as it is about knowing how large the response is going to be once it gets read out of the body. Can you check why https://github.com/julik/idempo/blob/main/lib/idempo.rb#L118 does not work? Doesn't Rails set content length by itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julik I actually traced it to a next line, that rejects to save this body -- since body is not an Array.
https://github.com/julik/idempo/blob/main/lib/idempo.rb#L120
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but do response_headers
contain a Content-Length
of some description in your testing? I would expect Rails sets it but I'm not sure
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 |
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
:And here is an output of
rails middleware
:But that is just a wild guess.
As a fix, I suggest trying converting body to array — this proved effective in our project.