diff --git a/.travis.yml b/.travis.yml index 70e8be4a5..25b1b1b91 100644 --- a/.travis.yml +++ b/.travis.yml @@ -67,7 +67,7 @@ before_script: # Install composer dependencies - composer validate - mkdir ./public - - if [[ $DB == PGSQL ]]; then composer require silverstripe/postgresql:2.0.x-dev --no-update; fi + - if [[ $DB == PGSQL ]]; then composer require silverstripe/postgresql:2.1.x-dev --no-update; fi - if [[ $DB == SQLITE ]]; then composer require silverstripe/sqlite3:2.0.x-dev --no-update; fi - composer require silverstripe/recipe-testing:^1 silverstripe/recipe-core:4.2.x-dev silverstripe/admin:1.2.x-dev silverstripe/versioned:1.2.x-dev --no-update - if [[ $PHPUNIT_TEST == cms ]]; then composer require silverstripe/recipe-cms:4.2.x-dev --no-update; fi diff --git a/src/Dev/SapphireTest.php b/src/Dev/SapphireTest.php index c4c39ddd4..988f7f087 100644 --- a/src/Dev/SapphireTest.php +++ b/src/Dev/SapphireTest.php @@ -78,6 +78,14 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly */ protected $usesDatabase = null; + /** + * This test will cleanup its state via transactions. + * If set to false a full schema is forced between tests, but at a performance cost. + * + * @var bool + */ + protected $usesTransactions = true; + /** * @var bool */ @@ -228,6 +236,14 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly return $this->usesDatabase; } + /** + * @return bool + */ + public function getUsesTransactions() + { + return $this->usesTransactions; + } + /** * @return array */ @@ -1188,7 +1204,7 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly if (strpos($fixtureFilePath, ':') !== false) { return ModuleResourceLoader::singleton()->resolvePath($fixtureFilePath); } - + // Support fixture paths relative to the test class, rather than relative to webroot // String checking is faster than file_exists() calls. $resolvedPath = realpath($this->getCurrentAbsolutePath() . '/' . $fixtureFilePath); diff --git a/src/Dev/State/FixtureTestState.php b/src/Dev/State/FixtureTestState.php index ccbe67f1a..80ac22e4f 100644 --- a/src/Dev/State/FixtureTestState.php +++ b/src/Dev/State/FixtureTestState.php @@ -19,6 +19,13 @@ class FixtureTestState implements TestState */ private $fixtureFactories = []; + /** + * Set if fixtures have been loaded + * + * @var bool + */ + protected $loaded = []; + /** * Called on setup * @@ -26,25 +33,41 @@ class FixtureTestState implements TestState */ public function setUp(SapphireTest $test) { - if ($this->testNeedsDB($test)) { - $tmpDB = $test::tempDB(); - if (!$tmpDB->isUsed()) { - $tmpDB->build(); - } - DataObject::singleton()->flushCache(); + if (!$this->testNeedsDB($test)) { + return; + } - if (!$tmpDB->hasStarted()) { - foreach ($test->getRequireDefaultRecordsFrom() as $className) { - $instance = singleton($className); - if (method_exists($instance, 'requireDefaultRecords')) { - $instance->requireDefaultRecords(); - } - if (method_exists($instance, 'augmentDefaultRecords')) { - $instance->augmentDefaultRecords(); - } - } - $this->loadFixtures($test); + // Ensure DB is built + $tmpDB = $test::tempDB(); + if (!$tmpDB->isUsed()) { + // Build base db + $tmpDB->build(); + + // Reset schema + $extraObjects = $test->getExtraDataObjects(); + if ($extraObjects) { + $tmpDB->resetDBSchema($extraObjects); } + } + + DataObject::singleton()->flushCache(); + + // Ensure DB is built and populated + if (!$this->getIsLoaded(get_class($test))) { + foreach ($test->getRequireDefaultRecordsFrom() as $className) { + $instance = singleton($className); + if (method_exists($instance, 'requireDefaultRecords')) { + $instance->requireDefaultRecords(); + } + if (method_exists($instance, 'augmentDefaultRecords')) { + $instance->augmentDefaultRecords(); + } + } + $this->loadFixtures($test); + } + + // Begin transactions if enabled + if ($test->getUsesTransactions()) { $tmpDB->startTransaction(); } } @@ -56,9 +79,21 @@ class FixtureTestState implements TestState */ public function tearDown(SapphireTest $test) { - if ($this->testNeedsDB($test)) { - $test::tempDB()->rollbackTransaction(); + if (!$this->testNeedsDB($test)) { + return; } + + // For transactional states, rollback if possible + if ($test->getUsesTransactions()) { + $success = $test::tempDB()->rollbackTransaction(); + if ($success) { + return; + } + } + + // Force reset if transaction failed, or disabled + $test::tempDB()->kill(); + $this->resetFixtureFactory(get_class($test)); } /** @@ -68,7 +103,7 @@ class FixtureTestState implements TestState */ public function setUpOnce($class) { - $this->fixtureFactories[strtolower($class)] = Injector::inst()->create(FixtureFactory::class); + $this->resetFixtureFactory($class); } /** @@ -130,6 +165,8 @@ class FixtureTestState implements TestState foreach ($paths as $fixtureFile) { $this->loadFixture($fixtureFile, $test); } + // Flag as loaded + $this->loaded[strtolower(get_class($test))] = true; } /** @@ -220,4 +257,27 @@ class FixtureTestState implements TestState return false; } + + /** + * Bootstrap a clean fixture factory for the given class + * + * @param string $class + */ + protected function resetFixtureFactory($class) + { + $class = strtolower($class); + $this->fixtureFactories[$class] = Injector::inst()->create(FixtureFactory::class); + $this->loaded[$class] = false; + } + + /** + * Check if fixtures need to be loaded for this class + * + * @param string $class Name of test to check + * @return bool + */ + protected function getIsLoaded($class) + { + return !empty($this->loaded[strtolower($class)]); + } } diff --git a/src/ORM/Connect/Database.php b/src/ORM/Connect/Database.php index 3e2910522..4c60ed085 100644 --- a/src/ORM/Connect/Database.php +++ b/src/ORM/Connect/Database.php @@ -654,22 +654,40 @@ abstract class Database * * @param string|boolean $savepoint Name of savepoint, or leave empty to rollback * to last savepoint + * @return bool|null Boolean is returned if success state is known, or null if + * unknown. Note: For error checking purposes null should not be treated as error. */ abstract public function transactionRollback($savepoint = false); /** * Commit everything inside this transaction so far * - * @param boolean $chain + * @param bool $chain + * @return bool|null Boolean is returned if success state is known, or null if + * unknown. Note: For error checking purposes null should not be treated as error. */ abstract public function transactionEnd($chain = false); + /** + * Return depth of current transaction + * + * @return int Nesting level, or 0 if not in a transaction + */ + public function transactionDepth() + { + // Placeholder error for transactional DBs that don't expose depth + if ($this->supportsTransactions()) { + user_error(get_class($this) . " does not support transactionDepth", E_USER_WARNING); + } + return 0; + } + /** * Determines if the used database supports application-level locks, * which is different from table- or row-level locking. * See {@link getLock()} for details. * - * @return boolean Flag indicating that locking is available + * @return bool Flag indicating that locking is available */ public function supportsLocks() { @@ -681,7 +699,7 @@ abstract class Database * See {@link supportsLocks()} to check if locking is generally supported. * * @param string $name Name of the lock - * @return boolean + * @return bool */ public function canLock($name) { @@ -703,7 +721,7 @@ abstract class Database * * @param string $name Name of lock * @param integer $timeout Timeout in seconds - * @return boolean + * @return bool */ public function getLock($name, $timeout = 5) { @@ -715,7 +733,7 @@ abstract class Database * (if the execution aborts (e.g. due to an error) all locks are automatically released). * * @param string $name Name of the lock - * @return boolean Flag indicating whether the lock was successfully released + * @return bool Flag indicating whether the lock was successfully released */ public function releaseLock($name) { @@ -756,7 +774,7 @@ abstract class Database * Determine if the database with the specified name exists * * @param string $name Name of the database to check for - * @return boolean Flag indicating whether this database exists + * @return bool Flag indicating whether this database exists */ public function databaseExists($name) { @@ -778,12 +796,12 @@ abstract class Database * database if it doesn't exist in the current schema. * * @param string $name Name of the database - * @param boolean $create Flag indicating whether the database should be created + * @param bool $create Flag indicating whether the database should be created * if it doesn't exist. If $create is false and the database doesn't exist * then an error will be raised - * @param int|boolean $errorLevel The level of error reporting to enable for the query, or false if no error + * @param int|bool $errorLevel The level of error reporting to enable for the query, or false if no error * should be raised - * @return boolean Flag indicating success + * @return bool Flag indicating success */ public function selectDatabase($name, $create = false, $errorLevel = E_USER_ERROR) { diff --git a/src/ORM/Connect/MySQLDatabase.php b/src/ORM/Connect/MySQLDatabase.php index fa3672db5..49f6dd62d 100644 --- a/src/ORM/Connect/MySQLDatabase.php +++ b/src/ORM/Connect/MySQLDatabase.php @@ -329,25 +329,78 @@ class MySQLDatabase extends Database public function transactionRollback($savepoint = false) { + // Named transaction if ($savepoint) { $this->query('ROLLBACK TO ' . $savepoint); - } else { - --$this->transactionNesting; - if ($this->transactionNesting > 0) { - $this->transactionRollback('NESTEDTRANSACTION' . $this->transactionNesting); - } else { - $this->query('ROLLBACK'); - } + return true; } + + // Fail if transaction isn't available + if (!$this->transactionNesting) { + return false; + } + --$this->transactionNesting; + if ($this->transactionNesting > 0) { + $this->transactionRollback('NESTEDTRANSACTION' . $this->transactionNesting); + } else { + $this->query('ROLLBACK'); + } + return true; + } + + public function transactionDepth() + { + return $this->transactionNesting; } public function transactionEnd($chain = false) { + // Fail if transaction isn't available + if (!$this->transactionNesting) { + return false; + } --$this->transactionNesting; if ($this->transactionNesting <= 0) { $this->transactionNesting = 0; $this->query('COMMIT AND ' . ($chain ? '' : 'NO ') . 'CHAIN'); } + return true; + } + + /** + * In error condition, set transactionNesting to zero + */ + protected function resetTransactionNesting() + { + $this->transactionNesting = 0; + } + + public function query($sql, $errorLevel = E_USER_ERROR) + { + $this->inspectQuery($sql); + return parent::query($sql, $errorLevel); + } + + public function preparedQuery($sql, $parameters, $errorLevel = E_USER_ERROR) + { + $this->inspectQuery($sql); + return parent::preparedQuery($sql, $parameters, $errorLevel); + } + + /** + * Inspect a SQL query prior to execution + * + * @param string $sql + */ + protected function inspectQuery($sql) + { + // Any DDL discards transactions. + // See https://dev.mysql.com/doc/internals/en/transactions-notes-on-ddl-and-normal-transaction.html + // on why we need to be over-eager + $isDDL = $this->getConnector()->isQueryDDL($sql); + if ($isDDL) { + $this->resetTransactionNesting(); + } } public function comparisonClause( diff --git a/src/ORM/Connect/TempDatabase.php b/src/ORM/Connect/TempDatabase.php index 832acd606..dce37959f 100644 --- a/src/ORM/Connect/TempDatabase.php +++ b/src/ORM/Connect/TempDatabase.php @@ -23,11 +23,6 @@ class TempDatabase */ protected $name = null; - /** - * @var bool If a transaction has been started - */ - protected $hasStarted = false; - /** * Create a new temp database * @@ -73,14 +68,6 @@ class TempDatabase return $this->isDBTemp($selected); } - /** - * @return bool - */ - public function hasStarted() - { - return $this->hasStarted; - } - /** * @return bool */ @@ -94,7 +81,6 @@ class TempDatabase */ public function startTransaction() { - $this->hasStarted = true; if (static::getConn()->supportsTransactions()) { static::getConn()->transactionStart(); } @@ -102,14 +88,28 @@ class TempDatabase /** * Rollback a transaction (or trash all data if the DB doesn't support databases + * + * @return bool True if successfully rolled back, false otherwise. On error the DB is + * killed and must be re-created. Note that calling rollbackTransaction() when there + * is no transaction is counted as a failure, user code should either kill or flush the DB + * as necessary */ public function rollbackTransaction() { - if (static::getConn()->supportsTransactions()) { - static::getConn()->transactionRollback(); - } else { - $this->hasStarted = false; - static::clearAllData(); + // Ensure a rollback can be performed + $success = static::getConn()->supportsTransactions() + && static::getConn()->transactionDepth(); + if (!$success) { + return false; + } + try { + // Explicit false = gnostic error from transactionRollback + if (static::getConn()->transactionRollback() === false) { + return false; + } + return true; + } catch (DatabaseException $ex) { + return false; } } @@ -118,11 +118,14 @@ class TempDatabase */ public function kill() { - // Delete our temporary database + // Nothing to kill if (!$this->isUsed()) { return; } + // Rollback any transactions (note: Success ignored) + $this->rollbackTransaction(); + // Check the database actually exists $dbConn = $this->getConn(); $dbName = $dbConn->getSelectedDatabase(); @@ -147,7 +150,6 @@ class TempDatabase */ public function clearAllData() { - $this->hasStarted = false; if (!$this->isUsed()) { return; } @@ -205,39 +207,12 @@ class TempDatabase } /** - * Clear all temp DBs on this connection + * Rebuild all database tables * - * Note: This will output results to stdout unless suppressOutput - * is set on the current db schema + * @param array $extraDataObjects */ - public function deleteAll() + protected function rebuildTables($extraDataObjects = []) { - $schema = $this->getConn()->getSchemaManager(); - foreach ($schema->databaseList() as $dbName) { - if ($this->isDBTemp($dbName)) { - $schema->dropDatabase($dbName); - $schema->alterationMessage("Dropped database \"$dbName\"", 'deleted'); - flush(); - } - } - } - - /** - * Reset the testing database's schema. - * - * @param array $extraDataObjects List of extra dataobjects to build - */ - public function resetDBSchema(array $extraDataObjects = []) - { - // pgsql doesn't allow schema updates inside transactions - // so we need to rollback any transactions before commencing a schema reset - if ($this->hasStarted()) { - $this->rollbackTransaction(); - } - if (!$this->isUsed()) { - return; - } - DataObject::reset(); // clear singletons, they're caching old extension info which is used in DatabaseAdmin->doBuild() @@ -273,4 +248,45 @@ class TempDatabase ClassInfo::reset_db_cache(); DataObject::singleton()->flushCache(); } + + /** + * Clear all temp DBs on this connection + * + * Note: This will output results to stdout unless suppressOutput + * is set on the current db schema + */ + public function deleteAll() + { + $schema = $this->getConn()->getSchemaManager(); + foreach ($schema->databaseList() as $dbName) { + if ($this->isDBTemp($dbName)) { + $schema->dropDatabase($dbName); + $schema->alterationMessage("Dropped database \"$dbName\"", 'deleted'); + flush(); + } + } + } + + /** + * Reset the testing database's schema. + * + * @param array $extraDataObjects List of extra dataobjects to build + */ + public function resetDBSchema(array $extraDataObjects = []) + { + // Skip if no DB + if (!$this->isUsed()) { + return; + } + + try { + $this->rebuildTables($extraDataObjects); + } catch (DatabaseException $ex) { + // In case of error during build force a hard reset + // e.g. pgsql doesn't allow schema updates inside transactions + $this->kill(); + $this->build(); + $this->rebuildTables($extraDataObjects); + } + } } diff --git a/tests/php/Security/GroupTest.php b/tests/php/Security/GroupTest.php index 163257602..076e04584 100644 --- a/tests/php/Security/GroupTest.php +++ b/tests/php/Security/GroupTest.php @@ -20,6 +20,11 @@ class GroupTest extends FunctionalTest TestMember::class ]; + protected function setUp() + { + parent::setUp(); + } + public function testGroupCodeDefaultsToTitle() { $g1 = new Group();