Fix Oracle insert AI inconsistency

.github/workflows/test-unit.yml

@@ -156,14 +156,12 @@ jobs:
           php -r '(new PDO("pgsql:host=postgres;dbname=atk4_test", "atk4_test", "atk4_pass"))->exec("ALTER ROLE atk4_test CONNECTION LIMIT 1");'
           if [ -n "$LOG_COVERAGE" ]; then mkdir coverage; fi
 
-      - name: "Run tests: SQLite (only for Phpunit)"
-        if: startsWith(matrix.type, 'Phpunit')
+      - name: "Run tests: SQLite"
         run: |
           php -d opcache.enable_cli=1 vendor/bin/phpunit --exclude-group none $(if [ -n "$LOG_COVERAGE" ]; then echo --coverage-text; else echo --no-coverage; fi) -v
           if [ -n "$LOG_COVERAGE" ]; then mv coverage/phpunit.cov coverage/phpunit-sqlite.cov; fi
 
-      - name: "Run tests: MySQL (only for Phpunit)"
-        if: startsWith(matrix.type, 'Phpunit')
+      - name: "Run tests: MySQL"
         env:
           DB_DSN: "mysql:host=mysql;dbname=atk4_test"
           DB_USER: atk4_test
@@ -172,8 +170,7 @@ jobs:
           php -d opcache.enable_cli=1 vendor/bin/phpunit --exclude-group none $(if [ -n "$LOG_COVERAGE" ]; then echo --coverage-text; else echo --no-coverage; fi) -v
           if [ -n "$LOG_COVERAGE" ]; then mv coverage/phpunit.cov coverage/phpunit-mysql.cov; fi
 
-      - name: "Run tests: MariaDB (only for Phpunit)"
-        if: startsWith(matrix.type, 'Phpunit')
+      - name: "Run tests: MariaDB"
         env:
           DB_DSN: "mysql:host=mariadb;dbname=atk4_test"
           DB_USER: atk4_test
@@ -182,8 +179,7 @@ jobs:
           php -d opcache.enable_cli=1 vendor/bin/phpunit --exclude-group none $(if [ -n "$LOG_COVERAGE" ]; then echo --coverage-text; else echo --no-coverage; fi) -v
           if [ -n "$LOG_COVERAGE" ]; then mv coverage/phpunit.cov coverage/phpunit-mariadb.cov; fi
 
-      - name: "Run tests: PostgreSQL (only for Phpunit)"
-        if: startsWith(matrix.type, 'Phpunit')
+      - name: "Run tests: PostgreSQL"
         env:
           DB_DSN: "pgsql:host=postgres;dbname=atk4_test"
           DB_USER: atk4_test
@@ -192,8 +188,7 @@ jobs:
           php -d opcache.enable_cli=1 vendor/bin/phpunit --exclude-group none $(if [ -n "$LOG_COVERAGE" ]; then echo --coverage-text; else echo --no-coverage; fi) -v
           if [ -n "$LOG_COVERAGE" ]; then mv coverage/phpunit.cov coverage/phpunit-postgres.cov; fi
 
-      - name: "Run tests: MSSQL (only for Phpunit)"
-        if: startsWith(matrix.type, 'Phpunit')
+      - name: "Run tests: MSSQL"
         env:
           DB_DSN: "sqlsrv:Server=mssql;Database=master"
           DB_USER: sa
@@ -202,8 +197,7 @@ jobs:
           php -d opcache.enable_cli=1 vendor/bin/phpunit --exclude-group none $(if [ -n "$LOG_COVERAGE" ]; then echo --coverage-text; else echo --no-coverage; fi) -v
           if [ -n "$LOG_COVERAGE" ]; then mv coverage/phpunit.cov coverage/phpunit-mssql.cov; fi
 
-      - name: "Run tests: Oracle (only for Phpunit)"
-        if: startsWith(matrix.type, 'Phpunit')
+      - name: "Run tests: Oracle"
         env:
           DB_DSN: "oci:dbname=oracle/xe;charset=UTF8"
           DB_USER: system

