Prefer lower case headers.#3
Conversation
ioquatix
force-pushed
the
lowercase-headers
branch
7 times, most recently
from
2e1a8cb to
6efa762
Compare
ioquatix
force-pushed
the
lowercase-headers
branch
from
6efa762 to
1c97581
Compare
| # find a cached entry that matches the request. | ||
| env = request.env | ||
| match = entries.detect{ |req,res| requests_match?((res['Vary'] || res['vary']), env, req) } | ||
| match = entries.detect{ |req,res| requests_match?((res['vary'] || res['vary']), env, req) } |
There was a problem hiding this comment.
Was it intentional to check the same value twice? Before, it was checking if there was a match for both the Capitalized Vary and the lowercase vary. But now that everything is lowercase, we only need to check one of them, right?
There was a problem hiding this comment.
Yes, that's correct, do you have time to submit a PR fixing this? Thanks!
|
|
||
| headers = persist_response(response) | ||
| headers.delete('Age') | ||
| headers.delete('age') |
There was a problem hiding this comment.
Similar comment here. Is it necessary to delete the same header twice?
There was a problem hiding this comment.
No, if you can submit a PR to fix this, please do! Thanks!
In PR rack#3, headers were converted to lowercase, which resulted in the same action being performed twice. This PR removes those duplicate actions.
In PR #3, headers were converted to lowercase, which resulted in the same action being performed twice. This PR removes those duplicate actions. Co-authored-by: Moncef Belyamani <moncef@example.com>
|
|
||
| _, res = match | ||
| if body = entity_store.open(res['X-Content-Digest']) | ||
| if body = entity_store.open(res['x-content-digest']) |
There was a problem hiding this comment.
With this change, after deploying with the updated version of this gem, you can end up in a scenario where a long-term cached item still exists in the metastore with the capitalized header, but when it goes to get the key here using the lowercase header, it returns nil, and passes nil to the open method, resulting in an exception further down, and since on lookups on exceptions, it calls pass instead of fetch, it isn't going to store the response with the updated header and will continue to hit the error
Could check here for both cases, but I think another fine option is just letting people know they should flush their cache if they're upgrading from an earlier version to this one or later.
There was a problem hiding this comment.
Do you mind creating an issue for this?
3 participants
See rack/rack#1812 for more details.