Deprecate automatic cache invalidation in Request#{GET,POST}

[jeremyevans]

Add Request#clear_{GET,POST} for users to perform manual cache invalidation when replacing env['QUERY_STRING'] or env['rack.input'].

With this invalidation, env[RACK_REQUEST_QUERY_STRING] and env[RACK_REQUEST_FORM_INPUT] are unnecessary.

It appears as though env[RACK_REQUEST_FORM_VARS] is already unnecessary, as the value is set but never accessed, dating back to its introduction in 6c80c6c. However, even though it is never used by Rack, it apparently is used by Rails. However, Rails usage appears to be limited to parameter filtering, and if the RACK_REQUEST_FORM_VARS key wasn't set, there would be nothing to filter. So it's possible Rails could be changed so that if the key was missing, there are no problems (maybe it works like that already, and only the Rails tests need updates).

Request#clear_{GET,POST} are pretty ugly as far as method naming goes, but so are Request#{GET,POST}, so it kind of fits. Open to better suggestions for names.

[ioquatix]

What is the expected use case of clear_*? It seems cumbersome in practice. Wouldn't it just make more sense for people to create a new clean env?

[jeremyevans]

The expected use case is if you modify env['QUERY_STRING'] or env['rack.input'], you can call the method to remove any cached values, so that Request#{GET,POST} use the new query string and request body.

Certainly creating a new clean env would also work. However, the reason behind the pull request is that you and @tenderlove both wanted to remove the current automatic cache invalidation in Request (or at least, that was my understanding from our last conversation on this). The first step of doing that is deprecating it (in 3.1), before removing it (in 3.2).

[ioquatix]

Right but we can remove the automatic cache invalidation without introducing manual cache invalidation?

[jeremyevans]

Right but we can remove the automatic cache invalidation without introducing manual cache invalidation?

Certainly it is possible. Introducing manual cache invalidation allows the user currently relying on automatic cache invalidation to more easily update their code. Removing automatic cache invalidation without introducing manual cache invalidation leaves it to the user to figure out how to perform the manual cache invalidation themselves.

[ioquatix]

Another option is to provide a blessed way to assign the query part of the URL, e.g. Rack::Request#query= and the equivalent for the other cached values.

[matthewd]

FWIW, I continue to insist that this is inherently a "no longer Rack" level of API breakage, because it assumes everyone is accessing env through the same Request instance, or knows when someone else might have modified it.

If we're willing to switch to a model where the Request object is the source of truth, caching becomes trivial. But as long as everyone is potentially building their own instance and Request is merely a convenience accessor to the true live values in env, I don't see how it can change.

Extending the spec with a series of "if you modify key 'x' in env, then you must delete the following keys: 'y', 'z'" statements seems... unpleasant, as well as incompatible (every existing downstream that modifies keys is immediately no longer compliant).

At least moving the cache to ivars would mean that a given Request is the extent of the confusion -- and can be more internally responsible. With the current arrangement of caching to env specifically so that later Request instances can re-use it, a seemingly-simple sequence of "Request-accessing middleware, basic raw env-modifying middleware, Request-using application" would break, forcing the application to know that something preceding it may have already invalidated the cache of the Request object it just created. 😕

(Emphasis that this is my opinion of the situation we're stuck in by virtue of Rack's fundamentally hash-based API, not a personal preference for said arrangement.)

[jeremyevans]

My understanding is @tenderlove, @ioquatix, and I all want to remove the automatic cache invalidation. If a user modifies rack.input or QUERY_STRING, Rack::Request should not attempt to detect that and compensate. For one, if they mutate the existing value without replacing it, that's a case that is already undetected.

This PR tries to be a little nicer and give Rack::Request users the ability to more easily perform manual cache invalidation in the cases where they do modify rack.input or QUERY_STRING. However, if that is going to be a point of contention, we can just deprecate and then remove the automatic cache invalidation, and tell users that if they modify rack.input or QUERY_STRING, they are responsible for handling any issues that arise from doing so.

This is unrelated to SPEC, it is purely related to Rack::Request. Note that the env keys removed by the manual cache invalidation are all Rack::Request specific keys. Rack users who are not using Rack::Request are unaffected by this change.

This deliberately doesn't document if you change QUERY_STRING, you need to remove x, and if you change rack.input, you need to remove y and z. It merely gives the ability for users to call a method that will remove the related env entries, so they don't need to figure that out themselves.

The issue with moving the cache to ivars is that by design, separate Rack::Request instances want to use the same cache. Now that input is no longer rewindable, they basically must use the same cache.

@tenderlove @ioquatix Can you provide your feedback on whether we should offer manual cache invalidation (as this PR does), or just remove the automatic cache invalidation and tell users they are on their own if they modify rack.input or QUERY_STRING?

[matthewd]

My concern is here:

tell users they are on their own if they someone else modifies rack.input or QUERY_STRING

[jeremyevans]

To me, whether the modification is done directly by the user's code or indirectly by some other code the user is choosing to run does not matter. Maybe @tenderlove or @ioquatix feels differently?