Skip to content

avoid duplicate requests to the cache proxy. #126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 14, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
},
"extra": {
"branch-alias": {
"dev-master": "1.0.x-dev"
"dev-master": "1.1.x-dev"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a new feature but no BC break (the behaviour changes, but not sending duplicate requests sounds like it should not break anything) - so i bump the minor version.

}
}
}
21 changes: 20 additions & 1 deletion src/ProxyClient/AbstractProxyClient.php
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,26 @@ public function flush()
*/
protected function queueRequest($method, $url, array $headers = array())
{
$this->queue[] = $this->createRequest($method, $url, $headers);
$signature = $this->getSignature($method, $url, $headers);
if (!isset($this->queue[$signature])) {
$this->queue[$signature] = $this->createRequest($method, $url, $headers);
}
}

/**
* Calculate a unique hash for the request, based on all significant information.
*
* @param string $method HTTP method
* @param string $url URL
* @param array $headers HTTP headers
*
* @return string A hash value for this request.
*/
private function getSignature($method, $url, array $headers)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this means that if somebody overwrites queueRequest, he will either have to copy this method or not use a hash. should we make it protected?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the queue is private, so you have to call the parent method anyway if you overwrite it. queueRequest is not designed to be overwritten in child classes, but to be called in child classes

{
ksort($headers);

return md5($method . "\n" . $url . "\n" . var_export($headers, true));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now respecting the headers too.

i opt to not play around with upper/lowercase as it complicates the code (and makes it slower) for no value unless people do really weird code, in which case they just get duplicate requests but no errors.

}

/**
Expand Down
35 changes: 35 additions & 0 deletions tests/Unit/ProxyClient/VarnishTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,41 @@ function ($requests) use ($self) {
);
}

public function testEliminateDuplicates()
{
$self = $this;
$client = \Mockery::mock('\Guzzle\Http\Client[send]', array('', null))
->shouldReceive('send')
->once()
->with(
\Mockery::on(
function ($requests) use ($self) {
/** @type Request[] $requests */
$self->assertCount(4, $requests);
foreach ($requests as $request) {
$self->assertEquals('PURGE', $request->getMethod());
}

return true;
}
)
)
->getMock();

$varnish = new Varnish(array('127.0.0.1', '127.0.0.2'), 'fos.lo', $client);

$this->assertEquals(
2,
$varnish
->purge('/c', array('a' => 'b', 'c' => 'd'))
->purge('/c', array('c' => 'd', 'a' => 'b')) // same request (header order is not significant)
->purge('/c') // different request as headers different
->purge('/c')
->flush()
);
}


protected function setUp()
{
$this->mock = new MockPlugin();
Expand Down