Added support for Last-Modified and ETag #8

Nyholm · 2016-07-29T12:43:44Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Related tickets	fixes #2
Documentation
License	MIT

What's in this PR?

We should allow the server to answer 304 which allows us to use a response in the cache.
The BC break here is that all existing items in the cache will be invalid. Is that an issue? It is a cache and not a storage.

I introduced a new config parameter cache_lifetime that says how long the item should be in cache without it being used.

I did also add a few parameters to the array we store in the cache such as expiresAt, createdAt, etag.

To Do

Fix the tests

sagikazarmark · 2016-07-29T12:50:14Z

src/CachePlugin.php

+                $cacheItem->set($data)->expiresAfter($this->config['cache_lifetime'] + $data['expiresAt']);
+                $this->pool->save($cacheItem);
+
+                return $this->createResponseFromCacheItem($cacheItem);


What if the server responds with 304 but there is no cache?

We do now that it is a cache. Or else the If-Modified-Since and If-None-Match would not have been added. And correct me if I'm wrong but I don't think the server will answer 304 without those headers in the request.

Yes, those headers may have been added before. I'll just make sure to pass the response then.

sagikazarmark · 2016-07-29T12:52:07Z

Why do we need cache lifetime? Normally the server tells us how long it should be cached, doesn't it?

Nyholm · 2016-07-29T12:57:30Z

Yes, but we would like to have the possibility to serve an expired response if the server answers 304.

Nyholm · 2016-07-29T13:08:57Z

T0: We make a request. Server responds saying the response is valid until T10. Return response form server
T5: We make a request. The response is in the cache and we know it is valid until T10. Return response from cache
T15: We make a request. The response is in the cache but is stale. We ask the server about the response and server answer 304 and that the response is valid until T25. Return response from cache
T20: We make a request. The response is in the cache and we know it is valid until T25. Return response from cache

Nyholm · 2016-07-31T14:37:27Z

spec/CachePluginSpec.php


        $pool->getItem('e3b717d5883a45ef9493d009741f7c64')->shouldBeCalled()->willReturn($item);
        $item->isHit()->willReturn(false);
-        $item->set(['response' => $response, 'body' => $httpBody])->willReturn($item)->shouldBeCalled();
-        $item->expiresAfter(60)->willReturn($item)->shouldBeCalled();
+        $item->set()->willReturn($item)->shouldBeCalled();


I can't figure out a good way to write this line. I cant specify the exact data since it is dependent on time().

Can I say $item->set(<any argument>)->willRetu....?

Nyholm · 2016-07-31T16:00:01Z

I've figured out the tests and I found some bugs.

Im done with the code changes and this PR is ready for review.

GrahamCampbell · 2016-08-02T09:07:06Z

src/CachePlugin.php

