From 58d0e8cbce08a32935a2e958569797dee60052af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Thu, 17 Aug 2023 00:57:19 +0200 Subject: [PATCH 1/6] Replace DBAL EventManager with configuration middleware (#1114) --- bootstrap-types.php | 5 ++ composer.json | 4 +- phpstan.neon.dist | 12 ++-- src/Persistence/Sql/Connection.php | 22 +++---- src/Persistence/Sql/DbalDriverMiddleware.php | 8 +-- src/Persistence/Sql/Expression.php | 6 +- src/Persistence/Sql/Oracle/Connection.php | 65 ++++++++++++++------ src/Persistence/Sql/Sqlite/Connection.php | 25 ++------ src/Schema/Migrator.php | 4 +- src/Schema/TestCase.php | 4 +- tests/ExpressionSqlTest.php | 12 ++-- tests/ModelAggregateTest.php | 8 +-- tests/Persistence/Sql/ConnectionTest.php | 6 +- tests/RandomTest.php | 4 +- tests/Schema/TestCaseTest.php | 4 +- tests/ScopeTest.php | 6 +- tests/TypecastingTest.php | 4 +- 17 files changed, 104 insertions(+), 95 deletions(-) diff --git a/bootstrap-types.php b/bootstrap-types.php index 18d9ecbc4..bc8a76a87 100644 --- a/bootstrap-types.php +++ b/bootstrap-types.php @@ -7,7 +7,12 @@ use Atk4\Data\Type\LocalObjectType; use Atk4\Data\Type\MoneyType; use Atk4\Data\Type\Types; +use Doctrine\DBAL\Platforms\SqlitePlatform; use Doctrine\DBAL\Types as DbalTypes; +// force Doctrine\DBAL\Platforms\SQLitePlatform class load as in DBAL 3.x it is named with a different case +// remove once DBAL 3.x support is dropped +new SqlitePlatform(); + DbalTypes\Type::addType(Types::LOCAL_OBJECT, LocalObjectType::class); DbalTypes\Type::addType(Types::MONEY, MoneyType::class); diff --git a/composer.json b/composer.json index 6141244f9..6feb9f9a5 100644 --- a/composer.json +++ b/composer.json @@ -35,13 +35,13 @@ "require": { "php": ">=7.4 <8.3", "atk4/core": "dev-develop", - "doctrine/dbal": "~3.4.5 || ~3.5.1 || ~3.6.0", + "doctrine/dbal": "~3.5.1 || ~3.6.0", "mvorisek/atk4-hintable": "~1.9.0" }, "require-release": { "php": ">=7.4 <8.3", "atk4/core": "~5.0.0", - "doctrine/dbal": "~3.4.5 || ~3.5.1 || ~3.6.0", + "doctrine/dbal": "~3.5.1 || ~3.6.0", "mvorisek/atk4-hintable": "~1.9.0" }, "require-dev": { diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 034dfc542..ee803158f 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -36,15 +36,11 @@ parameters: path: '*' count: 3 - # TODO for DBAL 4.0 upgrade + # FC for DBAL 4.0, remove once DBAL 3.x support is dropped - - message: '~^Instantiation of deprecated class Doctrine\\DBAL\\Event\\Listeners\\OracleSessionInit:\nUse \{@see \\Doctrine\\DBAL\\Driver\\OCI8\\Middleware\\InitializeSession\} instead\.$~' - path: 'src/Persistence/Sql/Oracle/Connection.php' - count: 1 - - - message: '~^(Fetching deprecated class constant postConnect of class Doctrine\\DBAL\\Events\.|Fetching class constant postConnect of deprecated class Doctrine\\DBAL\\Events\.|Parameter \$args of method postConnect\(\) in anonymous class has typehint with deprecated class Doctrine\\DBAL\\Event\\ConnectionEventArgs\.)$~' - path: 'src/Persistence/Sql/Sqlite/Connection.php' - count: 3 + message: '~^Class Doctrine\\DBAL\\Platforms\\SqlitePlatform referenced with incorrect case: Doctrine\\DBAL\\Platforms\\SQLitePlatform\.$~' + path: '*' + count: 33 # TODO these rules are generated, this ignores should be fixed in the code # for src/Schema/TestCase.php diff --git a/src/Persistence/Sql/Connection.php b/src/Persistence/Sql/Connection.php index a9cabbf4d..c961a2e67 100644 --- a/src/Persistence/Sql/Connection.php +++ b/src/Persistence/Sql/Connection.php @@ -5,8 +5,7 @@ namespace Atk4\Data\Persistence\Sql; use Atk4\Core\DiContainerTrait; -use Doctrine\Common\EventManager; -use Doctrine\DBAL\Configuration as DbalConfiguration; +use Doctrine\DBAL\Configuration; use Doctrine\DBAL\Connection as DbalConnection; use Doctrine\DBAL\ConnectionException as DbalConnectionException; use Doctrine\DBAL\Driver as DbalDriver; @@ -226,10 +225,10 @@ private static function getDriverNameFromDbalDriverConnection(DbalDriverConnecti return null; // @phpstan-ignore-line } - protected static function createDbalConfiguration(): DbalConfiguration + protected static function createDbalConfiguration(): Configuration { - $dbalConfiguration = new DbalConfiguration(); - $dbalConfiguration->setMiddlewares([ + $configuration = new Configuration(); + $configuration->setMiddlewares([ new class() implements DbalMiddleware { public function wrap(DbalDriver $driver): DbalDriver { @@ -238,12 +237,7 @@ public function wrap(DbalDriver $driver): DbalDriver }, ]); - return $dbalConfiguration; - } - - protected static function createDbalEventManager(): EventManager - { - return new EventManager(); + return $configuration; } /** @@ -260,8 +254,7 @@ protected static function connectFromDsn(array $dsn): DbalDriverConnection $dbalConnection = DriverManager::getConnection( $dsn, // @phpstan-ignore-line - (static::class)::createDbalConfiguration(), - (static::class)::createDbalEventManager() + (static::class)::createDbalConfiguration() ); return $dbalConnection->getWrappedConnection(); // @phpstan-ignore-line https://github.com/doctrine/dbal/issues/5199 @@ -271,8 +264,7 @@ protected static function connectFromDbalDriverConnection(DbalDriverConnection $ { $dbalConnection = DriverManager::getConnection( ['driver' => self::getDriverNameFromDbalDriverConnection($dbalDriverConnection)], - (static::class)::createDbalConfiguration(), - (static::class)::createDbalEventManager() + (static::class)::createDbalConfiguration() ); \Closure::bind(function () use ($dbalConnection, $dbalDriverConnection): void { $dbalConnection->_conn = $dbalDriverConnection; diff --git a/src/Persistence/Sql/DbalDriverMiddleware.php b/src/Persistence/Sql/DbalDriverMiddleware.php index 04366c0d2..7f5af1b8c 100644 --- a/src/Persistence/Sql/DbalDriverMiddleware.php +++ b/src/Persistence/Sql/DbalDriverMiddleware.php @@ -17,7 +17,7 @@ use Doctrine\DBAL\Platforms\AbstractPlatform; use Doctrine\DBAL\Platforms\OraclePlatform; use Doctrine\DBAL\Platforms\PostgreSQLPlatform; -use Doctrine\DBAL\Platforms\SqlitePlatform; +use Doctrine\DBAL\Platforms\SQLitePlatform; use Doctrine\DBAL\Platforms\SQLServerPlatform; use Doctrine\DBAL\Query as DbalQuery; use Doctrine\DBAL\Schema\AbstractSchemaManager; @@ -28,8 +28,8 @@ class DbalDriverMiddleware extends AbstractDriverMiddleware { protected function replaceDatabasePlatform(AbstractPlatform $platform): AbstractPlatform { - if ($platform instanceof SqlitePlatform) { - $platform = new class() extends SqlitePlatform { + if ($platform instanceof SQLitePlatform) { + $platform = new class() extends SQLitePlatform { use Sqlite\PlatformTrait; }; } elseif ($platform instanceof PostgreSQLPlatform) { @@ -64,7 +64,7 @@ public function createDatabasePlatformForVersion($version): AbstractPlatform */ public function getSchemaManager(DbalConnection $connection, AbstractPlatform $platform): AbstractSchemaManager { - if ($platform instanceof SqlitePlatform) { + if ($platform instanceof SQLitePlatform) { return new class($connection, $platform) extends SqliteSchemaManager { // @phpstan-ignore-line use Sqlite\SchemaManagerTrait; }; diff --git a/src/Persistence/Sql/Expression.php b/src/Persistence/Sql/Expression.php index 2fb194e1d..e3ab46e87 100644 --- a/src/Persistence/Sql/Expression.php +++ b/src/Persistence/Sql/Expression.php @@ -11,7 +11,7 @@ use Doctrine\DBAL\ParameterType; use Doctrine\DBAL\Platforms\OraclePlatform; use Doctrine\DBAL\Platforms\PostgreSQLPlatform; -use Doctrine\DBAL\Platforms\SqlitePlatform; +use Doctrine\DBAL\Platforms\SQLitePlatform; use Doctrine\DBAL\Platforms\SQLServerPlatform; use Doctrine\DBAL\Result as DbalResult; @@ -287,9 +287,9 @@ protected function escapeStringLiteral(string $value): string $sqlLeft = 'CAST(' . $sqlLeft . ' AS NVARCHAR(MAX))'; } - return ($platform instanceof SqlitePlatform ? '(' : 'CONCAT(') + return ($platform instanceof SQLitePlatform ? '(' : 'CONCAT(') . $sqlLeft - . ($platform instanceof SqlitePlatform ? ' || ' : ', ') + . ($platform instanceof SQLitePlatform ? ' || ' : ', ') . $buildConcatSqlFx($partsRight) . ')'; } diff --git a/src/Persistence/Sql/Oracle/Connection.php b/src/Persistence/Sql/Oracle/Connection.php index a2ef9b611..f26ebc5a7 100644 --- a/src/Persistence/Sql/Oracle/Connection.php +++ b/src/Persistence/Sql/Oracle/Connection.php @@ -5,34 +5,63 @@ namespace Atk4\Data\Persistence\Sql\Oracle; use Atk4\Data\Persistence\Sql\Connection as BaseConnection; -use Doctrine\Common\EventManager; -use Doctrine\DBAL\Event\Listeners\OracleSessionInit; +use Doctrine\DBAL\Configuration; +use Doctrine\DBAL\Driver; +use Doctrine\DBAL\Driver\Connection as DriverConnection; +use Doctrine\DBAL\Driver\Middleware; +use Doctrine\DBAL\Driver\Middleware\AbstractDriverMiddleware; class Connection extends BaseConnection { protected string $expressionClass = Expression::class; protected string $queryClass = Query::class; - protected static function createDbalEventManager(): EventManager + protected static function createDbalConfiguration(): Configuration { - $evm = parent::createDbalEventManager(); + $configuration = parent::createDbalConfiguration(); // setup connection globalization to use standard datetime format incl. microseconds support // and make comparison of character types case insensitive - $dateFormat = 'YYYY-MM-DD'; - $timeFormat = 'HH24:MI:SS.FF6'; - $tzFormat = 'TZH:TZM'; - $evm->addEventSubscriber(new OracleSessionInit([ - 'NLS_DATE_FORMAT' => $dateFormat, - 'NLS_TIME_FORMAT' => $timeFormat, - 'NLS_TIMESTAMP_FORMAT' => $dateFormat . ' ' . $timeFormat, - 'NLS_TIME_TZ_FORMAT' => $timeFormat . ' ' . $tzFormat, - 'NLS_TIMESTAMP_TZ_FORMAT' => $dateFormat . ' ' . $timeFormat . ' ' . $tzFormat, - 'NLS_COMP' => 'LINGUISTIC', - 'NLS_SORT' => 'BINARY_CI', - ])); - - return $evm; + // based on https://github.com/doctrine/dbal/blob/3.6.5/src/Driver/OCI8/Middleware/InitializeSession.php + $initializeSessionMiddleware = new class() implements Middleware { + public function wrap(Driver $driver): Driver + { + return new class($driver) extends AbstractDriverMiddleware { + public function connect( + #[\SensitiveParameter] + array $params + ): DriverConnection { + $connection = parent::connect($params); + + $dateFormat = 'YYYY-MM-DD'; + $timeFormat = 'HH24:MI:SS.FF6'; + $tzFormat = 'TZH:TZM'; + + $vars = []; + foreach ([ + 'NLS_DATE_FORMAT' => $dateFormat, + 'NLS_TIME_FORMAT' => $timeFormat, + 'NLS_TIMESTAMP_FORMAT' => $dateFormat . ' ' . $timeFormat, + 'NLS_TIME_TZ_FORMAT' => $timeFormat . ' ' . $tzFormat, + 'NLS_TIMESTAMP_TZ_FORMAT' => $dateFormat . ' ' . $timeFormat . ' ' . $tzFormat, + 'NLS_NUMERIC_CHARACTERS' => '.,', + 'NLS_COMP' => 'LINGUISTIC', + 'NLS_SORT' => 'BINARY_CI', + ] as $k => $v) { + $vars[] = $k . " = '" . $v . "'"; + } + + $connection->exec('ALTER SESSION SET ' . implode(' ', $vars)); + + return $connection; + } + }; + } + }; + + $configuration->setMiddlewares([...$configuration->getMiddlewares(), $initializeSessionMiddleware]); + + return $configuration; } public function lastInsertId(string $sequence = null): string diff --git a/src/Persistence/Sql/Sqlite/Connection.php b/src/Persistence/Sql/Sqlite/Connection.php index 129753d83..7a4332a41 100644 --- a/src/Persistence/Sql/Sqlite/Connection.php +++ b/src/Persistence/Sql/Sqlite/Connection.php @@ -5,33 +5,20 @@ namespace Atk4\Data\Persistence\Sql\Sqlite; use Atk4\Data\Persistence\Sql\Connection as BaseConnection; -use Doctrine\Common\EventManager; -use Doctrine\Common\EventSubscriber; -use Doctrine\DBAL\Event\ConnectionEventArgs; -use Doctrine\DBAL\Events; +use Doctrine\DBAL\Configuration; +use Doctrine\DBAL\Driver\AbstractSQLiteDriver\Middleware\EnableForeignKeys; class Connection extends BaseConnection { protected string $expressionClass = Expression::class; protected string $queryClass = Query::class; - protected static function createDbalEventManager(): EventManager + protected static function createDbalConfiguration(): Configuration { - $evm = parent::createDbalEventManager(); + $configuration = parent::createDbalConfiguration(); - // setup connection to always check foreign keys - $evm->addEventSubscriber(new class() implements EventSubscriber { - public function getSubscribedEvents(): array - { - return [Events::postConnect]; - } + $configuration->setMiddlewares([...$configuration->getMiddlewares(), new EnableForeignKeys()]); - public function postConnect(ConnectionEventArgs $args): void - { - $args->getConnection()->executeStatement('PRAGMA foreign_keys = 1'); - } - }); - - return $evm; + return $configuration; } } diff --git a/src/Schema/Migrator.php b/src/Schema/Migrator.php index fbb4a3b70..d4f20ad00 100644 --- a/src/Schema/Migrator.php +++ b/src/Schema/Migrator.php @@ -20,7 +20,7 @@ use Doctrine\DBAL\Platforms\MySQLPlatform; use Doctrine\DBAL\Platforms\OraclePlatform; use Doctrine\DBAL\Platforms\PostgreSQLPlatform; -use Doctrine\DBAL\Platforms\SqlitePlatform; +use Doctrine\DBAL\Platforms\SQLitePlatform; use Doctrine\DBAL\Platforms\SQLServerPlatform; use Doctrine\DBAL\Schema\AbstractAsset; use Doctrine\DBAL\Schema\AbstractSchemaManager; @@ -253,7 +253,7 @@ public function field(string $fieldName, array $options = []): self } if (in_array($type, ['string', 'text'], true)) { - if ($this->getDatabasePlatform() instanceof SqlitePlatform) { + if ($this->getDatabasePlatform() instanceof SQLitePlatform) { $column->setPlatformOption('collation', 'NOCASE'); } } diff --git a/src/Schema/TestCase.php b/src/Schema/TestCase.php index f5e070c38..5f0cd2257 100644 --- a/src/Schema/TestCase.php +++ b/src/Schema/TestCase.php @@ -11,7 +11,7 @@ use Doctrine\DBAL\ParameterType; use Doctrine\DBAL\Platforms\AbstractPlatform; use Doctrine\DBAL\Platforms\OraclePlatform; -use Doctrine\DBAL\Platforms\SqlitePlatform; +use Doctrine\DBAL\Platforms\SQLitePlatform; use Doctrine\DBAL\Platforms\SQLServerPlatform; abstract class TestCase extends BaseTestCase @@ -114,7 +114,7 @@ function ($matches) use ($platform) { $sql ); - if ($platform instanceof SqlitePlatform && $convertedSql !== $sql) { + if ($platform instanceof SQLitePlatform && $convertedSql !== $sql) { self::assertSame($sql, $convertedSql); } diff --git a/tests/ExpressionSqlTest.php b/tests/ExpressionSqlTest.php index e4dd289fd..4a7f918ea 100644 --- a/tests/ExpressionSqlTest.php +++ b/tests/ExpressionSqlTest.php @@ -7,7 +7,7 @@ use Atk4\Data\Model; use Atk4\Data\Schema\TestCase; use Doctrine\DBAL\Platforms\OraclePlatform; -use Doctrine\DBAL\Platforms\SqlitePlatform; +use Doctrine\DBAL\Platforms\SQLitePlatform; class ExpressionSqlTest extends TestCase { @@ -33,7 +33,7 @@ public function testBasic(): void $i->addField('total_vat', ['type' => 'float']); $i->addExpression('total_gross', ['expr' => '[total_net] + [total_vat]', 'type' => 'float']); - if ($this->getDatabasePlatform() instanceof SqlitePlatform) { + if ($this->getDatabasePlatform() instanceof SQLitePlatform) { self::assertSame( 'select `id`, `total_net`, `total_vat`, (`total_net` + `total_vat`) `total_gross` from `invoice`', $i->action('select')->render()[0] @@ -50,7 +50,7 @@ public function testBasic(): void $i->addExpression('double_total_gross', ['expr' => '[total_gross] * 2', 'type' => 'float']); - if ($this->getDatabasePlatform() instanceof SqlitePlatform) { + if ($this->getDatabasePlatform() instanceof SQLitePlatform) { self::assertSame( 'select `id`, `total_net`, `total_vat`, (`total_net` + `total_vat`) `total_gross`, ((`total_net` + `total_vat`) * 2) `double_total_gross` from `invoice`', $i->action('select')->render()[0] @@ -77,7 +77,7 @@ public function testBasicCallback(): void return '[total_net] + [total_vat]'; }, 'type' => 'float']); - if ($this->getDatabasePlatform() instanceof SqlitePlatform) { + if ($this->getDatabasePlatform() instanceof SQLitePlatform) { self::assertSame( 'select `id`, `total_net`, `total_vat`, (`total_net` + `total_vat`) `total_gross` from `invoice`', $i->action('select')->render()[0] @@ -107,7 +107,7 @@ public function testQuery(): void $i->addField('total_vat', ['type' => 'float']); $i->addExpression('sum_net', ['expr' => $i->action('fx', ['sum', 'total_net']), ['type' => 'integer']]); - if ($this->getDatabasePlatform() instanceof SqlitePlatform) { + if ($this->getDatabasePlatform() instanceof SQLitePlatform) { self::assertSame( 'select `id`, `total_net`, `total_vat`, (select sum(`total_net`) from `invoice`) `sum_net` from `invoice`', $i->action('select')->render()[0] @@ -141,7 +141,7 @@ public function testExpressions(): void $m->addField('surname'); $m->addField('cached_name'); - if ($this->getDatabasePlatform() instanceof SqlitePlatform || $this->getDatabasePlatform() instanceof OraclePlatform) { + if ($this->getDatabasePlatform() instanceof SQLitePlatform || $this->getDatabasePlatform() instanceof OraclePlatform) { $concatExpr = '[name] || \' \' || [surname]'; } else { $concatExpr = 'CONCAT([name], \' \', [surname])'; diff --git a/tests/ModelAggregateTest.php b/tests/ModelAggregateTest.php index fe17fb676..34a7871a9 100644 --- a/tests/ModelAggregateTest.php +++ b/tests/ModelAggregateTest.php @@ -8,7 +8,7 @@ use Atk4\Data\Model\Scope; use Atk4\Data\Model\Scope\Condition; use Atk4\Data\Schema\TestCase; -use Doctrine\DBAL\Platforms\SqlitePlatform; +use Doctrine\DBAL\Platforms\SQLitePlatform; class ModelAggregateTest extends TestCase { @@ -178,7 +178,7 @@ public function testGroupSelectCondition2(): void 'double', '>', // TODO Sqlite bind param does not work, expr needed, even if casted to float with DBAL type (comparison works only if casted to/bind as int) - $this->getDatabasePlatform() instanceof SqlitePlatform ? $aggregate->expr('10') : 10 + $this->getDatabasePlatform() instanceof SQLitePlatform ? $aggregate->expr('10') : 10 ); self::assertSame([ @@ -201,7 +201,7 @@ public function testGroupSelectCondition3(): void $aggregate->addCondition( 'double', // TODO Sqlite bind param does not work, expr needed, even if casted to float with DBAL type (comparison works only if casted to/bind as int) - $this->getDatabasePlatform() instanceof SqlitePlatform ? $aggregate->expr('38') : 38 + $this->getDatabasePlatform() instanceof SQLitePlatform ? $aggregate->expr('38') : 38 ); self::assertSame([ @@ -239,7 +239,7 @@ public function testGroupSelectScope(): void self::fixAllNonAggregatedFieldsInGroupBy($aggregate); // TODO Sqlite bind param does not work, expr needed, even if casted to float with DBAL type (comparison works only if casted to/bind as int) - $numExpr = $this->getDatabasePlatform() instanceof SqlitePlatform ? $aggregate->expr('4') : 4; + $numExpr = $this->getDatabasePlatform() instanceof SQLitePlatform ? $aggregate->expr('4') : 4; $scope = Scope::createAnd(new Condition('client_id', 2), new Condition('amount', $numExpr)); $aggregate->addCondition($scope); diff --git a/tests/Persistence/Sql/ConnectionTest.php b/tests/Persistence/Sql/ConnectionTest.php index bccd725f9..00f04554d 100644 --- a/tests/Persistence/Sql/ConnectionTest.php +++ b/tests/Persistence/Sql/ConnectionTest.php @@ -8,13 +8,13 @@ use Atk4\Data\Persistence; use Atk4\Data\Persistence\Sql\Connection; use Doctrine\DBAL\Platforms\AbstractPlatform; -use Doctrine\DBAL\Platforms\SqlitePlatform; +use Doctrine\DBAL\Platforms\SQLitePlatform; class DummyConnection extends Connection { public function getDatabasePlatform(): AbstractPlatform { - return new class() extends SqlitePlatform { + return new class() extends SQLitePlatform { public function getName() { return 'dummy'; @@ -27,7 +27,7 @@ class DummyConnection2 extends Connection { public function getDatabasePlatform(): AbstractPlatform { - return new class() extends SqlitePlatform { + return new class() extends SQLitePlatform { public function getName() { return 'dummy2'; diff --git a/tests/RandomTest.php b/tests/RandomTest.php index b813aaa00..ea152da76 100644 --- a/tests/RandomTest.php +++ b/tests/RandomTest.php @@ -10,7 +10,7 @@ use Atk4\Data\Model; use Atk4\Data\Persistence; use Atk4\Data\Schema\TestCase; -use Doctrine\DBAL\Platforms\SqlitePlatform; +use Doctrine\DBAL\Platforms\SQLitePlatform; class Model_Rate extends Model { @@ -508,7 +508,7 @@ public function testNoWriteActionDelete(): void public function testTableWithSchema(): void { - if ($this->getDatabasePlatform() instanceof SqlitePlatform) { + if ($this->getDatabasePlatform() instanceof SQLitePlatform) { $userSchema = 'db1'; $docSchema = 'db2'; $runWithDb = false; diff --git a/tests/Schema/TestCaseTest.php b/tests/Schema/TestCaseTest.php index 84c3edbce..d755143c8 100644 --- a/tests/Schema/TestCaseTest.php +++ b/tests/Schema/TestCaseTest.php @@ -7,7 +7,7 @@ use Atk4\Data\Model; use Atk4\Data\Schema\TestCase; use Doctrine\DBAL\Platforms\MySQLPlatform; -use Doctrine\DBAL\Platforms\SqlitePlatform; +use Doctrine\DBAL\Platforms\SQLitePlatform; class TestCaseTest extends TestCase { @@ -37,7 +37,7 @@ public function testLogQuery(): void ob_end_clean(); } - if (!$this->getDatabasePlatform() instanceof SqlitePlatform && !$this->getDatabasePlatform() instanceof MySQLPlatform) { + if (!$this->getDatabasePlatform() instanceof SQLitePlatform && !$this->getDatabasePlatform() instanceof MySQLPlatform) { return; } diff --git a/tests/ScopeTest.php b/tests/ScopeTest.php index c53f1011c..3176781ea 100644 --- a/tests/ScopeTest.php +++ b/tests/ScopeTest.php @@ -10,7 +10,7 @@ use Atk4\Data\Model\Scope\Condition; use Atk4\Data\Schema\TestCase; use Doctrine\DBAL\Platforms\MySQLPlatform; -use Doctrine\DBAL\Platforms\SqlitePlatform; +use Doctrine\DBAL\Platforms\SQLitePlatform; class SCountry extends Model { @@ -184,7 +184,7 @@ public function testConditionToWords(): void $condition = new Condition('country_id', 2); self::assertSame('Country ID is equal to 2 (\'Latvia\')', $condition->toWords($user)); - if ($this->getDatabasePlatform() instanceof SqlitePlatform || $this->getDatabasePlatform() instanceof MySQLPlatform) { + if ($this->getDatabasePlatform() instanceof SQLitePlatform || $this->getDatabasePlatform() instanceof MySQLPlatform) { $condition = new Condition('name', $user->expr('[surname]')); self::assertSame('Name is equal to expression \'`surname`\'', $condition->toWords($user)); } @@ -319,7 +319,7 @@ public function testConditionOnReferencedRecords(): void $user->addCondition('Tickets/user/country_id/Users/#', '>', 1); $user->addCondition('Tickets/user/country_id/Users/#', '>=', 2); $user->addCondition('Tickets/user/country_id/Users/country_id/Users/#', '>', 1); - if (!$this->getDatabasePlatform() instanceof SqlitePlatform) { + if (!$this->getDatabasePlatform() instanceof SQLitePlatform) { // not supported because of limitation/issue in Sqlite, the generated query fails // with error: "parser stack overflow" $user->addCondition('Tickets/user/country_id/Users/country_id/Users/name', '!=', null); // should be always true diff --git a/tests/TypecastingTest.php b/tests/TypecastingTest.php index cec8c8fb1..2eae1a8f0 100644 --- a/tests/TypecastingTest.php +++ b/tests/TypecastingTest.php @@ -8,7 +8,7 @@ use Atk4\Data\Model; use Atk4\Data\Schema\TestCase; use Doctrine\DBAL\Platforms\OraclePlatform; -use Doctrine\DBAL\Platforms\SqlitePlatform; +use Doctrine\DBAL\Platforms\SQLitePlatform; class TypecastingTest extends TestCase { @@ -116,7 +116,7 @@ public function testType(): void $m->load(2)->set('float', 8.202343767574732)->save(); // pdo_sqlite in truncating float, see https://github.com/php/php-src/issues/8510 // fixed since PHP 8.1, but if converted in SQL to string explicitly, the result is still rounded to 15 significant digits - if (!$this->getDatabasePlatform() instanceof SqlitePlatform || \PHP_VERSION_ID >= 80100) { + if (!$this->getDatabasePlatform() instanceof SQLitePlatform || \PHP_VERSION_ID >= 80100) { self::assertSame(8.202343767574732, $m->load(2)->get('float')); } } From edd55c936ba1ea5abc449c3e515737cc07978905 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Thu, 17 Aug 2023 11:48:42 +0200 Subject: [PATCH 2/6] Minor UA cleanup (#1115) --- src/Model/UserAction.php | 89 ++++++++++++++++++++++++---------------- tests/UserActionTest.php | 85 ++++++++++++++++++++++---------------- 2 files changed, 103 insertions(+), 71 deletions(-) diff --git a/src/Model/UserAction.php b/src/Model/UserAction.php index 2922424ad..c79af9253 100644 --- a/src/Model/UserAction.php +++ b/src/Model/UserAction.php @@ -32,15 +32,15 @@ class UserAction public const APPLIES_TO_MULTIPLE_RECORDS = 'multiple'; // e.g. delete public const APPLIES_TO_ALL_RECORDS = 'all'; // e.g. truncate - /** @var string by default action is for a single record */ - public $appliesTo = self::APPLIES_TO_SINGLE_RECORD; - /** Defining action modifier */ public const MODIFIER_CREATE = 'create'; // create new record(s) public const MODIFIER_UPDATE = 'update'; // update existing record(s) public const MODIFIER_DELETE = 'delete'; // delete record(s) public const MODIFIER_READ = 'read'; // just read, does not modify record(s) + /** @var string by default action is for a single record */ + public $appliesTo = self::APPLIES_TO_SINGLE_RECORD; + /** @var string How this action interact with record */ public $modifier; @@ -74,26 +74,28 @@ class UserAction /** @var bool Atomic action will automatically begin transaction before and commit it after completing. */ public $atomic = true; + private function _getOwner(): Model + { + return $this->getOwner(); // @phpstan-ignore-line; + } + public function isOwnerEntity(): bool { - /** @var Model */ - $owner = $this->getOwner(); // @phpstan-ignore-line + $owner = $this->_getOwner(); return $owner->isEntity(); } public function getModel(): Model { - /** @var Model */ - $owner = $this->getOwner(); // @phpstan-ignore-line + $owner = $this->_getOwner(); return $owner->getModel(true); } public function getEntity(): Model { - /** @var Model */ - $owner = $this->getOwner(); // @phpstan-ignore-line + $owner = $this->_getOwner(); $owner->assertIsEntity(); return $owner; @@ -104,8 +106,7 @@ public function getEntity(): Model */ public function getActionForEntity(Model $entity): self { - /** @var Model */ - $owner = $this->getOwner(); // @phpstan-ignore-line + $owner = $this->_getOwner(); $entity->assertIsEntity($owner); foreach ($owner->getUserActions() as $name => $action) { @@ -126,12 +127,13 @@ public function getActionForEntity(Model $entity): self */ public function execute(...$args) { + $passOwner = false; if ($this->callback === null) { - $fx = \Closure::fromCallable([$this->getEntity(), $this->shortName]); + $fx = \Closure::fromCallable([$this->_getOwner(), $this->shortName]); } elseif (is_string($this->callback)) { - $fx = \Closure::fromCallable([$this->getEntity(), $this->callback]); + $fx = \Closure::fromCallable([$this->_getOwner(), $this->callback]); } else { - array_unshift($args, $this->getEntity()); + $passOwner = true; $fx = $this->callback; } @@ -140,9 +142,13 @@ public function execute(...$args) try { $this->validateBeforeExecute(); + if ($passOwner) { + array_unshift($args, $this->_getOwner()); + } + return $this->atomic === false ? $fx(...$args) - : $this->getModel()->atomic(static fn () => $fx(...$args)); + : $this->_getOwner()->atomic(static fn () => $fx(...$args)); } catch (CoreException $e) { $e->addMoreInfo('action', $this); @@ -152,39 +158,39 @@ public function execute(...$args) protected function validateBeforeExecute(): void { - if ($this->enabled === false || ($this->enabled instanceof \Closure && ($this->enabled)($this->getEntity()) === false)) { - throw new Exception('This action is disabled'); + if ($this->enabled === false || ($this->enabled instanceof \Closure && ($this->enabled)($this->_getOwner()) === false)) { + throw new Exception('User action is disabled'); } - // Verify that model fields wouldn't be too dirty - if (is_array($this->fields)) { - $tooDirty = array_diff(array_keys($this->getEntity()->getDirtyRef()), $this->fields); + if (!is_bool($this->fields) && $this->fields !== []) { + $dirtyFields = array_keys($this->getEntity()->getDirtyRef()); + $tooDirtyFields = array_diff($dirtyFields, $this->fields); - if ($tooDirty) { - throw (new Exception('Calling user action on a Model with dirty fields that are not allowed by this action')) - ->addMoreInfo('too_dirty', $tooDirty) - ->addMoreInfo('dirty', array_keys($this->getEntity()->getDirtyRef())) - ->addMoreInfo('permitted', $this->fields); + if ($tooDirtyFields !== []) { + throw (new Exception('User action cannot be executed as unrelated fields are dirty')) + ->addMoreInfo('tooDirtyFields', $tooDirtyFields) + ->addMoreInfo('otherDirtyFields', array_diff($dirtyFields, $tooDirtyFields)); } - } elseif (!is_bool($this->fields)) { // @phpstan-ignore-line - throw (new Exception('Argument `fields` for the user action must be either array or boolean')) - ->addMoreInfo('fields', $this->fields); } - // Verify some records scope cases switch ($this->appliesTo) { case self::APPLIES_TO_NO_RECORDS: if ($this->getEntity()->isLoaded()) { - throw (new Exception('This user action can be executed on non-existing record only')) + throw (new Exception('User action can be executed on new entity only')) ->addMoreInfo('id', $this->getEntity()->getId()); } break; case self::APPLIES_TO_SINGLE_RECORD: if (!$this->getEntity()->isLoaded()) { - throw new Exception('This user action requires you to load existing record first'); + throw new Exception('User action can be executed on loaded entity only'); } + break; + case self::APPLIES_TO_MULTIPLE_RECORDS: + case self::APPLIES_TO_ALL_RECORDS: + $this->_getOwner()->assertIsModel(); + break; } } @@ -198,14 +204,27 @@ protected function validateBeforeExecute(): void */ public function preview(...$args) { + $passOwner = false; if (is_string($this->preview)) { - $fx = \Closure::fromCallable([$this->getEntity(), $this->preview]); + $fx = \Closure::fromCallable([$this->_getOwner(), $this->preview]); } else { - array_unshift($args, $this->getEntity()); + $passOwner = true; $fx = $this->preview; } - return $fx(...$args); + try { + $this->validateBeforeExecute(); + + if ($passOwner) { + array_unshift($args, $this->_getOwner()); + } + + return $fx(...$args); + } catch (CoreException $e) { + $e->addMoreInfo('action', $this); + + throw $e; + } } /** @@ -232,7 +251,7 @@ public function getConfirmation() } elseif ($this->confirmation === true) { $confirmation = 'Are you sure you wish to execute ' . $this->getCaption() - . ($this->getEntity()->getTitle() ? ' using ' . $this->getEntity()->getTitle() : '') + . ($this->isOwnerEntity() && $this->getEntity()->getTitle() ? ' using ' . $this->getEntity()->getTitle() : '') . '?'; return $confirmation; diff --git a/tests/UserActionTest.php b/tests/UserActionTest.php index 41318ac19..a7e95af3a 100644 --- a/tests/UserActionTest.php +++ b/tests/UserActionTest.php @@ -4,7 +4,7 @@ namespace Atk4\Data\Tests; -use Atk4\Core\Exception as CoreException; +use Atk4\Data\Exception; use Atk4\Data\Model; use Atk4\Data\Persistence; use Atk4\Data\Schema\TestCase; @@ -85,7 +85,7 @@ public function testBasic(): void $client->unload(); // test system action - $act2 = $client->getUserAction('backupClients'); + $act2 = $client->getModel()->getUserAction('backupClients'); // action takes no arguments. If it would, we should be able to find info about those self::assertSame([], $act2->args); @@ -108,6 +108,17 @@ public function testCustomSeedClass(): void self::assertSame($customClass, get_class($client->getUserAction('foo'))); } + public function testExecuteUndefinedMethodException(): void + { + $client = new UaClient($this->pers); + $client->addUserAction('new_client'); + $client = $client->load(1); + + $this->expectException(\Error::class); + $this->expectExceptionMessage('Call to undefined method'); + $client->executeUserAction('new_client'); + } + public function testPreview(): void { $client = new UaClient($this->pers); @@ -116,12 +127,13 @@ public function testPreview(): void }); $client = $client->load(1); + self::assertSame('John', $client->getUserAction('say_name')->execute()); - $client->getUserAction('say_name')->preview = function (UaClient $m, string $arg) { + $client->getUserAction('say_name')->preview = function (UaClient $m) { return 'will say ' . $m->get('name'); }; - self::assertSame('will say John', $client->getUserAction('say_name')->preview('x')); + self::assertSame('will say John', $client->getUserAction('say_name')->preview()); $client->getModel()->addUserAction('also_backup', ['callback' => 'backupClients']); self::assertSame('backs up all clients', $client->getUserAction('also_backup')->execute()); @@ -132,55 +144,68 @@ public function testPreview(): void self::assertSame('Also Backup UaClient', $client->getUserAction('also_backup')->getDescription()); } - public function testAppliesTo1(): void + public function testAppliesToSingleRecordNotEntityException(): void { $client = new UaClient($this->pers); - $client = $client->createEntity(); - $this->expectExceptionMessage('load existing record'); + $this->expectException(\TypeError::class); + $this->expectExceptionMessage('Expected entity, but instance is a model'); $client->executeUserAction('sendReminder'); } - public function testAppliesTo2(): void + public function testAppliesToAllRecordsEntityException(): void { $client = new UaClient($this->pers); - $client->addUserAction('new_client', ['appliesTo' => Model\UserAction::APPLIES_TO_NO_RECORDS]); $client = $client->load(1); - $this->expectExceptionMessage('can be executed on non-existing record'); - $client->executeUserAction('new_client'); + $this->expectException(\TypeError::class); + $this->expectExceptionMessage('Expected model, but instance is an entity'); + $client->executeUserAction('backupClients'); } - public function testAppliesTo3(): void + public function testAppliesToSingleRecordNotLoadedException(): void { $client = new UaClient($this->pers); - $client->addUserAction('new_client', ['appliesTo' => Model\UserAction::APPLIES_TO_NO_RECORDS, 'atomic' => false]); $client = $client->createEntity(); - $this->expectExceptionMessage('undefined method'); + $this->expectException(Exception::class); + $this->expectExceptionMessage('User action can be executed on loaded entity only'); + $client->executeUserAction('sendReminder'); + } + + public function testAppliesToNoRecordsLoadedRecordException(): void + { + $client = new UaClient($this->pers); + $client->addUserAction('new_client', ['appliesTo' => Model\UserAction::APPLIES_TO_NO_RECORDS]); + $client = $client->load(1); + + $this->expectException(Exception::class); + $this->expectExceptionMessage('User action can be executed on new entity only'); $client->executeUserAction('new_client'); } - public function testException1(): void + public function testNotDefinedException(): void { $client = new UaClient($this->pers); - $this->expectException(CoreException::class); + $this->expectException(Exception::class); + $this->expectExceptionMessage('User action is not defined'); $client->getUserAction('non_existent_action'); } - public function testDisabled1(): void + public function testDisabledBoolException(): void { $client = new UaClient($this->pers); $client = $client->load(1); $client->getUserAction('sendReminder')->enabled = false; - $this->expectExceptionMessage('disabled'); + $this->expectException(Exception::class); + $this->expectExceptionMessage('User action is disabled'); $client->getUserAction('sendReminder')->execute(); } - public function testDisabled2(): void + public function testDisabledClosureException(): void { $client = new UaClient($this->pers); $client = $client->load(1); @@ -194,7 +219,8 @@ public function testDisabled2(): void return false; }; - $this->expectExceptionMessage('disabled'); + $this->expectException(Exception::class); + $this->expectExceptionMessage('User action is disabled'); $client->getUserAction('sendReminder')->execute(); } @@ -211,7 +237,7 @@ public function testFields(): void self::assertSame('Peter', $client->get('name')); } - public function testFieldsTooDirty1(): void + public function testFieldsTooDirtyException(): void { $client = new UaClient($this->pers); $client->addUserAction('change_details', ['callback' => 'save', 'fields' => ['name']]); @@ -222,21 +248,8 @@ public function testFieldsTooDirty1(): void $client->set('name', 'Peter'); $client->set('reminder_sent', true); - $this->expectExceptionMessage('dirty fields'); - $client->getUserAction('change_details')->execute(); - } - - public function testFieldsIncorrect(): void - { - $client = new UaClient($this->pers); - $client->addUserAction('change_details', ['callback' => 'save', 'fields' => 'whops_forgot_brackets']); - - $client = $client->load(1); - - self::assertNotSame('Peter', $client->get('name')); - $client->set('name', 'Peter'); - - $this->expectExceptionMessage('must be either array or boolean'); + $this->expectException(Exception::class); + $this->expectExceptionMessage('User action cannot be executed as unrelated fields are dirty'); $client->getUserAction('change_details')->execute(); } From 9c36bd0698eb166ce03055ae2b9e49cc1567110c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Thu, 17 Aug 2023 17:26:39 +0200 Subject: [PATCH 3/6] improve wording --- src/Model/UserAction.php | 2 +- tests/UserActionTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Model/UserAction.php b/src/Model/UserAction.php index c79af9253..0c32c6e03 100644 --- a/src/Model/UserAction.php +++ b/src/Model/UserAction.php @@ -167,7 +167,7 @@ protected function validateBeforeExecute(): void $tooDirtyFields = array_diff($dirtyFields, $this->fields); if ($tooDirtyFields !== []) { - throw (new Exception('User action cannot be executed as unrelated fields are dirty')) + throw (new Exception('User action cannot be executed when unrelated fields are dirty')) ->addMoreInfo('tooDirtyFields', $tooDirtyFields) ->addMoreInfo('otherDirtyFields', array_diff($dirtyFields, $tooDirtyFields)); } diff --git a/tests/UserActionTest.php b/tests/UserActionTest.php index a7e95af3a..0c8450c34 100644 --- a/tests/UserActionTest.php +++ b/tests/UserActionTest.php @@ -249,7 +249,7 @@ public function testFieldsTooDirtyException(): void $client->set('reminder_sent', true); $this->expectException(Exception::class); - $this->expectExceptionMessage('User action cannot be executed as unrelated fields are dirty'); + $this->expectExceptionMessage('User action cannot be executed when unrelated fields are dirty'); $client->getUserAction('change_details')->execute(); } From 9408a9244853efaca38b6a44301ea4eee8cb00bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Thu, 17 Aug 2023 18:14:32 +0200 Subject: [PATCH 4/6] fix typo --- src/Model/UserAction.php | 2 +- src/Model/UserActionsTrait.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Model/UserAction.php b/src/Model/UserAction.php index 0c32c6e03..1c3b1d380 100644 --- a/src/Model/UserAction.php +++ b/src/Model/UserAction.php @@ -162,7 +162,7 @@ protected function validateBeforeExecute(): void throw new Exception('User action is disabled'); } - if (!is_bool($this->fields) && $this->fields !== []) { + if (!is_bool($this->fields) && $this->isOwnerEntity()) { $dirtyFields = array_keys($this->getEntity()->getDirtyRef()); $tooDirtyFields = array_diff($dirtyFields, $this->fields); diff --git a/src/Model/UserActionsTrait.php b/src/Model/UserActionsTrait.php index 3e91aadc6..1d39d7e70 100644 --- a/src/Model/UserActionsTrait.php +++ b/src/Model/UserActionsTrait.php @@ -155,7 +155,7 @@ protected function initUserActions(): void ]); $this->addUserAction('validate', [ - // 'appliesTo' => any! + // 'appliesTo' => any entity! 'description' => 'Provided with modified values will validate them but will not save', 'modifier' => UserAction::MODIFIER_READ, 'fields' => true, From 4797c78bf74ffce20ddc291a470786f32e2f000f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 20 Aug 2023 03:38:18 +0200 Subject: [PATCH 5/6] Allow to create index for multiple fields (#1118) --- src/Persistence/Sql/DbalDriverMiddleware.php | 21 +-- src/Schema/Migrator.php | 44 +++++-- tests/Schema/MigratorFkTest.php | 131 ++++++++++++++++++- 3 files changed, 162 insertions(+), 34 deletions(-) diff --git a/src/Persistence/Sql/DbalDriverMiddleware.php b/src/Persistence/Sql/DbalDriverMiddleware.php index 7f5af1b8c..691952f67 100644 --- a/src/Persistence/Sql/DbalDriverMiddleware.php +++ b/src/Persistence/Sql/DbalDriverMiddleware.php @@ -6,13 +6,11 @@ use Doctrine\DBAL\Connection as DbalConnection; use Doctrine\DBAL\Driver\API\ExceptionConverter; -use Doctrine\DBAL\Driver\API\SQLite\ExceptionConverter as SqliteExceptionConverter; use Doctrine\DBAL\Driver\API\SQLSrv\ExceptionConverter as SQLServerExceptionConverter; use Doctrine\DBAL\Driver\Exception as DbalDriverException; use Doctrine\DBAL\Driver\Middleware\AbstractDriverMiddleware; use Doctrine\DBAL\Exception\DatabaseObjectNotFoundException; use Doctrine\DBAL\Exception\DriverException as DbalDriverConvertedException; -use Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException; use Doctrine\DBAL\Exception\TableNotFoundException; use Doctrine\DBAL\Platforms\AbstractPlatform; use Doctrine\DBAL\Platforms\OraclePlatform; @@ -116,22 +114,7 @@ final protected static function getUnconvertedException(DbalDriverConvertedExcep public function getExceptionConverter(): ExceptionConverter { $exceptionConverter = parent::getExceptionConverter(); - if ($exceptionConverter instanceof SqliteExceptionConverter) { - $exceptionConverter = $this->createExceptionConvertorMiddleware( - $exceptionConverter, - function (DbalDriverConvertedException $convertedException, ?DbalQuery $query): DbalDriverConvertedException { - // fix FK violation exception conversion - // https://github.com/doctrine/dbal/issues/5496 - $exception = self::getUnconvertedException($convertedException); - $exceptionMessageLc = strtolower($exception->getMessage()); - if (str_contains($exceptionMessageLc, 'integrity constraint violation')) { - return new ForeignKeyConstraintViolationException($exception, $query); - } - - return $convertedException; - } - ); - } elseif ($exceptionConverter instanceof SQLServerExceptionConverter) { + if ($exceptionConverter instanceof SQLServerExceptionConverter) { $exceptionConverter = $this->createExceptionConvertorMiddleware( $exceptionConverter, function (DbalDriverConvertedException $convertedException, ?DbalQuery $query): DbalDriverConvertedException { @@ -140,7 +123,7 @@ function (DbalDriverConvertedException $convertedException, ?DbalQuery $query): if ($convertedException instanceof DatabaseObjectNotFoundException) { $exception = self::getUnconvertedException($convertedException); $exceptionMessageLc = strtolower($exception->getMessage()); - if (str_contains($exceptionMessageLc, 'cannot drop the table')) { + if (str_contains($exceptionMessageLc, 'cannot drop the table') && !$convertedException instanceof TableNotFoundException) { return new TableNotFoundException($exception, $query); } } diff --git a/src/Schema/Migrator.php b/src/Schema/Migrator.php index d4f20ad00..d681ff4f7 100644 --- a/src/Schema/Migrator.php +++ b/src/Schema/Migrator.php @@ -415,37 +415,53 @@ protected function fixTableNameForListMethod(string $tableName): string return $platform->quoteSingleIdentifier($tableName); } - public function isIndexExists(Field $field, bool $requireUnique): bool + /** + * @param list $fields + */ + public function isIndexExists(array $fields, bool $requireUnique): bool { - $field = $this->resolvePersistenceField($field); + $fields = array_map(fn ($field) => $this->resolvePersistenceField($field), $fields); + $table = reset($fields)->getOwner()->table; - $indexes = $this->createSchemaManager()->listTableIndexes($this->fixTableNameForListMethod($field->getOwner()->table)); + $indexes = $this->createSchemaManager()->listTableIndexes($this->fixTableNameForListMethod($table)); + $fieldPersistenceNames = array_map(fn ($field) => $field->getPersistenceName(), $fields); foreach ($indexes as $index) { - if ($index->getUnquotedColumns() === [$field->getPersistenceName()] && (!$requireUnique || $index->isUnique())) { - return true; + $indexPersistenceNames = $index->getUnquotedColumns(); + if ($requireUnique) { + if ($indexPersistenceNames === $fieldPersistenceNames && $index->isUnique()) { + return true; + } + } else { + if (array_slice($indexPersistenceNames, 0, count($fieldPersistenceNames)) === $fieldPersistenceNames) { + return true; + } } } return false; } - public function createIndex(Field $field, bool $isUnique): void + /** + * @param list $fields + */ + public function createIndex(array $fields, bool $isUnique): void { - $field = $this->resolvePersistenceField($field); + $fields = array_map(fn ($field) => $this->resolvePersistenceField($field), $fields); + $table = reset($fields)->getOwner()->table; $platform = $this->getDatabasePlatform(); $index = new Index( - \Closure::bind(function () use ($field) { + \Closure::bind(function () use ($table, $fields) { return (new Identifier(''))->_generateIdentifierName([ - $field->getOwner()->table, - $field->getPersistenceName(), + $table, + ...array_map(fn ($field) => $field->getPersistenceName(), $fields), ], 'uniq'); }, null, Identifier::class)(), - [$platform->quoteSingleIdentifier($field->getPersistenceName())], + array_map(fn ($field) => $platform->quoteSingleIdentifier($field->getPersistenceName()), $fields), $isUnique ); - $this->createSchemaManager()->createIndex($index, $platform->quoteIdentifier($field->getOwner()->table)); + $this->createSchemaManager()->createIndex($index, $platform->quoteIdentifier($table)); } /** @@ -459,8 +475,8 @@ public function createForeignKey($relation): void $localField = $this->resolvePersistenceField($localField); $foreignField = $this->resolvePersistenceField($foreignField); - if (!$this->isIndexExists($foreignField, true)) { - $this->createIndex($foreignField, true); + if (!$this->isIndexExists([$foreignField], true)) { + $this->createIndex([$foreignField], true); } $platform = $this->getDatabasePlatform(); diff --git a/tests/Schema/MigratorFkTest.php b/tests/Schema/MigratorFkTest.php index d4c17f4eb..3c8866457 100644 --- a/tests/Schema/MigratorFkTest.php +++ b/tests/Schema/MigratorFkTest.php @@ -9,12 +9,38 @@ use Atk4\Data\Schema\TestCase; use Doctrine\DBAL\Exception as DbalException; use Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; +use Doctrine\DBAL\Platforms\MySQLPlatform; use Doctrine\DBAL\Schema\ForeignKeyConstraint; +use Doctrine\DBAL\Schema\Index; class MigratorFkTest extends TestCase { /** - * @return array, string, array}> + * @return list, bool}> + */ + protected function listTableIndexes(string $localTable): array + { + $indexes = $this->getConnection()->createSchemaManager()->listTableIndexes($localTable); + + self::assertArrayHasKey('primary', $indexes); + unset($indexes['primary']); + + $res = array_map(function (Index $v) { + self::assertFalse($v->isPrimary()); + + return [ + $v->getUnquotedColumns(), + $v->isUnique(), + ]; + }, $indexes); + sort($res); + + return $res; + } + + /** + * @return list, string, list}> */ protected function listTableForeignKeys(string $localTable): array { @@ -32,6 +58,109 @@ protected function listTableForeignKeys(string $localTable): array return $res; } + public function testCreateIndexNonUnique(): void + { + $client = new Model($this->db, ['table' => 'client']); + $client->addField('name'); + + $this->createMigrator($client)->create(); + self::assertSame([], $this->listTableIndexes('client')); + self::assertTrue($this->createMigrator()->isIndexExists([$client->getField('id')], false)); + self::assertTrue($this->createMigrator()->isIndexExists([$client->getField('id')], true)); + self::assertFalse($this->createMigrator()->isIndexExists([$client->getField('name')], false)); + + $this->createMigrator()->createIndex([$client->getField('name')], false); + self::assertSame([[['name'], false]], $this->listTableIndexes('client')); + self::assertTrue($this->createMigrator()->isIndexExists([$client->getField('name')], false)); + self::assertFalse($this->createMigrator()->isIndexExists([$client->getField('name')], true)); + self::assertFalse($this->createMigrator()->isIndexExists([$client->getField('id'), $client->getField('name')], false)); + self::assertFalse($this->createMigrator()->isIndexExists([$client->getField('name'), $client->getField('id')], false)); + + $client->insert(['name' => 'Michael']); + $client->insert(['name' => 'Denise']); + $client->insert(['name' => null]); + $client->insert(['name' => 'Michael']); + $client->insert(['name' => null]); + + self::assertSameExportUnordered([ + ['id' => 1, 'name' => 'Michael'], + ['id' => 2, 'name' => 'Denise'], + ['id' => 3, 'name' => null], + ['id' => 4, 'name' => 'Michael'], + ['id' => 5, 'name' => null], + ], $client->export()); + } + + public function testCreateIndexUnique(): void + { + $client = new Model($this->db, ['table' => 'client']); + $client->addField('name'); + + $this->createMigrator($client)->create(); + + if ($this->getDatabasePlatform() instanceof MySQLPlatform) { + $serverVersion = $this->getConnection()->getConnection()->getWrappedConnection()->getServerVersion(); // @phpstan-ignore-line + if (preg_match('~^5\.6~', $serverVersion)) { + self::markTestIncomplete('TODO MySQL 5.6: Unique key exceed max key (767 bytes) length'); + } + } + + $this->createMigrator()->createIndex([$client->getField('name')], true); + self::assertSame([[['name'], true]], $this->listTableIndexes('client')); + self::assertTrue($this->createMigrator()->isIndexExists([$client->getField('name')], false)); + self::assertTrue($this->createMigrator()->isIndexExists([$client->getField('name')], true)); + self::assertFalse($this->createMigrator()->isIndexExists([$client->getField('id'), $client->getField('name')], false)); + self::assertFalse($this->createMigrator()->isIndexExists([$client->getField('name'), $client->getField('id')], false)); + + $client->insert(['name' => 'Michael']); + $client->insert(['name' => 'Denise']); + $client->insert(['name' => null]); + $client->insert(['name' => null]); + + self::assertSameExportUnordered([ + ['id' => 1, 'name' => 'Michael'], + ['id' => 2, 'name' => 'Denise'], + ['id' => 3, 'name' => null], + ['id' => 4, 'name' => null], + ], $client->export()); + + $this->expectException(Exception::class); + try { + $client->insert(['name' => 'Michael']); + } catch (Exception $e) { + $dbalException = $e->getPrevious()->getPrevious(); + self::assertInstanceOf(UniqueConstraintViolationException::class, $dbalException); + + throw $e; + } + } + + public function testCreateIndexMultipleFields(): void + { + $client = new Model($this->db, ['table' => 'client']); + $client->addField('a'); + $client->addField('b'); + + $this->createMigrator($client)->create(); + self::assertSame([], $this->listTableIndexes('client')); + + if ($this->getDatabasePlatform() instanceof MySQLPlatform) { + $serverVersion = $this->getConnection()->getConnection()->getWrappedConnection()->getServerVersion(); // @phpstan-ignore-line + if (preg_match('~^5\.6~', $serverVersion)) { + self::markTestIncomplete('TODO MySQL 5.6: Unique key exceed max key (767 bytes) length'); + } + } + + $this->createMigrator($client)->createIndex([$client->getField('a'), $client->getField('b')], true); + self::assertSame([[['a', 'b'], true]], $this->listTableIndexes('client')); + self::assertTrue($this->createMigrator()->isIndexExists([$client->getField('a'), $client->getField('b')], false)); + self::assertTrue($this->createMigrator()->isIndexExists([$client->getField('a'), $client->getField('b')], true)); + self::assertTrue($this->createMigrator()->isIndexExists([$client->getField('a')], false)); + self::assertFalse($this->createMigrator()->isIndexExists([$client->getField('a')], true)); + self::assertFalse($this->createMigrator()->isIndexExists([$client->getField('b')], false)); + self::assertFalse($this->createMigrator()->isIndexExists([$client->getField('b'), $client->getField('a')], false)); + } + public function testForeignKeyViolation(): void { $country = new Model($this->db, ['table' => 'country']); From d2f0816230f9c9b44223a97c568cca9c2b018456 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 20 Aug 2023 04:17:42 +0200 Subject: [PATCH 6/6] Fix DBAL unique index creation for MSSQL & Oracle (#1117) --- phpstan.neon.dist | 4 +- src/Persistence/Sql/Mssql/PlatformTrait.php | 15 +++++ src/Persistence/Sql/Oracle/PlatformTrait.php | 30 ++++++++++ src/Schema/Migrator.php | 27 ++++++++- tests/Schema/MigratorFkTest.php | 61 ++++++++++++++++++++ 5 files changed, 132 insertions(+), 5 deletions(-) diff --git a/phpstan.neon.dist b/phpstan.neon.dist index ee803158f..64e430f08 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -17,9 +17,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|getCurrentDatabaseExpression|initializeDoctrineTypeMappings)\(\) of deprecated class Doctrine\\DBAL\\Platforms\\(PostgreSQLPlatform|SQLServerPlatform|AbstractPlatform):\nUse.+instead\.$~' + message: '~^Call to method (getVarcharTypeDeclarationSQL|getClobTypeDeclarationSQL|getCreateIndexSQL|getCreateTableSQL|getCurrentDatabaseExpression|initializeDoctrineTypeMappings)\(\) of deprecated class Doctrine\\DBAL\\Platforms\\(PostgreSQLPlatform|SQLServerPlatform|AbstractPlatform):\nUse.+instead\.$~' path: '*' - count: 5 + count: 6 # https://github.com/phpstan/phpstan-deprecation-rules/issues/75 - message: '~^Call to deprecated method getVarcharTypeDeclarationSQL\(\) of class AnonymousClass\w+:\nUse \{@link getStringTypeDeclarationSQL\(\)\} instead\.$~' diff --git a/src/Persistence/Sql/Mssql/PlatformTrait.php b/src/Persistence/Sql/Mssql/PlatformTrait.php index 9c44c2f6c..279279a2b 100644 --- a/src/Persistence/Sql/Mssql/PlatformTrait.php +++ b/src/Persistence/Sql/Mssql/PlatformTrait.php @@ -4,6 +4,9 @@ namespace Atk4\Data\Persistence\Sql\Mssql; +use Doctrine\DBAL\Platforms\AbstractPlatform; +use Doctrine\DBAL\Schema\Index; + trait PlatformTrait { public function getVarcharTypeDeclarationSQL(array $column) @@ -40,6 +43,18 @@ public function getCurrentDatabaseExpression(bool $includeSchema = false): strin return parent::getCurrentDatabaseExpression(); } + public function getCreateIndexSQL(Index $index, $table) + { + // workaround https://github.com/doctrine/dbal/issues/5507 + // no side effect on DBAL index list observed, but multiple null values cannot be inserted + // the only, very complex, solution would be using intermediate view + // SQL Server should be fixed to allow FK creation when there is an unique index + // with "WHERE xxx IS NOT NULL" as FK does not restrict NULL values anyway + return $index->hasFlag('atk4-not-null') + ? AbstractPlatform::getCreateIndexSQL($index, $table) + : parent::getCreateIndexSQL($index, $table); + } + // 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/PlatformTrait.php b/src/Persistence/Sql/Oracle/PlatformTrait.php index 595fabe59..2df7971c6 100644 --- a/src/Persistence/Sql/Oracle/PlatformTrait.php +++ b/src/Persistence/Sql/Oracle/PlatformTrait.php @@ -5,7 +5,11 @@ namespace Atk4\Data\Persistence\Sql\Oracle; use Doctrine\DBAL\Platforms\OraclePlatform; +use Doctrine\DBAL\Schema\AbstractAsset; +use Doctrine\DBAL\Schema\Index; use Doctrine\DBAL\Schema\Sequence; +use Doctrine\DBAL\Schema\Table; +use Doctrine\DBAL\Schema\UniqueConstraint; trait PlatformTrait { @@ -101,6 +105,32 @@ public function getCreateAutoincrementSql($name, $table, $start = 1) return $sqls; } + public function getCreateIndexSQL(Index $index, $table) + { + // workaround https://github.com/doctrine/dbal/issues/5508 + // no side effect on multiple null values or DBAL index list observed + if ($index->isUnique()) { + $uniqueConstraint = new UniqueConstraint( + '0.0', + ['0.0'], + $index->getFlags(), + $index->getOptions() + ); + \Closure::bind(function () use ($index, $uniqueConstraint) { + $uniqueConstraint->_name = $index->_name; + $uniqueConstraint->_namespace = $index->_namespace; + $uniqueConstraint->_quoted = $index->_quoted; + $uniqueConstraint->columns = $index->_columns; + }, null, AbstractAsset::class)(); + + $tableName = $table instanceof Table ? $table->getQuotedName($this) : $table; + + return $this->getCreateUniqueConstraintSQL($uniqueConstraint, $tableName); + } + + return parent::getCreateIndexSQL($index, $table); + } + public function getListDatabasesSQL(): string { // ignore Oracle maintained schemas, improve tests performance diff --git a/src/Schema/Migrator.php b/src/Schema/Migrator.php index d681ff4f7..27d4adc7d 100644 --- a/src/Schema/Migrator.php +++ b/src/Schema/Migrator.php @@ -450,6 +450,17 @@ public function createIndex(array $fields, bool $isUnique): void $table = reset($fields)->getOwner()->table; $platform = $this->getDatabasePlatform(); + + $mssqlNullable = null; + if ($platform instanceof SQLServerPlatform) { + $mssqlNullable = false; + foreach ($fields as $field) { + if ($field->nullable && !$field->required) { + $mssqlNullable = true; + } + } + } + $index = new Index( \Closure::bind(function () use ($table, $fields) { return (new Identifier(''))->_generateIdentifierName([ @@ -458,7 +469,9 @@ public function createIndex(array $fields, bool $isUnique): void ], 'uniq'); }, null, Identifier::class)(), array_map(fn ($field) => $platform->quoteSingleIdentifier($field->getPersistenceName()), $fields), - $isUnique + $isUnique, + false, + $mssqlNullable === false ? ['atk4-not-null'] : [] ); $this->createSchemaManager()->createIndex($index, $platform->quoteIdentifier($table)); @@ -475,11 +488,19 @@ public function createForeignKey($relation): void $localField = $this->resolvePersistenceField($localField); $foreignField = $this->resolvePersistenceField($foreignField); + $platform = $this->getDatabasePlatform(); + if (!$this->isIndexExists([$foreignField], true)) { - $this->createIndex([$foreignField], true); + if ($foreignField->nullable && !$foreignField->required && $platform instanceof SQLServerPlatform) { + $foreignFieldForIndex = clone $foreignField; + $foreignFieldForIndex->nullable = false; + } else { + $foreignFieldForIndex = $foreignField; + } + + $this->createIndex([$foreignFieldForIndex], true); } - $platform = $this->getDatabasePlatform(); $foreignKey = new ForeignKeyConstraint( [$platform->quoteSingleIdentifier($localField->getPersistenceName())], '0.0', diff --git a/tests/Schema/MigratorFkTest.php b/tests/Schema/MigratorFkTest.php index 3c8866457..84b2a251b 100644 --- a/tests/Schema/MigratorFkTest.php +++ b/tests/Schema/MigratorFkTest.php @@ -11,6 +11,7 @@ use Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException; use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use Doctrine\DBAL\Platforms\MySQLPlatform; +use Doctrine\DBAL\Platforms\SQLServerPlatform; use Doctrine\DBAL\Schema\ForeignKeyConstraint; use Doctrine\DBAL\Schema\Index; @@ -227,4 +228,64 @@ public function testForeignKeyViolationDuringSetup(): void $this->expectException(DbalException::class); $this->createMigrator()->createForeignKey($client->getReference('country_id')); } + + public function testForeignKeyViolationWithoutPk(): void + { + $currency = new Model($this->db, ['table' => 'currency']); + $currency->addField('code'); + $currency->addField('name'); + + $price = new Model($this->db, ['table' => 'price']); + $price->addField('amount', ['type' => 'float']); + $price->addField('currency'); + + $this->createMigrator($currency)->create(); + $this->createMigrator($price)->create(); + + $currency->insert(['code' => 'EUR', 'name' => 'Euro']); + $currency->insert(['code' => 'USD', 'name' => 'United States dollar']); + $currency->insert(['code' => 'CZK', 'name' => 'Česká koruna']); + + if ($this->getDatabasePlatform() instanceof MySQLPlatform) { + $serverVersion = $this->getConnection()->getConnection()->getWrappedConnection()->getServerVersion(); // @phpstan-ignore-line + if (preg_match('~^5\.6~', $serverVersion)) { + self::markTestIncomplete('TODO MySQL 5.6: Unique key exceed max key (767 bytes) length'); + } + } + + $this->createMigrator()->createForeignKey([$price->getField('currency'), $currency->getField('code')]); + + $price->insert(['amount' => 0.5, 'currency' => 'EUR']); + $price->insert(['amount' => 1, 'currency' => 'EUR']); + $price->insert(['amount' => 2, 'currency' => 'USD']); + $price->insert(['amount' => 3, 'currency' => null]); + $price->insert(['amount' => 4, 'currency' => null]); + + self::assertSameExportUnordered([ + ['id' => 1, 'amount' => 0.5, 'currency' => 'EUR'], + ['id' => 2, 'amount' => 1.0, 'currency' => 'EUR'], + ['id' => 3, 'amount' => 2.0, 'currency' => 'USD'], + ['id' => 4, 'amount' => 3.0, 'currency' => null], + ['id' => 5, 'amount' => 4.0, 'currency' => null], + ], $price->export()); + + $currency->insert(['code' => null, 'name' => 'Reward A']); + if ($this->getDatabasePlatform() instanceof SQLServerPlatform) { + // MSSQL unique index does not allow duplicate NULL values, if the index is created + // with "WHERE xxx IS NOT NULL" then FK cannot be created + // https://github.com/doctrine/dbal/issues/5507 + } else { + $currency->insert(['code' => null, 'name' => 'Reward B']); + } + + $this->expectException(Exception::class); + try { + $price->insert(['amount' => 5, 'currency' => 'JPY']); + } catch (Exception $e) { + $dbalException = $e->getPrevious()->getPrevious(); + self::assertInstanceOf(ForeignKeyConstraintViolationException::class, $dbalException); + + throw $e; + } + } }