Prevent [response].flatten from recursing infinitely.

[myronmarston]

This was causing problems for RSpec users (see this comment) since rspec-expecations has a place where it uses Array#flatten on an array containing objects passed to RSpec by users, and if that array contained a rails response, it would recurse infinitely.

It would also be nice to backport this and get it in upcoming patch fixes in whatever other recent versions suffer from this problem.

[chancancode]

I traced the commits and seems like this is here to support custom splatting (status, header, _ = a_response), so we can't just remove it. Looking at rspec/rspec-expectations#166 and rack/rack#419, it seems like the issue is that the return value of to_ary should not contain self. Is it possible to fix this in a similar fashion as rack/rack@5b251a9 instead?

[chancancode]

cc @josevalim

[myronmarston]

Hmm, would have been nice if there was a test showing the splat behavior, so that I knew to_ary wasn't needed. The fact that removing it broke no tests suggested that no public APIs relied upon its presence. Do you consider the custom splatting to be a public API given that there were no tests for it?

If so, I can add a test for that and look into an alternate way to fix this. I honestly expected to have to do a more complicated fix; removing to_ary was just the first attempt and then it happened to worked.

[myronmarston]

@chancancode -- I force pushed 2 alternate commits. Note that [response].flatten no longer returns [response], which is a little odd, but I believe it's impossible to provide that behavior and support the destructuring/splatting behavior you mentioned. They both operate off the to_ary protocol.

[chancancode]

Hey @myronmarston, thanks for following up on this. Yeah, I think it makes sense to add a test for splatting. I suppose the reason we didn't have a test for it was that it only works on 1.9.2+ and it was added when we still support 1.8.7.

The updated patch seems fine to me, but this is getting into territories I'm not very familiar with so I'll try to loop in someone else to sign it off.

[chancancode]

cc @spastorino @tenderlove

[myronmarston]

I'm having a hard time getting the build green. There are railties tests that are failing that appear to be unrelated but must be exercising a code path not covered by the actionpack tests.

First, I pushed 450f69bd2aa8f327a40f0f212d51007e2a2e2ada and it failed in the railties tests on travis with:

  1) Failure:
ApplicationTests::SendfileTest#test_config.action_dispatch.x_sendfile_header_can_be_set [test/application/middleware/sendfile_test.rb:47]:
--- expected
+++ actual
@@ -1 +1 @@
-"/home/travis/build/rails/rails/railties/test/application/middleware/sendfile_test.rb"
+nil
  2) Failure:
ApplicationTests::SendfileTest#test_config.action_dispatch.x_sendfile_header_is_sent_to_Rack::Sendfile [test/application/middleware/sendfile_test.rb:58]:
--- expected
+++ actual
@@ -1 +1 @@
-"/home/travis/build/rails/rails/railties/test/application/middleware/sendfile_test.rb"
+nil

Troubleshooting it locally, I discovered that if I delegated :to_path as well it would fix it, so I force pushed that update in ed6f1fa, but now the travis build is failing with a different railties failure:

  1) Failure:
ApplicationTests::MiddlewareTest#test_conditional_get_+_etag_middlewares_handle_http_caching_based_on_body [test/application/middleware_test.rb:197]:
--- expected
+++ actual
@@ -1 +1 @@
-"max-age=0, private, must-revalidate"
+"no-cache"

Troubleshooting it locally, I've discovered I can get it to pass if I remove the :to_path delegation, but that's needed for the other failure. So I'm not sure how to get it to pass both tests.

Maybe @tenderlove, @spastorino or @josevalim can figure out a way to get this green or write up an alternate fix? I really don't care if my PR is merged or not as long as the infinite recursion is fixed.