Merge pull request #11 from php-openapi/151-remove-unnecessary-null-o…

…r-default-related-migrations-in-pgsql Resolve: Remove unnecessary null or default related migrations in PgSQL cebe#151
php-openapi · Nov 12, 2024 · 6fbf532 · 6fbf532
2 parents 65d9146 + e73d9a0
commit 6fbf532
Show file tree

Hide file tree

Showing 10 changed files with 29 additions and 15 deletions.
diff --git a/README.md b/README.md
@@ -398,7 +398,7 @@ User:
 `NOT NULL` in DB migrations is determined by `nullable` and `required` properties of the OpenAPI schema.
 e.g. attribute = 'my_property'.
 
-- If you define attribute neither "required" nor via "nullable", then it is by default `NULL`:
+- If you define attribute neither "required" nor via "nullable", then it is by default `NULL` ([opposite of OpenAPI spec](https://swagger.io/specification/v3/?sbsearch=nullable)):
 
 ```yaml
   ExampleSchema:

diff --git a/src/lib/ColumnToCode.php b/src/lib/ColumnToCode.php
@@ -69,7 +69,7 @@ class ColumnToCode
      * @var bool
      * Built In Type means the \cebe\yii2openapi\lib\items\Attribute::$type or \cebe\yii2openapi\lib\items\Attribute::$dbType is in list of Yii abstract data type list or not. And if is found we can use \yii\db\SchemaBuilderTrait methods to build migration instead of putting raw SQL
      */
-    private $isBuiltinType = false;
+    public $isBuiltinType = false;
 
     /**
      * @var bool
@@ -384,6 +384,7 @@ private function getIsBuiltinType($type, $dbType)
         if ($this->isEnum()) {
             return false;
         }
+
         if ($this->fromDb === true) {
             return isset(
                 (new ColumnSchemaBuilder(''))->categoryMap[$type]
@@ -465,7 +466,7 @@ private function resolveDefaultValue():void
 
     private function isDefaultAllowed():bool
     {
-        // default expression with parenthases is allowed
+        // default expression with parentheses is allowed
         if ($this->column->defaultValue instanceof \yii\db\Expression) {
             return true;
         }

diff --git a/src/lib/migrations/MigrationRecordBuilder.php b/src/lib/migrations/MigrationRecordBuilder.php
@@ -51,6 +51,12 @@ final class MigrationRecordBuilder
      */
     private $dbSchema;
 
+    /**
+     * @var bool
+     * Only required for PgSQL alter column for set null/set default related statement
+     */
+    public $isBuiltInType = false;
+
     public function __construct(Schema $dbSchema)
     {
         $this->dbSchema = $dbSchema;
@@ -134,6 +140,7 @@ public function alterColumnType(string $tableAlias, ColumnSchema $column, bool $
     {
         if (property_exists($column, 'xDbType') && is_string($column->xDbType) && !empty($column->xDbType)) {
             $converter = $this->columnToCode($tableAlias, $column, false, false, true, true);
+            $this->isBuiltInType = $converter->isBuiltinType;
             return sprintf(
                 ApiGenerator::isPostgres() ? self::ALTER_COLUMN_RAW_PGSQL : self::ALTER_COLUMN_RAW,
                 $tableAlias,
@@ -142,6 +149,7 @@ public function alterColumnType(string $tableAlias, ColumnSchema $column, bool $
             );
         }
         $converter = $this->columnToCode($tableAlias, $column, false);
+        $this->isBuiltInType = $converter->isBuiltinType;
         return sprintf(self::ALTER_COLUMN, $tableAlias, $column->name, $converter->getAlterExpression($addUsing));
     }
 
@@ -153,6 +161,7 @@ public function alterColumnTypeFromDb(string $tableAlias, ColumnSchema $column,
     {
         if (property_exists($column, 'xDbType') && is_string($column->xDbType) && !empty($column->xDbType)) {
             $converter = $this->columnToCode($tableAlias, $column, true, false, true, true);
+            $this->isBuiltInType = $converter->isBuiltinType;
             return sprintf(
                 ApiGenerator::isPostgres() ? self::ALTER_COLUMN_RAW_PGSQL : self::ALTER_COLUMN_RAW,
                 $tableAlias,
@@ -161,6 +170,7 @@ public function alterColumnTypeFromDb(string $tableAlias, ColumnSchema $column,
             );
         }
         $converter = $this->columnToCode($tableAlias, $column, true);
+        $this->isBuiltInType = $converter->isBuiltinType;
         return sprintf(self::ALTER_COLUMN, $tableAlias, $column->name, $converter->getAlterExpression($addUsing));
     }
 

diff --git a/src/lib/migrations/PostgresMigrationBuilder.php b/src/lib/migrations/PostgresMigrationBuilder.php
@@ -62,16 +62,20 @@ protected function buildColumnChanges(ColumnSchema $current, ColumnSchema $desir
             // This action require several steps and can't be applied during single transaction
             return;
         }
-
+        $forUp = $forDown = false;
         if (!empty(array_intersect(['type', 'size'
                     , 'dbType', 'phpType'
                     , 'precision', 'scale', 'unsigned'
         ], $changed))) {
             $addUsing = $this->isNeedUsingExpression($current->dbType, $desired->dbType);
             $this->migration->addUpCode($this->recordBuilder->alterColumnType($tableName, $desired, $addUsing));
+            $forUp = $this->recordBuilder->isBuiltInType;
             $this->migration->addDownCode($this->recordBuilder->alterColumnTypeFromDb($tableName, $current, $addUsing));
+            $forDown = $this->recordBuilder->isBuiltInType;
         }
-        if (in_array('allowNull', $changed, true)) {
+        if (in_array('allowNull', $changed, true)
+            && ($forUp === false || $forDown === false)
+        ) {
             if ($desired->allowNull === true) {
                 $this->migration->addUpCode($this->recordBuilder->dropColumnNotNull($tableName, $desired));
                 $this->migration->addDownCode($this->recordBuilder->setColumnNotNull($tableName, $current), true);
@@ -80,6 +84,9 @@ protected function buildColumnChanges(ColumnSchema $current, ColumnSchema $desir
                 $this->migration->addDownCode($this->recordBuilder->dropColumnNotNull($tableName, $current), true);
             }
         }
+
+        $this->recordBuilder->isBuiltInType = $forUp = $forDown = false;
+
         if (in_array('defaultValue', $changed, true)) {
             $upCode = $desired->defaultValue === null
                 ? $this->recordBuilder->dropColumnDefault($tableName, $desired)
@@ -96,9 +103,6 @@ protected function buildColumnChanges(ColumnSchema $current, ColumnSchema $desir
         }
         if ($isChangeFromEnum) {
             $this->migration->addUpCode($this->recordBuilder->dropEnum($tableName, $current->name));
-        }
-
-        if ($isChangeFromEnum) {
             $this->migration
                 ->addDownCode($this->recordBuilder->createEnum($tableName, $current->name, $current->enumValues));
         }

diff --git a/...tring_varchar_datatype_149/app/migrations_pgsql_db/m200000_000000_change_table_fruits.php b/...tring_varchar_datatype_149/app/migrations_pgsql_db/m200000_000000_change_table_fruits.php
@@ -8,12 +8,10 @@ class m200000_000000_change_table_fruits extends \yii\db\Migration
     public function safeUp()
     {
         $this->alterColumn('{{%fruits}}', 'name', $this->string(151)->notNull());
-        $this->alterColumn('{{%fruits}}', 'name', "SET NOT NULL");
     }
 
     public function safeDown()
     {
         $this->alterColumn('{{%fruits}}', 'name', $this->string(150)->null());
-        $this->alterColumn('{{%fruits}}', 'name', "DROP NOT NULL");
     }
 }
diff --git a/...ype/edit_column/pgsql/app/migrations_pgsql_db/m200000_000001_change_table_editcolumns.php b/...ype/edit_column/pgsql/app/migrations_pgsql_db/m200000_000001_change_table_editcolumns.php
@@ -14,7 +14,6 @@ public function safeUp()
         $this->db->createCommand('ALTER TABLE {{%editcolumns}} ALTER COLUMN "name" SET DATA TYPE varchar(254)')->execute();
         $this->alterColumn('{{%editcolumns}}', 'name', "SET DEFAULT 'Horse-2'");
         $this->alterColumn('{{%editcolumns}}', 'string_col', 'text NULL USING "string_col"::text');
-        $this->alterColumn('{{%editcolumns}}', 'string_col', "DROP NOT NULL");
         $this->db->createCommand('ALTER TABLE {{%editcolumns}} ALTER COLUMN "dec_col" SET DATA TYPE decimal(12,2) USING "dec_col"::decimal(12,2)')->execute();
         $this->alterColumn('{{%editcolumns}}', 'dec_col', "SET DEFAULT 3.14");
         $this->alterColumn('{{%editcolumns}}', 'str_col_def', "SET NOT NULL");
@@ -25,6 +24,7 @@ public function safeUp()
         $this->alterColumn('{{%editcolumns}}', 'json_col_2', "SET NOT NULL");
         $this->alterColumn('{{%editcolumns}}', 'json_col_2', "SET DEFAULT '[]'");
         $this->db->createCommand('ALTER TABLE {{%editcolumns}} ALTER COLUMN "numeric_col" SET DATA TYPE double precision USING "numeric_col"::double precision')->execute();
+        $this->alterColumn('{{%editcolumns}}', 'numeric_col', "SET NOT NULL");
     }
 
     public function safeDown()
@@ -39,13 +39,13 @@ public function safeDown()
         $this->dropColumn('{{%editcolumns}}', 'json_col_def_n');
         $this->dropColumn('{{%editcolumns}}', 'first_name');
         $this->alterColumn('{{%editcolumns}}', 'name', "SET DEFAULT 'Horse'");
-        $this->alterColumn('{{%editcolumns}}', 'string_col', "SET NOT NULL");
         $this->alterColumn('{{%editcolumns}}', 'dec_col', "DROP DEFAULT");
         $this->alterColumn('{{%editcolumns}}', 'str_col_def', "DROP NOT NULL");
         $this->alterColumn('{{%editcolumns}}', 'str_col_def', "SET DEFAULT 'hi there'");
         $this->alterColumn('{{%editcolumns}}', 'json_col', "DROP NOT NULL");
         $this->alterColumn('{{%editcolumns}}', 'json_col', "DROP DEFAULT");
         $this->alterColumn('{{%editcolumns}}', 'json_col_2', "DROP NOT NULL");
         $this->alterColumn('{{%editcolumns}}', 'json_col_2', "DROP DEFAULT");
+        $this->alterColumn('{{%editcolumns}}', 'numeric_col', "DROP NOT NULL");
     }
 }
diff --git a/...x_db_type/fresh/pgsql/app/migrations_pgsql_db/m200000_000001_create_table_editcolumns.php b/...x_db_type/fresh/pgsql/app/migrations_pgsql_db/m200000_000001_create_table_editcolumns.php
@@ -17,7 +17,7 @@ public function safeUp()
             3 => '"str_col_def" varchar NOT NULL',
             4 => '"json_col" text NOT NULL DEFAULT \'fox jumps over dog\'',
             5 => '"json_col_2" jsonb NOT NULL DEFAULT \'[]\'',
-            6 => '"numeric_col" double precision NULL DEFAULT NULL',
+            6 => '"numeric_col" double precision NOT NULL',
             7 => '"json_col_def_n" json NOT NULL DEFAULT \'[]\'',
             8 => '"json_col_def_n_2" json NOT NULL DEFAULT \'[]\'',
             9 => '"text_col_array" text[] NULL DEFAULT NULL',

diff --git a/tests/specs/x_db_type/fresh/pgsql/x_db_type_pgsql.yaml b/tests/specs/x_db_type/fresh/pgsql/x_db_type_pgsql.yaml
@@ -516,6 +516,7 @@ components:
         numeric_col:
           type: string
           x-db-type: double precision
+          nullable: false
         json_col_def_n:
           type: string
           x-db-type: json

diff --git a/...type/new_column/pgsql/app/migrations_pgsql_db/m200000_000001_create_table_editcolumns.php b/...type/new_column/pgsql/app/migrations_pgsql_db/m200000_000001_create_table_editcolumns.php
@@ -17,7 +17,7 @@ public function safeUp()
             3 => '"str_col_def" varchar NOT NULL',
             4 => '"json_col" text NOT NULL DEFAULT \'fox jumps over dog\'',
             5 => '"json_col_2" jsonb NOT NULL DEFAULT \'[]\'',
-            6 => '"numeric_col" double precision NULL DEFAULT NULL',
+            6 => '"numeric_col" double precision NOT NULL',
             7 => '"json_col_def_n" json NOT NULL DEFAULT \'[]\'',
             8 => '"json_col_def_n_2" json NOT NULL DEFAULT \'[]\'',
             9 => '"text_col_array" text[] NULL DEFAULT NULL',

diff --git a/tests/specs/x_db_type/rules_and_more/pgsql/app/models/pgsqlmodel/base/Editcolumn.php b/tests/specs/x_db_type/rules_and_more/pgsql/app/models/pgsqlmodel/base/Editcolumn.php
@@ -35,7 +35,7 @@ public function rules()
     {
         return [
             'trim' => [['name', 'tag', 'first_name', 'string_col', 'str_col_def', 'json_col'], 'trim'],
-            'required' => [['name', 'str_col_def', 'json_col', 'json_col_2'], 'required'],
+            'required' => [['name', 'str_col_def', 'json_col', 'json_col_2', 'numeric_col'], 'required'],
             'name_string' => [['name'], 'string', 'max' => 254],
             'name_default' => [['name'], 'default', 'value' => 'Horse-2'],
             'tag_string' => [['tag'], 'string'],