Update CachedHttpClient.php #265

jrean · 2015-04-30T09:56:45Z

I used your implementation of the filesystem cache.

This issue comes from the use of $path:

protected function createRequest($httpMethod, $path, $body = null, array $headers = array(), array $options = array())

Here:

if ($modifiedAt = $this->getCache()->getModifiedSince($path)) {

And here:

if ($etag = $this->getCache()->getETag($path)) {

And

public function request($path, $body = null, $httpMethod = 'GET', array $headers = array(), array $options = array())

Here:

if (304 == $response->getStatusCode()) {
    $cacheResponse = $this->getCache()->get($path);
    $this->lastCachedResponse = $cacheResponse;
    return $cacheResponse;
}
$this->getCache()->set($path, $response);

$path doesn't take care of the $options array which contains the sub array for the query parameters!
So each time getModifiedSince($path) and getETag($path) will work with an incomplete path because it doesn't know query parameters (if they exists). $path value will always be repos/:user/:repo/method and it should be repos/:user/:repo/method[?:encoded_query_parameters]

See my code modifications to fix the problem.

By applying this patch we can now use the cache as it should be :)

I started this issue a few days ago.
#262

I used your implementation of the filesystem cache. - Delete all files under the cache directory `github-api-cache`. - Set properly your authentication method (I used token). - Run the following code once after a proper Client instantiation with cache: ``` $commits_article_1 = ResponseMediator::getContent($client->getHttpClient()->get("repos/jrean/askjong.com/commits?sha=master&path=articles/hello-world.md")); $rateLimit = ResponseMediator::getContent($client->getHttpClient()->get("rate_limit")); var_dump($rateLimit); $commits_article_2 = ResponseMediator::getContent($client->getHttpClient()->get("repos/jrean/askjong.com/commits?sha=master&path=articles/this-is-a-first-article.md")); $rateLimit = ResponseMediator::getContent($client->getHttpClient()->get("rate_limit")); var_dump($rateLimit); ``` 4 requests against Github Api: - 1 query to fetch the commits for the `articles/hello-world.md` file. (`Guzzle\Http\Message\Response` status code is 200) - 1 query to fetch the commits for the `articles/this-is-a-first-article.md` file. (`Guzzle\Http\Message\Response` status code is 200) - 2 queries to fetch the rate_limit (doesn't count again the rate limit). (`Guzzle\Http\Message\Response` status code is 200) Once this code has run the cache directory `github-api-cache` will have 6 files (3 for each queries including the `rate_limit` query + 3 `.etag` files) **The `rate_limit` should / must decrease by 2** (a call to `rate_limit` doesn't count against Github). -- - Run the same code again. 4 requests against Github Api: - 1 query to fetch the commits for the `articles/hello-world.md` file. (`Guzzle\Http\Message\Response` status code is 304, cache is used) - 1 query to fetch the commits for the `articles/this-is-a-first-article.md` file. (`Guzzle\Http\Message\Response` status code is 304, cache is user) - 2 queries to fetch the rate_limit (doesn't count again the rate limit). (`Guzzle\Http\Message\Response` status code is 200) **The `rate_limit` don't move** (a call to `rate_limit` doesn't count against Github). -- - Run the following code which use your wrapping methods against Github Api (Notice we are requesting the same informations): **It should use the cache but it will not.** (I explain why just after). ``` $commits_article_1 = $client->api('repo')->commits()->all('jrean', 'askjong.com', array('sha' => 'master', 'path' => 'articles/hello-world.md')); $rateLimit = ResponseMediator::getContent($client->getHttpClient()->get("rate_limit")); var_dump($rateLimit); $commits_article_2 = $client->api('repo')->commits()->all('jrean', 'askjong.com', array('sha' => 'master', 'path' => 'articles/this-is-a-first-article.md')); $rateLimit = ResponseMediator::getContent($client->getHttpClient()->get("rate_limit")); var_dump($rateLimit); ``` 4 requests against Github Api: - 1 query to fetch the commits for the `articles/hello-world.md` file. (`Guzzle\Http\Message\Response` status code is 200) - 1 query to fetch the commits for the `articles/this-is-a-first-article.md` file. (`Guzzle\Http\Message\Response` status code is 200) - 2 queries to fetch the rate_limit (doesn't count again the rate limit). (`Guzzle\Http\Message\Response` status code is 200) Once this code has run the cache directory `github-api-cache` will have new files... **The `rate_limit` decreases by 2** (a call to `rate_limit` doesn't count against Github). It should not decrease because we should use the cache! -- - Run the same code again. 4 requests against Github Api: - 1 query to fetch the commits for the `articles/hello-world.md` file. (`Guzzle\Http\Message\Response` status code is 200) - 1 query to fetch the commits for the `articles/this-is-a-first-article.md` file. (`Guzzle\Http\Message\Response` status code is 200) - 2 queries to fetch the rate_limit (doesn't count again the rate limit). (`Guzzle\Http\Message\Response` status code is 200) **The `rate_limit` decreases by 2 again** (a call to `rate_limit` doesn't count against Github). It should not decrease because we should use the cache! - Run the same code again and it will still decrease your `rate_limit`... -- # Solution This issue comes from the use of `$path`: `protected function createRequest($httpMethod, $path, $body = null, array $headers = array(), array $options = array())` Then here: `if ($modifiedAt = $this->getCache()->getModifiedSince($path)) {` And: `if ($etag = $this->getCache()->getETag($path)) {` `$path` doesn't take care of the `$options` array which contains the sub array for the `query` parameters! So each time `getModifiedSince($path)` and `getETag($path)` will work with an incomplete path because it doesn't know query parameters (if they exists). In that exemple `$path` value will always be `repos/jrean/askjong.com/commits` and it should be `repos/jrean/askjong.com/commits?sha=master&path=articles/hello-world.md`. See my code modifications to fix the problem. (I didn't use `$request->getQuery()` because it returns query parameters but encoded and it is not acceptable because when we want to construct our own queries like `repos/jrean/askjong.com/commits?sha=master&path=articles/hello-world.md` we won't encode parameters) By applying this patch we can now use the cache as it should be :) I started this issue a few days ago. #262

stof · 2015-04-30T13:05:51Z

lib/Github/HttpClient/CachedHttpClient.php

+     *
+     * @var string
+     */
+    private $path;


please don't store this in a property. The more state is added in the class, the more complex reasonning about it is (I already think that having getLastResponse in the HttpClientInterface is a big mistake).

and btw, the naming is wrong: the value stored in this property is not the path

@stof

please don't store this in a property.

I added it in a property because it is called in two methods ! request() and createRequest(). What do you suggest so? You prefer a private method that returns something called into request() and createRequest() methods? Properties exists to be used and in that class only 2 already exist (plus eventually mine) then 4 for the parent class. This is acceptable don't you think?

"and btw, the naming is wrong: the value stored in this property is not the path"

$path is implemented everywhere and in other places it is called $id...
When I choose $this->path it was to respect what as already been implemented. The original variable was $path as you can see.
How do you want to call $path + query (if query parameters exist)?
By suggesting this pull request I'm not fighting for naming but simply trying to fix something that doesn't work as it should be. I'm totally open for constructive suggestions. What do you suggest so?

@stof

Some cleaning according the chat with @stof + format the identifier (`$id`) with encoded query parameters.

jrean · 2015-05-01T13:12:37Z

@stof Can I have your feedback?

jrean · 2015-05-05T05:51:11Z

@pilot can we merge it please? (I need the fix)

Update CachedHttpClient.php

jrean · 2015-05-05T08:44:48Z

Thank you.

pilot · 2015-05-05T08:45:26Z

@jrean good catch 👍

jrean · 2015-05-05T09:06:52Z

@pilot can you release the 1.4.8 with that patch?
https://github.com/GrahamCampbell/Laravel-GitHub/blob/master/composer.json
GrahamCampbell/Laravel-GitHub is using "knplabs/github-api": "~1.3" so he won't get the patch until the next version.

Thank you :)

pilot · 2015-05-05T09:08:19Z

https://github.com/KnpLabs/php-github-api/releases/tag/1.4.8

jrean · 2015-05-05T09:09:19Z

You rock!

jrean mentioned this pull request Apr 30, 2015

X-RateLimit-Remaining decreasing when cache should be used #262

Closed

stof reviewed Apr 30, 2015
View reviewed changes

Update CachedHttpClient.php

ec371af

Some cleaning according the chat with @stof + format the identifier (`$id`) with encoded query parameters.

pilot added a commit that referenced this pull request May 5, 2015

Merge pull request #265 from jrean/patch-2

b53ff0c

Update CachedHttpClient.php

pilot merged commit b53ff0c into KnpLabs:master May 5, 2015

jrean deleted the patch-2 branch May 5, 2015 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Update CachedHttpClient.php #265

Update CachedHttpClient.php #265

Uh oh!

jrean commented Apr 30, 2015

Uh oh!

stof Apr 30, 2015

Uh oh!

stof Apr 30, 2015

Uh oh!

jrean Apr 30, 2015

Uh oh!

jrean commented May 1, 2015

Uh oh!

jrean commented May 5, 2015

Uh oh!

jrean commented May 5, 2015

Uh oh!

pilot commented May 5, 2015

Uh oh!

jrean commented May 5, 2015

Uh oh!

pilot commented May 5, 2015

Uh oh!

jrean commented May 5, 2015

Uh oh!

Uh oh!

Uh oh!

Update CachedHttpClient.php #265

Update CachedHttpClient.php #265

Uh oh!

Conversation

jrean commented Apr 30, 2015

Uh oh!

stof Apr 30, 2015

Choose a reason for hiding this comment

Uh oh!

stof Apr 30, 2015

Choose a reason for hiding this comment

Uh oh!

jrean Apr 30, 2015

Choose a reason for hiding this comment

Uh oh!

jrean commented May 1, 2015

Uh oh!

jrean commented May 5, 2015

Uh oh!

jrean commented May 5, 2015

Uh oh!

pilot commented May 5, 2015

Uh oh!

jrean commented May 5, 2015

Uh oh!

pilot commented May 5, 2015

Uh oh!

jrean commented May 5, 2015

Uh oh!

Uh oh!