Add Support for Caching 422 (Unprocessable Entity) Responses#93
Add Support for Caching 422 (Unprocessable Entity) Responses#93devesh-shetty wants to merge 1 commit intomainfrom
Conversation
|
👋 It's been a while I didn't look at this gem, so my comment may be wrong. I think this is not going to work as is, and I'm not sure if it's a good idea to make it work. The issue is that 422 are sent back when a POST request is submitted and post requests are not idempotent. The first issue is that this gem only accepts GET and HEAD request: response_bank/lib/response_bank/controller.rb Lines 7 to 8 in cb744f5 The second issue is that the computation of the cache key only include the query string and the path A user that tries so submit a form with invalid input will be presented with the cache the next time it submits the form with valid input. |
|
couldn't I override these two methods to be method and body aware🤔 def cacheable_request?
return false if params[:cache] == "false"
request.get? || request.head? ||
request.post?
end
# Make the cache key body-aware so a corrected submission won’t hit a 422 cache for a different body.
def cache_key_data
end |
|
We could, but I worry that we may end up in many pitfalls. For instance how do we cache multipart form where a user upload a gigantic file that ends up being invalid server side (e.g. not a pdf or whatever), the computation of the key is going to be expensive. It's also not compliant with the RFC where POST request can't be cached https://datatracker.ietf.org/doc/html/rfc2616#section-9.5 unless specific headers like the Expiry are set which this gems don't do explicitly. Line 8 in cb744f5 I think the tricky part is the cache invalidation (as always 😅), because the "success" of a request is not necessarily tied to the form data. It's rare but submitting the same request with the same form data may result in different outcome (blips, race condition). Those are the thing I can think of that make this change non trivial but there may be others, which is one of the reason why it's usually not advised to cache POST requests. |
|
I think @edouardchin makes some good points above..
and
Since this this gem is intended for those purposes (i.e. GETs) to make it work with our 422s (POSTs) for OOS errors, we'd have to make it more customisable with...
Just adding 422 to the cacheable statuses in the gem (without making it optional) may have unintended effects on other places in SFR where we currently use this cache... While it would be nice to have our cache handled at middleware level (and with compression), I think our proxy cache use-case is quite specific and unique (aiming to protect making reqs to core) - rather than a generic cache (that this gem is solving) I think it does make sense for 401s tho, as shown in @soules older draft PR |
Summary
This PR extends ResponseBank's caching capabilities to include 422 (Unprocessable Entity) responses alongside the existing support for 200, 301, and 404 status codes. This allows API validation error responses to be cached, reducing redundant processing for frequently repeated invalid requests.
Motivation
422 responses are commonly used in REST APIs to indicate that the request was well-formed but contains semantic errors (e.g., validation failures). These responses are typically:
By caching 422 responses, we can: