From 4328a4e9e30dc4e69156652d0994b00261d8edbe Mon Sep 17 00:00:00 2001 From: romanb Date: Fri, 23 Oct 2009 15:03:00 +0000 Subject: [PATCH] [2.0] Small refactorings on the cache drivers. Introduced flag to control whether the cache driver should manage the cache keys since this is an advanced feature that is not always needed and can have negative side-effects (higher probability for cache slams). --- .../Common/Annotations/AnnotationReader.php | 4 +- lib/Doctrine/Common/Cache/AbstractCache.php | 68 ++++++++++++--- lib/Doctrine/DBAL/Connection.php | 82 +++++++++++-------- lib/Doctrine/ORM/AbstractQuery.php | 6 +- .../ORM/Mapping/ClassMetadataInfo.php | 4 +- lib/Doctrine/ORM/UnitOfWork.php | 2 + .../Tests/Common/Cache/ApcCacheTest.php | 1 + .../Tests/Common/Cache/ArrayCacheTest.php | 1 + .../Doctrine/Tests/Common/Cache/CacheTest.php | 7 ++ ...ManyToManyBidirectionalAssociationTest.php | 12 ++- .../Tests/ORM/Functional/QueryCacheTest.php | 1 + .../Tests/ORM/Functional/ResultCacheTest.php | 5 +- 12 files changed, 135 insertions(+), 58 deletions(-) diff --git a/lib/Doctrine/Common/Annotations/AnnotationReader.php b/lib/Doctrine/Common/Annotations/AnnotationReader.php index fa5e747fe..f7b91682f 100644 --- a/lib/Doctrine/Common/Annotations/AnnotationReader.php +++ b/lib/Doctrine/Common/Annotations/AnnotationReader.php @@ -62,8 +62,8 @@ class AnnotationReader private $_cache; /** - * Constructor. Initiaizes a new AnnotationReader that uses the given - * Cache provider to cache annotations. + * Constructor. Initializes a new AnnotationReader that uses the given + * Cache provider. * * @param Cache $cache The cache provider to use. */ diff --git a/lib/Doctrine/Common/Cache/AbstractCache.php b/lib/Doctrine/Common/Cache/AbstractCache.php index 961b69794..c1fe5fad1 100644 --- a/lib/Doctrine/Common/Cache/AbstractCache.php +++ b/lib/Doctrine/Common/Cache/AbstractCache.php @@ -22,7 +22,7 @@ namespace Doctrine\Common\Cache; /** - * Abstract cache driver class + * Base class for cache driver implementations. * * @license http://www.opensource.org/licenses/lgpl-license.php LGPL * @link www.doctrine-project.org @@ -34,14 +34,44 @@ namespace Doctrine\Common\Cache; */ abstract class AbstractCache implements Cache { - /* @var string $cacheIdsIndexId The cache id to store the index of cache ids under */ + /** @var string The cache id to store the index of cache ids under */ private $_cacheIdsIndexId = 'doctrine_cache_ids'; - /* @var string $namespace The namespace to prefix all cache ids with */ + /** @var string The namespace to prefix all cache ids with */ private $_namespace = null; + + /** @var boolean Whether to manage cache keys or not. */ + private $_manageCacheKeys = false; + + /** + * Sets whether cache keys should be managed by the cache driver + * separately from the cache entries. This allows more granular + * cache clearing through {@link deleteByPrefix}, {@link deleteByRegex}, + * {@link deleteBySuffix} and some other operations such as {@link count} + * and {@link getIds}. Managing cache keys comes at the cost of a higher + * probability for cache slams due to the single cache key used for + * managing all other keys. + * + * @param boolean $bool + */ + public function setManageCacheKeys($bool) + { + $this->_manageCacheKeys = $bool; + } + + /** + * Checks whether cache keys are managed by this cache driver. + * + * @return boolean + * @see setManageCacheKeys() + */ + public function getManageCacheKeys() + { + return $this->_manageCacheKeys; + } /** - * Set the namespace to prefix all cache ids with + * Set the namespace to prefix all cache ids with. * * @param string $namespace * @return void @@ -56,8 +86,7 @@ abstract class AbstractCache implements Cache */ public function fetch($id) { - $id = $this->_getNamespacedId($id); - return $this->_doFetch($id); + return $this->_doFetch($this->_getNamespacedId($id)); } /** @@ -65,8 +94,7 @@ abstract class AbstractCache implements Cache */ public function contains($id) { - $id = $this->_getNamespacedId($id); - return $this->_doContains($id); + return $this->_doContains($this->_getNamespacedId($id)); } /** @@ -76,7 +104,9 @@ abstract class AbstractCache implements Cache { $id = $this->_getNamespacedId($id); if ($this->_doSave($id, $data, $lifeTime)) { - $this->_saveId($id); + if ($this->_manageCacheKeys) { + $this->_saveId($id); + } return true; } @@ -95,7 +125,9 @@ abstract class AbstractCache implements Cache } if ($this->_doDelete($id)) { - $this->_deleteId($id); + if ($this->_manageCacheKeys) { + $this->_deleteId($id); + } return true; } @@ -109,6 +141,7 @@ abstract class AbstractCache implements Cache */ public function deleteAll() { + $this->_errorIfCacheKeysNotManaged(); $ids = $this->getIds(); foreach ($ids as $id) { $this->delete($id); @@ -124,6 +157,7 @@ abstract class AbstractCache implements Cache */ public function deleteByRegex($regex) { + $this->_errorIfCacheKeysNotManaged(); $deleted = array(); $ids = $this->getIds(); foreach ($ids as $id) { @@ -143,6 +177,7 @@ abstract class AbstractCache implements Cache */ public function deleteByPrefix($prefix) { + $this->_errorIfCacheKeysNotManaged(); $deleted = array(); $ids = $this->getIds(); foreach ($ids as $id) { @@ -162,6 +197,7 @@ abstract class AbstractCache implements Cache */ public function deleteBySuffix($suffix) { + $this->_errorIfCacheKeysNotManaged(); $deleted = array(); $ids = $this->getIds(); foreach ($ids as $id) { @@ -180,6 +216,7 @@ abstract class AbstractCache implements Cache */ public function count() { + $this->_errorIfCacheKeysNotManaged(); $ids = $this->getIds(); return $ids ? count($ids) : 0; } @@ -191,6 +228,7 @@ abstract class AbstractCache implements Cache */ public function getIds() { + $this->_errorIfCacheKeysNotManaged(); $ids = $this->fetch($this->_cacheIdsIndexId); return $ids ? $ids : array(); } @@ -243,6 +281,16 @@ abstract class AbstractCache implements Cache } return false; } + + /** + * @throws BadMethodCallException If the cache driver does not manage cache keys. + */ + private function _errorIfCacheKeysNotManaged() + { + if ( ! $this->_manageCacheKeys) { + throw new \BadMethodCallException("Operation not supported if cache keys are not managed."); + } + } /** * Fetches an entry from the cache. diff --git a/lib/Doctrine/DBAL/Connection.php b/lib/Doctrine/DBAL/Connection.php index 186f25d46..7aaca52b3 100644 --- a/lib/Doctrine/DBAL/Connection.php +++ b/lib/Doctrine/DBAL/Connection.php @@ -96,35 +96,28 @@ class Connection * * @var boolean */ - protected $_isConnected = false; + private $_isConnected = false; /** * The transaction nesting level. * * @var integer */ - protected $_transactionNestingLevel = 0; + private $_transactionNestingLevel = 0; /** * The currently active transaction isolation level. * * @var integer */ - protected $_transactionIsolationLevel; + private $_transactionIsolationLevel; /** * The parameters used during creation of the Connection instance. * * @var array */ - protected $_params = array(); - - /** - * The query count. Represents the number of executed database queries by the connection. - * - * @var integer - */ - protected $_queryCount = 0; + private $_params = array(); /** * The DatabasePlatform object that provides information about the @@ -147,6 +140,13 @@ class Connection * @var Doctrine\DBAL\Driver */ protected $_driver; + + /** + * Flag that indicates whether the current transaction is marked for rollback only. + * + * @var boolean + */ + private $_isRollbackOnly = false; /** * Initializes a new instance of the Connection class. @@ -602,8 +602,6 @@ class Connection $stmt = $this->_conn->query($query); } - $this->_queryCount++; - return $stmt; } @@ -629,22 +627,10 @@ class Connection } else { $result = $this->_conn->exec($query); } - - $this->_queryCount++; return $result; } - /** - * Returns the number of queries executed by the connection. - * - * @return integer - */ - public function getQueryCount() - { - return $this->_queryCount; - } - /** * Returns the current transaction nesting level. * @@ -702,7 +688,7 @@ class Connection * if trying to set a savepoint and there is no active transaction * a new transaction is being started. * - * @return boolean + * @return void */ public function beginTransaction() { @@ -713,8 +699,6 @@ class Connection } ++$this->_transactionNestingLevel; - - return true; } /** @@ -722,13 +706,17 @@ class Connection * progress or release a savepoint. This function may only be called when * auto-committing is disabled, otherwise it will fail. * - * @return boolean FALSE if commit couldn't be performed, TRUE otherwise + * @return void + * @throws ConnectionException If the commit failed. */ public function commit() { if ($this->_transactionNestingLevel == 0) { throw ConnectionException::commitFailedNoActiveTransaction(); } + if ($this->_isRollbackOnly) { + throw ConnectionException::commitFailedRollbackOnly(); + } $this->connect(); @@ -737,8 +725,6 @@ class Connection } --$this->_transactionNestingLevel; - - return true; } /** @@ -751,8 +737,7 @@ class Connection * eventlistener methods * * @param string $savepoint Name of a savepoint to rollback to. - * @throws Doctrine\DBAL\ConnectionException If the rollback operation fails at database level. - * @return boolean FALSE if rollback couldn't be performed, TRUE otherwise. + * @throws ConnectionException If the rollback operation fails at database level. */ public function rollback() { @@ -765,11 +750,10 @@ class Connection if ($this->_transactionNestingLevel == 1) { $this->_transactionNestingLevel = 0; $this->_conn->rollback(); + $this->_isRollbackOnly = false; } else { --$this->_transactionNestingLevel; } - - return true; } /** @@ -798,4 +782,32 @@ class Connection return $this->_schemaManager; } + + /** + * Marks the current transaction so that the only possible + * outcome for the transaction to be rolled back. + * + * @throws BadMethodCallException If no transaction is active. + */ + public function setRollbackOnly() + { + if ($this->_transactionNestingLevel == 0) { + throw ConnectionException::noActiveTransaction(); + } + $this->_isRollbackOnly = true; + } + + /** + * Check whether the current transaction is marked for rollback only. + * + * @return boolean + * @throws BadMethodCallException If no transaction is active. + */ + public function getRollbackOnly() + { + if ($this->_transactionNestingLevel == 0) { + throw ConnectionException::noActiveTransaction(); + } + return $this->_isRollbackOnly; + } } diff --git a/lib/Doctrine/ORM/AbstractQuery.php b/lib/Doctrine/ORM/AbstractQuery.php index 86ad2f1ed..a59e470ff 100644 --- a/lib/Doctrine/ORM/AbstractQuery.php +++ b/lib/Doctrine/ORM/AbstractQuery.php @@ -262,14 +262,14 @@ abstract class AbstractQuery } /** - * Defines if the resultset cache is active or not. + * Defines if the result cache is active or not. * * @param boolean $expire Whether or not to force resultset cache expiration. * @return Doctrine_ORM_Query */ public function setExpireResultCache($expire = true) { - $this->_expireResultCache = (bool) $expire; + $this->_expireResultCache = $expire; return $this; } @@ -445,7 +445,7 @@ abstract class AbstractQuery // Check result cache if ($cacheDriver = $this->getResultCacheDriver()) { $id = $this->_getResultCacheId($params); - $cached = ($this->_expireResultCache) ? false : $cacheDriver->fetch($id); + $cached = $this->_expireResultCache ? false : $cacheDriver->fetch($id); if ($cached === false) { // Cache miss. diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php index 1c745ba50..2e09fa0d3 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php @@ -91,7 +91,7 @@ class ClassMetadataInfo const GENERATOR_TYPE_IDENTITY = 4; /** * NONE means the class does not have a generated id. That means the class - * must have a natural id. + * must have a natural, manually assigned id. */ const GENERATOR_TYPE_NONE = 5; /** @@ -105,7 +105,7 @@ class ClassMetadataInfo /** * DEFERRED_EXPLICIT means that changes of entities are calculated at commit-time * by doing a property-by-property comparison with the original data. This will - * be done only for entities that were explicitly saved (through save() or cascade). + * be done only for entities that were explicitly saved (through persist() or a cascade). */ const CHANGETRACKING_DEFERRED_EXPLICIT = 2; /** diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 2c5e7ba0f..4831b7efc 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -204,6 +204,7 @@ class UnitOfWork implements PropertyChangedListener private $_collectionPersisters = array(); /** + * EXPERIMENTAL: * Flag for whether or not to make use of the C extension. * * @var boolean @@ -315,6 +316,7 @@ class UnitOfWork implements PropertyChangedListener $conn->commit(); } catch (\Exception $e) { + $conn->setRollbackOnly(); $conn->rollback(); $this->clear(); throw $e; diff --git a/tests/Doctrine/Tests/Common/Cache/ApcCacheTest.php b/tests/Doctrine/Tests/Common/Cache/ApcCacheTest.php index 48df93ac8..556068d39 100644 --- a/tests/Doctrine/Tests/Common/Cache/ApcCacheTest.php +++ b/tests/Doctrine/Tests/Common/Cache/ApcCacheTest.php @@ -30,6 +30,7 @@ class ApcCacheTest extends \Doctrine\Tests\DoctrineTestCase // Test delete $cache->save('test_key2', 'test2'); + $cache->delete('test_key2'); $this->assertFalse($cache->contains('test_key2')); } diff --git a/tests/Doctrine/Tests/Common/Cache/ArrayCacheTest.php b/tests/Doctrine/Tests/Common/Cache/ArrayCacheTest.php index af84e160f..a1cacc9ce 100644 --- a/tests/Doctrine/Tests/Common/Cache/ArrayCacheTest.php +++ b/tests/Doctrine/Tests/Common/Cache/ArrayCacheTest.php @@ -11,6 +11,7 @@ class ArrayCacheTest extends \Doctrine\Tests\DoctrineTestCase public function testArrayCacheDriver() { $cache = new ArrayCache(); + $cache->setManageCacheKeys(true); // Test save $cache->save('test_key', 'testing this out'); diff --git a/tests/Doctrine/Tests/Common/Cache/CacheTest.php b/tests/Doctrine/Tests/Common/Cache/CacheTest.php index 5c0a96008..3f5dbce4b 100644 --- a/tests/Doctrine/Tests/Common/Cache/CacheTest.php +++ b/tests/Doctrine/Tests/Common/Cache/CacheTest.php @@ -11,6 +11,7 @@ class CacheTest extends \Doctrine\Tests\DoctrineTestCase public function testCount() { $cache = new ArrayCache(); + $cache->setManageCacheKeys(true); $cache->save('test_key1', '1'); $cache->save('test_key2', '2'); $this->assertEquals($cache->count(), 2); @@ -19,6 +20,7 @@ class CacheTest extends \Doctrine\Tests\DoctrineTestCase public function testDeleteAll() { $cache = new ArrayCache(); + $cache->setManageCacheKeys(true); $cache->save('test_key1', '1'); $cache->save('test_key2', '2'); $cache->deleteAll(); @@ -29,6 +31,7 @@ class CacheTest extends \Doctrine\Tests\DoctrineTestCase public function testDeleteByRegex() { $cache = new ArrayCache(); + $cache->setManageCacheKeys(true); $cache->save('test_key1', '1'); $cache->save('test_key2', '2'); $cache->deleteByRegex('/test_key[0-9]/'); @@ -39,6 +42,7 @@ class CacheTest extends \Doctrine\Tests\DoctrineTestCase public function testDeleteByPrefix() { $cache = new ArrayCache(); + $cache->setManageCacheKeys(true); $cache->save('test_key1', '1'); $cache->save('test_key2', '2'); $cache->deleteByPrefix('test_key'); @@ -49,6 +53,7 @@ class CacheTest extends \Doctrine\Tests\DoctrineTestCase public function testDeleteBySuffix() { $cache = new ArrayCache(); + $cache->setManageCacheKeys(true); $cache->save('1test_key', '1'); $cache->save('2test_key', '2'); $cache->deleteBySuffix('test_key'); @@ -59,6 +64,7 @@ class CacheTest extends \Doctrine\Tests\DoctrineTestCase public function testDeleteByWildcard() { $cache = new ArrayCache(); + $cache->setManageCacheKeys(true); $cache->save('test_key1', '1'); $cache->save('test_key2', '2'); $cache->delete('test_key*'); @@ -69,6 +75,7 @@ class CacheTest extends \Doctrine\Tests\DoctrineTestCase public function testNamespace() { $cache = new ArrayCache(); + $cache->setManageCacheKeys(true); $cache->setNamespace('test_'); $cache->save('key1', 'test'); $this->assertTrue($cache->contains('key1')); diff --git a/tests/Doctrine/Tests/ORM/Functional/ManyToManyBidirectionalAssociationTest.php b/tests/Doctrine/Tests/ORM/Functional/ManyToManyBidirectionalAssociationTest.php index a4352ae51..07d01112a 100644 --- a/tests/Doctrine/Tests/ORM/Functional/ManyToManyBidirectionalAssociationTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/ManyToManyBidirectionalAssociationTest.php @@ -90,7 +90,7 @@ class ManyToManyBidirectionalAssociationTest extends AbstractManyToManyAssociati $query = $this->_em->createQuery('SELECT c FROM Doctrine\Tests\Models\ECommerce\ECommerceCategory c order by c.id'); $categories = $query->getResult(); - $this->assertLoadingOfInverseSide($categories); + $this->assertLoadingOfInverseSide($categories); } public function testLazyLoadsCollectionOnTheOwningSide() @@ -107,7 +107,7 @@ class ManyToManyBidirectionalAssociationTest extends AbstractManyToManyAssociati $this->assertEquals(0, count($products[0]->getCategories()->unwrap())); $this->assertEquals(0, count($products[1]->getCategories()->unwrap())); - $this->assertLoadingOfOwningSide($products); + $this->assertLoadingOfOwningSide($products); } @@ -161,5 +161,13 @@ class ManyToManyBidirectionalAssociationTest extends AbstractManyToManyAssociati { $this->assertTrue($categories[0] instanceof ECommerceCategory); $this->assertTrue($categories[1] instanceof ECommerceCategory); + + $this->assertEquals(2, count($categories[0]->getProducts())); + $this->assertTrue($categories[0]->getProducts()->get(0) instanceof ECommerceProduct); + $this->assertTrue($categories[0]->getProducts()->get(1) instanceof ECommerceProduct); + + $this->assertEquals(2, count($categories[1]->getProducts())); + $this->assertTrue($categories[1]->getProducts()->get(0) instanceof ECommerceProduct); + $this->assertTrue($categories[1]->getProducts()->get(1) instanceof ECommerceProduct); } } diff --git a/tests/Doctrine/Tests/ORM/Functional/QueryCacheTest.php b/tests/Doctrine/Tests/ORM/Functional/QueryCacheTest.php index 369d50b83..84ac58a7e 100644 --- a/tests/Doctrine/Tests/ORM/Functional/QueryCacheTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/QueryCacheTest.php @@ -33,6 +33,7 @@ class QueryCacheTest extends \Doctrine\Tests\OrmFunctionalTestCase $query = $this->_em->createQuery('select ux from Doctrine\Tests\Models\CMS\CmsUser ux'); $cache = new ArrayCache; + $cache->setManageCacheKeys(true); $query->setQueryCacheDriver($cache); $this->assertEquals(0, $cache->count()); diff --git a/tests/Doctrine/Tests/ORM/Functional/ResultCacheTest.php b/tests/Doctrine/Tests/ORM/Functional/ResultCacheTest.php index 519dc1dd9..8e6c21bb3 100644 --- a/tests/Doctrine/Tests/ORM/Functional/ResultCacheTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/ResultCacheTest.php @@ -31,14 +31,12 @@ class ResultCacheTest extends \Doctrine\Tests\OrmFunctionalTestCase $query = $this->_em->createQuery('select ux from Doctrine\Tests\Models\CMS\CmsUser ux'); $cache = new ArrayCache; + $cache->setManageCacheKeys(true); $query->setResultCache($cache); $this->assertEquals(0, $cache->count()); - $initialQueryCount = $this->_em->getConnection()->getQueryCount(); - $users = $query->getResult(); - $this->assertEquals($initialQueryCount + 1, $this->_em->getConnection()->getQueryCount()); $this->assertEquals(1, $cache->count()); $this->assertEquals(1, count($users)); $this->assertEquals('Roman', $users[0]->name); @@ -50,7 +48,6 @@ class ResultCacheTest extends \Doctrine\Tests\OrmFunctionalTestCase $users = $query2->getResult(); - $this->assertEquals($initialQueryCount + 1, $this->_em->getConnection()->getQueryCount()); $this->assertEquals(1, $cache->count()); $this->assertEquals(1, count($users)); $this->assertEquals('Roman', $users[0]->name);