@@ -194,11 +231,73 @@ private function getMaxAge(ResponseInterface $response)
    private function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults([
+            'cache_lifetime' => 2592000, // 30 days


might be clearer to actually have an arithmetic expression here since it's only executed once

Nyholm · 2016-08-02T09:28:14Z

Thank you @GrahamCampbell. I've update the PR according to your suggestions.

[ci skip] [skip ci]

GrahamCampbell · 2016-08-02T10:50:01Z

spec/CachePluginSpec.php

@@ -15,7 +16,7 @@ class CachePluginSpec extends ObjectBehavior
 {
    function let(CacheItemPoolInterface $pool, StreamFactory $streamFactory)
    {
-        $this->beConstructedWith($pool, $streamFactory, ['default_ttl'=>60]);
+        $this->beConstructedWith($pool, $streamFactory, ['default_ttl'=>60, 'cache_lifetime'=>1000]);


GrahamCampbell · 2016-08-02T11:03:40Z

spec/CachePluginSpec.php

-        $this->beConstructedWith($pool, $streamFactory, ['default_ttl'=>60, 'cache_lifetime'=>1000]);
+        $this->beConstructedWith($pool, $streamFactory, [
+            'default_ttl' => 60,
+            'cache_lifetime'=>1000


missing spaces?

GrahamCampbell · 2016-08-02T11:06:07Z

src/CachePlugin.php

+            }
+        }
+
+        return;


don't need this statement?

See #8 (comment)

yeh, i replied there

GrahamCampbell · 2016-08-02T11:09:53Z

src/CachePlugin.php

+                $cacheItem
+                    ->expiresAfter($this->config['cache_lifetime'] + $maxAge)
+                    ->set([
+                    'response' => $response,


is this one level not enough indented?

Thank you. But shouldn't StyleCi pick this up?

Symfony has no rules about indentation like this I think?

GrahamCampbell · 2016-08-02T11:13:06Z

Looks good. I've not physically tested this in the real world yet though.

GrahamCampbell · 2016-08-02T11:22:21Z

src/CachePlugin.php

+                $maxAge = $this->getMaxAge($response);
+                $currentTime = time();
+                $cacheItem
+                    ->expiresAfter($this->config['cache_lifetime'] + $maxAge)


Why are we doing this please?

We would like to serve Responses that the server once said should expire.

See #8 (comment)

when doing cache validation, we keep outdated caches and send If-Modified-Since / If-None-Match requests to the server to check if the cache is ok to be used. we might add some comment in the code though, to make it clear.

dbu · 2016-08-02T13:15:41Z

went over the code, looks good to me.

for a moment i was wondering about stale-while-validate but this is just content validation and not advanced proxy operations. in async mode we theoretically could use stale-while-validate to return the expired response and re-fetch to our local cache. but this has nothing to do with this PR and anyways seems like a bad use case - one should use a dedicated caching proxy for such things, not implement this on client side.

Nyholm · 2016-08-02T14:20:58Z

TODO:

Decide on how to handle updates in the array that is stored in the cache. isset or cache busting Added support for Last-Modified and ETag #8 (comment)

Nyholm · 2016-08-02T15:49:11Z

We use issets.

This PR is ready to be merged

sagikazarmark · 2016-08-02T20:56:00Z

Thanks, will tag a release soon.

Nyholm · 2016-08-02T21:00:51Z

Awesome. Thank you for all the reviews and feedback.

sagikazarmark reviewed Jul 29, 2016
View reviewed changes

Nyholm reviewed Jul 31, 2016
View reviewed changes

GrahamCampbell reviewed Aug 2, 2016
View reviewed changes

Nyholm added 11 commits August 2, 2016 11:36

Added support for Last-Modified and ETag

824b9aa

style fix

3b2f720

Return the 304 response if we do not have a response in cache

3f36fb5

Bugfix and started on the tests

39c0cfe

Style fixes

33c6e1c

Tests are passing

83e22ce

Style fix

370b0a0

Added tests

d89d233

Updated comment

23d0cd4

Applied fixes from StyleCI

bf2e511

[ci skip] [skip ci]

Made the code easier to read

eb2cd58

Nyholm force-pushed the issue-2 branch from 8f8cc5e to eb2cd58 Compare August 2, 2016 09:36

Updated hash

0e6792a

GrahamCampbell reviewed Aug 2, 2016
View reviewed changes

Minor

8c5047f

GrahamCampbell reviewed Aug 2, 2016
View reviewed changes

cs

68edac2

GrahamCampbell reviewed Aug 2, 2016
View reviewed changes

Removed return statement

4ffc484

GrahamCampbell reviewed Aug 2, 2016
View reviewed changes

cs

91a031a

GrahamCampbell reviewed Aug 2, 2016
View reviewed changes

Nyholm added 2 commits August 2, 2016 15:45

Added comment

75db148

Removed period

dfa5f17

Added docs about isset

cc8ac75

sagikazarmark merged commit 4bf191d into master Aug 2, 2016

sagikazarmark deleted the issue-2 branch August 2, 2016 20:55

This was referenced Aug 3, 2016

Bumped branch alias #15

Merged

changelog for cache validation #16

Merged

Nyholm mentioned this pull request Aug 5, 2016

Use sha1 to reduce the risk of collisions #12

Merged

3 tasks

+                          }
+                      }
+                      return;

Added support for Last-Modified and ETag #8

Added support for Last-Modified and ETag #8

Uh oh!

Conversation

Nyholm commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this PR?

To Do

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Jul 29, 2016

Uh oh!

Nyholm commented Jul 29, 2016

Uh oh!

Nyholm commented Jul 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Jul 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Aug 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GrahamCampbell commented Aug 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Aug 2, 2016

Uh oh!

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

Uh oh!

Uh oh!

Nyholm commented Aug 2, 2016

Uh oh!

sagikazarmark commented Aug 2, 2016

Uh oh!

Nyholm commented Aug 2, 2016

Uh oh!

Uh oh!

Nyholm commented Jul 29, 2016 •

edited

Loading

Nyholm commented Aug 2, 2016 •

edited

Loading