Merge pull request #114 from xsist10/sql_mode_options

Added SQL strict mode check

resources/mysql/sample.sql

@@ -1,5 +1,6 @@
 SET GLOBAL innodb_file_per_table=OFF;
 SET GLOBAL sql_require_primary_key=OFF;
+SET GLOBAL sql_mode='ONLY_FULL_GROUP_BY,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO';
 
 CREATE DATABASE IF NOT EXISTS test;
 USE test;

src/Cli/Command/RunCommand.php

@@ -16,6 +16,7 @@
 use Cadfael\Engine\Check\Column\SaneAutoIncrement;
 use Cadfael\Engine\Check\Column\UUIDStorage;
 use Cadfael\Engine\Check\Database\InnoDbFilePerTable;
+use Cadfael\Engine\Check\Database\StrictSqlMode;
 use Cadfael\Engine\Check\Index\IndexPrefix;
 use Cadfael\Engine\Check\Index\LowCardinality;
 use Cadfael\Engine\Check\Query\FunctionsOnIndex;
@@ -219,6 +220,7 @@ protected function runChecksAgainstSchema(
             new LockedAccount(),
             new AccountsWithSuperPermission(),
             new InnoDbFilePerTable(),
+            new StrictSqlMode()
         );
 
         if ($load_performance_schema) {

src/Engine/Check/Database/StrictSqlMode.php

@@ -0,0 +1,86 @@
+<?php
+
+declare(strict_types=1);
+
+
+namespace Cadfael\Engine\Check\Database;
+
+use Cadfael\Engine\Check;
+use Cadfael\Engine\Entity\Database;
+use Cadfael\Engine\Entity\Database\SqlModes;
+use Cadfael\Engine\Exception\MySQL\UnknownVersion;
+use Cadfael\Engine\Report;
+
+/**
+ * Class RequirePrimaryKey
+ * @package Cadfael\Engine\Check\Schema
+ */
+class StrictSqlMode implements Check
+{
+    public function supports($entity): bool
+    {
+        if (!$entity instanceof Database) {
+            return false;
+        }
+        // If we can't determine the version we can't do this check
+        try {
+            $version = $entity->getVersion();
+            // We only want to examine MySQL versions >= 8.0.0.
+            // TODO: Add sane defaults for < 8.0.0
+            return version_compare($version, '8.0.0', '>=');
+        } catch (UnknownVersion $e) {
+            return false;
+        }
+    }
+
+    public function run($entity): ?Report
+    {
+        $variables = $entity->getVariables();
+        $sql_mode = SqlModes::normaliseMode($variables['sql_mode']);
+
+        $has_strict_all_tables = in_array('STRICT_ALL_TABLES', $sql_mode) !== false;
+        $has_strict_trans_tables = in_array('STRICT_TRANS_TABLES', $sql_mode) !== false;
+
+        if (!$has_strict_all_tables && !$has_strict_trans_tables) {
+            return new Report(
+                $this,
+                $entity,
+                Report::STATUS_WARNING,
+                [
+                    "It is recommend that you run in strict mode.",
+                    "Either STRICT_ALL_TABLES or STRICT_TRANS_TABLES should be enabled."
+                ]
+            );
+        }
+
+        return new Report(
+            $this,
+            $entity,
+            Report::STATUS_OK
+        );
+    }
+
+    /**
+     * @codeCoverageIgnore
+     */
+    public function getReferenceUri(): string
+    {
+        return 'https://github.com/xsist10/cadfael/wiki/Sql-Strict-Mode';
+    }
+
+    /**
+     * @codeCoverageIgnore
+     */
+    public function getName(): string
+    {
+        return 'Strict SQL Mode';
+    }
+
+    /**
+     * @codeCoverageIgnore
+     */
+    public function getDescription(): string
+    {
+        return 'Recommend that sql strict mode is set avoid data truncation, bad charset conversion, or other issues.';
+    }
+}

src/Engine/Entity/Database/SqlModes.php

@@ -0,0 +1,51 @@
+<?php
+
+declare(strict_types=1);
+
+
+namespace Cadfael\Engine\Entity\Database;
+
+class SqlModes
+{
+    const COMBINED_MODE = [
+        'ANSI' => [
+            'ANSI_QUOTES',
+            'IGNORE_SPACE',
+            'ONLY_FULL_GROUP_BY',
+            'PIPES_AS_CONCAT',
+            'REAL_AS_FLOAT',
+        ],
+        'TRADITIONAL' => [
+            'ERROR_FOR_DIVISION_BY_ZERO',
+            'NO_ENGINE_SUBSTITUTION',
+            'NO_ZERO_DATE',
+            'NO_ZERO_IN_DATE',
+            'STRICT_ALL_TABLES',
+            'STRICT_TRANS_TABLES',
+        ]
+    ];
+
+    const DEFAULT_MODE = [
+        'ERROR_FOR_DIVISION_BY_ZERO',
+        'NO_ENGINE_SUBSTITUTION',
+        'NO_ZERO_DATE',
+        'NO_ZERO_IN_DATE',
+        'ONLY_FULL_GROUP_BY',
+        'STRICT_TRANS_TABLES',
+    ];
+
+    public static function normaliseMode(?string $mode): array
+    {
+        $modes = $mode ? explode(',', $mode) : self::DEFAULT_MODE;
+        // Search and replace any COMBINED_MODE value you find
+        $normalised_modes = [];
+        foreach ($modes as $entry) {
+            if (isset(self::COMBINED_MODE[$entry])) {
+                $normalised_modes += self::COMBINED_MODE[$entry];
+            } else {
+                $normalised_modes[] = $entry;
+            }
+        }
+        return array_unique($normalised_modes);
+    }
+}

tests/Engine/Check/Database/StrictSqlModeTest.php

@@ -0,0 +1,76 @@
+<?php
+
+declare(strict_types=1);
+
+
+namespace Cadfael\Tests\Engine\Check\Database;
+
+use Cadfael\Engine\Check\Database\StrictSqlMode;
+use Cadfael\Engine\Report;
+use Cadfael\Tests\Engine\BaseTest;
+
+class StrictSqlModeTest extends BaseTest
+{
+    private array $databases;
+
+    public function setUp(): void
+    {
+        $this->databases = [
+            '8.0_NOT_SET'     => $this->createDatabase([ 'version' => '8.0.0', 'sql_mode' => null]),
+            '8.0_STRICT_A'    => $this->createDatabase([ 'version' => '8.0.0', 'sql_mode' => 'STRICT_ALL_TABLES' ]),
+            '8.0_STRICT_T'    => $this->createDatabase([ 'version' => '8.0.0', 'sql_mode' => 'STRICT_TRANS_TABLES' ]),
+            '8.0_TRADITIONAL' => $this->createDatabase([ 'version' => '8.0.0', 'sql_mode' => 'TRADITIONAL' ]),
+            '8.0_NOT_STRICT'  => $this->createDatabase([ 'version' => '8.0.0', 'sql_mode' => 'ON' ]),
+            '5.0_TRADITIONAL' => $this->createDatabase([ 'version' => '5.0.0', 'sql_mode' => 'TRADITIONAL' ])
+        ];
+    }
+
+    public function testSupports()
+    {
+        $check = new StrictSqlMode();
+
+        $this->assertTrue($check->supports($this->databases['8.0_NOT_SET']), "Ensure that we only care about databases >= 8.0.");
+        $this->assertTrue($check->supports($this->databases['8.0_STRICT_A']), "Ensure that we only care about databases >= 8.0.");
+        $this->assertTrue($check->supports($this->databases['8.0_STRICT_T']), "Ensure that we only care about databases >= 8.0.");
+        $this->assertTrue($check->supports($this->databases['8.0_TRADITIONAL']), "Ensure that we only care about databases >= 8.0.");
+        $this->assertTrue($check->supports($this->databases['8.0_NOT_STRICT']), "Ensure that we only care about databases >= 8.0.");
+        $this->assertFalse($check->supports($this->databases['5.0_TRADITIONAL']), "Ensure that we only care about databases >= 8.0.");
+        $this->assertFalse($check->supports($this->createTable()), "We only support database entities.");
+    }
+
+    public function testRun()
+    {
+        $check = new StrictSqlMode();
+
+        $this->assertEquals(
+            Report::STATUS_OK,
+            $check->run($this->databases['8.0_NOT_SET'])->getStatus(),
+            "Ensure we return " . Report::STATUS_OK . " if nothing is set (default)."
+        );
+
+        $this->assertEquals(
+            Report::STATUS_OK,
+            $check->run($this->databases['8.0_STRICT_A'])->getStatus(),
+            "Ensure we return " . Report::STATUS_OK . " if STRICT_ALL_TABLES is set."
+        );
+
+        $this->assertEquals(
+            Report::STATUS_OK,
+            $check->run($this->databases['8.0_STRICT_T'])->getStatus(),
+            "Ensure we return " . Report::STATUS_OK . " if STRICT_TRANS_TABLES is set."
+        );
+
+        $this->assertEquals(
+            Report::STATUS_OK,
+            $check->run($this->databases['8.0_TRADITIONAL'])->getStatus(),
+            "Ensure we return " . Report::STATUS_OK . " if TRADITIONAL is set (strict included)."
+        );
+
+        $this->assertEquals(
+            Report::STATUS_WARNING,
+            $check->run($this->databases['8.0_NOT_STRICT'])->getStatus(),
+            "Ensure we return " . Report::STATUS_WARNING . " if strict is not enabled."
+        );
+    }
+
+}