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,13 +42,18 @@ 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
     {
-        // values for PostgreSQL are stored natively, but we need to encode first
-        // to hold the binary type info for PDO parameter type binding
+        // 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
@@ -78,8 +83,9 @@ public function typecastLoadField(Field $field, $value)
         $value = parent::typecastLoadField($field, $value);
 
         if ($value !== null && $this->binaryTypeIsEncodeNeeded($field->getTypeObject())) {
-            if (!$this->getDatabasePlatform() instanceof PostgreSQL94Platform
-                || $this->binaryTypeValueIsEncoded($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);
             }
         }

src/Persistence/Sql/Expression.php

@@ -11,6 +11,7 @@
 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;
 
 /**
@@ -530,7 +531,8 @@ public function execute(object $connection = null): object
                     } elseif (is_string($val)) {
                         $type = ParameterType::STRING;
 
-                        if ($this->connection->getDatabasePlatform() instanceof PostgreSQL94Platform) {
+                        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)();

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 darabase 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],
         ];