Use sha1 to reduce the risk of collisions by GrahamCampbell · Pull Request #12 · php-http/cache-plugin

GrahamCampbell · 2016-08-01T22:40:38Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
License	MIT

What's in this PR?

Switches the key generation algorithm over to use sha1 rather than md5.

Why?

The performance is similar, and the collision risk is lower.

Example Usage

Checklist

Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
Documentation pull request created (if not simply a bugfix)

To Do

If the PR is not complete but you want to discuss the approach, list what remains to be done here

sagikazarmark · 2016-08-01T22:47:11Z

Maybe we can make this configurable? Valid values are the results of hash_algos().

GrahamCampbell · 2016-08-01T22:49:28Z

Maybe we should just allow people to implement a key generator interface and inject that into the constructor and have it do createCacheKey from a message. If you're ok with that, I can make that abstraction now.

Updated tests

sagikazarmark · 2016-08-01T22:53:34Z

Hm, okay, but I would be happier if we could make it kind of optional: add a hash option in config defaulting to sha1, allowed values are null and result of hash_algos. (createCacheKey in null case should throw a LogicException if no hash and no CacheKeyGenerator instances are provided). Also, a custom CacheKeyGenerator (name ok?) that could be provided via a setter (with a HashCacheKeyGenerator implementation used as the default).

IMO in most cases such implementation might be an overkill, so I think it would be better to provide a "fallback"/default solution.

WDYT?

GrahamCampbell · 2016-08-01T22:55:22Z

Why would anyone want to customize this? If they do, a CacheKeyGenerator would be fine?

GrahamCampbell · 2016-08-01T22:55:54Z

PS I really dislike setters. I prefer things to be immutable after instantiation.

sagikazarmark · 2016-08-01T22:58:08Z

PS I really dislike setters. I prefer things to be immutable after instantiation.

Me too, but considering that this adds an extra configuration effort, this seems to be a half-way solution to me.

Also, changing the constructor would be a BC break (unless the generator is optional?), while adding an extra config option and an optionally used setter is not.

GrahamCampbell · 2016-08-01T22:59:54Z

I was thinking making it optional, with a default value of null, then filling it using a ternary statement, yeh.

GrahamCampbell · 2016-08-01T23:00:41Z

Hmm, just looked at the constructor signature. I think using the options resolver might be better.

sagikazarmark · 2016-08-01T23:01:57Z

Hmm, just looked at the constructor signature. I think using the options resolver might be better.

Hmm, right. You can still fall back to a string then using the hash implementation?

GrahamCampbell · 2016-08-01T23:08:35Z

I've implemented the hash algo selection. How does that look?

