Skip to content

Commit

Permalink
MDL-69581 dml: Improve MySQL/MariaDB server version detection
Browse files Browse the repository at this point in the history 
Add 'versionfromdb' to opt-in for VERSION() query.

mariadb_native_moodle_database::get_server_info() has been removed
to avoid the specific override: everything is now managed in the base
method which takes care of both MySQL and MariaDB versioning details.

New protected methods have been added too to improve the testing strategy,
allowing to mock the results from an actual DB connection.
The new testing strategy enables simulating even the Azure scenarios.
  • Loading branch information
@scara
scara committed Dec 28, 2022
1 parent 15a695d commit 0131c3b
Show file tree
Hide file tree
Showing 5 changed files with 278 additions and 18 deletions.
8 changes: 8 additions & 0 deletions config-dist.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@
// can be removed for MySQL (by default it will
// use 'utf8mb4_unicode_ci'. This option should
// be removed for all other databases.
// 'versionfromdb' => false, // On MySQL and MariaDB, this can force
// the DB version to be evaluated using
// the VERSION function instead of the version
// provided by the PHP client which could be
// wrong based on the DB server infrastructure,
// e.g. PaaS on Azure. Default is false/unset.
// Uncomment and set to true to force MySQL and
// MariaDB to use 'SELECT VERSION();'.
// 'fetchbuffersize' => 100000, // On PostgreSQL, this option sets a limit
// on the number of rows that are fetched into
// memory when doing a large recordset query
Expand Down
14 changes: 0 additions & 14 deletions lib/dml/mariadb_native_moodle_database.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,20 +74,6 @@ protected function get_dbtype() {
return 'mariadb';
}

/**
* Returns database server info array
* @return array Array containing 'description' and 'version' info
*/
public function get_server_info() {
$version = $this->mysqli->server_info;
$matches = null;
if (preg_match('/^5\.5\.5-(10\..+)-MariaDB/i', $version, $matches)) {
// Looks like MariaDB decided to use these weird version numbers for better BC with MySQL...
$version = $matches[1];
}
return array('description'=>$this->mysqli->server_info, 'version'=>$version);
}