phpstan.neon.dist

@@ -34,7 +34,7 @@ parameters:
             message: '~^(Call to an undefined method Doctrine\\DBAL\\Driver\\Connection::getWrappedConnection\(\)\.|Caught class Doctrine\\DBAL\\DBALException not found\.|Call to static method notSupported\(\) on an unknown class Doctrine\\DBAL\\DBALException\.|Access to an undefined property Doctrine\\DBAL\\Driver\\PDO\\Connection::\$connection\.|Method Atk4\\Data\\Persistence\\Sql\\Expression::execute\(\) should return Doctrine\\DBAL\\Result\|PDOStatement but returns bool\.|Class Doctrine\\DBAL\\Platforms\\MySqlPlatform referenced with incorrect case: Doctrine\\DBAL\\Platforms\\MySQLPlatform\.)$~'
             path: '*'
             # count for DBAL 3.x matched in "src/Persistence/GenericPlatform.php" file
-            count: 11
+            count: 10
 
         # TODO these rules are generated, this ignores should be fixed in the code
         # for src/Schema/TestCase.php
@@ -52,7 +52,6 @@ parameters:
         # for src/Model/ReferencesTrait.php (in context of class Atk4\Data\Model)
         - '~^Call to an undefined method Atk4\\Data\\Reference::refLink\(\)\.$~'
         # for src/Persistence/Sql.php
-        - '~^Call to an undefined method Atk4\\Data\\Persistence\\Sql\\Query::sequence\(\)\.$~'
         - '~^Call to an undefined method Atk4\\Data\\Persistence::expr\(\)\.$~'
         - '~^Call to an undefined method Atk4\\Data\\Persistence::exprNow\(\)\.$~'
         # for src/Persistence/Sql/Join.php

src/Model.php

@@ -147,13 +147,6 @@ class Model implements \IteratorAggregate
      */
     public $table_alias;
 
