Optionally enable caching of any request method #24

tuupola · 2017-02-10T09:15:22Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Documentation	php-http/documentation#174
License	MIT

What's in this PR?

Adds new methods setting which allows you to change which request methods are allowed to be cached.

Why?

Sometimes it is desirable to cache POST requests. For example when using HTTPlug based SOAP client. I am currently using this in live system for SOAP requests.

Example Usage

To enable caching of GET and POST requests you can do the following.

$cache = new CachePlugin($pool, $streamFactory, [
    "default_ttl" => 3600 * 48,
    "respect_cache_headers" => true,
    "methods" => ["GET", "POST"]
]);

Checklist

Updated CHANGELOG.md to describe new feature.
Documentation pull request created.

To Do

Should PUT and PATCH et al. also be allowed if user requests so?
Make sure tests are actually ok.

This is useful for example when using a HTTPlug based SoapClient. php-http/client-common#26

dbu

my first reaction was "wtf, why?", then i read soap and think thats a reasonable usecase. and after some more thinking, i actually feel we should allow arbitrary methods to be cached if we allow to configure that at all.

dbu · 2017-02-10T15:04:04Z

src/CachePlugin.php

        $resolver->setAllowedValues('hash_algo', hash_algos());
+        $resolver->setAllowedValues('methods', function ($value) {
+            return 0 === count(array_diff($value, ['GET', 'HEAD', 'POST']));


i think once we allow to configure, we should allow any methods. we need a big warning sign for this anyways, as people should know what they do before caching post and other potentially modifying requests... for example, it could make sense to cache a PROPFIND request (webdav, its a non-modifying request)...

I agree. Question is since RFC2616 says valid request method can be pretty much any string (not sure if A..Z or alphanumeric) should any request method be allowed or just the common methods defined in RFC2616-sec9

i'd not do any validation apart from requiring an array of strings.

Found the spec. RFC2616 says request method should be a token and RFC7230 defines token to be any visible USASCII character except delimiters "(),/:;<=>?@[\]{}.

dbu · 2017-02-10T15:05:35Z

src/CachePlugin.php

@@ -225,6 +224,10 @@ private function getCacheControlDirective(ResponseInterface $response, $name)
     */
    private function createCacheKey(RequestInterface $request)
    {
+        if ('POST' === $request->getMethod()) {
+            return hash($this->config['hash_algo'], $request->getMethod().' '.$request->getUri().' '.$request->getBody());


i don't like that we specifically do this for POST (as i want to support configuring all request methods as cacheable). i think using the body in hash should not be an issue - its usually just empty. technically, GET requests can have a body as well, for example with elasticsearch. so its actually an improvement to also use the body for the hash.

Agree on this one too. However some might consider changing the way how keys are generated a BC break. See discussion in #12. Although if I read it correctly the consensus in the end was this would not be considered a BC break.

i would not consider that a BC break. chances are you replace the cache on a new deployment anyways, and even if not, its just a cache.

So what's the verdict? Do we change this to allow any methods?

dbu · 2017-02-10T15:09:09Z

btw, caching proxies like varnish usually do a GET when seeing a HEAD request they have no cache for. then they store the response and return just the headers to honor the HEAD request. but i don't think we want to do that here. if people will do a GET anyways, they should not do a HEAD request first. they might be implementing their own cache, but then they should probably not use the cache-plugin.

sagikazarmark · 2017-02-10T20:59:17Z

spec/CachePluginSpec.php

@@ -97,6 +97,49 @@ function it_doesnt_store_post_requests(CacheItemPoolInterface $pool, CacheItemIn
        $this->handleRequest($request, $next, function () {});
    }

+    function it_stores_post_requests_when_allowed(CacheItemPoolInterface $pool, CacheItemInterface $item, RequestInterface $request, ResponseInterface $response, StreamFactory $streamFactory, StreamInterface $stream)


Can you please format this according to PSR-1/2?

There were some other PSR-2 related problems too so I made a separate PR #25.

sagikazarmark · 2017-02-10T21:00:00Z

spec/CachePluginSpec.php

+
+        $response->getStatusCode()->willReturn(200);
+        $response->getBody()->willReturn($stream);
+        $response->getHeader('Cache-Control')->willReturn(array())->shouldBeCalled();


Please use short array syntax (StyleCI is turned off for spec files because of the snake_case).

I was following the CS of the rest of the file. Long array() syntax is used elsewhere the spec file.

sagikazarmark · 2017-02-10T21:00:11Z

spec/CachePluginSpec.php

+        $response->getHeader('Expires')->willReturn(array())->shouldBeCalled();
+        $response->getHeader('ETag')->willReturn(array())->shouldBeCalled();
+
+        $pool->getItem('e4311a9af932c603b400a54efab21b6d7dea7a90')->shouldBeCalled()->willReturn($item);


Please split this too

sagikazarmark · 2017-02-10T21:01:12Z

src/CachePlugin.php

@@ -225,6 +224,10 @@ private function getCacheControlDirective(ResponseInterface $response, $name)
     */
    private function createCacheKey(RequestInterface $request)
    {
+        if ('POST' === $request->getMethod()) {
+            return hash($this->config['hash_algo'], $request->getMethod().' '.$request->getUri().' '.$request->getBody());


So what's the verdict? Do we change this to allow any methods?

sagikazarmark · 2017-02-10T21:02:59Z

Yeah, this one was a shocker for me at first too. 😄

For the record: we should probably rethink our BDD(-misuse) strategy and rather use PHPUnit or something. Tests are almost unreadable with all the PSR-7 mocks.

Request method can contain any visible USASCII character except delimiters "(),/:;<=>?@[\]{} https://tools.ietf.org/html/rfc7230#section-3.1.1 https://tools.ietf.org/html/rfc7230#section-3.2.6

tuupola · 2017-02-13T14:31:27Z

Updated the PR. It is now possible to enable caching for any arbitrary request method which conforms RFC7230. Also request body is always used when creating the cache key.

dbu · 2017-02-13T16:30:45Z

src/CachePlugin.php

        $resolver->setAllowedValues('hash_algo', hash_algos());
+        $resolver->setAllowedValues('methods', function ($value) {
+            $matches = preg_grep('/[^[:alnum:]!#$%&\'*\/+\-.^_`|~]/', $value);


can you please add a comment here with the RFC number that defines this regular expression? just for future reference and to make it clear we did not come up with a random expression here.

i am horrible with regular expressions. is this forcing upper case? we should either force that or uppercase ourselves when processing the options. PSR-7 forces method to upper case. when you specify something lower case, it would never match.

the doc should explain that if you specify methods, you also need to specify GET if you still want those cached. and that you should do specific clients with different setup if you do soap and other stuff, to be sure only the right requests get cached...

It does not force uppercase. [:alnum:] is alias for [0-9A-Za-z]. RFC2616 says method is case-insensitive so technically both get and GET should be valid but just different methods.

I have no strong feelings whether we should force uppercase or not. I think most of the time people probably use uppercase methods.

The regexp basically says is request method has any other characters than these the method is invalid.

dbu · 2017-02-13T16:32:55Z

cool, i like this.

commits need to be squashed and we should have a documentation pull request (no big thing, just mention the feature in the right place). then its ready to merge imo

tuupola · 2017-02-14T06:39:46Z

Documentation added. GitHub now has squash before merge feature which I think is better than doing squash + force push myself.

dbu · 2017-02-14T13:09:54Z

It does not force uppercase. |[:alnum:]| is alias for |[0-9A-Za-z]|.
RFC2616 https://tools.ietf.org/html/rfc2616#section-5.1.1 says method
is case-insensitive so technically both |get| and |GET| should be valid
but just different methods.

I have no strong feelings whether we should force uppercase or not. I
think most of the time people probably use uppercase methods.

PSR-7 says:

 * While HTTP method names are typically all uppercase characters, HTTP
 * method names are case-sensitive and thus implementations SHOULD NOT
 * modify the given string.

but for example guzzle psr-7 says:
https://github.com/guzzle/psr7/blob/5f17a79615bd36d52d3b3ce44075aec02605d8bf/src/Request.php#L92-L97

public function withMethod($method)
{
    $new = clone $this;
    $new->method = strtoupper($method);
    return $new;
}

i am surprised by this, but feel like then we should not force upper case but just make sure all our examples use all caps and maybe mention that the option is case sensitive.

dbu

please change the CachePlugin constructor phpdoc to mention the new option for method (and say that its case sensitive)

dbu · 2017-02-15T10:06:34Z

src/CachePlugin.php

@@ -45,6 +45,7 @@
     *     @var int $cache_lifetime (seconds) To support serving a previous stale response when the server answers 304
     *              we have to store the cache for a longer time than the server originally says it is valid for.
     *              We store a cache item for $cache_lifetime + max age of the response.
+     *     @var array $methods case insensitive list of request methods which can be cached.


its case sensitive, not insensitive

Ah yes of course. Brain malfunction...

dbu · 2017-02-15T10:32:29Z

looks good to me.

what do the others think? can we merge?

Nyholm

Great PR. Thanks. I had some questions.

Nyholm · 2017-02-16T13:27:23Z

src/CachePlugin.php

@@ -225,7 +225,7 @@ private function getCacheControlDirective(ResponseInterface $response, $name)
     */
    private function createCacheKey(RequestInterface $request)
    {
-        return hash($this->config['hash_algo'], $request->getMethod().' '.$request->getUri());
+        return hash($this->config['hash_algo'], $request->getMethod().' '.$request->getUri().$request->getBody());


What if the body is super large? Do we care?

Also, I suggest a space (or any other non-common delimiter) between uri and body.

Without space this is backwards compatible with previous versions if body is empty. Adding a space there will invalidate old caches, but it is of course ok with me if needed.

Great. I did not think about that. How about doing something like:

$body = (string) $request->getBody() if (!empty($body)) { $body = ' ' . $body; } return hash($this->config['hash_algo'], $request->getMethod().' '.$request->getUri().$body);

i think we do not need to worry about keeping caches valid over deployments. but nyholms idea is good. as GET usually has empty body, it happens to keep old caches.

regarding big body: this method is only executed on methods that should be cached, right? so body will be usually empty, except for things like soap which hopefully won't be big file uploads. you would not want to cache a file upload, anyways.

GET and HEAD has (EDIT: generally) no bodies. If you change the default behavior, then it is on you if you get performance issues.

I can add it. Why is the space needed BTW?

GET can have body, for example when talking to elasticsearch: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-body.html

To make a difference between these two requests

POST /foo Host: www.example.com bar

POST /foob Host: www.example.com ar

I feel we could unconditionally add the space. It is only cache so it is not that big deal if old cache gets invalidated when plugin is updated. What do you think?

Hm, sorry for the late response.

I would prefer not invalidating cache when we can.. Im using Google Translate paid services and I know I will have to pay 100USD if my cache gets invalidated.

Technically, this is okey. But we could be more nice to our users. =)

Nyholm · 2017-02-16T13:29:25Z

src/CachePlugin.php

@@ -45,6 +45,7 @@
     *     @var int $cache_lifetime (seconds) To support serving a previous stale response when the server answers 304
     *              we have to store the cache for a longer time than the server originally says it is valid for.
     *              We store a cache item for $cache_lifetime + max age of the response.
+     *     @var array $methods case sensitive list of request methods which can be cached.


hm.. Yes, they are case sensitive but lower case will always fail..

@var array $methods list of request methods which can be cached. Methods should always be upper case.

Why it will fail?

when you use the guzzle psr implementation, the method is forced to uppercase. but i pasted the fragment from psr-7 somewhere here that says that the getMethod of the request SHOULD preserve case. so depending on implementations, you could need lower case headers. i think its correct to allow lower case and not transform case.

i think its correct to allow lower case and not transform case.

But that would lead to unexpected behavior. I think we should do a case insensitive comparison or force input to be uppercase.

Cache plugin also converts the request method to uppercase. While this is not technically correct it is probably ok for 99% of the use cases. I think the question is should we follow RFC or not.

oh, missed that. ok, that would be a BC break if we stop doing this...

whatever, lets uppercase the method and force configuration to use uppercase. (thats better than magically uppercasing and confusing people that expected some specific behaviour). i think this will be correct in 99.999999% of the cases. would be surprised if anybody ever wants GET to be cached but not get...

Nyholm · 2017-02-16T13:32:03Z

src/CachePlugin.php

        $resolver->setAllowedValues('hash_algo', hash_algos());
+        $resolver->setAllowedValues('methods', function ($value) {
+            /* Any VCHAR, except delimiters. RFC7230 sections 3.1.1 and 3.2.6 */
+            $matches = preg_grep('/[^[:alnum:]!#$%&\'*\/+\-.^_`|~]/', $value);


This looks really complicated. Why not use [A-Z]+. That will also make sure you use upper values. Or am I too specific?

Unless I have misunderstood the specs request method can also contain lowercase letters. Valid request methods are described in RFC7230-3.1.1 which states it is a token. Token is defined in RFC7230-3.2.6 which says any visible USASCII character except delimiters "(),/:;<=>?@[\]{}.

Sorry, yes you are correct. (Reading the code and RFC more carefully...). I was wrong assuming that !#$%& etc should not be a part of the HTTP method.

HTTP methods are generally uppercase but PSR-7 specifies that case should not change. I think we are correct by running strtoupper(). This implementation will, however, always fail when using a lower case method in the configuration. I suggest changing the regex to fail directly if a lower-case method are used.

$matches = preg_grep('/[^[A-Z0-9]!#$%&\'*\/+\-.^_`|~]/', $value);

were do we strtoupper? i think we should leave the method unchanged, given how psr-7 is specified. as mentioned above, guzzle psr-7 request does not follow the SHOULD of the spec.

Nyholm · 2017-02-16T14:50:28Z

See #24 (comment) and the method doc block. I think we should do a case insensitive comparison.

dbu · 2017-02-16T16:54:51Z

fine for me

Technically RFC7230 allows lowercase request methods but consensus was they might be too confusing for users.

Without delimiter the following requests would get the same cache key: POST /foo bar POST /foob ar

dbu · 2017-02-18T17:06:18Z

I would prefer not invalidating cache when we can.. Im using Google Translate paid services and I know I will have to pay 100USD if my cache gets invalidated.

Technically, this is okey. But we could be more nice to our users. =)

ok, i guess that is a good argument in favor of not concating ' '.$body if the body is empty (which it usually is for GET). sorry, can you please adjust that @tuupola ? i think then we are really ready. @sagikazarmark or do you see any issues that need to be adressed?

tuupola · 2017-02-19T04:43:26Z

Done. I will update the docs when I know this is frozen.

dbu · 2017-02-20T07:43:39Z

thanks a lot! lets wrap up the doc PR as well. work for more flexibility will continue in #3

Nyholm · 2017-02-20T07:57:02Z

Thank you @tuupola

tuupola added 2 commits February 10, 2017 12:01

Enable caching of POST requests when requested

52b1777

This is useful for example when using a HTTPlug based SoapClient. php-http/client-common#26

Make StyleCI happy

e0c1c0e

tuupola changed the title ~~Enable caching of POST requests when requested~~ Optionally enable caching of POST requests Feb 10, 2017

dbu approved these changes Feb 10, 2017

View reviewed changes

sagikazarmark requested changes Feb 10, 2017

View reviewed changes

tuupola mentioned this pull request Feb 11, 2017

PSR-2 compliance #25

Closed

tuupola added 5 commits February 11, 2017 12:58

PSR-2 and short array syntax

0a3b7c8

Allow any valid request method as defined by RFC7230

8151701

Request method can contain any visible USASCII character except delimiters "(),/:;<=>?@[\]{} https://tools.ietf.org/html/rfc7230#section-3.1.1 https://tools.ietf.org/html/rfc7230#section-3.2.6

Include request body in cache key calculation

baf4fbd

Make StyleCI happy

8d9d48c

PHP 5.4 does not have ::class

5b21d4b

tuupola changed the title ~~Optionally enable caching of POST requests~~ Optionally enable caching of any request method Feb 13, 2017

dbu reviewed Feb 13, 2017

View reviewed changes

Mention RFC7230 which defines valid request methods

cfa6135

dbu requested changes Feb 14, 2017

View reviewed changes

Add new methods option to docblock

8d8f8e9

dbu reviewed Feb 15, 2017

View reviewed changes

Request methods are case sensitive

fbc1dc7

dbu approved these changes Feb 15, 2017

View reviewed changes

Nyholm reviewed Feb 16, 2017

View reviewed changes

Do not allow lowercase request methods

e94b74a

Technically RFC7230 allows lowercase request methods but consensus was they might be too confusing for users.

tuupola added 2 commits February 17, 2017 21:24

Add delimiter between uri and body

65d819d

Without delimiter the following requests would get the same cache key: POST /foo bar POST /foob ar

Remove unneeded use statement

10da16c

dbu mentioned this pull request Feb 18, 2017

Cache-Control header handling #3

Closed

tuupola added 3 commits February 19, 2017 11:26

Maintain cache key bc with v1.2.0

9703885

Make StyleCI happy

490da8b

Make StyleCI happy

727cbf0

dbu mentioned this pull request Feb 20, 2017

Basic documentation of cache plugin methods option php-http/documentation#174

Merged

dbu merged commit c8dfcc5 into php-http:master Feb 20, 2017

acrobat mentioned this pull request Mar 3, 2017

Implement caching types #26

Merged

3 tasks

Optionally enable caching of any request method #24

Optionally enable caching of any request method #24

Conversation

tuupola commented Feb 10, 2017 • edited Loading

What's in this PR?

Why?

Example Usage

Checklist

To Do

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuupola Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark commented Feb 10, 2017 • edited Loading

tuupola commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Feb 13, 2017

tuupola commented Feb 14, 2017

dbu commented Feb 14, 2017

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Feb 15, 2017

Nyholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuupola Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuupola Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuupola Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Feb 16, 2017

dbu commented Feb 16, 2017 via email

dbu commented Feb 18, 2017

tuupola commented Feb 19, 2017

dbu commented Feb 20, 2017

Nyholm commented Feb 20, 2017

tuupola commented Feb 10, 2017 •

edited

Loading

tuupola Feb 10, 2017 •

edited

Loading

sagikazarmark commented Feb 10, 2017 •

edited

Loading

tuupola Feb 16, 2017 •

edited

Loading

Nyholm Feb 16, 2017 •

edited

Loading

tuupola Feb 16, 2017 •

edited

Loading

tuupola Feb 16, 2017 •

edited

Loading