Commit 6682ec9a authored by Cédric Anne's avatar Cédric Anne Committed by Johan Cwiklinski
Browse files

Trigger warnings on MyISAM tables creation

parent 2a2da6c6
......@@ -56,6 +56,7 @@ The present file will list all changes made to the project; according to the
- `Transfer::transferDropdownNetpoint()` has been renamed to `Transfer::transferDropdownSocket()`.
#### Deprecated
- Usage of `MyISAM` engine in database, in favor of `InnoDB` engine.
- Usage of `utf8mb3` charset/collation in database in favor of `utf8mb4` charset/collation.
- Handling of encoded/escaped value in `autoName()`
- `Netpoint` has been deprecated and replaced by `Socket`
......
......@@ -187,6 +187,7 @@ abstract class AbstractConfigureCommand extends AbstractCommand implements Force
* @param OutputInterface $output
* @param bool $auto_config_flags
* @param bool $use_utf8mb4
* @param bool $allow_myisam
*
* @throws InvalidArgumentException
*
......@@ -196,7 +197,8 @@ abstract class AbstractConfigureCommand extends AbstractCommand implements Force
InputInterface $input,
OutputInterface $output,
bool $auto_config_flags = true,
bool $use_utf8mb4 = false
bool $use_utf8mb4 = false,
bool $allow_myisam = true
) {
$db_pass = $input->getOption('db-password');
......@@ -275,6 +277,7 @@ abstract class AbstractConfigureCommand extends AbstractCommand implements Force
};
$config_flags = $db->getComputedConfigBooleanFlags();
$use_utf8mb4 = $config_flags[DBConnection::PROPERTY_USE_UTF8MB4] ?? $use_utf8mb4;
$allow_myisam = $config_flags[DBConnection::PROPERTY_ALLOW_MYISAM] ?? $allow_myisam;
}
DBConnection::setConnectionCharset($mysqli, $use_utf8mb4);
......@@ -285,7 +288,16 @@ abstract class AbstractConfigureCommand extends AbstractCommand implements Force
'<comment>' . __('Saving configuration file...') . '</comment>',
OutputInterface::VERBOSITY_VERBOSE
);
if (!DBConnection::createMainConfig($db_hostport, $db_user, $db_pass, $db_name, $log_deprecation_warnings, $use_utf8mb4)) {
$result = DBConnection::createMainConfig(
$db_hostport,
$db_user,
$db_pass,
$db_name,
$log_deprecation_warnings,
$use_utf8mb4,
$allow_myisam
);
if (!$result) {
$message = sprintf(
__('Cannot write configuration file "%s".'),
GLPI_CONFIG_DIR . DIRECTORY_SEPARATOR . 'config_db.php'
......@@ -298,13 +310,30 @@ abstract class AbstractConfigureCommand extends AbstractCommand implements Force
}
// Set $db instance to use new connection properties
$this->db = new class($db_hostport, $db_user, $db_pass, $db_name, $log_deprecation_warnings, $use_utf8mb4) extends DBmysql {
public function __construct($dbhost, $dbuser, $dbpassword, $dbdefault, $log_deprecation_warnings, $use_utf8mb4) {
$this->db = new class(
$db_hostport,
$db_user,
$db_pass,
$db_name,
$log_deprecation_warnings,
$use_utf8mb4,
$allow_myisam
) extends DBmysql {
public function __construct(
$dbhost,
$dbuser,
$dbpassword,
$dbdefault,
$log_deprecation_warnings,
$use_utf8mb4,
$allow_myisam
) {
$this->dbhost = $dbhost;
$this->dbuser = $dbuser;
$this->dbpassword = $dbpassword;
$this->dbdefault = $dbdefault;
$this->use_utf8mb4 = $use_utf8mb4;
$this->allow_myisam = $allow_myisam;
$this->log_deprecation_warnings = $log_deprecation_warnings;
......
......@@ -165,7 +165,7 @@ class InstallCommand extends AbstractConfigureCommand {
}
if (!$this->isDbAlreadyConfigured() || $input->getOption('reconfigure')) {
$result = $this->configureDatabase($input, $output, false, true);
$result = $this->configureDatabase($input, $output, false, true, false);
if (self::ABORTED_BY_USER === $result) {
return 0; // Considered as success
......
......@@ -36,7 +36,8 @@ if (!defined('GLPI_ROOT')) {
die("Sorry. You can't access this file directly");
}
use DB;
use DBConnection;
use DBmysql;
use Glpi\Console\AbstractCommand;
use Symfony\Component\Console\Helper\QuestionHelper;
use Symfony\Component\Console\Input\InputInterface;
......@@ -52,6 +53,13 @@ class MyIsamToInnoDbCommand extends AbstractCommand {
*/
const ERROR_TABLE_MIGRATION_FAILED = 1;
/**
* Error code returned if DB configuration file cannot be updated.
*
* @var integer
*/
const ERROR_UNABLE_TO_UPDATE_CONFIG = 2;
protected function configure() {
parent::configure();
......@@ -74,9 +82,7 @@ class MyIsamToInnoDbCommand extends AbstractCommand {
if (0 === $myisam_tables->count()) {
$output->writeln('<info>' . __('No migration needed.') . '</info>');
return 0;
}
} else {
if (!$no_interaction) {
// Ask for confirmation (unless --no-interaction)
/** @var QuestionHelper $question_helper */
......@@ -96,7 +102,7 @@ class MyIsamToInnoDbCommand extends AbstractCommand {
}
foreach ($myisam_tables as $table) {
$table_name = DB::quoteName($table['TABLE_NAME']);
$table_name = DBmysql::quoteName($table['TABLE_NAME']);
$output->writeln(
'<comment>' . sprintf(__('Migrating table "%s"...'), $table_name) . '</comment>',
OutputInterface::VERBOSITY_VERBOSE
......@@ -117,8 +123,18 @@ class MyIsamToInnoDbCommand extends AbstractCommand {
return self::ERROR_TABLE_MIGRATION_FAILED;
}
}
}
if (!DBConnection::updateConfigProperty(DBConnection::PROPERTY_ALLOW_MYISAM, false)) {
throw new \Glpi\Console\Exception\EarlyExitException(
'<error>' . __('Unable to update DB configuration file.') . '</error>',
self::ERROR_UNABLE_TO_UPDATE_CONFIG
);
}
if ($myisam_tables->count() > 0) {
$output->writeln('<info>' . __('Migration done.') . '</info>');
}
return 0; // Success
}
......
......@@ -52,6 +52,12 @@ class DBConnection extends CommonDBTM {
*/
public const PROPERTY_USE_UTF8MB4 = 'use_utf8mb4';
/**
* "Allow MyISAM" property name.
* @var string
*/
public const PROPERTY_ALLOW_MYISAM = 'allow_myisam';
static protected $notable = true;
......@@ -71,6 +77,7 @@ class DBConnection extends CommonDBTM {
* @param string $dbname The name of the DB
* @param boolean $log_deprecation_warnings Flag that indicates if DB deprecation warnings should be logged
* @param boolean $use_utf8mb4 Flag that indicates if utf8mb4 charset/collation should be used
* @param boolean $allow_myisam Flag that indicates if MyISAM engine usage should be allowed
* @param string $config_dir
*
* @return boolean
......@@ -82,6 +89,7 @@ class DBConnection extends CommonDBTM {
string $dbname,
bool $log_deprecation_warnings = false,
bool $use_utf8mb4 = false,
bool $allow_myisam = true,
string $config_dir = GLPI_CONFIG_DIR
): bool {
......@@ -97,6 +105,9 @@ class DBConnection extends CommonDBTM {
if ($use_utf8mb4) {
$properties[self::PROPERTY_USE_UTF8MB4] = true;
}
if (!$allow_myisam) {
$properties[self::PROPERTY_ALLOW_MYISAM] = false;
}
$config_str = '<?php' . "\n" . 'class DB extends DBmysql {' . "\n";
foreach ($properties as $name => $value) {
......@@ -193,6 +204,7 @@ class DBConnection extends CommonDBTM {
* @param string $dbname The name of the DB
* @param boolean $log_deprecation_warnings Flag that indicates if DB deprecation warnings should be logged
* @param boolean $use_utf8mb4 Flag that indicates if utf8mb4 charset/collation should be used
* @param boolean $allow_myisam Flag that indicates if MyISAM engine usage should be allowed
* @param string $config_dir
*
* @return boolean for success
......@@ -204,6 +216,7 @@ class DBConnection extends CommonDBTM {
string $dbname,
bool $log_deprecation_warnings = false,
bool $use_utf8mb4 = false,
bool $allow_myisam = true,
string $config_dir = GLPI_CONFIG_DIR
): bool {
......@@ -226,6 +239,9 @@ class DBConnection extends CommonDBTM {
if ($use_utf8mb4) {
$properties[self::PROPERTY_USE_UTF8MB4] = true;
}
if (!$allow_myisam) {
$properties[self::PROPERTY_ALLOW_MYISAM] = false;
}
$config_str = '<?php' . "\n" . 'class DB extends DBmysql {' . "\n";
foreach ($properties as $name => $value) {
......@@ -268,7 +284,15 @@ class DBConnection extends CommonDBTM {
**/
static function createDBSlaveConfig() {
global $DB;
self::createSlaveConnectionFile("localhost", "glpi", "glpi", "glpi", $DB->log_deprecation_warnings, $DB->use_utf8mb4);
self::createSlaveConnectionFile(
"localhost",
"glpi",
"glpi",
"glpi",
$DB->log_deprecation_warnings,
$DB->use_utf8mb4,
$DB->allow_myisam
);
}
......@@ -282,7 +306,15 @@ class DBConnection extends CommonDBTM {
**/
static function saveDBSlaveConf($host, $user, $password, $DBname) {
global $DB;
self::createSlaveConnectionFile($host, $user, $password, $DBname, $DB->log_deprecation_warnings, $DB->use_utf8mb4);
self::createSlaveConnectionFile(
$host,
$user,
$password,
$DBname,
$DB->log_deprecation_warnings,
$DB->use_utf8mb4,
$DB->allow_myisam
);
}
......
......@@ -127,6 +127,14 @@ class DBmysql {
*/
public $use_utf8mb4 = false;
/**
* Determine if MyISAM engine usage should be allowed for tables creation/altering operations.
* Defaults to true to keep backward compatibility with old DB.
*
* @var bool
*/
public $allow_myisam = true;
/** Is it a first connection ?
* Indicates if the first connection attempt is successful or not
......@@ -311,28 +319,7 @@ class DBmysql {
$TIMER->start();
}
if (preg_match('/(ALTER|CREATE)\s+TABLE\s+/', $query)) {
$matches = [];
if ($this->use_utf8mb4 && preg_match('/(?<invalid>(utf8(_[^\';\s]+)?))[\';\s]/', $query, $matches)) {
trigger_error(
sprintf(
'Usage of "%s" charset/collation detected, should be "%s"',
$matches['invalid'],
str_replace('utf8', 'utf8mb4', $matches['invalid'])
),
E_USER_WARNING
);
} else if (!$this->use_utf8mb4 && preg_match('/(?<invalid>(utf8mb4(_[^\';\s]+)?))[\';\s]/', $query, $matches)) {
trigger_error(
sprintf(
'Usage of "%s" charset/collation detected, should be "%s"',
$matches['invalid'],
str_replace('utf8mb4', 'utf8', $matches['invalid'])
),
E_USER_WARNING
);
}
}
$this->checkForDeprecatedTableOptions($query);
$res = $this->dbh->query($query);
if (!$res) {
......@@ -589,10 +576,20 @@ class DBmysql {
/**
* Returns tables using "MyIsam" engine.
*
* @param bool $exclude_plugins
*
* @return DBmysqlIterator
*/
public function getMyIsamTables(): DBmysqlIterator {
$iterator = $this->listTables('glpi\_%', ['engine' => 'MyIsam']);
public function getMyIsamTables(bool $exclude_plugins = false): DBmysqlIterator {
$criteria = [
'engine' => 'MyIsam',
];
if ($exclude_plugins) {
$criteria[] = ['NOT' => ['information_schema.tables.table_name' => ['LIKE', 'glpi\_plugin\_%']]];
}
$iterator = $this->listTables('glpi\_%', $criteria);
return $iterator;
}
......@@ -1755,6 +1752,47 @@ class DBmysql {
}
}
/**
* Check for deprecated table options during ALTER/CREATE TABLE queries.
*
* @param string $query
*
* @return void
*/
private function checkForDeprecatedTableOptions(string $query): void {
if (preg_match('/(ALTER|CREATE)\s+TABLE\s+/', $query) !== 1) {
return;
}
// Wrong UTF8 charset/collation
$matches = [];
if ($this->use_utf8mb4 && preg_match('/(?<invalid>(utf8(_[^\';\s]+)?))([\';\s]|$)/', $query, $matches)) {
trigger_error(
sprintf(
'Usage of "%s" charset/collation detected, should be "%s"',
$matches['invalid'],
str_replace('utf8', 'utf8mb4', $matches['invalid'])
),
E_USER_WARNING
);
} else if (!$this->use_utf8mb4 && preg_match('/(?<invalid>(utf8mb4(_[^\';\s]+)?))([\';\s]|$)/', $query, $matches)) {
trigger_error(
sprintf(
'Usage of "%s" charset/collation detected, should be "%s"',
$matches['invalid'],
str_replace('utf8mb4', 'utf8', $matches['invalid'])
),
E_USER_WARNING
);
}
// Usage of MyISAM
$matches = [];
if (!$this->allow_myisam && preg_match('/[)\s]engine\s*=\s*\'?myisam([\';\s]|$)/i', $query, $matches)) {
trigger_error('Usage of "MyISAM" engine is discouraged, please use "InnoDB" engine.', E_USER_WARNING);
}
}
/**
* Return configuration boolean properties computed using current state of tables.
*
......@@ -1768,6 +1806,11 @@ class DBmysql {
$config_flags[DBConnection::PROPERTY_USE_UTF8MB4] = true;
}
if ($this->getMyIsamTables(true)->count() === 0) {
// Disallow MyISAM if there is no core table still using this engine.
$config_flags[DBConnection::PROPERTY_ALLOW_MYISAM] = false;
}
return $config_flags;
}
}
......@@ -264,7 +264,16 @@ function step4 ($databasename, $newdatabasename) {
prev_form($host, $user, $password);
} else {
if (DBConnection::createMainConfig($host, $user, $password, $databasename, false, true)) {
$success = DBConnection::createMainConfig(
$host,
$user,
$password,
$databasename,
false,
true,
false
);
if ($success) {
Toolbox::createSchema($_SESSION["glpilanguage"]);
echo "<p>".__('OK - database was initialized')."</p>";
......@@ -281,7 +290,16 @@ function step4 ($databasename, $newdatabasename) {
if ($link->select_db($newdatabasename)) {
echo "<p>".__('Database created')."</p>";
if (DBConnection::createMainConfig($host, $user, $password, $newdatabasename, false, true)) {
$success = DBConnection::createMainConfig(
$host,
$user,
$password,
$newdatabasename,
false,
true,
false
);
if ($success) {
Toolbox::createSchema($_SESSION["glpilanguage"]);
echo "<p>".__('OK - database was initialized')."</p>";
next_form();
......@@ -295,9 +313,21 @@ function step4 ($databasename, $newdatabasename) {
if ($link->query("CREATE DATABASE IF NOT EXISTS `".$newdatabasename."`")) {
echo "<p>".__('Database created')."</p>";
if ($link->select_db($newdatabasename)
&& DBConnection::createMainConfig($host, $user, $password, $newdatabasename, false, true)) {
$select_db = $link->select_db($newdatabasename);
$success = false;
if ($select_db) {
$success = DBConnection::createMainConfig(
$host,
$user,
$password,
$newdatabasename,
false,
true,
false
);
}
if ($success) {
Toolbox::createSchema($_SESSION["glpilanguage"]);
echo "<p>".__('OK - database was initialized')."</p>";
next_form();
......
......@@ -441,7 +441,83 @@ OTHER EXPRESSION;"
->string($this->testedInstance->removeSqlRemarks($sql))->isIdenticalTo($expected);
}
public function testCollationWarnings() {
protected function tableOptionProvider(): iterable {
yield [
'table_options' => '',
'db_properties' => [],
'warning' => null,
];
// Warnings related to MyISAM usage
$myisam_declarations = [
'engine=MyISAM', // without ending `;`
'engine=MyISAM;', // with ending `;`
' Engine = myisam ', // mixed case
' ENGINE = MYISAM ', // uppercase with lots of spaces
" ENGINE = 'MyISAM'", // surrounded by quotes
"ROW_FORMAT=DYNAMIC ENGINE=MyISAM", // preceded by another option
"ENGINE=MyISAM ROW_FORMAT=DYNAMIC", // followed by another option
];
foreach ($myisam_declarations as $table_options) {
yield [
'table_options' => $table_options,
'db_properties' => [
'allow_myisam' => true,
],
'warning' => null,
];
yield [
'table_options' => $table_options,
'db_properties' => [
'allow_myisam' => false,
],
'warning' => 'Usage of "MyISAM" engine is discouraged, please use "InnoDB" engine.',
];
}
// Warnings related to 'utf8mb4' usage when DB not yet migrated to 'utf8mb4'
yield [
'table_options' => 'ENGINE = InnoDB ROW_FORMAT = DYNAMIC DEFAULT CHARSET = utf8 COLLATE = utf8_unicode_ci',
'db_properties' => [
'use_utf8mb4' => false,
],
'warning' => null,
];
yield [
'table_options' => 'ENGINE = InnoDB ROW_FORMAT = DYNAMIC DEFAULT CHARSET = utf8mb4 COLLATE = utf8mb4_unicode_ci',
'db_properties' => [
'use_utf8mb4' => false,
],
'warning' => 'Usage of "utf8mb4" charset/collation detected, should be "utf8"',
];
// Warnings related to 'utf8' usage when DB has been migrated to 'utf8mb4'
yield [
'table_options' => 'ENGINE = InnoDB ROW_FORMAT = DYNAMIC DEFAULT CHARSET = utf8 COLLATE = utf8_unicode_ci',
'db_properties' => [
'use_utf8mb4' => true,
],
'warning' => 'Usage of "utf8" charset/collation detected, should be "utf8mb4"',
];
yield [
'table_options' => 'ENGINE = InnoDB ROW_FORMAT = DYNAMIC DEFAULT CHARSET = utf8mb4 COLLATE = utf8mb4_unicode_ci',
'db_properties' => [
'use_utf8mb4' => true,
],
'warning' => null,
];
}
/**
* @dataProvider tableOptionProvider
*/
public function testAlterOrCreateTableWarnings(
string $table_options,
array $db_properties,
?string $warning = null
) {
$db = new \mock\DB();
$create_query_template = <<<SQL
......@@ -450,52 +526,27 @@ OTHER EXPRESSION;"
`itemtype` varchar(100) NOT NULL,
`items_id` int NOT NULL DEFAULT '0',
PRIMARY KEY (`id`)
) ENGINE = InnoDB ROW_FORMAT = DYNAMIC DEFAULT CHARSET = %s COLLATE = %s
)%s
SQL;
$drop_query_template = 'DROP TABLE `%s`';
$db->use_utf8mb4 = false;
$this->when(
function () use ($db, $create_query_template, $drop_query_template) {
$table = sprintf('glpitests_%s', uniqid());
$db->query(sprintf($create_query_template, $table, 'utf8', 'utf8_unicode_ci'));
$db->query(sprintf($drop_query_template, $table));
$db->log_deprecation_warnings = false; // Prevent deprecation warning from MySQL server
foreach ($db_properties as $db_property => $value) {
$db->$db_property = $value;
}
)->error()->notExists();
$this->when(
function () use ($db, $create_query_template, $drop_query_template) {
$table = sprintf('glpitests_%s', uniqid());
$db->query(sprintf($create_query_template, $table, 'utf8mb4', 'utf8mb4_unicode_ci'));
$db->query(sprintf($drop_query_template, $table));
}
)->error()
->withType(E_USER_WARNING)
->withMessage('Usage of "utf8mb4" charset/collation detected, should be "utf8"')
->exists();
$db->use_utf8mb4 = true;
$db->log_deprecation_warnings = false;
$asserter = $warning === null ? 'notExists' : 'exists';
$this->when(
function () use ($db, $create_query_template, $drop_query_template) {
function () use ($db, $create_query_template, $drop_query_template, $table_options) {
$table = sprintf('glpitests_%s', uniqid());
$db->query(sprintf($create_query_template, $table, 'utf8', 'utf8_unicode_ci'));
$db->query(sprintf($create_query_template, $table, $table_options));
$db->query(sprintf($drop_query_template, $table));
}
)->error()
->withType(E_USER_WARNING)
->withMessage('Usage of "utf8" charset/collation detected, should be "utf8mb4"')
->exists();
$this->when(
function () use ($db, $create_query_template, $drop_query_template) {
$table = sprintf('glpitests_%s', uniqid());
$db->query(sprintf($create_query_template, $table, 'utf8mb4', 'utf8mb4_unicode_ci'));
$db->query(sprintf($drop_query_template, $table));
}
)->error()->notExists();
->withMessage($warning)
->$asserter();
}
public function testSavepoints() {
......
......@@ -80,6 +80,7 @@ class DBConnection extends \GLPITestCase {
'name' => 'glpi_db',
'log_deprecation_warnings' => false,
'use_utf8mb4' => false,
'allow_myisam' => true,
'expected' => <<<'PHP'
<?php
class DB extends DBmysql {
......@@ -98,6 +99,7 @@ PHP
'name' => 'db',
'log_deprecation_warnings' => false,
'use_utf8mb4' => true,
'allow_myisam' => false,
'expected' => <<<'PHP'
<?php
class DB extends DBmysql {
......@@ -106,6 +108,7 @@ class DB extends DBmysql {
public $dbpassword = '';
public $dbdefault = 'db';
public $use_utf8mb4 = true;
public $allow_myisam = false;
}
PHP
......@@ -117,6 +120,7 @@ PHP
'name' => 'db',
'log_deprecation_warnings' => true,
'use_utf8mb4' => false,
'allow_myisam' => true,
'expected' => <<<'PHP'
<?php
class DB extends DBmysql {
......@@ -142,11 +146,21 @@ PHP
string $name,
bool $log_deprecation_warnings,
bool $use_utf8mb4,
bool $allow_myisam,
string $expected
): void {
vfsStream::setup('config-dir', null, []);
$result = \DBConnection::createMainConfig($host, $user, $password, $name, $log_deprecation_warnings, $use_utf8mb4, vfsStream::url('config-dir'));