From 41a95d2c646aba8d6a66ee046c532a51a9022784 Mon Sep 17 00:00:00 2001 From: Patrick Webster Date: Sun, 18 Nov 2012 20:38:58 -0600 Subject: [PATCH 1/5] [ticket/11219] Update sequence values after loading fixtures If a value is provide for an auto_increment type of column, certain DBMSes do not update their internal sequencers. If a row is inserted later, it can be given an ID that is already in use, resulting in an error. The database test cases now resynchronise the sequencers before the tests are run. PHPBB3-11219 --- .../phpbb_database_test_case.php | 10 ++ ...phpbb_database_test_connection_manager.php | 129 ++++++++++++++++++ 2 files changed, 139 insertions(+) diff --git a/tests/test_framework/phpbb_database_test_case.php b/tests/test_framework/phpbb_database_test_case.php index 75a3c0944b..0916679108 100644 --- a/tests/test_framework/phpbb_database_test_case.php +++ b/tests/test_framework/phpbb_database_test_case.php @@ -28,6 +28,16 @@ abstract class phpbb_database_test_case extends PHPUnit_Extensions_Database_Test ); } + protected function setUp() + { + parent::setUp(); + + $config = $this->get_database_config(); + $manager = $this->create_connection_manager($config); + $manager->connect(); + $manager->post_setup_synchronisation(); + } + public function createXMLDataSet($path) { $db_config = $this->get_database_config(); diff --git a/tests/test_framework/phpbb_database_test_connection_manager.php b/tests/test_framework/phpbb_database_test_connection_manager.php index a43215bcf2..cae1720587 100644 --- a/tests/test_framework/phpbb_database_test_connection_manager.php +++ b/tests/test_framework/phpbb_database_test_connection_manager.php @@ -426,4 +426,133 @@ class phpbb_database_test_connection_manager $this->pdo->exec($query); } } + + /** + * Performs synchronisations on the database after a fixture has been loaded + */ + public function post_setup_synchronisation() + { + $this->ensure_connected(__METHOD__); + $queries = array(); + + switch ($this->config['dbms']) + { + case 'oracle': + // Get all of the information about the sequences + $sql = "SELECT t.table_name, tc.column_name, d.referenced_name as sequence_name + FROM USER_TRIGGERS t + JOIN USER_DEPENDENCIES d on d.name = t.trigger_name + JOIN USER_TRIGGER_COLS tc on (tc.trigger_name = t.trigger_name) + WHERE d.referenced_type = 'SEQUENCE' + AND d.type = 'TRIGGER'"; + $result = $this->pdo->query($sql); + + while ($row = $result->fetch(PDO::FETCH_ASSOC)) + { + // Get the current max value of the table + $sql = "SELECT MAX({$row['COLUMN_NAME']}) + 1 FROM {$row['TABLE_NAME']}"; + + $max_result = $this->pdo->query($sql); + $max_row = $max_result->fetch(PDO::FETCH_NUM); + + if (!$max_row) + { + continue; + } + + $maxval = current($max_row); + if ($maxval == null) + { + $maxval = 1; + } + + // Get the sequence's next value + $sql = "SELECT {$row['SEQUENCE_NAME']}.nextval FROM dual"; + try + { + $nextval_result = $this->pdo->query($sql); + } + catch (PDOException $e) + { + // If we catch an exception here it's because the sequencer hasn't been initialized yet. + // If the table hasn't been used, then there's nothing to do. + continue; + } + $nextval_row = $nextval_result->fetch(PDO::FETCH_NUM); + + if ($nextval_row) + { + $nextval = current($nextval_row); + + // Make sure we aren't setting the new increment to zero. + if ($maxval != $nextval) + { + // This is a multi-step process. First we need to get the sequence back into position. + // That means either advancing it or moving it backwards. Sequences have a minimum value + // of 1, so you cannot go past that. Once the offset it determined, you have to request + // the next sequence value to actually move the pointer into position. So if you're at 21 + // and need to be back at 1, set the incrementer to -20. When it's requested, it'll give + // you 1. Then we have to set the increment amount back to 1 to resume normal behavior. + + // Move the sequence to the correct position. + $sql = "ALTER SEQUENCE {$row['SEQUENCE_NAME']} INCREMENT BY " . ($maxval - $nextval); + $this->pdo->exec($sql); + + // Select the next value to actually update the sequence values + $sql = "SELECT {$row['SEQUENCE_NAME']}.nextval FROM dual"; + $this->pdo->query($sql); + + // Reset the sequence's increment amount + $sql = "ALTER SEQUENCE {$row['SEQUENCE_NAME']} INCREMENT BY 1"; + $this->pdo->exec($sql); + } + } + } + break; + + case 'postgres': + // First get the sequences + $sequences = array(); + $sql = "SELECT relname FROM pg_class WHERE relkind = 'S'"; + $result = $this->pdo->query($sql); + while ($row = $result->fetch(PDO::FETCH_ASSOC)) + { + $sequences[] = $row['relname']; + } + + // Now get the name of the column using it + foreach ($sequences as $sequence) + { + $table = str_replace('_seq', '', $sequence); + $sql = "SELECT column_name FROM information_schema.columns + WHERE table_name = '$table' + AND column_default = 'nextval(''$sequence''::regclass)'"; + $result = $this->pdo->query($sql); + $row = $result->fetch(PDO::FETCH_ASSOC); + + // Finally, set the new sequence value + if ($row) + { + $column = $row['column_name']; + + // Get the old value if it exists, or use 1 if it doesn't + $sql = "SELECT COALESCE((SELECT MAX({$column}) + 1 FROM {$table}), 1) AS val"; + $result = $this->pdo->query($sql); + $row = $result->fetch(PDO::FETCH_ASSOC); + + if ($row) + { + // The last parameter is false so that the system doesn't increment it again + $queries[] = "SELECT SETVAL('{$sequence}', {$row['val']}, false)"; + } + } + } + break; + } + + foreach ($queries as $query) + { + $this->pdo->exec($query); + } + } } From a7404409a8376e7db9f295e5cf598ccee59523b5 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 19 Nov 2012 13:49:04 +0100 Subject: [PATCH 2/5] [ticket/11219] Add unit test for inserting into a sequence column PHPBB3-11219 --- tests/dbal/write_sequence_test.php | 55 ++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 tests/dbal/write_sequence_test.php diff --git a/tests/dbal/write_sequence_test.php b/tests/dbal/write_sequence_test.php new file mode 100644 index 0000000000..d2c30b4e89 --- /dev/null +++ b/tests/dbal/write_sequence_test.php @@ -0,0 +1,55 @@ +createXMLDataSet(dirname(__FILE__).'/fixtures/three_users.xml'); + } + + static public function write_sequence_data() + { + return array( + array( + 'ticket/11219', + 4, + ), + ); + } + + /** + * @dataProvider write_sequence_data + */ + public function test_write_sequence($username, $expected) + { + $db = $this->new_dbal(); + + $sql = 'INSERT INTO phpbb_users ' . $db->sql_build_array('INSERT', array( + 'username' => $username, + 'username_clean' => $username, + 'user_permissions' => '', + 'user_sig' => '', + 'user_occ' => '', + 'user_interests' => '', + )); + $db->sql_query($sql); + + $this->assertEquals($expected, $db->sql_nextid()); + + $sql = "SELECT user_id + FROM phpbb_users + WHERE username_clean = '" . $db->sql_escape($username) . "'"; + $result = $db->sql_query_limit($sql, 1); + + $this->assertEquals($expected, $db->sql_fetchfield('user_id')); + } +} From 1dff6d1bf988bb0d11a393fad0c0d0183366a5c9 Mon Sep 17 00:00:00 2001 From: Patrick Webster Date: Tue, 20 Nov 2012 04:40:06 -0600 Subject: [PATCH 3/5] [ticket/11219] Recreate Oracle sequences instead of altering them The previous method would always leave a gap between the last value and the new one due to how you have to update the sequence values. To remove gaps in all situations, the options are to alter the USER_SEQUENCES table or just drop the sequence and recreate it. The prior requires elevated priveleges and the latter can break attached objects. Since we don't attach objects to the sequences, we won't have any problems doing it for the tests. PHPBB3-11219 --- ...phpbb_database_test_connection_manager.php | 69 +++++-------------- 1 file changed, 17 insertions(+), 52 deletions(-) diff --git a/tests/test_framework/phpbb_database_test_connection_manager.php b/tests/test_framework/phpbb_database_test_connection_manager.php index cae1720587..e79a764e1d 100644 --- a/tests/test_framework/phpbb_database_test_connection_manager.php +++ b/tests/test_framework/phpbb_database_test_connection_manager.php @@ -439,10 +439,11 @@ class phpbb_database_test_connection_manager { case 'oracle': // Get all of the information about the sequences - $sql = "SELECT t.table_name, tc.column_name, d.referenced_name as sequence_name + $sql = "SELECT t.table_name, tc.column_name, d.referenced_name as sequence_name, s.increment_by, s.min_value FROM USER_TRIGGERS t - JOIN USER_DEPENDENCIES d on d.name = t.trigger_name - JOIN USER_TRIGGER_COLS tc on (tc.trigger_name = t.trigger_name) + JOIN USER_DEPENDENCIES d ON (d.name = t.trigger_name) + JOIN USER_TRIGGER_COLS tc ON (tc.trigger_name = t.trigger_name) + JOIN USER_SEQUENCES s ON (s.sequence_name = d.referenced_name) WHERE d.referenced_type = 'SEQUENCE' AND d.type = 'TRIGGER'"; $result = $this->pdo->query($sql); @@ -450,63 +451,27 @@ class phpbb_database_test_connection_manager while ($row = $result->fetch(PDO::FETCH_ASSOC)) { // Get the current max value of the table - $sql = "SELECT MAX({$row['COLUMN_NAME']}) + 1 FROM {$row['TABLE_NAME']}"; - + $sql = "SELECT MAX({$row['COLUMN_NAME']}) AS max FROM {$row['TABLE_NAME']}"; $max_result = $this->pdo->query($sql); - $max_row = $max_result->fetch(PDO::FETCH_NUM); + $max_row = $max_result->fetch(PDO::FETCH_ASSOC); if (!$max_row) { continue; } - $maxval = current($max_row); - if ($maxval == null) - { - $maxval = 1; - } + $maxval = (int)$max_row['MAX']; + $maxval++; - // Get the sequence's next value - $sql = "SELECT {$row['SEQUENCE_NAME']}.nextval FROM dual"; - try - { - $nextval_result = $this->pdo->query($sql); - } - catch (PDOException $e) - { - // If we catch an exception here it's because the sequencer hasn't been initialized yet. - // If the table hasn't been used, then there's nothing to do. - continue; - } - $nextval_row = $nextval_result->fetch(PDO::FETCH_NUM); - - if ($nextval_row) - { - $nextval = current($nextval_row); - - // Make sure we aren't setting the new increment to zero. - if ($maxval != $nextval) - { - // This is a multi-step process. First we need to get the sequence back into position. - // That means either advancing it or moving it backwards. Sequences have a minimum value - // of 1, so you cannot go past that. Once the offset it determined, you have to request - // the next sequence value to actually move the pointer into position. So if you're at 21 - // and need to be back at 1, set the incrementer to -20. When it's requested, it'll give - // you 1. Then we have to set the increment amount back to 1 to resume normal behavior. - - // Move the sequence to the correct position. - $sql = "ALTER SEQUENCE {$row['SEQUENCE_NAME']} INCREMENT BY " . ($maxval - $nextval); - $this->pdo->exec($sql); - - // Select the next value to actually update the sequence values - $sql = "SELECT {$row['SEQUENCE_NAME']}.nextval FROM dual"; - $this->pdo->query($sql); - - // Reset the sequence's increment amount - $sql = "ALTER SEQUENCE {$row['SEQUENCE_NAME']} INCREMENT BY 1"; - $this->pdo->exec($sql); - } - } + // This is not the "proper" way, but the proper way does not allow you to completely reset + // tables with no rows since you have to select the next value to make the change go into effct. + // You would have to go past the minimum value to set it correctly, but that's illegal. + // Since we have no objects attached to our sequencers (triggers aren't attached), this works fine. + $queries[] = 'DROP SEQUENCE ' . $row['SEQUENCE_NAME']; + $queries[] = "CREATE SEQUENCE {$row['SEQUENCE_NAME']} + MINVALUE {$row['MIN_VALUE']} + INCREMENT BY {$row['INCREMENT_BY']} + START WITH $maxval"; } break; From 720ef233b122d00ef9d2128c9a0a518ff017b0d7 Mon Sep 17 00:00:00 2001 From: Patrick Webster Date: Sat, 1 Dec 2012 22:34:03 -0600 Subject: [PATCH 4/5] [ticket/11219] Only update sequences that are affected by a fixture PHPBB3-11219 --- .../phpbb_database_test_case.php | 18 +++-- ...phpbb_database_test_connection_manager.php | 73 +++++++++++-------- 2 files changed, 55 insertions(+), 36 deletions(-) diff --git a/tests/test_framework/phpbb_database_test_case.php b/tests/test_framework/phpbb_database_test_case.php index 0916679108..429bb92bf1 100644 --- a/tests/test_framework/phpbb_database_test_case.php +++ b/tests/test_framework/phpbb_database_test_case.php @@ -13,6 +13,8 @@ abstract class phpbb_database_test_case extends PHPUnit_Extensions_Database_Test protected $test_case_helpers; + protected $fixture_xml_data; + public function __construct($name = NULL, array $data = array(), $dataName = '') { parent::__construct($name, $data, $dataName); @@ -32,10 +34,14 @@ abstract class phpbb_database_test_case extends PHPUnit_Extensions_Database_Test { parent::setUp(); - $config = $this->get_database_config(); - $manager = $this->create_connection_manager($config); - $manager->connect(); - $manager->post_setup_synchronisation(); + // Resynchronise tables if a fixture was loaded + if (isset($this->fixture_xml_data)) + { + $config = $this->get_database_config(); + $manager = $this->create_connection_manager($config); + $manager->connect(); + $manager->post_setup_synchronisation($this->fixture_xml_data); + } } public function createXMLDataSet($path) @@ -57,7 +63,9 @@ abstract class phpbb_database_test_case extends PHPUnit_Extensions_Database_Test $path = $meta_data['uri']; } - return parent::createXMLDataSet($path); + $this->fixture_xml_data = parent::createXMLDataSet($path); + + return $this->fixture_xml_data; } public function get_test_case_helpers() diff --git a/tests/test_framework/phpbb_database_test_connection_manager.php b/tests/test_framework/phpbb_database_test_connection_manager.php index e79a764e1d..97281a0812 100644 --- a/tests/test_framework/phpbb_database_test_connection_manager.php +++ b/tests/test_framework/phpbb_database_test_connection_manager.php @@ -429,12 +429,19 @@ class phpbb_database_test_connection_manager /** * Performs synchronisations on the database after a fixture has been loaded + * + * @param PHPUnit_Extensions_Database_DataSet_XmlDataSet $tables Tables contained within the loaded fixture + * + * @return null */ - public function post_setup_synchronisation() + public function post_setup_synchronisation($xmlDataSet) { $this->ensure_connected(__METHOD__); $queries = array(); + // Get escaped versions of the table names used in the fixture + $table_names = array_map(array($this->pdo, 'PDO::quote'), $xmlDataSet->getTableNames()); + switch ($this->config['dbms']) { case 'oracle': @@ -445,7 +452,9 @@ class phpbb_database_test_connection_manager JOIN USER_TRIGGER_COLS tc ON (tc.trigger_name = t.trigger_name) JOIN USER_SEQUENCES s ON (s.sequence_name = d.referenced_name) WHERE d.referenced_type = 'SEQUENCE' - AND d.type = 'TRIGGER'"; + AND d.type = 'TRIGGER' + AND t.table_name IN (" . implode(', ', array_map('strtoupper', $table_names)) . ')'; + $result = $this->pdo->query($sql); while ($row = $result->fetch(PDO::FETCH_ASSOC)) @@ -476,41 +485,43 @@ class phpbb_database_test_connection_manager break; case 'postgres': - // First get the sequences - $sequences = array(); - $sql = "SELECT relname FROM pg_class WHERE relkind = 'S'"; + // Get the sequences attached to the tables + $sql = 'SELECT column_name, table_name FROM information_schema.columns + WHERE table_name IN (' . implode(', ', $table_names) . ") + AND strpos(column_default, '_seq''::regclass') > 0"; $result = $this->pdo->query($sql); + + $setval_queries = array(); while ($row = $result->fetch(PDO::FETCH_ASSOC)) { - $sequences[] = $row['relname']; + // Get the columns used in the fixture for this table + $column_names = $xmlDataSet->getTableMetaData($row['table_name'])->getColumns(); + + // Skip sequences that weren't specified in the fixture + if (!in_array($row['column_name'], $column_names)) + { + continue; + } + + // Get the old value if it exists, or use 1 if it doesn't + $sql = "SELECT COALESCE((SELECT MAX({$row['column_name']}) + 1 FROM {$row['table_name']}), 1) AS val"; + $result_max = $this->pdo->query($sql); + $row_max = $result_max->fetch(PDO::FETCH_ASSOC); + + if ($row_max) + { + $seq_name = $this->pdo->quote($row['table_name'] . '_seq'); + $max_val = (int) $row_max['val']; + + // The last parameter is false so that the system doesn't increment it again + $setval_queries[] = "SETVAL($seq_name, $max_val, false)"; + } } - // Now get the name of the column using it - foreach ($sequences as $sequence) + // Combine all of the SETVALs into one query + if (sizeof($setval_queries)) { - $table = str_replace('_seq', '', $sequence); - $sql = "SELECT column_name FROM information_schema.columns - WHERE table_name = '$table' - AND column_default = 'nextval(''$sequence''::regclass)'"; - $result = $this->pdo->query($sql); - $row = $result->fetch(PDO::FETCH_ASSOC); - - // Finally, set the new sequence value - if ($row) - { - $column = $row['column_name']; - - // Get the old value if it exists, or use 1 if it doesn't - $sql = "SELECT COALESCE((SELECT MAX({$column}) + 1 FROM {$table}), 1) AS val"; - $result = $this->pdo->query($sql); - $row = $result->fetch(PDO::FETCH_ASSOC); - - if ($row) - { - // The last parameter is false so that the system doesn't increment it again - $queries[] = "SELECT SETVAL('{$sequence}', {$row['val']}, false)"; - } - } + $queries[] = 'SELECT ' . implode(', ', $setval_queries); } break; } From dbb54b217b4d0c0669a566f9c950e8331887d276 Mon Sep 17 00:00:00 2001 From: Patrick Webster Date: Wed, 5 Dec 2012 22:57:06 -0600 Subject: [PATCH 5/5] [ticket/11219] Coding guidelines and naming consistency changes PHPBB3-11219 --- tests/dbal/write_sequence_test.php | 2 +- ...phpbb_database_test_connection_manager.php | 24 ++++++++++--------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/tests/dbal/write_sequence_test.php b/tests/dbal/write_sequence_test.php index d2c30b4e89..8975cfbfb1 100644 --- a/tests/dbal/write_sequence_test.php +++ b/tests/dbal/write_sequence_test.php @@ -13,7 +13,7 @@ class phpbb_dbal_write_sequence_test extends phpbb_database_test_case { public function getDataSet() { - return $this->createXMLDataSet(dirname(__FILE__).'/fixtures/three_users.xml'); + return $this->createXMLDataSet(dirname(__FILE__) . '/fixtures/three_users.xml'); } static public function write_sequence_data() diff --git a/tests/test_framework/phpbb_database_test_connection_manager.php b/tests/test_framework/phpbb_database_test_connection_manager.php index 97281a0812..d7c2804aa7 100644 --- a/tests/test_framework/phpbb_database_test_connection_manager.php +++ b/tests/test_framework/phpbb_database_test_connection_manager.php @@ -430,17 +430,17 @@ class phpbb_database_test_connection_manager /** * Performs synchronisations on the database after a fixture has been loaded * - * @param PHPUnit_Extensions_Database_DataSet_XmlDataSet $tables Tables contained within the loaded fixture + * @param PHPUnit_Extensions_Database_DataSet_XmlDataSet $xml_data_set Information about the tables contained within the loaded fixture * * @return null */ - public function post_setup_synchronisation($xmlDataSet) + public function post_setup_synchronisation($xml_data_set) { $this->ensure_connected(__METHOD__); $queries = array(); // Get escaped versions of the table names used in the fixture - $table_names = array_map(array($this->pdo, 'PDO::quote'), $xmlDataSet->getTableNames()); + $table_names = array_map(array($this->pdo, 'PDO::quote'), $xml_data_set->getTableNames()); switch ($this->config['dbms']) { @@ -469,18 +469,20 @@ class phpbb_database_test_connection_manager continue; } - $maxval = (int)$max_row['MAX']; - $maxval++; + $max_val = (int) $max_row['MAX']; + $max_val++; - // This is not the "proper" way, but the proper way does not allow you to completely reset - // tables with no rows since you have to select the next value to make the change go into effct. - // You would have to go past the minimum value to set it correctly, but that's illegal. - // Since we have no objects attached to our sequencers (triggers aren't attached), this works fine. + /** + * This is not the "proper" way, but the proper way does not allow you to completely reset + * tables with no rows since you have to select the next value to make the change go into effect. + * You would have to go past the minimum value to set it correctly, but that's illegal. + * Since we have no objects attached to our sequencers (triggers aren't attached), this works fine. + */ $queries[] = 'DROP SEQUENCE ' . $row['SEQUENCE_NAME']; $queries[] = "CREATE SEQUENCE {$row['SEQUENCE_NAME']} MINVALUE {$row['MIN_VALUE']} INCREMENT BY {$row['INCREMENT_BY']} - START WITH $maxval"; + START WITH $max_val"; } break; @@ -495,7 +497,7 @@ class phpbb_database_test_connection_manager while ($row = $result->fetch(PDO::FETCH_ASSOC)) { // Get the columns used in the fixture for this table - $column_names = $xmlDataSet->getTableMetaData($row['table_name'])->getColumns(); + $column_names = $xml_data_set->getTableMetaData($row['table_name'])->getColumns(); // Skip sequences that weren't specified in the fixture if (!in_array($row['column_name'], $column_names))