Store binary values natively for PostgreSQL and MSSQL (#921)

* Store binary values natively for PostgreSQL

* Fix DBAL PDO connection create for MSSQL

* Store binary values natively for MSSQL

Author: mvorisek

phpstan.neon.dist

@@ -28,10 +28,10 @@ parameters:
         # for Doctrine DBAL 2.x, remove the support once Doctrine ORM 2.10 is released
         # see https://github.com/doctrine/orm/issues/8526
         -
-            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\.)$~'
+            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\.|Parameter #1 \$dsn of class Doctrine\\DBAL\\Driver\\PDO\\SQLSrv\\Connection constructor expects string, Doctrine\\DBAL\\Driver\\PDO\\Connection given\.|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: 12
 
         # TODO these rules are generated, this ignores should be fixed in the code
         # for src/Schema/TestCase.php

src/Persistence/GenericPlatform.php

@@ -22,6 +22,7 @@ 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/BinaryTypeCompatibilityTypecastTrait.php

@@ -42,12 +42,19 @@ private function binaryTypeValueDecode(string $value): string
             throw new Exception('Unexpected binary value crc');
         }
 
-        return hex2bin($hex);
+        $res = hex2bin($hex);
+        if ($this->binaryTypeValueIsEncoded($res)) {
+            throw new Exception('Unexpected double encoded binary value');
+        }
+
+        return $res;
     }
 
     private function binaryTypeIsEncodeNeeded(Type $type): bool
     {
-        // TODO PostgreSQL tests fail without binary compatibility typecast
+        // binary values for PostgreSQL and MSSQL databases are stored natively, but we need
+        // to encode first to hold the binary type info for PDO parameter type binding
+
         $platform = $this->getDatabasePlatform();
         if ($platform instanceof PostgreSQL94Platform
             || $platform instanceof SQLServer2012Platform
@@ -76,7 +83,11 @@ public function typecastLoadField(Field $field, $value)
         $value = parent::typecastLoadField($field, $value);
 
         if ($value !== null && $this->binaryTypeIsEncodeNeeded($field->getTypeObject())) {
-            $value = $this->binaryTypeValueDecode($value);
+            // always decode for Oracle platform to assert the value is always encoded,
+            // on other platforms, binary values are stored natively
+            if ($this->getDatabasePlatform() instanceof OraclePlatform || $this->binaryTypeValueIsEncoded($value)) {
+                $value = $this->binaryTypeValueDecode($value);
+            }
         }
 
         return $value;

src/Persistence/Sql/Connection.php

@@ -132,22 +132,22 @@ public static function connect($dsn, $user = null, $password = null, $args = [])
         // If it's already PDO or DbalConnection object, then we simply use it
         if ($dsn instanceof \PDO) {
             $connectionClass = self::resolveConnectionClass($dsn->getAttribute(\PDO::ATTR_DRIVER_NAME));
-            $connectionArg = $connectionClass::connectDbalConnection(['pdo' => $dsn]);
+            $dbalConnection = $connectionClass::connectDbalConnection(['pdo' => $dsn]);
         } elseif ($dsn instanceof DbalConnection) {
             /** @var \PDO */
             $pdo = self::isComposerDbal2x()
                 ? $dsn->getWrappedConnection()
                 : $dsn->getWrappedConnection()->getWrappedConnection();
             $connectionClass = self::resolveConnectionClass($pdo->getAttribute(\PDO::ATTR_DRIVER_NAME));
-            $connectionArg = $dsn;
+            $dbalConnection = $dsn;
         } else {
             $dsn = static::normalizeDsn($dsn, $user, $password);
             $connectionClass = self::resolveConnectionClass($dsn['driverSchema']);
-            $connectionArg = $connectionClass::connectDbalConnection($dsn);
+            $dbalConnection = $connectionClass::connectDbalConnection($dsn);
         }
 
         return new $connectionClass(array_merge([
-            'connection' => $connectionArg,
+            'connection' => $dbalConnection,
         ], $args));
     }
 
@@ -241,8 +241,13 @@ protected static function connectDbalConnection(array $dsn)
             \Closure::bind(function () use ($pdoConnection, $pdo): void {
                 $pdoConnection->connection = $pdo;
             }, null, \Doctrine\DBAL\Driver\PDO\Connection::class)();
+            $pdoAttrDriverName = $pdo->getAttribute(\PDO::ATTR_DRIVER_NAME);
+            if ($pdoAttrDriverName === 'sqlsrv') {
+                $pdoConnection = new \Doctrine\DBAL\Driver\PDO\SQLSrv\Connection($pdoConnection);
+            }
+
             $dbalConnection = DriverManager::getConnection([
-                'driver' => 'pdo_' . $pdo->getAttribute(\PDO::ATTR_DRIVER_NAME),
+                'driver' => 'pdo_' . $pdoAttrDriverName,
             ], null, (static::class)::createDbalEventManager());
             \Closure::bind(function () use ($dbalConnection, $pdoConnection): void {
                 $dbalConnection->_conn = $pdoConnection;

src/Persistence/Sql/Expression.php

@@ -5,11 +5,13 @@
 namespace Atk4\Data\Persistence\Sql;
 
 use Atk4\Core\WarnDynamicPropertyTrait;
+use Atk4\Data\Persistence\Sql as SqlPersistence;
 use Doctrine\DBAL\Connection as DbalConnection;
 use Doctrine\DBAL\Exception as DbalException;
 use Doctrine\DBAL\ParameterType;
 use Doctrine\DBAL\Platforms\OraclePlatform;
 use Doctrine\DBAL\Platforms\PostgreSQL94Platform;
+use Doctrine\DBAL\Platforms\SQLServer2012Platform;
 use Doctrine\DBAL\Result as DbalResult;
 
 /**
@@ -528,6 +530,15 @@ public function execute(object $connection = null): object
                         $type = ParameterType::STRING;
                     } elseif (is_string($val)) {
                         $type = ParameterType::STRING;
+
+                        if ($this->connection->getDatabasePlatform() instanceof PostgreSQL94Platform
+                            || $this->connection->getDatabasePlatform() instanceof SQLServer2012Platform) {
+                            $dummySqlPersistence = new SqlPersistence($this->connection);
+                            if (\Closure::bind(fn () => $dummySqlPersistence->binaryTypeValueIsEncoded($val), null, SqlPersistence::class)()) {
+                                $val = \Closure::bind(fn () => $dummySqlPersistence->binaryTypeValueDecode($val), null, SqlPersistence::class)();
+                                $type = ParameterType::BINARY;
+                            }
+                        }
                     } elseif (is_resource($val)) {
                         throw new Exception('Resource type is not supported, set value as string instead');
                     } else {

src/Persistence/Sql/Mssql/PlatformTrait.php

@@ -6,19 +6,6 @@
 
 trait PlatformTrait
 {
-    // SQL Server database requires explicit conversion when using binary column,
-    // workaround by using a standard non-binary column with custom encoding/typecast
-
-    protected function getBinaryTypeDeclarationSQLSnippet($length, $fixed)
-    {
-        return $this->getVarcharTypeDeclarationSQLSnippet($length, $fixed);
-    }
-
-    public function getBlobTypeDeclarationSQL(array $column)
-    {
-        return $this->getClobTypeDeclarationSQL($column);
-    }
-
     // remove once https://github.com/doctrine/dbal/pull/4987 is fixed
     // and also $this->markDoctrineTypeCommented('text') below
     public function getClobTypeDeclarationSQL(array $column)
@@ -32,8 +19,6 @@ protected function initializeCommentedDoctrineTypes()
     {
         parent::initializeCommentedDoctrineTypes();
 
-        $this->markDoctrineTypeCommented('binary');
-        $this->markDoctrineTypeCommented('blob');
         $this->markDoctrineTypeCommented('text');
     }
 

tests/Schema/ModelTest.php

@@ -7,6 +7,7 @@
 use Atk4\Data\Model;
 use Atk4\Data\Schema\TestCase;
 use Doctrine\DBAL\Platforms\OraclePlatform;
+use Doctrine\DBAL\Platforms\SQLServer2012Platform;
 
 class ModelTest extends TestCase
 {
@@ -186,7 +187,7 @@ private function makePseudoRandomString(bool $isBinary, int $lengthBytes): strin
     public function testCharacterTypeFieldLong(string $type, bool $isBinary, int $lengthBytes): void
     {
         if ($this->getDatabasePlatform() instanceof OraclePlatform) {
-            $lengthBytes = min($lengthBytes, 500);
+            $lengthBytes = min($lengthBytes, $type === 'binary' ? 100 : 500);
         }
 
         $str = $this->makePseudoRandomString($isBinary, $lengthBytes);
@@ -200,7 +201,11 @@ public function testCharacterTypeFieldLong(string $type, bool $isBinary, int $le
 
         $this->createMigrator($model)->create();
 
-        $model->import([['v' => $str . ($isBinary ? "\0" : '.')]]);
+        $model->import([['v' => $str . (
+            // MSSQL database ignores trailing \0 characters even with binary comparison
+            // https://dba.stackexchange.com/questions/48660/comparing-binary-0x-and-0x00-turns-out-to-be-equal-on-sql-server
+            $isBinary ? $this->getDatabasePlatform() instanceof SQLServer2012Platform ? ' ' : "\0" : '.'
+        )]]);
         $model->import([['v' => $str]]);
 
         $model->addCondition('v', $str);
@@ -218,7 +223,7 @@ public function providerCharacterTypeFieldLongData(): array
     {
         return [
             ['string', false, 250],
-            ['binary', true, 100],
+            ['binary', true, 250],
             ['text', false, 256 * 1024],
             ['blob', true, 256 * 1024],
         ];