protected function has_breaking_change_quoted_defaults() {
$version = $this->get_server_info()['version'];
// Breaking change since 10.2.7: MDEV-13132.
Expand Down
99 changes: 96 additions & 3 deletions lib/dml/mysqli_native_moodle_database.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ class mysqli_native_moodle_database extends moodle_database {
protected $mysqli = null;
/** @var bool is compressed row format supported cache */
protected $compressedrowformatsupported = null;
/** @var string DB server actual version */
protected $serverversion = null;

private $transactions_supported = null;

Expand Down Expand Up @@ -666,11 +668,102 @@ protected function can_use_readonly(int $type, string $sql): bool {
}

/**
* Returns database server info array
* @return array Array containing 'description' and 'version' info
* Returns the version of the MySQL server, as reported by the PHP client connection.
*
* Wrap $this->mysqli->server_info to improve testing strategy.
*
* @return string A string representing the version of the MySQL server that the MySQLi extension is connected to.
*/
protected function get_mysqli_server_info(): string {
return $this->mysqli->server_info;
}

/**
* Returns the version of the MySQL server, as reported by 'SELECT VERSION()' query.
*
* @return string A string that indicates the MySQL server version.
* @throws dml_read_exception If the execution of 'SELECT VERSION()' query will fail.
*/
protected function get_version_from_db(): string {
$version = null;
// Query the DB server for the server version.
$sql = "SELECT VERSION() version;";
try {
$result = $this->mysqli->query($sql);
if ($result) {
if ($row = $result->fetch_assoc()) {
$version = $row['version'];
}
$result->close();
unset($row);
}
} catch (\Throwable $e) { // Exceptions in case of MYSQLI_REPORT_STRICT.
// It looks like we've an issue out of the expected boolean 'false' result above.
throw new dml_read_exception($e->getMessage(), $sql);
}
if (empty($version)) {
// Exception dml_read_exception usually reports raw mysqli errors i.e. not localised by Moodle.
throw new dml_read_exception("Unable to read the DB server version.", $sql);
}

return $version;
}

/**
* Returns whether $CFG->dboptions['versionfromdb'] has been set to boolean `true`.
*
* @return bool True if $CFG->dboptions['versionfromdb'] has been set to boolean `true`. Otherwise, `false`.
*/
protected function should_db_version_be_read_from_db(): bool {
if (!empty($this->dboptions['versionfromdb'])) {
return true;
}

return false;
}

/**
* Returns database server info array.
* @return array Array containing 'description' and 'version' info.
* @throws dml_read_exception If the execution of 'SELECT VERSION()' query will fail.
*/
public function get_server_info() {
return array('description'=>$this->mysqli->server_info, 'version'=>$this->mysqli->server_info);
$version = $this->serverversion;
if (empty($version)) {
$version = $this->get_mysqli_server_info();
// The version returned by the PHP client could not be the actual DB server version.
// For example in MariaDB, it was prefixed by the RPL_VERSION_HACK, "5.5.5-" (MDEV-4088), starting from 10.x,
// when not using an authentication plug-in.
// Strip the RPL_VERSION_HACK prefix off - it will be "always" there in MariaDB until MDEV-28910 will be implemented.
$version = str_replace('5.5.5-', '', $version);

// Should we use the VERSION function to get the actual DB version instead of the PHP client version above?
if ($this->should_db_version_be_read_from_db()) {
// Try to query the actual version of the target database server: indeed some cloud providers, e.g. Azure,
// put a gateway in front of the actual instance which reports its own version to the PHP client
// and it doesn't represent the actual version of the DB server the PHP client is connected to.
// Refs:
// - https://learn.microsoft.com/en-us/azure/mariadb/concepts-supported-versions
// - https://learn.microsoft.com/en-us/azure/mysql/single-server/concepts-connect-to-a-gateway-node .
// Reset the version returned by the PHP client with the actual DB version reported by 'VERSION' function.
$version = $this->get_version_from_db();
}

// The version here starts with the following naming scheme: 'X.Y.Z[-<suffix>]'.
// Example: in MariaDB at least one suffix is "always" there, hardcoded in 'mysql_version.h.in':
// #define MYSQL_SERVER_VERSION "@VERSION@-MariaDB"
// MariaDB and MySQL server version could have extra suffixes too, set by the compilation environment,
// e.g. '-debug', '-embedded', '-log' or any other vendor specific suffix (e.g. build information).
// Strip out any suffix.
$parts = explode('-', $version, 2);
// Finally, keep just major, minor and patch versions (X.Y.Z) from the reported DB server version.
$this->serverversion = $parts[0];
}

return [
'description' => $this->get_mysqli_server_info(),
'version' => $this->serverversion
];
}

/**
Expand Down
171 changes: 171 additions & 0 deletions lib/dml/tests/dml_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6143,6 +6143,177 @@ public function test_four_byte_character_insertion() {

$dbman->drop_table($table);
}

/**
* Mock the methods used by {@see \mysqli_native_moodle_database::get_server_info()}.
*
* Mocking allows to test it without the need of an actual MySQL-ish running DB server.
*
* @param string $mysqliserverinfo A string representing the server info as provided by the MySQLi extension.
* @param string $versionfromdb A string representing the result of VERSION function.
* @param bool $cfgversionfromdb A boolean representing !empty($CFG->dboptions['versionfromdb']).
* @param string $expecteddbversion A string representing the expected DB version.
* @see \mysqli_native_moodle_database::get_server_info()
* @covers \mysqli_native_moodle_database::get_server_info
* @dataProvider get_server_info_mysql_provider
*/
public function test_get_server_info_mysql(
string $mysqliserverinfo, string $versionfromdb, bool $cfgversionfromdb, string $expecteddbversion) {
// Avoid to run MySQL-ish related tests when running tests on other DB families.
$DB = $this->tdb;
if ($DB->get_dbfamily() != 'mysql') {
$this->markTestSkipped("Not MySQL family");
}

// Mock the methods used by get_server_info() to simulate different MySQL-ish DB servers.
$methods = [
'get_mysqli_server_info',
'get_version_from_db',
'should_db_version_be_read_from_db',
];
$mysqlinativemoodledatabase = $this->getMockBuilder('\mysqli_native_moodle_database')
->onlyMethods($methods)
->getMock();
$mysqlinativemoodledatabase->method('get_mysqli_server_info')->willReturn($mysqliserverinfo);
$mysqlinativemoodledatabase->method('get_version_from_db')->willReturn($versionfromdb);
$mysqlinativemoodledatabase->method('should_db_version_be_read_from_db')->willReturn($cfgversionfromdb);

['description' => $description, 'version' => $version] = $mysqlinativemoodledatabase->get_server_info();
$this->assertEquals($mysqliserverinfo, $description);
$this->assertEquals($expecteddbversion, $version);
}

/**
* Data provider to test {@see \mysqli_native_moodle_database::get_server_info} when mocking
* the results of a connection to the DB server.
*
* The set of the data is represented by the following array items:
* - a string representing the server info as provided by the MySQLi extension
* - a string representing the result of VERSION function
* - a boolean representing !empty($CFG->dboptions['versionfromdb'])
* - a string representing the expected DB version
*
* @return array[]
* @see \mysqli_native_moodle_database::get_server_info
*/
public function get_server_info_mysql_provider() {
return [
'MySQL 5.7.39 - MySQLi version' => [
'5.7.39-log',
'',
false,
'5.7.39'
],
'MySQL 5.7.40 - MySQLi version' => [
'5.7.40',
'',
false,
'5.7.40'
],
'MySQL 8.0.31 - MySQLi version' => [
'8.0.31',
'',
false,
'8.0.31'
],
'MariaDB 10.4.26 (https://moodle.org/mod/forum/discuss.php?d=441156#p1774957) - MySQLi version' => [
'10.4.26-MariaDB-1:10.4.26+mariadb~deb10',
'',
false,
'10.4.26'
],
'MariaDB 10.4.27 - MySQLi version' => [
'5.5.5-10.4.27-MariaDB',
'',
false,
'10.4.27'
],
'MariaDB 10.4.27 - DB version' => [
'',
'10.4.27-MariaDB',
true,
'10.4.27'
],
'MariaDB 10.7.7 - MySQLi version' => [
'10.7.7-MariaDB-1:10.7.7+maria~ubu2004',
'',
false,
'10.7.7'
],
'MariaDB 10.7.7 - DB version' => [
'',
'10.7.7-MariaDB-1:10.7.7+maria~ubu2004',
true,
'10.7.7'
],
'MariaDB 10.2.32 on Azure via gateway - MySQLi version' => [
'5.6.42.0',
'10.2.32-MariaDB',
false,
'5.6.42.0'
],
'MariaDB 10.2.32 on Azure via gateway - DB version' => [
'5.6.42.0',
'10.2.32-MariaDB',
true,
'10.2.32'
],
'MariaDB 10.3.23 on Azure via gateway - DB version' => [
'5.6.47.0',
'10.3.23-MariaDB',
true,
'10.3.23'
],
];
}

/**
* Test {@see \mysqli_native_moodle_database::get_server_info()} with the actual DB Server.
* @see \mysqli_native_moodle_database::get_server_info
* @covers \mysqli_native_moodle_database::get_server_info
*/
public function test_get_server_info_dbfamily_mysql() {
$DB = $this->tdb;
if ($DB->get_dbfamily() != 'mysql') {
$this->markTestSkipped("Not MySQL family");
}

$cfg = $DB->export_dbconfig();
if (!isset($cfg->dboptions)) {
$cfg->dboptions = [];
}
// By default, DB Server version is read from the PHP client.
$this->assertTrue(empty($cfg->dboptions['versionfromdb']));
$rc = new \ReflectionClass(\mysqli_native_moodle_database::class);
$rcm = $rc->getMethod('should_db_version_be_read_from_db');
$rcm->setAccessible(true);
$this->assertFalse($rcm->invokeArgs($DB, []));

['description' => $description, 'version' => $version] = $DB->get_server_info();
// MariaDB RPL_VERSION_HACK sanity check: "5.5.5" has never been released!
$this->assertNotSame('5.5.5', $version,
"Found invalid DB server version i.e. RPL_VERSION_HACK: '${version}' (${description}).");
// DB version format is: "X.Y.Z".
$this->assertMatchesRegularExpression('/^\d+\.\d+\.\d+$/', $version,
"Found invalid DB server version format: '${version}' (${description}).");

// Alter the DB options to force the read from DB and check for the same assertions above.
$cfg->dboptions['versionfromdb'] = true;
// Open a new DB connection with the forced setting.
$db2 = moodle_database::get_driver_instance($cfg->dbtype, $cfg->dblibrary);
$db2->connect($cfg->dbhost, $cfg->dbuser, $cfg->dbpass, $cfg->dbname, $cfg->prefix, $cfg->dboptions);
$cfg2 = $db2->export_dbconfig();
$cfg = null;
$this->assertNotEmpty($cfg2->dboptions);
$this->assertFalse(empty($cfg2->dboptions['versionfromdb']), 'Invalid test state!');
$this->assertTrue($rcm->invokeArgs($db2, []), 'Invalid test state!');
['description' => $description, 'version' => $version] = $db2->get_server_info();
$this->assertNotSame('5.5.5', $version,
"Found invalid DB server version when reading version from DB i.e. RPL_VERSION_HACK: '${version}' (${description}).");
$this->assertMatchesRegularExpression('/^\d+\.\d+\.\d+$/', $version,
"Found invalid DB server version format when reading version from DB: '${version}' (${description}).");
$db2->dispose();
}
}

/**
Expand Down
4 changes: 3 additions & 1 deletion lib/upgrade.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ information provided here is intended especially for developers.
groups_get_members_join which could also accept an array of groups, or the constant USERSWITHOUTGROUP.
This has now been made consistent. These enrol methods now accept all the group-related options that
groups_get_members_join can handle.

* New DB parameter 'versionfromdb', only available for MySQL and MariaDB drivers. It allows to force the DB version to be
evaluated through an explicit call to VERSION() to skip the PHP client version which appears to be sometimes fooled by the
underlying infrastructure, e.g. PaaS on Azure.

=== 4.1 ===

Expand Down

0 comments on commit 0131c3b

Please sign in to comment.