Merge #59 remote-tracking branch 'php-openapi/58-create-migration-for-column-position-change-if-a-field-position-is-changed-in-spec'

* php-openapi/58-create-migration-for-column-position-change-if-a-field-position-is-changed-in-spec: (26 commits)
  Modify variable name
  Add more tests + refactor `setColumnsPositions()` to remove unwanted migrations
  Refactor
  Add docs + refactor
  Fix bug
  Change tests and add few more
  Fix bugs + fix failing tests 2
  Fix bugs + fix failing tests
  Fix other failing tests
  Fix failing tests
  Fix bugs + fix failing tests + add more tests + refactor
  Refactor
  Fix bugs
  Fix bugs + fix failing tests + add more tests
  Resolve TODOs + add tests
  Implement for many test cases
  Implementation in progress + add more tests
  Refactor
  Add more tests 2
  Add more tests
  ...

Author: cebe

src/db/ColumnSchema.php

@@ -25,4 +25,24 @@ class ColumnSchema extends \yii\db\ColumnSchema
      * ```
      */
     public $xDbType;
+
+    /**
+     * Used only for MySQL/MariaDB
+     * @var array|null
+     * [
+     *      index => int # position: starts from 1
+     *      after => ?string # after column
+     *      before => ?string # before column
+     * ]
+     * If `before` is null then column is last
+     * If `after` is null then column is first
+     * If both are null then table has only 1 column
+     */
+    public ?array $fromPosition = null;
+    public ?array $toPosition = null;
+
+    /**
+     * From `$this->fromPosition` and `$this->toPosition` we can check if the position is changed or not. This is done in `BaseMigrationBuilder::setColumnsPositions()`
+     */
+    public bool $isPositionChanged = false;
 }

src/lib/migrations/BaseMigrationBuilder.php

@@ -171,6 +171,7 @@ public function buildSecondary(?ManyToManyRelation $relation = null):MigrationMo
     {
         $this->migration = Yii::createObject(MigrationModel::class, [$this->model, false, $relation, []]);
         $this->newColumns = $relation->columnSchema ?? $this->model->attributesToColumnSchema();
+        $this->setColumnsPositions();
         $wantNames = array_keys($this->newColumns);
         $haveNames = $this->tableSchema->columnNames;
         $columnsForCreate = array_map(
@@ -184,7 +185,7 @@ function (string $missingColumn) {
             function (string $unknownColumn) {
                 return $this->tableSchema->columns[$unknownColumn];
             },
-            array_diff($haveNames, $wantNames)
+            array_reverse(array_diff($haveNames, $wantNames), true)
         );
 
         $columnsForChange = array_intersect($wantNames, $haveNames);
@@ -220,6 +221,7 @@ function (string $unknownColumn) {
         } else {
             $this->buildRelations();
         }
+
         return $this->migration;
     }
 
@@ -249,12 +251,12 @@ protected function buildColumnsDrop(array $columns):void
     {
         foreach ($columns as $column) {
             $tableName = $this->model->getTableAlias();
+            $position = $this->findPosition($column, true);
             if ($column->isPrimaryKey && !$column->autoIncrement) {
                 $pkName = 'pk_' . $this->model->tableName . '_' . $column->name;
                 $this->migration->addDownCode($this->recordBuilder->addPrimaryKey($tableName, [$column->name], $pkName))
                                 ->addUpCode($this->recordBuilder->dropPrimaryKey($tableName, [$column->name], $pkName));
             }
-            $position = $this->findPosition($column, true);
             $this->migration->addDownCode($this->recordBuilder->addDbColumn($tableName, $column, $position))
                             ->addUpCode($this->recordBuilder->dropColumn($tableName, $column->name));
         }
@@ -513,47 +515,6 @@ public function isDefaultValueChanged(
         return false;
     }
 
-    /**
-     * Given a column, compute its previous column name present in OpenAPI schema
-     * @return ?string
-     * `null` if column is added at last
-     * 'FIRST' if column is added at first position
-     * 'AFTER <columnName>' if column is added in between e.g. if 'email' is added after 'username' then 'AFTER username'
-     */
-    public function findPosition(ColumnSchema $column, bool $forDrop = false): ?string
-    {
-        $columnNames = array_keys($forDrop ? $this->tableSchema->columns : $this->newColumns);
-
-        $key = array_search($column->name, $columnNames);
-        if ($key > 0) {
-            $prevColName = $columnNames[$key-1];
-
-            if (!isset($columnNames[$key+1])) { // if new col is added at last then no need to add 'AFTER' SQL part. This is checked as if next column is present or not
-                return null;
-            }
-
-            // in case of `down()` code of migration, putting 'after <colName>' in add column statmenet is erroneous because <colName> may not exist.
-            // Example: From col a, b, c, d, if I drop c and d then their migration code will be generated like:
-            // `up()` code
-            // drop c
-            // drop d
-            // `down()` code
-            // add d after c (c does not exist! Error!)
-            // add c
-            if ($forDrop) {
-                return null;
-            }
-
-
-            return self::POS_AFTER . ' ' . $prevColName;
-
-        // if no `$columnSchema` is found, previous column does not exist. This happens when 'after column' is not yet added in migration or added after currently undertaken column
-        } elseif ($key === 0) {
-            return self::POS_FIRST;
-        }
-
-        return null;
-    }
 
     public function modifyDesiredFromDbInContextOfDesired(ColumnSchema $desired, ColumnSchema $desiredFromDb): void
     {
@@ -574,4 +535,19 @@ public function modifyDesiredInContextOfDesiredFromDb(ColumnSchema $desired, Col
         }
         $desired->dbType = $desiredFromDb->dbType;
     }
+
+    /**
+     * Only for MySQL and MariaDB
+     * Given a column, compute its previous column name present in OpenAPI schema
+     * @param ColumnSchema $column
+     * @param bool $forDrop
+     * @param bool $forAlter
+     * @return ?string
+     * `null` if column is added at last
+     * 'FIRST' if column is added at first position
+     * 'AFTER <columnName>' if column is added in between e.g. if 'email' is added after 'username' then 'AFTER username'
+     */
+    abstract public function findPosition(ColumnSchema $column, bool $forDrop = false, bool $forAlter = false): ?string;
+
+    abstract public function setColumnsPositions();
 }

src/lib/migrations/MigrationRecordBuilder.php

@@ -118,28 +118,32 @@ public function addDbColumn(string $tableAlias, ColumnSchema $column, ?string $p
     /**
      * @throws \yii\base\InvalidConfigException
      */
-    public function alterColumn(string $tableAlias, ColumnSchema $column):string
+    public function alterColumn(string $tableAlias, ColumnSchema $column, ?string $position = null):string
     {
         if (property_exists($column, 'xDbType') && is_string($column->xDbType) && !empty($column->xDbType)) {
-            $converter = $this->columnToCode($tableAlias, $column, true, false, true, true);
+            $converter = $this->columnToCode($tableAlias, $column, true, false, true, true, $position);
             return sprintf(
                 ApiGenerator::isPostgres() ? self::ALTER_COLUMN_RAW_PGSQL : self::ALTER_COLUMN_RAW,
                 $tableAlias,
                 $column->name,
                 ColumnToCode::escapeQuotes($converter->getCode())
             );
         }
-        $converter = $this->columnToCode($tableAlias, $column, true);
+        $converter = $this->columnToCode($tableAlias, $column, true, false, false, false, $position);
         return sprintf(self::ALTER_COLUMN, $tableAlias, $column->name, $converter->getCode(true));
     }
 
     /**
      * @throws \yii\base\InvalidConfigException
      */
-    public function alterColumnType(string $tableAlias, ColumnSchema $column, bool $addUsing = false):string
-    {
+    public function alterColumnType(
+        string $tableAlias,
+        ColumnSchema $column,
+        bool $addUsing = false,
+        ?string $position = null
+    ):string {
         if (property_exists($column, 'xDbType') && is_string($column->xDbType) && !empty($column->xDbType)) {
-            $converter = $this->columnToCode($tableAlias, $column, false, false, true, true);
+            $converter = $this->columnToCode($tableAlias, $column, false, false, true, true, $position);
             $this->isBuiltInType = $converter->isBuiltinType;
             return sprintf(
                 ApiGenerator::isPostgres() ? self::ALTER_COLUMN_RAW_PGSQL : self::ALTER_COLUMN_RAW,
@@ -148,7 +152,7 @@ public function alterColumnType(string $tableAlias, ColumnSchema $column, bool $
                 rtrim(ltrim($converter->getAlterExpression($addUsing), "'"), "'")
             );
         }
-        $converter = $this->columnToCode($tableAlias, $column, false);
+        $converter = $this->columnToCode($tableAlias, $column, false, false, false, false, $position);
         $this->isBuiltInType = $converter->isBuiltinType;
         return sprintf(self::ALTER_COLUMN, $tableAlias, $column->name, $converter->getAlterExpression($addUsing));
     }
@@ -157,10 +161,14 @@ public function alterColumnType(string $tableAlias, ColumnSchema $column, bool $
      * This method is only used in Pgsql
      * @throws \yii\base\InvalidConfigException
      */
-    public function alterColumnTypeFromDb(string $tableAlias, ColumnSchema $column, bool $addUsing = false) :string
-    {
+    public function alterColumnTypeFromDb(
+        string $tableAlias,
+        ColumnSchema $column,
+        bool $addUsing = false,
+        ?string $position = null
+    ) :string {
         if (property_exists($column, 'xDbType') && is_string($column->xDbType) && !empty($column->xDbType)) {
-            $converter = $this->columnToCode($tableAlias, $column, true, false, true, true);
+            $converter = $this->columnToCode($tableAlias, $column, true, false, true, true, $position);
             $this->isBuiltInType = $converter->isBuiltinType;
             return sprintf(
                 ApiGenerator::isPostgres() ? self::ALTER_COLUMN_RAW_PGSQL : self::ALTER_COLUMN_RAW,
@@ -169,7 +177,7 @@ public function alterColumnTypeFromDb(string $tableAlias, ColumnSchema $column,
                 rtrim(ltrim($converter->getAlterExpression($addUsing), "'"), "'")
             );
         }
-        $converter = $this->columnToCode($tableAlias, $column, true);
+        $converter = $this->columnToCode($tableAlias, $column, true, false, false, false, $position);
         $this->isBuiltInType = $converter->isBuiltinType;
         return sprintf(self::ALTER_COLUMN, $tableAlias, $column->name, $converter->getAlterExpression($addUsing));
     }

src/lib/migrations/MysqlMigrationBuilder.php

@@ -14,9 +14,7 @@
 use yii\db\ColumnSchema;
 use yii\db\IndexConstraint;
 use yii\db\Schema;
-use \Yii;
 use yii\helpers\ArrayHelper;
-use yii\helpers\VarDumper;
 
 final class MysqlMigrationBuilder extends BaseMigrationBuilder
 {
@@ -25,15 +23,22 @@ final class MysqlMigrationBuilder extends BaseMigrationBuilder
      */
     protected function buildColumnChanges(ColumnSchema $current, ColumnSchema $desired, array $changed):void
     {
+        $positionCurrent = $positionDesired = null;
+        if (in_array('position', $changed, true)) {
+            $positionDesired = $this->findPosition($desired, false, true);
+            $positionCurrent = $this->findPosition($desired, true, true);
+            $key = array_search('position', $changed, true);
+            unset($changed[$key]);
+        }
         $newColumn = clone $current;
         foreach ($changed as $attr) {
             $newColumn->$attr = $desired->$attr;
         }
         if (static::isEnum($newColumn)) {
             $newColumn->dbType = 'enum'; // TODO this is concretely not correct
         }
-        $this->migration->addUpCode($this->recordBuilder->alterColumn($this->model->getTableAlias(), $newColumn))
-                        ->addDownCode($this->recordBuilder->alterColumn($this->model->getTableAlias(), $current));
+        $this->migration->addUpCode($this->recordBuilder->alterColumn($this->model->getTableAlias(), $newColumn, $positionDesired))
+            ->addDownCode($this->recordBuilder->alterColumn($this->model->getTableAlias(), $current, $positionCurrent));
     }
 
     protected function compareColumns(ColumnSchema $current, ColumnSchema $desired):array
@@ -68,6 +73,11 @@ protected function compareColumns(ColumnSchema $current, ColumnSchema $desired):
                 }
             }
         }
+
+        if (property_exists($desired, 'isPositionChanged') && $desired->isPositionChanged) {
+            $changedAttributes[] = 'position';
+        }
+
         return $changedAttributes;
     }
 
@@ -159,4 +169,139 @@ public function modifyDesiredInContextOfCurrent(ColumnSchema $current, ColumnSch
             $desired->size = $current->size;
         }
     }
+
+    /**
+     * {@inheritDoc}
+     */
+    public function findPosition(ColumnSchema $column, bool $forDrop = false, bool $forAlter = false): ?string
+    {
+        $columnNames = array_keys($forDrop ? $this->tableSchema->columns : $this->newColumns);
+
+        $key = array_search($column->name, $columnNames);
+        if ($key > 0) {
+            $prevColName = $columnNames[$key - 1];
+            if (($key === count($columnNames) - 1) && !$forAlter) {
+                return null;
+            }
+
+            if (array_key_exists($prevColName, $forDrop ? $this->tableSchema->columns : $this->newColumns)) {
+                if ($forDrop && !$forAlter) {
+                    // if the previous column is the last one in the want names then no need for AFTER
+                    $cols = array_keys($this->newColumns);
+                    if ($prevColName === array_pop($cols)) {
+                        return null;
+                    }
+                }
+                if ($forAlter && $forDrop) {
+                    if (!array_key_exists($prevColName, $this->newColumns)) {
+                        return null;
+                    }
+                }
+                return self::POS_AFTER . ' ' . $prevColName;
+            }
+            return null;
+
+        // if no `$columnSchema` is found, previous column does not exist. This happens when 'after column' is not yet added in migration or added after currently undertaken column
+        } elseif ($key === 0) {
+            return self::POS_FIRST;
+        }
+
+        return null;
+    }
+
+    public function setColumnsPositions()
+    {
+        $i = 0;
+        $haveColumns = $this->tableSchema->columns;
+        $wantNames = array_keys($this->newColumns);
+        $haveNames = array_keys($haveColumns);
+
+        // Part 1/2 compute from and to position
+        foreach ($this->newColumns as $name => $column) {
+            /** @var \cebe\yii2openapi\db\ColumnSchema $column */
+            $column->toPosition = [
+                'index' => $i + 1,
+                'after' => $i === 0 ? null : $wantNames[$i - 1],
+                'before' => $i === (count($wantNames) - 1) ? null : $wantNames[$i + 1],
+            ];
+
+            if (isset($haveColumns[$name])) {
+                $index = array_search($name, $haveNames) + 1;
+                $column->fromPosition = [
+                    'index' => $index,
+                    'after' => $haveNames[$index - 2] ?? null,
+                    'before' => $haveNames[$index] ?? null,
+                ];
+            }
+
+            $i++;
+        }
+
+        // Part 2/2 compute is position is really changed
+
+        // check if only new columns are added without any explicit position change
+        $namesForCreate = array_diff($wantNames, $haveNames);
+        $wantNamesWoNewCols = array_values(array_diff($wantNames, $namesForCreate));
+        if ($namesForCreate && $haveNames === $wantNamesWoNewCols) {
+            return;
+        }
+        // check if only existing columns are deleted without any explicit position change
+        $namesForDrop = array_diff($haveNames, $wantNames);
+        $haveNamesWoDropCols = array_values(array_diff($haveNames, $namesForDrop));
+        if ($namesForDrop && $wantNames === $haveNamesWoDropCols) {
+            return;
+        }
+        // check both above simultaneously
+        if ($namesForCreate && $namesForDrop && ($wantNamesWoNewCols === $haveNamesWoDropCols)) {
+            return;
+        }
+
+        $takenIndices = $nonRedundantIndices = []; # $nonRedundantIndices are the wanted ones which are created by moving of one or more columns. Example: if a column is moved from 2nd to 8th position then we will consider only one column is moved ignoring index/position change(-1) of 4rd to 8th column (4->3, 5->4 ...). So migration for this unwanted indices changes won't be generated. `$takenIndices` might have redundant indices
+        foreach ($this->newColumns as $column) {
+            /** @var \cebe\yii2openapi\db\ColumnSchema $column */
+
+            if (!$column->fromPosition || !$column->toPosition) {
+                continue;
+            }
+            if (is_int(array_search([$column->toPosition['index'], $column->fromPosition['index']], $takenIndices))) {
+                continue;
+            }
+            if ($column->fromPosition === $column->toPosition) {
+                continue;
+            }
+            if ($column->fromPosition['index'] === $column->toPosition['index']) {
+                continue;
+            }
+
+            $column->isPositionChanged = true;
+            $takenIndices[] = [$column->fromPosition['index'], $column->toPosition['index']];
+
+            // -------
+            if (($column->fromPosition['before'] !== $column->toPosition['before']) &&
+                ($column->fromPosition['after'] !== $column->toPosition['after'])
+            ) {
+                $nonRedundantIndices[] = [$column->fromPosition['index'], $column->toPosition['index']];
+            }
+        }
+
+        foreach ($this->newColumns as $column) {
+            /** @var \cebe\yii2openapi\db\ColumnSchema $column */
+
+            if (!isset($column->toPosition['index'], $column->fromPosition['index'])) {
+                continue;
+            }
+            $condition = (abs($column->toPosition['index'] - $column->fromPosition['index']) === count($nonRedundantIndices));
+            if (($column->fromPosition['before'] === $column->toPosition['before'])
+                && $condition
+            ) {
+                $column->isPositionChanged = false;
+                continue;
+            }
+            if (($column->fromPosition['after'] === $column->toPosition['after'])
+                && $condition
+            ) {
+                $column->isPositionChanged = false;
+            }
+        }
+    }
 }