From 74964e7d855673052c51ed67279d331ea4c471e5 Mon Sep 17 00:00:00 2001 From: Menno Holtkamp Date: Mon, 13 Apr 2015 23:31:19 +0200 Subject: [PATCH 1/6] Improved testing region->getMultiple() --- .../Tests/ORM/Cache/DefaultRegionTest.php | 25 +++++++++++++++++++ .../Tests/ORM/Cache/MultiGetRegionTest.php | 7 ++++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Cache/DefaultRegionTest.php b/tests/Doctrine/Tests/ORM/Cache/DefaultRegionTest.php index 94cc99e35..914bcf191 100644 --- a/tests/Doctrine/Tests/ORM/Cache/DefaultRegionTest.php +++ b/tests/Doctrine/Tests/ORM/Cache/DefaultRegionTest.php @@ -3,10 +3,12 @@ namespace Doctrine\Tests\ORM\Cache; use Doctrine\Common\Cache\ArrayCache; +use Doctrine\ORM\Cache\CollectionCacheEntry; use Doctrine\ORM\Cache\Region\DefaultRegion; use Doctrine\Tests\Mocks\CacheEntryMock; use Doctrine\Tests\Mocks\CacheKeyMock; + /** * @group DDC-2183 */ @@ -72,4 +74,27 @@ class DefaultRegionTest extends AbstractRegionTest $region->evictAll(); } + + public function testGetMulti() + { + $key1 = new CacheKeyMock('key.1'); + $value1 = new CacheEntryMock(array('id' => 1, 'name' => 'bar')); + + $key2 = new CacheKeyMock('key.2'); + $value2 = new CacheEntryMock(array('id' => 2, 'name' => 'bar')); + + $this->assertFalse($this->region->contains($key1)); + $this->assertFalse($this->region->contains($key2)); + + $this->region->put($key1, $value1); + $this->region->put($key2, $value2); + + $this->assertTrue($this->region->contains($key1)); + $this->assertTrue($this->region->contains($key2)); + + $actual = $this->region->getMultiple(new CollectionCacheEntry(array($key1, $key2))); + + $this->assertEquals($value1, $actual[0]); + $this->assertEquals($value2, $actual[1]); + } } \ No newline at end of file diff --git a/tests/Doctrine/Tests/ORM/Cache/MultiGetRegionTest.php b/tests/Doctrine/Tests/ORM/Cache/MultiGetRegionTest.php index 4c3258a12..091ec672a 100644 --- a/tests/Doctrine/Tests/ORM/Cache/MultiGetRegionTest.php +++ b/tests/Doctrine/Tests/ORM/Cache/MultiGetRegionTest.php @@ -22,10 +22,10 @@ class MultiGetRegionTest extends AbstractRegionTest public function testGetMulti() { $key1 = new CacheKeyMock('key.1'); - $value1 = new CacheEntryMock(array('id'=>1, 'name' => 'bar')); + $value1 = new CacheEntryMock(array('id' => 1, 'name' => 'bar')); $key2 = new CacheKeyMock('key.2'); - $value2 = new CacheEntryMock(array('id'=>2, 'name' => 'bar')); + $value2 = new CacheEntryMock(array('id' => 2, 'name' => 'bar')); $this->assertFalse($this->region->contains($key1)); $this->assertFalse($this->region->contains($key2)); @@ -33,6 +33,9 @@ class MultiGetRegionTest extends AbstractRegionTest $this->region->put($key1, $value1); $this->region->put($key2, $value2); + $this->assertTrue($this->region->contains($key1)); + $this->assertTrue($this->region->contains($key2)); + $actual = $this->region->getMultiple(new CollectionCacheEntry(array($key1, $key2))); $this->assertEquals($value1, $actual[0]); From c236a670961d6501695a22df1a2668fbbf4f67b7 Mon Sep 17 00:00:00 2001 From: Menno Holtkamp Date: Mon, 13 Apr 2015 23:33:09 +0200 Subject: [PATCH 2/6] Used index as key to retrieve proper entry --- lib/Doctrine/ORM/Cache/Region/DefaultRegion.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php b/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php index f4525ee11..94a4f56ba 100644 --- a/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php +++ b/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php @@ -120,7 +120,7 @@ class DefaultRegion implements Region $returnableItems = array(); foreach ($keysToRetrieve as $index => $key) { - $returnableItems[$index] = $items[$key]; + $returnableItems[$index] = $items[$index]; } return $returnableItems; From 012f33524bca44e2db417c76e85ac45011132481 Mon Sep 17 00:00:00 2001 From: Menno Holtkamp Date: Tue, 14 Apr 2015 09:43:22 +0200 Subject: [PATCH 3/6] Fixed some typo's --- lib/Doctrine/ORM/Cache/MultiGetRegion.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Doctrine/ORM/Cache/MultiGetRegion.php b/lib/Doctrine/ORM/Cache/MultiGetRegion.php index 6de9c888d..be32b08f0 100644 --- a/lib/Doctrine/ORM/Cache/MultiGetRegion.php +++ b/lib/Doctrine/ORM/Cache/MultiGetRegion.php @@ -23,7 +23,7 @@ namespace Doctrine\ORM\Cache; /** * Defines a region that supports multi-get reading. * - * With one method call we can get multipe items. + * With one method call we can get multiple items. * * @since 2.5 * @author Asmir Mustafic @@ -31,7 +31,7 @@ namespace Doctrine\ORM\Cache; interface MultiGetRegion { /** - * Get all items from the cache indentifed by $keys. + * Get all items from the cache identified by $keys. * It returns NULL if some elements can not be found. * * @param CollectionCacheEntry $collection The collection of the items to be retrieved. From dbc29d28d2eeaa891e7fc11809330049091d43e4 Mon Sep 17 00:00:00 2001 From: Menno Holtkamp Date: Tue, 14 Apr 2015 09:47:57 +0200 Subject: [PATCH 4/6] Simplified way to fetch multiple entries when index does not matter --- .../ORM/Cache/Region/DefaultRegion.php | 29 ++++++------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php b/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php index 94a4f56ba..b210a8d8d 100644 --- a/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php +++ b/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php @@ -100,30 +100,19 @@ class DefaultRegion implements Region */ public function getMultiple(CollectionCacheEntry $collection) { - $keysToRetrieve = array(); + $result = array(); - foreach ($collection->identifiers as $index => $key) { - $keysToRetrieve[$index] = $this->name . '_' . $key->hash; - } - - $items = array_filter( - array_map([$this->cache, 'fetch'], $keysToRetrieve), - function ($retrieved) { - return false !== $retrieved; + foreach ($collection->identifiers as $key) { + $entry = $this->cache->fetch($this->name . '_' . $key->hash); + if ($entry === false) { + $result = null; + break; + } else { + $result[] = $entry; } - ); - - if (count($items) !== count($keysToRetrieve)) { - return null; } - $returnableItems = array(); - - foreach ($keysToRetrieve as $index => $key) { - $returnableItems[$index] = $items[$index]; - } - - return $returnableItems; + return $result; } /** From 34b6ce925900c77b9e96247ac84a91d3a6984fe7 Mon Sep 17 00:00:00 2001 From: Menno Holtkamp Date: Tue, 14 Apr 2015 09:52:25 +0200 Subject: [PATCH 5/6] Introduced getCacheEntryKey() to combine region name and cache key --- .../Cache/Region/DefaultMultiGetRegion.php | 4 +++- .../ORM/Cache/Region/DefaultRegion.php | 21 ++++++++++++++----- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/Doctrine/ORM/Cache/Region/DefaultMultiGetRegion.php b/lib/Doctrine/ORM/Cache/Region/DefaultMultiGetRegion.php index 39bf9c9e8..7ecf73311 100644 --- a/lib/Doctrine/ORM/Cache/Region/DefaultMultiGetRegion.php +++ b/lib/Doctrine/ORM/Cache/Region/DefaultMultiGetRegion.php @@ -57,8 +57,9 @@ class DefaultMultiGetRegion extends DefaultRegion public function getMultiple(CollectionCacheEntry $collection) { $keysToRetrieve = array(); + foreach ($collection->identifiers as $index => $key) { - $keysToRetrieve[$index] = $this->name . '_' . $key->hash; + $keysToRetrieve[$index] = $this->getCacheEntryKey($key); } $items = $this->cache->fetchMultiple($keysToRetrieve); @@ -70,6 +71,7 @@ class DefaultMultiGetRegion extends DefaultRegion foreach ($keysToRetrieve as $index => $key) { $returnableItems[$index] = $items[$key]; } + return $returnableItems; } } diff --git a/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php b/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php index b210a8d8d..324e27544 100644 --- a/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php +++ b/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php @@ -36,6 +36,8 @@ use Doctrine\ORM\Cache\Region; */ class DefaultRegion implements Region { + const REGION_KEY_SEPARATOR = '_'; + /** * @var CacheAdapter */ @@ -84,7 +86,7 @@ class DefaultRegion implements Region */ public function contains(CacheKey $key) { - return $this->cache->contains($this->name . '_' . $key->hash); + return $this->cache->contains($this->getCacheEntryKey($key)); } /** @@ -92,7 +94,7 @@ class DefaultRegion implements Region */ public function get(CacheKey $key) { - return $this->cache->fetch($this->name . '_' . $key->hash) ?: null; + return $this->cache->fetch($this->getCacheEntryKey($key)) ?: null; } /** @@ -103,7 +105,7 @@ class DefaultRegion implements Region $result = array(); foreach ($collection->identifiers as $key) { - $entry = $this->cache->fetch($this->name . '_' . $key->hash); + $entry = $this->cache->fetch($this->getCacheEntryKey($key)); if ($entry === false) { $result = null; break; @@ -115,12 +117,21 @@ class DefaultRegion implements Region return $result; } + /** + * @param CacheKey $key + * @return string + */ + protected function getCacheEntryKey(CacheKey $key) + { + return $this->name . self::REGION_KEY_SEPARATOR . $key->hash; + } + /** * {@inheritdoc} */ public function put(CacheKey $key, CacheEntry $entry, Lock $lock = null) { - return $this->cache->save($this->name . '_' . $key->hash, $entry, $this->lifetime); + return $this->cache->save($this->getCacheEntryKey($key), $entry, $this->lifetime); } /** @@ -128,7 +139,7 @@ class DefaultRegion implements Region */ public function evict(CacheKey $key) { - return $this->cache->delete($this->name . '_' . $key->hash); + return $this->cache->delete($this->getCacheEntryKey($key)); } /** From 5f891435f1c94db9e074407a74815e1f37ab32ec Mon Sep 17 00:00:00 2001 From: Menno Holtkamp Date: Tue, 14 Apr 2015 16:02:36 +0200 Subject: [PATCH 6/6] Use early return --- lib/Doctrine/ORM/Cache/Region/DefaultRegion.php | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php b/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php index 324e27544..3f214d0b0 100644 --- a/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php +++ b/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php @@ -105,13 +105,14 @@ class DefaultRegion implements Region $result = array(); foreach ($collection->identifiers as $key) { - $entry = $this->cache->fetch($this->getCacheEntryKey($key)); - if ($entry === false) { - $result = null; - break; - } else { - $result[] = $entry; + $entryKey = $this->getCacheEntryKey($key); + $entryValue = $this->cache->fetch($entryKey); + + if ($entryValue === false) { + return null; } + + $result[] = $entryValue; } return $result;