sagikazarmark · 2016-08-01T23:09:59Z


 return $next($request)->then(function (ResponseInterface $response) use ($cacheItem) {
- if ($this->isCacheable($response)) {
+ if ($this->isCacheable($response) && ($maxAge = $this->getMaxAge($response)) > 0) {


Isn't this from another PR?

Err, oops...

Doing this from the github online editor, lol.

sagikazarmark · 2016-08-01T23:12:48Z

Can you please add change log? We follow keep a change log format.

GrahamCampbell · 2016-08-01T23:14:16Z

Wanted to avoid conflicts with other PRs. Do you mind adding something on merge?

Nyholm · 2016-08-02T04:24:26Z

Thank you. 👍

sagikazarmark · 2016-08-02T08:34:31Z

Looks good to me, thanks @GrahamCampbell

egeloen · 2016-08-05T11:21:21Z

I'm a little bit late here but this PR is a BC break in a minor version. All my caches are become broken due to the new hash algo used for the cache key generation.

@sagikazarmark Can you confirm it has been done by mistake?

Anyway, as the new tag has been released, let it as it is, I just need to change my cache config.

GrahamCampbell · 2016-08-05T11:25:41Z

I don't think it would be considered broken to just loose items from the cache?

dbu · 2016-08-05T11:25:49Z

did you get errors, or just cache misses? i think cache misses on a minor version sound ok. i hope we did not release it as a patch release though, that would be unexpected.

sagikazarmark · 2016-08-05T11:39:46Z

I agree, cache misses doesn't sound like BC break.

egeloen · 2016-08-05T12:06:56Z

Yes, I probaby abuse when saying BC break as technically everything is working fine (just differently)... I use it in a specific context: caching http requests done to a third service secured by IPs and use the generated cache for tests execution on Travis but after upgrading, my cache is becomes broken. As already said, not a big deal to fix, just want to discuss your BC policy.

Nyholm · 2016-08-05T12:17:57Z

That is an interesting question. Our general BC promise is here: http://php-http.readthedocs.io/en/latest/httplug/backwards-compatibility.html

But should we consider the cache keys as a part of the BC promise? Whatever we decide should be documented.

I suggest that we are allowed to update the cache keys for minor versions but not for patches. But I would try to avoid it.

For the record:
This PR also updated the cache keys: #8 (merged and released)
And the PR for this issue will likely update the cache keys: #4

joelwurtz · 2016-08-05T18:25:00Z

I would say the following :

Do not make our keys part of our BC promise, but consider them as it was the case.
So we can make changes to the key in a minor or patch version, but we should try our best to avoid that.

sagikazarmark · 2016-08-06T09:46:57Z

Actually this specific PR could even be a bugfix: with massive usage it's possible that some requests collide using md5.

GrahamCampbell added 2 commits August 1, 2016 23:39

Use sha1 to reduce the risk of collisions
a50f796

Only cache if the max age is positive
5d3d98c

GrahamCampbell added 2 commits August 1, 2016 23:50

Updated tests
49369f0

Merge pull request#2from GrahamCampbell/patch-4
16543f6
Updated tests

Support setting a hash_algo
5094fef

CS
b3aa063

sagikazarmark reviewed Aug 1, 2016
View reviewed changes

Reverted accidental change
82b565b

sagikazarmark merged commit c1608f5 into php-http:masterAug 2, 2016

GrahamCampbell deleted the patch-3 branch August 2, 2016 08:55

tuupola mentioned this pull request Feb 10, 2017
Optionally enable caching of any request method #24
Merged
4 tasks

qkdreyer mentioned this pull request Jan 23, 2023
docs: add hash_algo php-http/documentation#302
Merged

Uh oh!

Conversation

GrahamCampbell commented Aug 1, 2016

What's in this PR?

Why?

Example Usage

Checklist

To Do

Uh oh!

sagikazarmark commented Aug 1, 2016

Uh oh!

GrahamCampbell commented Aug 1, 2016

Uh oh!

sagikazarmark commented Aug 1, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GrahamCampbell commented Aug 1, 2016

Uh oh!

GrahamCampbell commented Aug 1, 2016

Uh oh!

sagikazarmark commented Aug 1, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GrahamCampbell commented Aug 1, 2016

Uh oh!

GrahamCampbell commented Aug 1, 2016

Uh oh!

sagikazarmark commented Aug 1, 2016

Uh oh!

GrahamCampbell commented Aug 1, 2016

Uh oh!

sagikazarmarkAug 1, 2016

Choose a reason for hiding this comment

Uh oh!

GrahamCampbellAug 1, 2016

Choose a reason for hiding this comment

Uh oh!

GrahamCampbellAug 1, 2016

Choose a reason for hiding this comment

Uh oh!

sagikazarmarkAug 1, 2016

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Aug 1, 2016

Uh oh!

GrahamCampbell commented Aug 1, 2016

Uh oh!

Nyholm commented Aug 2, 2016

Uh oh!

sagikazarmark commented Aug 2, 2016

Uh oh!

egeloen commented Aug 5, 2016

Uh oh!

GrahamCampbell commented Aug 5, 2016

Uh oh!

dbu commented Aug 5, 2016

Uh oh!

sagikazarmark commented Aug 5, 2016

Uh oh!

egeloen commented Aug 5, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nyholm commented Aug 5, 2016

Uh oh!

joelwurtz commented Aug 5, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sagikazarmark commented Aug 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sagikazarmark commented Aug 1, 2016•
edited
Loading

sagikazarmark commented Aug 1, 2016•
edited
Loading

egeloen commented Aug 5, 2016•
edited
Loading

joelwurtz commented Aug 5, 2016•
edited
Loading