Fixed merging nullable with non nullable schema definitions (#1111)

Fixed mergning nullable with non nullable schema definitions
flow-php · Jul 4, 2024 · f888699 · f888699
1 parent 03178b2
commit f888699
Show file tree

Hide file tree

Showing 4 changed files with 107 additions and 55 deletions.
diff --git a/src/core/etl/src/Flow/ETL/Row/Schema.php b/src/core/etl/src/Flow/ETL/Row/Schema.php
@@ -176,17 +176,11 @@ public function merge(self $schema) : self
         foreach ($schema->definitions as $entry => $definition) {
             if (!\array_key_exists($definition->entry()->name(), $newDefinitions)) {
                 $newDefinitions[$entry] = $definition->makeNullable();
-            } elseif (!$newDefinitions[$entry]->isEqual($definition)) {
+            } else {
                 $newDefinitions[$entry] = $newDefinitions[$entry]->merge($definition);
             }
         }
 
-        foreach ($schema->definitions as $entry => $definition) {
-            if (!\array_key_exists($definition->entry()->name(), $newDefinitions)) {
-                $newDefinitions[$entry] = $definition->makeNullable();
-            }
-        }
-
         foreach ($newDefinitions as $entry => $definition) {
             if (!\array_key_exists($definition->entry()->name(), $schema->definitions)) {
                 $newDefinitions[$entry] = $definition->makeNullable();

diff --git a/src/core/etl/tests/Flow/ETL/Tests/Unit/PHP/Type/Logical/DateTimeTypeTest.php b/src/core/etl/tests/Flow/ETL/Tests/Unit/PHP/Type/Logical/DateTimeTypeTest.php
@@ -28,6 +28,22 @@ public function test_is_valid() : void
         self::assertFalse(type_datetime()->isValid('2020-01-01 00:00:00'));
     }
 
+    public function test_merge_non_nullable_with_non_nullable() : void
+    {
+        self::assertFalse(type_datetime()->merge(type_datetime())->nullable());
+    }
+
+    public function test_merge_non_nullable_with_nullable() : void
+    {
+        self::assertTrue(type_datetime()->merge(type_datetime(true))->nullable());
+        self::assertTrue(type_datetime(true)->merge(type_datetime(false))->nullable());
+    }
+
+    public function test_merge_nullable_with_nullable() : void
+    {
+        self::assertTrue(type_datetime(true)->merge(type_datetime(true))->nullable());
+    }
+
     public function test_to_string() : void
     {
         self::assertSame(

diff --git a/src/core/etl/tests/Flow/ETL/Tests/Unit/Row/Schema/DefinitionTest.php b/src/core/etl/tests/Flow/ETL/Tests/Unit/Row/Schema/DefinitionTest.php
@@ -4,7 +4,8 @@
 
 namespace Flow\ETL\Tests\Unit\Row\Schema;
 
-use function Flow\ETL\DSL\{int_entry,
+use function Flow\ETL\DSL\{datetime_schema,
+    int_entry,
     str_entry,
     struct_element,
     struct_entry,
@@ -75,6 +76,19 @@ public function test_merge_definitions() : void
         );
     }
 
+    public function test_merge_nullable_with_non_nullable_dateime_definitions() : void
+    {
+        self::assertEquals(
+            datetime_schema('col', true),
+            datetime_schema('col')->merge(datetime_schema('col', true))
+        );
+
+        self::assertEquals(
+            datetime_schema('col'),
+            datetime_schema('col')->merge(datetime_schema('col'))
+        );
+    }
+
     public function test_merging_anything_and_string() : void
     {
         self::assertEquals(

diff --git a/src/core/etl/tests/Flow/ETL/Tests/Unit/Row/SchemaMergeTest.php b/src/core/etl/tests/Flow/ETL/Tests/Unit/Row/SchemaMergeTest.php
@@ -4,82 +4,82 @@
 
 namespace Flow\ETL\Tests\Unit\Row;
 
-use Flow\ETL\Row\{Schema};
+use function Flow\ETL\DSL\{bool_schema, datetime_schema, float_schema, int_schema, object_schema, schema, str_schema, type_object};
 use PHPUnit\Framework\TestCase;
 
 final class SchemaMergeTest extends TestCase
 {
     public function test_merge_different_schemas() : void
     {
-        $schema = (new Schema(
-            Schema\Definition::integer('id'),
-            Schema\Definition::string('name', nullable: true)
+        $schema = (schema(
+            int_schema('id'),
+            str_schema('name', nullable: true)
         ))->merge(
-            new Schema(
-                Schema\Definition::boolean('test'),
+            schema(
+                bool_schema('test'),
             )
         );
 
         self::assertEquals(
-            new Schema(
-                Schema\Definition::integer('id', nullable: true),
-                Schema\Definition::string('name', nullable: true),
-                Schema\Definition::boolean('test', nullable: true),
+            schema(
+                int_schema('id', nullable: true),
+                str_schema('name', nullable: true),
+                bool_schema('test', nullable: true),
             ),
             $schema
         );
     }
 
     public function test_merge_different_schemas_with_common_parts() : void
     {
-        $schema = (new Schema(
-            Schema\Definition::integer('id'),
-            Schema\Definition::string('name')
+        $schema = (schema(
+            int_schema('id'),
+            str_schema('name')
         ))->merge(
-            new Schema(
-                Schema\Definition::boolean('test'),
-                Schema\Definition::string('name')
+            schema(
+                bool_schema('test'),
+                str_schema('name')
             )
         );
 
         self::assertEquals(
-            new Schema(
-                Schema\Definition::integer('id', nullable: true),
-                Schema\Definition::string('name'),
-                Schema\Definition::boolean('test', nullable: true),
+            schema(
+                int_schema('id', nullable: true),
+                str_schema('name'),
+                bool_schema('test', nullable: true),
             ),
             $schema
         );
     }
 
     public function test_merge_different_schemas_with_common_parts_but_different_nullable_definitions() : void
     {
-        $schema = (new Schema(
-            Schema\Definition::integer('id'),
-            Schema\Definition::string('name', nullable: true)
+        $schema = (schema(
+            int_schema('id'),
+            str_schema('name', nullable: true)
         ))->merge(
-            new Schema(
-                Schema\Definition::boolean('test'),
-                Schema\Definition::string('name', nullable: false)
+            schema(
+                bool_schema('test'),
+                str_schema('name', nullable: false)
             )
         );
 
         self::assertEquals(
-            new Schema(
-                Schema\Definition::integer('id', nullable: true),
-                Schema\Definition::string('name', nullable: true),
-                Schema\Definition::boolean('test', nullable: true),
+            schema(
+                int_schema('id', nullable: true),
+                str_schema('name', nullable: true),
+                bool_schema('test', nullable: true),
             ),
             $schema
         );
     }
 
     public function test_merge_int_empty_schema() : void
     {
-        $schema = (new Schema())->merge(
-            $notEmptySchema = new Schema(
-                Schema\Definition::integer('id', nullable: true),
-                Schema\Definition::string('name', nullable: true)
+        $schema = (schema())->merge(
+            $notEmptySchema = schema(
+                int_schema('id', nullable: true),
+                str_schema('name', nullable: true)
             )
         );
 
@@ -91,37 +91,65 @@ public function test_merge_int_empty_schema() : void
 
     public function test_merge_schema() : void
     {
-        $schema = (new Schema(
-            Schema\Definition::integer('id', nullable: true),
-            Schema\Definition::string('name', nullable: true)
+        $schema = (schema(
+            int_schema('id', nullable: true),
+            str_schema('name', nullable: true)
         ))->merge(
-            new Schema(
-                Schema\Definition::string('test'),
+            schema(
+                str_schema('test'),
             )
         );
 
         self::assertEquals(
-            new Schema(
-                Schema\Definition::integer('id', nullable: true),
-                Schema\Definition::string('name', nullable: true),
-                Schema\Definition::string('test', nullable: true),
+            schema(
+                int_schema('id', nullable: true),
+                str_schema('name', nullable: true),
+                str_schema('test', nullable: true),
             ),
             $schema
         );
     }
 
     public function test_merge_with_empty_schema() : void
     {
-        $schema = ($notEmptySchema = new Schema(
-            Schema\Definition::integer('id', nullable: true),
-            Schema\Definition::string('name', nullable: true)
+        $schema = ($notEmptySchema = schema(
+            int_schema('id', nullable: true),
+            str_schema('name', nullable: true)
         ))->merge(
-            new Schema()
+            schema()
         );
 
         self::assertEquals(
             $notEmptySchema,
             $schema
         );
     }
+
+    public function test_nullable_with_non_nullable_schema() : void
+    {
+        self::assertEquals(
+            schema(str_schema('col', nullable: true)),
+            schema(str_schema('col'))->merge(schema(str_schema('col', nullable: true)))
+        );
+        self::assertEquals(
+            schema(int_schema('col', nullable: true)),
+            schema(int_schema('col'))->merge(schema(int_schema('col', nullable: true)))
+        );
+        self::assertEquals(
+            schema(bool_schema('col', nullable: true)),
+            schema(bool_schema('col'))->merge(schema(bool_schema('col', nullable: true)))
+        );
+        self::assertEquals(
+            schema(float_schema('col', nullable: true)),
+            schema(float_schema('col'))->merge(schema(float_schema('col', nullable: true)))
+        );
+        self::assertEquals(
+            schema(object_schema('col', type_object(\stdClass::class, nullable: true))),
+            schema(object_schema('col', type_object(\stdClass::class)))->merge(schema(object_schema('col', type_object(\stdClass::class, nullable: true))))
+        );
+        self::assertEquals(
+            schema(datetime_schema('col', nullable: true)),
+            schema(datetime_schema('col'))->merge(schema(datetime_schema('col', nullable: true)))
+        );
+    }
 }