Fix table /w schema support for all DBs (#1024)

mvorisek · web-flow · commit 704a82736f3b · 2022-07-12T01:54:16.000+02:00
diff --git a/phpstan.neon.dist b/phpstan.neon.dist
@@ -32,9 +32,9 @@ parameters:
 
         # fix https://github.com/phpstan/phpstan-deprecation-rules/issues/52 and https://github.com/phpstan/phpstan/issues/6444
         -
-            message: '~^Call to method (getVarcharTypeDeclarationSQL|getClobTypeDeclarationSQL|getCreateTableSQL)\(\) of deprecated class Doctrine\\DBAL\\Platforms\\(SQLServerPlatform|AbstractPlatform):\nUse.+instead\.$~'
+            message: '~^Call to method (getVarcharTypeDeclarationSQL|getClobTypeDeclarationSQL|getCreateTableSQL|getCurrentDatabaseExpression)\(\) of deprecated class Doctrine\\DBAL\\Platforms\\(PostgreSQLPlatform|SQLServerPlatform|AbstractPlatform):\nUse.+instead\.$~'
             path: '*'
-            count: 3
+            count: 5
 
         # TODO these rules are generated, this ignores should be fixed in the code
         # for src/Schema/TestCase.php
diff --git a/src/Model/Join.php b/src/Model/Join.php
@@ -240,7 +240,7 @@ protected function init(): void
             }
 
             if (!$this->foreign_field) {
-                $this->foreign_field = preg_replace('~^.+?\.~', '', $this->getModelTableString($this->getOwner())) . '_' . $id_field;
+                $this->foreign_field = preg_replace('~^.+\.~s', '', $this->getModelTableString($this->getOwner())) . '_' . $id_field;
             }
         } else {
             $this->reverse = false;
diff --git a/src/Persistence/Sql/Mssql/PlatformTrait.php b/src/Persistence/Sql/Mssql/PlatformTrait.php
@@ -31,6 +31,15 @@ public function getClobTypeDeclarationSQL(array $column)
 //        $this->markDoctrineTypeCommented('text');
 //    }
 
+    public function getCurrentDatabaseExpression(bool $includeSchema = false): string
+    {
+        if ($includeSchema) {
+            return 'CONCAT(DB_NAME(), \'.\', SCHEMA_NAME())';
+        }
+
+        return parent::getCurrentDatabaseExpression();
+    }
+
     // SQL Server DBAL platform has buggy identifier escaping, fix until fixed officially, see:
     // https://github.com/doctrine/dbal/pull/4360
 
diff --git a/src/Persistence/Sql/Oracle/Connection.php b/src/Persistence/Sql/Oracle/Connection.php
@@ -37,7 +37,7 @@ protected static function createDbalEventManager(): EventManager
     public function lastInsertId(string $sequence = null): string
     {
         if ($sequence) {
-            return $this->dsql()->field($this->expr('{}.CURRVAL', [$sequence]))->getOne();
+            return $this->dsql()->field($this->expr('{{}}.CURRVAL', [$sequence]))->getOne();
         }
 
         return parent::lastInsertId($sequence);
diff --git a/src/Persistence/Sql/Oracle/PlatformTrait.php b/src/Persistence/Sql/Oracle/PlatformTrait.php
@@ -56,6 +56,10 @@ public function getCreateAutoincrementSql($name, $table, $start = 1)
     {
         $sqls = parent::getCreateAutoincrementSql($name, $table, $start);
 
+        // fix table name when name /w schema is used
+        // TODO submit a PR with fixed OraclePlatform to DBAL
+        $sqls[0] = preg_replace('~(?<=WHERE TABLE_NAME = \').+\.(?=.+?\')~', '', $sqls[0]);
+
         // replace trigger from https://github.com/doctrine/dbal/blob/3.1.3/src/Platforms/OraclePlatform.php#L526-L546
         $tableIdentifier = \Closure::bind(fn () => $this->normalizeIdentifier($table), $this, OraclePlatform::class)();
         $nameIdentifier = \Closure::bind(fn () => $this->normalizeIdentifier($name), $this, OraclePlatform::class)();
@@ -68,20 +72,20 @@ public function getCreateAutoincrementSql($name, $table, $start = 1)
         $sqls[count($sqls) - 1] = $conn->expr(
             // else branch should be maybe (because of concurrency) put into after update trigger
             str_replace('[pk_seq]', '\'' . str_replace('\'', '\'\'', $pkSeq) . '\'', <<<'EOT'
-                CREATE TRIGGER {trigger}
+                CREATE TRIGGER {{trigger}}
                     BEFORE INSERT OR UPDATE
-                    ON {table}
+                    ON {{table}}
                     FOR EACH ROW
                 DECLARE
-                    atk4__pk_seq_last__ {table}.{pk}%TYPE;
+                    atk4__pk_seq_last__ {{table}}.{pk}%TYPE;
                 BEGIN
                     IF (:NEW.{pk} IS NULL) THEN
-                        SELECT {pk_seq}.NEXTVAL INTO :NEW.{pk} FROM DUAL;
+                        SELECT {{pk_seq}}.NEXTVAL INTO :NEW.{pk} FROM DUAL;
                     ELSE
                         SELECT LAST_NUMBER INTO atk4__pk_seq_last__ FROM USER_SEQUENCES WHERE SEQUENCE_NAME = [pk_seq];
                         WHILE atk4__pk_seq_last__ <= :NEW.{pk}
                         LOOP
-                            SELECT {pk_seq}.NEXTVAL + 1 INTO atk4__pk_seq_last__ FROM DUAL;
+                            SELECT {{pk_seq}}.NEXTVAL + 1 INTO atk4__pk_seq_last__ FROM DUAL;
                         END LOOP;
                     END IF;
                 END;
@@ -96,4 +100,21 @@ public function getCreateAutoincrementSql($name, $table, $start = 1)
 
         return $sqls;
     }
+
+    public function getListDatabasesSQL(): string
+    {
+        // ignore Oracle maintained schemas, improve tests performance
+        return 'SELECT username FROM sys.all_users'
+            . ' WHERE oracle_maintained = \'N\'';
+    }
+
+    public function getListTablesSQL(): string
+    {
+        // ignore Oracle maintained tables, improve tests performance
+        // self::getListViewsSQL() does not need filtering, as there is no Oracle VIEW by default
+        return 'SELECT * FROM sys.user_tables'
+            . ' LEFT JOIN sys.user_objects ON user_objects.object_type = \'TABLE\''
+            . ' AND user_objects.object_name = user_tables.table_name'
+            . ' WHERE oracle_maintained = \'N\'';
+    }
 }
diff --git a/src/Persistence/Sql/Postgresql/PlatformTrait.php b/src/Persistence/Sql/Postgresql/PlatformTrait.php
@@ -41,6 +41,15 @@ public function getClobTypeDeclarationSQL(array $column)
         return 'CITEXT';
     }
 
+    public function getCurrentDatabaseExpression(bool $includeSchema = false): string
+    {
+        if ($includeSchema) {
+            return 'CONCAT(CURRENT_DATABASE(), \'.\', CURRENT_SCHEMA())';
+        }
+
+        return parent::getCurrentDatabaseExpression();
+    }
+
     // PostgreSQL DBAL platform uses SERIAL column type for autoincrement which does not increment
     // when a row with a not-null PK is inserted like Sqlite or MySQL does, unify the behaviour
 
@@ -65,15 +74,15 @@ protected function getCreateAutoincrementSql(Table $table, Column $pkColumn): ar
             // else branch should be maybe (because of concurrency) put into after update trigger
             // with pure nextval instead of setval with a loop like in Oracle trigger
             str_replace('[pk_seq]', '\'' . $pkSeqName . '\'', <<<'EOF'
-                CREATE OR REPLACE FUNCTION {trigger_func}()
+                CREATE OR REPLACE FUNCTION {{trigger_func}}()
                 RETURNS trigger AS $$
                 DECLARE
                     atk4__pk_seq_last__ {table}.{pk}%TYPE;
                 BEGIN
                     IF (NEW.{pk} IS NULL) THEN
                         NEW.{pk} := nextval([pk_seq]);
                     ELSE
-                        SELECT COALESCE(last_value, 0) INTO atk4__pk_seq_last__ FROM {pk_seq};
+                        SELECT COALESCE(last_value, 0) INTO atk4__pk_seq_last__ FROM {{pk_seq}};
                         IF (atk4__pk_seq_last__ <= NEW.{pk}) THEN
                             atk4__pk_seq_last__  := setval([pk_seq], NEW.{pk}, true);
                         END IF;
@@ -83,24 +92,24 @@ protected function getCreateAutoincrementSql(Table $table, Column $pkColumn): ar
                 $$ LANGUAGE plpgsql
                 EOF),
             [
-                'table' => $table->getName(),
+                'table' => $table->getShortestName($table->getNamespaceName()), // TODO should be probably name /w schema, but it is not supported, get variable type differently
                 'pk' => $pkColumn->getName(),
                 'pk_seq' => $pkSeqName,
-                'trigger_func' => $table->getName() . '_AI_FUNC', // TODO create only one function per schema
+                'trigger_func' => $table->getName() . '_AI_FUNC',
             ]
         )->render()[0];
 
         $sqls[] = $conn->expr(
             <<<'EOF'
                 CREATE TRIGGER {trigger}
                     BEFORE INSERT OR UPDATE
-                    ON {table}
+                    ON {{table}}
                     FOR EACH ROW
-                EXECUTE PROCEDURE {trigger_func}()
+                EXECUTE PROCEDURE {{trigger_func}}()
                 EOF,
             [
                 'table' => $table->getName(),
-                'trigger' => $table->getName() . '_AI_PK',
+                'trigger' => $table->getShortestName($table->getNamespaceName()) . '_AI_PK',
                 'trigger_func' => $table->getName() . '_AI_FUNC',
             ]
         )->render()[0];
diff --git a/src/Persistence/Sql/Query.php b/src/Persistence/Sql/Query.php
@@ -370,7 +370,7 @@ public function join(
         }
 
         // Split and deduce fields
-        // NOTE that this will not allow table names with dots in there !!!
+        // TODO this will not allow table names with dots in there !!!
         [$f1, $f2] = array_pad(explode('.', $foreign_table, 2), 2, null);
 
         if (is_object($master_field)) {
diff --git a/src/Reference/HasMany.php b/src/Reference/HasMany.php
@@ -29,7 +29,7 @@ public function getTheirFieldName(Model $theirModel = null): string
         // this is pure guess, verify if such field exist, otherwise throw
         // TODO probably remove completely in the future
         $ourModel = $this->getOurModel(null);
-        $theirFieldName = preg_replace('~^.+?\.~', '', $this->getModelTableString($ourModel)) . '_' . $ourModel->id_field;
+        $theirFieldName = preg_replace('~^.+\.~s', '', $this->getModelTableString($ourModel)) . '_' . $ourModel->id_field;
         if (!$this->createTheirModel()->hasField($theirFieldName)) {
             throw (new Exception('Their model does not contain fallback field'))
                 ->addMoreInfo('their_fallback_field', $theirFieldName);
diff --git a/src/Schema/Migrator.php b/src/Schema/Migrator.php
@@ -11,11 +11,14 @@
 use Atk4\Data\Persistence;
 use Atk4\Data\Persistence\Sql\Connection;
 use Atk4\Data\Reference\HasOne;
-use Doctrine\DBAL\Exception as DbalException;
+use Doctrine\DBAL\Driver\Exception as DbalDriverException;
+use Doctrine\DBAL\Exception\DatabaseObjectNotFoundException;
+use Doctrine\DBAL\Exception\TableNotFoundException;
 use Doctrine\DBAL\Platforms\AbstractPlatform;
 use Doctrine\DBAL\Platforms\MySQLPlatform;
 use Doctrine\DBAL\Platforms\OraclePlatform;
 use Doctrine\DBAL\Platforms\SqlitePlatform;
+use Doctrine\DBAL\Platforms\SQLServerPlatform;
 use Doctrine\DBAL\Schema\AbstractSchemaManager;
 use Doctrine\DBAL\Schema\Table;
 
@@ -79,11 +82,28 @@ protected function createSchemaManager(): AbstractSchemaManager
 
     public function table(string $tableName): self
     {
-        $this->table = new Table($this->getDatabasePlatform()->quoteIdentifier($tableName));
+        $table = new Table('0.0');
         if ($this->getDatabasePlatform() instanceof MySQLPlatform) {
-            $this->table->addOption('charset', 'utf8mb4');
+            $table->addOption('charset', 'utf8mb4');
         }
 
+        // fix namespaced table name split for MSSQL/PostgreSQL
+        // https://github.com/doctrine/dbal/blob/3.3.7/src/Schema/AbstractAsset.php#L55
+        // https://github.com/doctrine/dbal/pull/5494
+        \Closure::bind(function () use ($table, $tableName) {
+            $table->_quoted = true;
+            $lastDotPos = strrpos($tableName, '.');
+            if ($lastDotPos !== false) {
+                $table->_namespace = substr($tableName, 0, $lastDotPos);
+                $table->_name = substr($tableName, $lastDotPos + 1);
+            } else {
+                $table->_namespace = null;
+                $table->_name = $tableName;
+            }
+        }, null, Table::class)();
+
+        $this->table = $table;
+
         return $this;
     }
 
@@ -105,8 +125,19 @@ public function create(): self
 
     public function drop(): self
     {
-        $this->createSchemaManager()
-            ->dropTable($this->getDatabasePlatform()->quoteIdentifier($this->table->getName()));
+        try {
+            $this->createSchemaManager()
+                ->dropTable($this->table->getQuotedName($this->getDatabasePlatform()));
+        } catch (DatabaseObjectNotFoundException $e) {
+            // fix exception not converted to TableNotFoundException for MSSQL
+            // https://github.com/doctrine/dbal/pull/5492
+            if ($this->getDatabasePlatform() instanceof SQLServerPlatform && $e->getPrevious() instanceof DbalDriverException
+                && preg_match('~[cC]annot drop the table \'.*\', because it does not exist or you do not have permission\.~', $e->getMessage())) {
+                throw new TableNotFoundException($e->getPrevious(), $e->getQuery());
+            }
+
+            throw $e;
+        }
 
         $this->createdTableNames = array_diff($this->createdTableNames, [$this->table->getName()]);
 
@@ -117,7 +148,7 @@ public function dropIfExists(): self
     {
         try {
             $this->drop();
-        } catch (DbalException $e) {
+        } catch (TableNotFoundException $e) {
         }
 
         $this->createdTableNames = array_diff($this->createdTableNames, [$this->table->getName()]);
@@ -126,10 +157,14 @@ public function dropIfExists(): self
         // but if AI trigger is not present, AI sequence is not dropped
         // https://github.com/doctrine/dbal/issues/4997
         if ($this->getDatabasePlatform() instanceof OraclePlatform) {
-            $dropTriggerSql = $this->getDatabasePlatform()->getDropAutoincrementSql($this->table->getName())[1];
+            $schemaManager = $this->createSchemaManager();
+            $dropTriggerSql = $this->getDatabasePlatform()
+                ->getDropAutoincrementSql($this->table->getQuotedName($this->getDatabasePlatform()))[1];
             try {
-                $this->getConnection()->expr($dropTriggerSql)->executeStatement();
-            } catch (Exception $e) {
+                \Closure::bind(function () use ($schemaManager, $dropTriggerSql) {
+                    $schemaManager->_execSql($dropTriggerSql);
+                }, null, AbstractSchemaManager::class)();
+            } catch (DatabaseObjectNotFoundException $e) {
             }
         }
 
diff --git a/src/Schema/TestCase.php b/src/Schema/TestCase.php
@@ -35,6 +35,11 @@ protected function setUp(): void
 
         $this->db = Persistence::connect($_ENV['DB_DSN'], $_ENV['DB_USER'], $_ENV['DB_PASSWORD']);
 
+        if ($this->db->getDatabasePlatform() instanceof SqlitePlatform) {
+            $this->db->getConnection()->expr(
+                'PRAGMA foreign_keys = 1'
+            )->executeStatement();
+        }
         if ($this->db->getDatabasePlatform() instanceof MySQLPlatform) {
             $this->db->getConnection()->expr(
                 'SET SESSION auto_increment_increment = 1, SESSION auto_increment_offset = 1'
@@ -103,7 +108,7 @@ private function convertSqlFromSqlite(string $sql): string
         $platform = $this->getDatabasePlatform();
 
         $convertedSql = preg_replace_callback(
-            '~\'(?:[^\'\\\\]+|\\\\.)*\'|"(?:[^"\\\\]+|\\\\.)*"|:(\w+)~s',
+            '~\'(?:[^\'\\\\]+|\\\\.)*+\'|"(?:[^"\\\\]+|\\\\.)*+"|:(\w+)~s',
             function ($matches) use ($platform) {
                 if (isset($matches[1])) {
                     return ':' . ($platform instanceof OraclePlatform ? 'xxaaa' : '') . $matches[1];
diff --git a/tests/RandomTest.php b/tests/RandomTest.php
@@ -12,10 +12,7 @@
 use Atk4\Data\Persistence\Sql\Expression;
 use Atk4\Data\Persistence\Sql\Query;
 use Atk4\Data\Schema\TestCase;
-use Doctrine\DBAL\Platforms\OraclePlatform;
-use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
 use Doctrine\DBAL\Platforms\SqlitePlatform;
-use Doctrine\DBAL\Platforms\SQLServerPlatform;
 
 class Model_Rate extends Model
 {
@@ -570,19 +567,11 @@ public function testTableWithSchema(): void
             $runWithDb = false;
         } else {
             $dbSchema = $this->db->getConnection()->dsql()
-                ->field(new Expression($this->getDatabasePlatform()->getCurrentDatabaseExpression()))
+                ->field(new Expression($this->getDatabasePlatform()->getCurrentDatabaseExpression(true))) // @phpstan-ignore-line
                 ->getOne();
             $userSchema = $dbSchema;
             $docSchema = $dbSchema;
             $runWithDb = true;
-
-            if ($this->getDatabasePlatform() instanceof PostgreSQLPlatform
-                || $this->getDatabasePlatform() instanceof SQLServerPlatform
-                || $this->getDatabasePlatform() instanceof OraclePlatform) {
-                $userSchema = 'functional_is_failing_db1';
-                $docSchema = 'functional_is_failing_db2';
-                $runWithDb = false;
-            }
         }
 
         $user = new Model($this->db, ['table' => $userSchema . '.user']);
@@ -600,8 +589,10 @@ public function testTableWithSchema(): void
         $this->assertSame($render, $selectAction->render());
         $this->assertSame($render, $doc->action('select')->render());
 
+        $userTableQuoted = '"' . str_replace('.', '"."', $userSchema) . '"."user"';
+        $docTableQuoted = '"' . str_replace('.', '"."', $docSchema) . '"."doc"';
         $this->assertSameSql(
-            'select "id", "name", "user_id", (select "name" from "' . $userSchema . '"."user" "_u_e8701ad48ba0" where "id" = "' . $docSchema . '"."doc"."user_id") "user" from "' . $docSchema . '"."doc" where (select "name" from "' . $userSchema . '"."user" "_u_e8701ad48ba0" where "id" = "' . $docSchema . '"."doc"."user_id") = :a',
+            'select "id", "name", "user_id", (select "name" from ' . $userTableQuoted . ' "_u_e8701ad48ba0" where "id" = ' . $docTableQuoted . '."user_id") "user" from ' . $docTableQuoted . ' where (select "name" from ' . $userTableQuoted . ' "_u_e8701ad48ba0" where "id" = ' . $docTableQuoted . '."user_id") = :a',
             $render[0]
         );
 
diff --git a/tests/Schema/BasicTest.php b/tests/Schema/BasicTest.php
diff --git a/tests/Schema/MigratorTest.php b/tests/Schema/MigratorTest.php

Original file line number	Diff line number	Diff line change
`@@ -240,7 +240,7 @@ protected function init(): void`
`240`	`240`	`}`
`241`	`241`
`242`	`242`	`if (!$this->foreign_field) {`
`243`		`- $this->foreign_field = preg_replace('~^.+?\.~', '', $this->getModelTableString($this->getOwner())) . '_' . $id_field;`
	`243`	`+ $this->foreign_field = preg_replace('~^.+\.~s', '', $this->getModelTableString($this->getOwner())) . '_' . $id_field;`
`244`	`244`	`}`
`245`	`245`	`} else {`
`246`	`246`	`$this->reverse = false;`
Original file line number	Diff line number	Diff line change
`@@ -37,7 +37,7 @@ protected static function createDbalEventManager(): EventManager`
`37`	`37`	`public function lastInsertId(string $sequence = null): string`
`38`	`38`	`{`
`39`	`39`	`if ($sequence) {`
`40`		`- return $this->dsql()->field($this->expr('{}.CURRVAL', [$sequence]))->getOne();`
	`40`	`+ return $this->dsql()->field($this->expr('{{}}.CURRVAL', [$sequence]))->getOne();`
`41`	`41`	`}`
`42`	`42`
`43`	`43`	`return parent::lastInsertId($sequence);`
Original file line number	Diff line number	Diff line change
`@@ -370,7 +370,7 @@ public function join(`
`370`	`370`	`}`
`371`	`371`
`372`	`372`	`// Split and deduce fields`
`373`		`- // NOTE that this will not allow table names with dots in there !!!`
	`373`	`+ // TODO this will not allow table names with dots in there !!!`
`374`	`374`	`[$f1, $f2] = array_pad(explode('.', $foreign_table, 2), 2, null);`
`375`	`375`
`376`	`376`	`if (is_object($master_field)) {`