From 3b992e6a13d18141a432d9c0d42fb72bdca425f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Kr=C3=B6g?= Date: Tue, 9 Apr 2024 01:28:56 +0200 Subject: [PATCH] Preserve check constraint during column move MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Krög --- js/src/table/structure.js | 8 +- .../Table/Structure/MoveColumnsController.php | 187 +++++++----------- psalm-baseline.xml | 54 +---- .../Structure/MoveColumnsControllerTest.php | 127 ++++++++++++ 4 files changed, 212 insertions(+), 164 deletions(-) create mode 100644 test/classes/Controllers/Table/Structure/MoveColumnsControllerTest.php diff --git a/js/src/table/structure.js b/js/src/table/structure.js index 497d0011ebff..3ad81fc68ed0 100644 --- a/js/src/table/structure.js +++ b/js/src/table/structure.js @@ -322,9 +322,15 @@ AJAX.registerOnload('table/structure.js', function () { designerModalPreviewModal.addEventListener('shown.bs.modal', () => { const modalBody = designerModalPreviewModal.querySelector('.modal-body'); const $form = $('#move_column_form'); + const serialized = $form.serialize(); + if (serialized === $form.data('serialized-unmoved')) { + modalBody.innerHTML = ''; + return; + } + const formUrl = $form.attr('action'); const sep = CommonParams.get('arg_separator'); - const formData = $form.serialize() + + const formData = serialized + sep + 'preview_sql=1' + sep + 'ajax_request=1'; $.post({ diff --git a/libraries/classes/Controllers/Table/Structure/MoveColumnsController.php b/libraries/classes/Controllers/Table/Structure/MoveColumnsController.php index e945055b736e..13f9ac39b2df 100644 --- a/libraries/classes/Controllers/Table/Structure/MoveColumnsController.php +++ b/libraries/classes/Controllers/Table/Structure/MoveColumnsController.php @@ -8,26 +8,24 @@ use PhpMyAdmin\DatabaseInterface; use PhpMyAdmin\Message; use PhpMyAdmin\ResponseRenderer; -use PhpMyAdmin\Table; +use PhpMyAdmin\SqlParser\Components\CreateDefinition; +use PhpMyAdmin\SqlParser\Parser; +use PhpMyAdmin\SqlParser\Statements\CreateStatement; use PhpMyAdmin\Template; use PhpMyAdmin\Util; use function __; +use function array_diff; use function array_keys; +use function array_search; use function array_splice; +use function assert; use function count; use function implode; -use function in_array; use function is_array; -use function mb_strtoupper; -use function sprintf; -use function str_replace; final class MoveColumnsController extends AbstractController { - /** @var Table The table object */ - private $tableObj; - /** @var DatabaseInterface */ private $dbi; @@ -40,123 +38,29 @@ public function __construct( ) { parent::__construct($response, $template, $db, $table); $this->dbi = $dbi; - $this->tableObj = $this->dbi->getTable($this->db, $this->table); } public function __invoke(): void { - if (! isset($_POST['move_columns']) || ! is_array($_POST['move_columns']) || ! $this->response->isAjax()) { + $moveColumns = $_POST['move_columns'] ?? null; + $previewSql = $_REQUEST['preview_sql'] ?? null; + if (! is_array($moveColumns) || ! $this->response->isAjax()) { + $this->response->setRequestStatus(false); + return; } $this->dbi->selectDb($this->db); + $createTableSql = $this->dbi->getTable($this->db, $this->table)->showCreate(); + $sql_query = $this->generateAlterTableSql($createTableSql, $moveColumns); - /** - * load the definitions for all columns - */ - $columns = $this->dbi->getColumnsFull($this->db, $this->table); - $column_names = array_keys($columns); - $changes = []; - - // @see https://mariadb.com/kb/en/library/changes-improvements-in-mariadb-102/#information-schema - $usesLiteralNull = $this->dbi->isMariaDB() && $this->dbi->getVersion() >= 100200; - $defaultNullValue = $usesLiteralNull ? 'NULL' : null; - // move columns from first to last - for ($i = 0, $l = count($_POST['move_columns']); $i < $l; $i++) { - $column = $_POST['move_columns'][$i]; - // is this column already correctly placed? - if ($column_names[$i] == $column) { - continue; - } - - // it is not, let's move it to index $i - $data = $columns[$column]; - $extracted_columnspec = Util::extractColumnSpec($data['Type']); - if (isset($data['Extra']) && $data['Extra'] === 'on update CURRENT_TIMESTAMP') { - $extracted_columnspec['attribute'] = $data['Extra']; - unset($data['Extra']); - } - - $timeType = $data['Type'] === 'timestamp' || $data['Type'] === 'datetime'; - $timeDefault = $data['Default'] === 'CURRENT_TIMESTAMP' || $data['Default'] === 'current_timestamp()'; - $current_timestamp = $timeType && $timeDefault; - - $uuidType = $data['Type'] === 'uuid'; - $uuidDefault = $data['Default'] === 'UUID' || $data['Default'] === 'uuid()'; - $uuid = $uuidType && $uuidDefault; - - // @see https://mariadb.com/kb/en/library/information-schema-columns-table/#examples - if ($data['Null'] === 'YES' && in_array($data['Default'], [$defaultNullValue, null])) { - $default_type = 'NULL'; - } elseif ($current_timestamp) { - $default_type = 'CURRENT_TIMESTAMP'; - } elseif ($uuid) { - $default_type = 'UUID'; - } elseif ($data['Default'] === null) { - $default_type = 'NONE'; - } else { - $default_type = 'USER_DEFINED'; - } - - $virtual = [ - 'VIRTUAL', - 'PERSISTENT', - 'VIRTUAL GENERATED', - 'STORED GENERATED', - ]; - $data['Virtuality'] = ''; - $data['Expression'] = ''; - if (isset($data['Extra']) && in_array($data['Extra'], $virtual)) { - $data['Virtuality'] = str_replace(' GENERATED', '', $data['Extra']); - $expressions = $this->tableObj->getColumnGenerationExpression($column); - $data['Expression'] = is_array($expressions) ? $expressions[$column] : null; - } - - $changes[] = 'CHANGE ' . Table::generateAlter( - $column, - $column, - mb_strtoupper($extracted_columnspec['type']), - $extracted_columnspec['spec_in_brackets'], - $extracted_columnspec['attribute'], - $data['Collation'] ?? '', - $data['Null'] === 'YES' ? 'YES' : 'NO', - $default_type, - $current_timestamp ? '' : $data['Default'], - isset($data['Extra']) && $data['Extra'] !== '' ? $data['Extra'] - : false, - isset($data['COLUMN_COMMENT']) && $data['COLUMN_COMMENT'] !== '' - ? $data['COLUMN_COMMENT'] : false, - $data['Virtuality'], - $data['Expression'], - $i === 0 ? '-first' : $column_names[$i - 1] - ); - // update current column_names array, first delete old position - for ($j = 0, $ll = count($column_names); $j < $ll; $j++) { - if ($column_names[$j] != $column) { - continue; - } - - unset($column_names[$j]); - } - - // insert moved column - array_splice($column_names, $i, 0, $column); - } - - if (empty($changes) && ! isset($_REQUEST['preview_sql'])) { // should never happen + if ($sql_query === null) { $this->response->setRequestStatus(false); return; } - // query for moving the columns - $sql_query = sprintf( - 'ALTER TABLE %s %s', - Util::backquote($this->table), - implode(', ', $changes) - ); - - if (isset($_REQUEST['preview_sql'])) { // preview sql + if ($previewSql) { $this->response->addJSON( 'sql_data', $this->template->render('preview_sql', ['query_data' => $sql_query]) @@ -178,6 +82,65 @@ public function __invoke(): void __('The columns have been moved successfully.') ); $this->response->addJSON('message', $message); - $this->response->addJSON('columns', $column_names); + $this->response->addJSON('columns', $moveColumns); + } + + /** + * @param array $moveColumns + * @psalm-param list $moveColumns + */ + private function generateAlterTableSql(string $createTableSql, array $moveColumns): ?string + { + $parser = new Parser($createTableSql); + /** @var CreateStatement $statement */ + $statement = $parser->statements[0]; + /** @var CreateDefinition[] $fields */ + $fields = $statement->fields; + $columns = []; + foreach ($fields as $field) { + if ($field->name === null) { + continue; + } + + $columns[$field->name] = $field; + } + + $columnNames = array_keys($columns); + // Ensure the columns from client match the columns from the table + if ( + count($columnNames) !== count($moveColumns) || + array_diff($columnNames, $moveColumns) !== [] + ) { + return null; + } + + $changes = []; + + // move columns from first to last + /** @psalm-var list $moveColumns */ + foreach ($moveColumns as $i => $columnName) { + // is this column already correctly placed? + if ($columnNames[$i] == $columnName) { + continue; + } + + $changes[] = + 'CHANGE ' . Util::backquote($columnName) . ' ' . CreateDefinition::build($columns[$columnName]) . + ($i === 0 ? ' FIRST' : ' AFTER ' . Util::backquote($columnNames[$i - 1])); + + // Move column to its new position + /** @var int $j */ + $j = array_search($columnName, $columnNames, true); + array_splice($columnNames, $j, 1); + array_splice($columnNames, $i, 0, $columnName); + } + + if ($changes === []) { + return null; + } + + assert($statement->name !== null, 'Alter table statement has no name'); + + return 'ALTER TABLE ' . Util::backquote($statement->name->table) . "\n " . implode(",\n ", $changes); } } diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 955301ac82de..6e23b046b451 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -3814,57 +3814,9 @@ - - $current_timestamp ? '' : $data['Default'] - $data['Collation'] ?? '' - $data['Expression'] - $data['Extra'] - $data['Type'] - $data['Virtuality'] - $extracted_columnspec['attribute'] - $extracted_columnspec['spec_in_brackets'] - $extracted_columnspec['type'] - $i === 0 ? '-first' : $column_names[$i - 1] - - - $data['Collation'] - $data['Default'] - $data['Default'] - $data['Expression'] - $data['Null'] - $data['Null'] - $data['Type'] - $data['Type'] - $data['Virtuality'] - - - $data['Expression'] - $data['Virtuality'] - - - $data - $data['Expression'] - - - - $column - $column - $column - - - $columns[$column] - $expressions[$column] - - - $column - $column - - - $current_timestamp ? '' : $data['Default'] - $data['Collation'] ?? '' - $data['Expression'] - $data['Virtuality'] - + + $moveColumns + diff --git a/test/classes/Controllers/Table/Structure/MoveColumnsControllerTest.php b/test/classes/Controllers/Table/Structure/MoveColumnsControllerTest.php new file mode 100644 index 000000000000..0d393d7daa59 --- /dev/null +++ b/test/classes/Controllers/Table/Structure/MoveColumnsControllerTest.php @@ -0,0 +1,127 @@ + $columnNames + * @psalm-param list $columnNames + * + * @dataProvider providerForTestGenerateAlterTableSql + */ + public function testGenerateAlterTableSql(string $createStatement, array $columnNames, ?string $expected): void + { + $class = new ReflectionClass(MoveColumnsController::class); + $method = $class->getMethod('generateAlterTableSql'); + $method->setAccessible(true); + + $controller = new MoveColumnsController( + new ResponseStub(), + new Template(), + 'test-db', + 'test', + $this->dbi + ); + /** @var string|null $alterStatement */ + $alterStatement = $method->invoke($controller, $createStatement, $columnNames); + + $expected = $expected === null ? null : preg_replace('/\r?\n/', "\n", $expected); + $alterStatement = $alterStatement === null ? null : preg_replace('/\r?\n/', "\n", $alterStatement); + $this->assertSame($expected, $alterStatement); + } + + /** + * Data provider for testGenerateAlterTableSql + * + * @return array> + * @psalm-return list,string}> + */ + public static function providerForTestGenerateAlterTableSql(): array + { + return [ + // MariaDB / column CHECK constraint + [ + <<<'SQL' +CREATE TABLE `test` ( + `id` int(11) NOT NULL AUTO_INCREMENT, + `name` varchar(45) DEFAULT NULL, + `data` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL CHECK (json_valid(`json`)), + PRIMARY KEY (`id`) +) +SQL +, + ['id', 'data', 'name'], + <<<'SQL' +ALTER TABLE `test` + CHANGE `data` `data` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL CHECK (json_valid(`json`)) AFTER `id` +SQL +, + ], + // MariaDB / text column with uuid() default + [ + <<<'SQL' +CREATE TABLE `test` ( + `Id` int(11) NOT NULL, + `First` text NOT NULL DEFAULT uuid(), + `Second` text NOT NULL DEFAULT uuid() +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci +SQL +, + ['Id', 'Second', 'First'], + <<<'SQL' +ALTER TABLE `test` + CHANGE `Second` `Second` text NOT NULL DEFAULT uuid() AFTER `Id` +SQL +, + ], + // MySQL 8.0.13 text column with uuid() default + [ + <<<'SQL' +CREATE TABLE `test` ( + `Id` int(11) NOT NULL, + `First` text COLLATE utf8mb4_general_ci NOT NULL DEFAULT (uuid()), + `Second` text COLLATE utf8mb4_general_ci NOT NULL DEFAULT (uuid()) +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci +SQL +, + ['Id', 'Second', 'First'], + <<<'SQL' +ALTER TABLE `test` + CHANGE `Second` `Second` text COLLATE utf8mb4_general_ci NOT NULL DEFAULT (uuid()) AFTER `Id` +SQL +, + ], + // enum with default + [ + <<<'SQL' +CREATE TABLE `test` ( + `id` int(11) NOT NULL, + `enum` enum('yes','no') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'no', + PRIMARY KEY (`id`) +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3 COLLATE=utf8mb3_unicode_ci +SQL +, + ['enum', 'id'], + <<<'SQL' +ALTER TABLE `test` + CHANGE `enum` `enum` enum('yes','no') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'no' FIRST +SQL +, + ], + ]; + } +}