-    /**
-     * Sequence name. Some DB engines use sequence for generating auto_increment IDs.
-     *
-     * @var string
-     */
-    public $sequence;
-
     /**
      * Persistence driver inherited from Atk4\Data\Persistence.
      *

src/Persistence.php

@@ -90,6 +90,9 @@ public function add(Model $m, array $defaults = []): Model
         $m->persistence = $this;
         $m->persistence_data = [];
         $this->initPersistence($m);
+
+        // invokes Model::init()
+        // model is not added to elements as it does not implement TrackableTrait trait
         $m = $this->_add($m);
 
         $this->hook(self::HOOK_AFTER_ADD, [$m]);

src/Persistence/Array_/Action.php

@@ -51,7 +51,7 @@ public function filter(Model\Scope\AbstractScope $condition)
     }
 
     /**
-     * Calculates SUM|AVG|MIN|MAX aggragate values for $field.
+     * Calculates SUM|AVG|MIN|MAX aggregate values for $field.
      *
      * @param string $fx
      * @param string $field

src/Persistence/GenericPlatform.php

@@ -20,7 +20,6 @@ private function createNotSupportedException(): \Exception // DbalException once
                 \Doctrine\DBAL\DBALException::notSupported('SQL');
                 \Doctrine\DBAL\DBALException::notSupported('SQL');
                 \Doctrine\DBAL\DBALException::notSupported('SQL');
-                \Doctrine\DBAL\DBALException::notSupported('SQL');
             }
 
             return \Doctrine\DBAL\DBALException::notSupported('SQL');

src/Persistence/Sql.php

@@ -13,8 +13,9 @@
 use Atk4\Data\Persistence\Sql\Exception as DsqlException;
 use Atk4\Data\Persistence\Sql\Expression;
 use Atk4\Data\Persistence\Sql\Query;
-use Doctrine\DBAL\Platforms;
+use Doctrine\DBAL\Platforms\AbstractPlatform;
 use Doctrine\DBAL\Platforms\OraclePlatform;
+use Doctrine\DBAL\Platforms\PostgreSQL94Platform;
 use Doctrine\DBAL\Platforms\SQLServer2012Platform;
 
 class Sql extends Persistence
@@ -134,7 +135,7 @@ public function atomic(\Closure $fx)
         return $this->connection->atomic($fx);
     }
 
-    public function getDatabasePlatform(): Platforms\AbstractPlatform
+    public function getDatabasePlatform(): AbstractPlatform
     {
         return $this->connection->getDatabasePlatform();
     }
@@ -170,11 +171,6 @@ public function add(Model $model, array $defaults = []): Model
             //$m->getField($m->id_field)->type = 'integer';
         }
 
-        // Sequence support
-        if ($model->sequence && $model->hasField($model->id_field)) {
-            $model->getField($model->id_field)->default = $this->dsql()->mode('seq_nextval')->sequence($model->sequence);
-        }
-
         return $model;
     }
 
@@ -565,8 +561,6 @@ public function insert(Model $model, array $data): string
 
         if ($model->id_field && !isset($data[$model->id_field])) {
             unset($data[$model->id_field]);
-
-            $this->syncIdSequence($model);
         }
 
         $insert->set($this->typecastSaveRow($model, $data));
@@ -585,8 +579,6 @@ public function insert(Model $model, array $data): string
 
         if ($model->id_field && isset($data[$model->id_field])) {
             $id = (string) $data[$model->id_field];
-
-            $this->syncIdSequence($model);
         } else {
             $id = $this->lastInsertId($model);
         }
@@ -732,47 +724,20 @@ public function getFieldSqlExpression(Field $field, Expression $expression): Exp
         return $expression->expr($mask, $prop);
     }
 
-    private function getIdSequenceName(Model $model): ?string
-    {
-        $sequenceName = $model->sequence ?: null;
-
-        if ($sequenceName === null) {
-            // PostgreSQL uses sequence internally for PK autoincrement,
-            // use default name if not set explicitly
-            if ($this->connection instanceof \Atk4\Data\Persistence\Sql\Postgresql\Connection) {
-                $sequenceName = $model->table . '_' . $model->getField($model->id_field)->getPersistenceName() . '_seq';
-            }
-        }
-
-        return $sequenceName;
-    }
-
     public function lastInsertId(Model $model): string
     {
-        // TODO: Oracle does not support lastInsertId(), only for testing
-        // as this does not support concurrent inserts
-        if ($this->connection instanceof \Atk4\Data\Persistence\Sql\Oracle\Connection) {
-            if (!$model->id_field) {
-                return ''; // TODO code should never call lastInsertId() if id field is not defined
-            }
-
-            $query = $this->connection->dsql()->table($model->table);
-            $query->field($query->expr('max({id_col})', ['id_col' => $model->getField($model->id_field)->getPersistenceName()]), 'max_id');
-
-            return $query->getOne();
-        }
-
-        return $this->connection->lastInsertId($this->getIdSequenceName($model));
-    }
-
-    protected function syncIdSequence(Model $model): void
-    {
-        // PostgreSQL sequence must be manually synchronized if a row with explicit ID was inserted
-        if ($this->connection instanceof \Atk4\Data\Persistence\Sql\Postgresql\Connection) {
-            $this->connection->expr(
-                'select setval([], coalesce(max({}), 0) + 1, false) from {}',
-                [$this->getIdSequenceName($model), $model->getField($model->id_field)->getPersistenceName(), $model->table]
-            )->execute();
-        }
+        // PostgreSQL and Oracle DBAL platforms use sequence internally for PK autoincrement,
+        // use default name if not set explicitly
+        $sequenceName = null;
+        if ($this->connection->getDatabasePlatform() instanceof PostgreSQL94Platform) {
+            $sequenceName = $this->connection->getDatabasePlatform()->getIdentitySequenceName(
+                $model->table,
+                $model->getField($model->id_field)->getPersistenceName()
+            );
+        } elseif ($this->connection->getDatabasePlatform() instanceof OraclePlatform) {
+            $sequenceName = $model->table . '_SEQ';
+        }
+
+        return $this->connection->lastInsertId($sequenceName);
     }
 }