I don't understand this change. I see that it's changing the tests to use body, but surely the test code is exercising reading parameters. How are these equivalent tests?

The test code is exercising a feature in ActiveDispatch::Request:

    # Read the request \body. This is useful for web services that need to
    # work with raw requests directly.
    def raw_post
      unless has_header? "RAW_POST_DATA"
        raw_post_body = body
        set_header("RAW_POST_DATA", raw_post_body.read(content_length))
        raw_post_body.rewind if raw_post_body.respond_to?(:rewind)
      end
      get_header "RAW_POST_DATA"
    end

Submitting a request with content-type: multipart/form-data or similar, will cause the input body to be read by Rack::Request.new(env).params and cached (In this case, by the Rack::MethodOverride middleware IIUC). Previously, it would also be rewound, but now it's not. When invoking raw_post, if the input is already consumed, the raw post data would be empty.

By changing from params to body, we still exercise raw_post, without running into the above described issue where the post body is consumed by Rack::Request.

Honestly, we should either move raw_post to rack itself, or remove it from Rails, at best the feature seems unreliable, at worst, a memory hog. Another option would be to consider using the Rack::RewindableInput::Middleware but I feel this is a very bad long term direction, setting the bar for compatibility at the wrong place. Maybe we could advice people who absolutely depended on raw_post handling multipart data to use that middleware (but not by default in Rails v7.1).

The goal of this PR is to make the minimum changes to preserve the existing semantics. So the test still exercises the raw_post method, and is compatible on both Rack v2 and v3.

@tenderlove any chance I can get your feedback on this PR? Thanks!

cc @rafaelfranca another minor change to the tests to avoid running into "rewindable input" being removed in Rack 3.

I would personally vote to remove raw_post but this PR at least makes the tests compatible across the board.

@ioquatix the updated test in this PR test_body_stream is not the same as the other 2 tests, it does not hit the raw_post method from what I can see, so I'm not also sure changing to body is right for that test. I ran the test:

RACK="~> 3" bin/test test/controller/test_case_test.rb:256 -w

Also, I think you need to rebase, because upstream is now running tests for rack 2 + 3.

If rack.input is no longer rewindable according to rack, could we force it on rails side for now (those other 2 tests will pass), that way the params can remain?

def raw_post
       unless has_header? "RAW_POST_DATA"
         raw_post_body = body
+       raw_post_body.rewind if raw_post_body.respond_to?(:rewind) # and eof? check?
         set_header("RAW_POST_DATA", raw_post_body.read(content_length))
         raw_post_body.rewind if raw_post_body.respond_to?(:rewind)
       end

To a certain extent, it's possible, but it's not guaranteed as it's no longer a requirement so any middleware that's inserted by the user could replace the input with a non-rewindable one and it would break again.

@hahmed I could not identify the problem you talked about:

samuel@sakura ~/P/i/r/actionpack (rack-3-post-bodies)> RACK="~> 3" bin/test test/controller/test_case_test.rb:256 -w
/Users/samuel/.gem/ruby/3.2.1/gems/rack-3.0.4.1/lib/rack/chunked.rb:6: warning: Rack::Chunked is deprecated and will be removed in Rack 3.1
Running 130 tests in parallel using 4 processes
Run options: --seed 26250

# Running:

.

Finished in 0.472482s, 2.1165 runs/s, 2.1165 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

Can you clarify what issue you were seeing?

@ioquatix Do you know if this PR is still required?

There aren't any failing tests in actionpack with rack 3.0 in the main branch

I'll check.

It looks like this is no longer required to make the tests pass, so I'll close it.

However, I think the tests are mixing up too many concepts. I don't think query serialisation and deserialisation belongs as part of the tests. I think this PR is still a good idea - it simplifies the tests to focus on what the test says it is testing and not parameter/query handling. If serialisation and deserialization should be tested in this regard, it should probably rename the tests or have separate tests.

@ioquatix I think you're right, even when the tests are passing the name of the tests doesn't coincide with the code being tested, I'll check into this later this week.