Harmonize Rack dependency and tests #27
Conversation
julik
commented
Nov 4, 2024
•
julik
commented
- Set up Appraisal for testing against both Rack 2 and 3
- Ensure all HTTP header uses are lowercase
- Support non-rewindable request bodies
- Use Rack::Lint when running tests
- Bring tests more in line with the Rack spec
Since rack 3 is now supported by Rails, it seems important to ensure that this middleware has a proper support for it. Issues with rack 3 are difficult to notice or debug. To help prevent issues with compatibility and the time that it takes to manually debug and resolve those - I suggest that we integrate `Rack::Lint` into the test suite. Rack::Lint would ensure that middleware adheres to the SPEC. Please be patient, since I' still figuring out how this works ;-) Related documentation (for me to read through multiple times): - https://github.com/rack/rack/blob/744f92d099653f873ced3380e425a6f82f0c6c6c/UPGRADE-GUIDE.md?plain=1#L185 - https://github.com/rack/rack/blob/744f92d099653f873ced3380e425a6f82f0c6c6c/SPEC.rdoc --------- Co-authored-by: Julik Tarkhanov <me@julik.nl>
Make sure fingerprinting works with non-rewindable request bodies, and add tests to make sure everything contributes to the fingerprint correctly
Something that supports to_ary "just" supports it anyway
as there are quite a few
as it doesn't make much sense with Rack 3.0 streaming bodies - we may be able to read out the request body, or we may not be able to. If we are not able - the fingerprint will be the same.
as Lint is appropriately hinting
because if it works on 2.7 it will work on 3 just as well, and we are not using 3.x specific syntax (and have standardrb rules configured accordingly)
|
@skatkov feel free to chime in on the review if you feel like it, I've integrated as much of your prior work as I could |
julik
changed the title
| yielder.yield(env["rack.input"].read) | ||
| end | ||
| [200, {"X-Foo" => "bar", "Content-Length" => "9999999999999"}, body] | ||
| pre_sized_body = PreSizedBody.new(999999999) |
There was a problem hiding this comment.
suggestion: Interesting trick with PreSizedBody!
I see that we're not testing that small request body doesn't get saved. Should be easy to add with PreSizedBody class?
There was a problem hiding this comment.
A small request body should get saved. What we are not testing is that while the body is not saved, there is still concurrency protection - but testing that would need bypassing rack-spec, because there is no provision for multiple Fibers running simultaneous requests in it.
There was a problem hiding this comment.
Ah, true! Misread the code, thanks for pointing that out.
|
@franzliedke take a look when you have a moment |
| runs-on: ubuntu-22.04 | ||
| if: github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository | ||
| strategy: | ||
| matrix: | ||
| ruby: | ||
| - "2.7" | ||
| - "3.2" |
There was a problem hiding this comment.
Was this intentionall removed? Testing on a recent Ruby version seems useful.
There was a problem hiding this comment.
Yes, because then the matrix turns into 4 versions - and I had some trouble loading older ActiveRecord on the newer Ruby versions. I presume what works on 2.7 will work on 3.x anyway, so it felt like a good comproimse
| it "is not idempotent if the request body is different" do | ||
| post "/", "somedata", "HTTP_X_IDEMPOTENCY_KEY" => "idem" | ||
| expect(last_response).to be_ok | ||
| expect(last_response.headers["X-Foo"]).to eq("bar") | ||
| first_response_body = last_response.body | ||
|
|
||
| post "/", "somedata2", "HTTP_X_IDEMPOTENCY_KEY" => "idem" | ||
| expect(last_response).to be_ok | ||
| expect(last_response.headers["X-Foo"]).to eq("bar") | ||
| expect(last_response.body).not_to eq(first_response_body) # response should not have been reused | ||
| end |
There was a problem hiding this comment.
What about the removed specs?
There was a problem hiding this comment.
Some were irrelevant - this one, for example, does not work with Rack 3 because the request body is no longer rewindable - and thus can't contribute to the fingerprint. Other tests do cover the "standard happy path" well IMO.
| it "takes all the key HTTP headers and the body into account" do | ||
| combinations = { | ||
| "rack.input" => [StringIO.new(""), StringIO.new("some body")], | ||
| "SCRIPT_NAME" => ["script", ""], | ||
| "PATH_INFO" => ["/hello", "/hello?one=1", ""], | ||
| "REQUEST_METHOD" => ["GET", "POST", "PUT"], | ||
| "HTTP_AUTHORIZATION" => ["Bearer abcdef", nil] | ||
| } | ||
|
|
||
| # Create combinations of all possible values of different headers, and | ||
| # make sure they all produce different fingerprints | ||
| initial_array = Array(combinations.values.first) | ||
| product_with = combinations.values[1..] | ||
| all_value_permutations = initial_array.product(*product_with) | ||
| fingerprints = all_value_permutations.flat_map do |values| | ||
| combination_of_possible_rack_env_values = combinations.keys.zip(values).to_h | ||
| ["key1", "key2"].map do |idempotency_key| | ||
| request = Rack::Request.new(combination_of_possible_rack_env_values) | ||
| described_class.call(idempotency_key, request) | ||
| end | ||
| end | ||
|
|
||
| expect(fingerprints.uniq.length).to eq(fingerprints.length) | ||
| end |
There was a problem hiding this comment.
I like this test. 😁 👍🏼
(The extra variables initial_array and product_with led me a bit astray first, but I guess inlining it all wouldn't be more readable either.)
