diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

bug #38605 [DoctrinBridge] make Uid types stricter (nicolas-grekas)

Browse Source 
This PR was merged into the 5.x branch.

Discussion
----------

[DoctrinBridge] make Uid types stricter

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Reviewing #38600 made me realize we don't need to deal with converting strings to db values.
We should only support converting actual `AbstractUid` instances in the DB.
Also, the binary types should *not* extend `GuidType`.

Commits
-------

ba31d0ee59 [DoctrinBridge] make Uid types stricter
This commit is contained in:
Fabien Potencier 2020-10-17 07:39:20 +02:00
commit fdf9a43433
6 changed files with 78 additions and 125 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
src/Symfony/Bridge/Doctrine/Tests/Types/UlidBinaryTypeTest.php
View File

@ -52,24 +52,18 @@ class UlidBinaryTypeTest extends TestCase
$this->assertEquals($expected, $actual); $this->assertEquals($expected, $actual);
} }
public function testStringUlidConvertsToDatabaseValue() public function testNotSupportedStringUlidConversionToDatabaseValue()
{
$expected = Ulid::fromString(self::DUMMY_ULID)->toBinary();
$actual = $this->type->convertToDatabaseValue(self::DUMMY_ULID, $this->platform);
$this->assertEquals($expected, $actual);
}
public function testInvalidUlidConversionForDatabaseValue()
{ {
$this->expectException(ConversionException::class); $this->expectException(ConversionException::class);
$this->type->convertToDatabaseValue('abcdefg', $this->platform); $this->type->convertToDatabaseValue(self::DUMMY_ULID, $this->platform);
} }
public function testNotSupportedTypeConversionForDatabaseValue() public function testNotSupportedTypeConversionForDatabaseValue()
{ {
$this->assertNull($this->type->convertToDatabaseValue(new \stdClass(), $this->platform)); $this->expectException(ConversionException::class);
$this->type->convertToDatabaseValue(new \stdClass(), $this->platform);
} }
public function testNullConversionForDatabaseValue() public function testNullConversionForDatabaseValue()

40
src/Symfony/Bridge/Doctrine/Tests/Types/UlidTypeTest.php
View File

@ -44,7 +44,7 @@ final class UlidTypeTest extends TestCase
$this->type = Type::getType('ulid'); $this->type = Type::getType('ulid');
} }
public function testUlidConvertsToDatabaseValue(): void public function testUlidConvertsToDatabaseValue()
{ {
$ulid = Ulid::fromString(self::DUMMY_ULID); $ulid = Ulid::fromString(self::DUMMY_ULID);
@ -54,7 +54,7 @@ final class UlidTypeTest extends TestCase
$this->assertEquals($expected, $actual); $this->assertEquals($expected, $actual);
} }
public function testUlidInterfaceConvertsToDatabaseValue(): void public function testUlidInterfaceConvertsToDatabaseValue()
{ {
$ulid = $this->createMock(AbstractUid::class); $ulid = $this->createMock(AbstractUid::class);
@ -68,34 +68,26 @@ final class UlidTypeTest extends TestCase
$this->assertEquals('foo', $actual); $this->assertEquals('foo', $actual);
} }
public function testUlidStringConvertsToDatabaseValue(): void public function testNotSupportedUlidStringConversionToDatabaseValue()
{
$actual = $this->type->convertToDatabaseValue(self::DUMMY_ULID, $this->platform);
$ulid = Ulid::fromString(self::DUMMY_ULID);
$expected = $ulid->toRfc4122();
$this->assertEquals($expected, $actual);
}
public function testInvalidUlidConversionForDatabaseValue(): void
{ {
$this->expectException(ConversionException::class); $this->expectException(ConversionException::class);
$this->type->convertToDatabaseValue('abcdefg', $this->platform); $this->type->convertToDatabaseValue(self::DUMMY_ULID, $this->platform);
} }
public function testNotSupportedTypeConversionForDatabaseValue() public function testNotSupportedTypeConversionForDatabaseValue()
{ {
$this->assertNull($this->type->convertToDatabaseValue(new \stdClass(), $this->platform)); $this->expectException(ConversionException::class);
$this->type->convertToDatabaseValue(new \stdClass(), $this->platform);
} }
public function testNullConversionForDatabaseValue(): void public function testNullConversionForDatabaseValue()
{ {
$this->assertNull($this->type->convertToDatabaseValue(null, $this->platform)); $this->assertNull($this->type->convertToDatabaseValue(null, $this->platform));
} }
public function testUlidInterfaceConvertsToPHPValue(): void public function testUlidInterfaceConvertsToPHPValue()
{ {
$ulid = $this->createMock(AbstractUid::class); $ulid = $this->createMock(AbstractUid::class);
$actual = $this->type->convertToPHPValue($ulid, $this->platform); $actual = $this->type->convertToPHPValue($ulid, $this->platform);
@ -103,7 +95,7 @@ final class UlidTypeTest extends TestCase
$this->assertSame($ulid, $actual); $this->assertSame($ulid, $actual);
} }
public function testUlidConvertsToPHPValue(): void public function testUlidConvertsToPHPValue()
{ {
$ulid = $this->type->convertToPHPValue(self::DUMMY_ULID, $this->platform); $ulid = $this->type->convertToPHPValue(self::DUMMY_ULID, $this->platform);
@ -111,36 +103,36 @@ final class UlidTypeTest extends TestCase
$this->assertEquals(self::DUMMY_ULID, $ulid->__toString()); $this->assertEquals(self::DUMMY_ULID, $ulid->__toString());
} }
public function testInvalidUlidConversionForPHPValue(): void public function testInvalidUlidConversionForPHPValue()
{ {
$this->expectException(ConversionException::class); $this->expectException(ConversionException::class);
$this->type->convertToPHPValue('abcdefg', $this->platform); $this->type->convertToPHPValue('abcdefg', $this->platform);
} }
public function testNullConversionForPHPValue(): void public function testNullConversionForPHPValue()
{ {
$this->assertNull($this->type->convertToPHPValue(null, $this->platform)); $this->assertNull($this->type->convertToPHPValue(null, $this->platform));
} }
public function testReturnValueIfUlidForPHPValue(): void public function testReturnValueIfUlidForPHPValue()
{ {
$ulid = new Ulid(); $ulid = new Ulid();
$this->assertSame($ulid, $this->type->convertToPHPValue($ulid, $this->platform)); $this->assertSame($ulid, $this->type->convertToPHPValue($ulid, $this->platform));
} }
public function testGetName(): void public function testGetName()
{ {
$this->assertEquals('ulid', $this->type->getName()); $this->assertEquals('ulid', $this->type->getName());
} }
public function testGetGuidTypeDeclarationSQL(): void public function testGetGuidTypeDeclarationSQL()
{ {
$this->assertEquals('DUMMYVARCHAR()', $this->type->getSqlDeclaration(['length' => 36], $this->platform)); $this->assertEquals('DUMMYVARCHAR()', $this->type->getSqlDeclaration(['length' => 36], $this->platform));
} }
public function testRequiresSQLCommentHint(): void public function testRequiresSQLCommentHint()
{ {
$this->assertTrue($this->type->requiresSQLCommentHint($this->platform)); $this->assertTrue($this->type->requiresSQLCommentHint($this->platform));
} }

18
src/Symfony/Bridge/Doctrine/Tests/Types/UuidBinaryTypeTest.php
View File

@ -52,21 +52,11 @@ class UuidBinaryTypeTest extends TestCase
$this->assertEquals($expected, $actual); $this->assertEquals($expected, $actual);
} }
public function testStringUuidConvertsToDatabaseValue() public function testNotSupportedStringUuidConversionToDatabaseValue()
{
$uuid = self::DUMMY_UUID;
$expected = uuid_parse(self::DUMMY_UUID);
$actual = $this->type->convertToDatabaseValue($uuid, $this->platform);
$this->assertEquals($expected, $actual);
}
public function testInvalidUuidConversionForDatabaseValue()
{ {
$this->expectException(ConversionException::class); $this->expectException(ConversionException::class);
$this->type->convertToDatabaseValue('abcdefg', $this->platform); $this->type->convertToDatabaseValue(self::DUMMY_UUID, $this->platform);
} }
public function testNullConversionForDatabaseValue() public function testNullConversionForDatabaseValue()
@ -90,7 +80,9 @@ class UuidBinaryTypeTest extends TestCase
public function testNotSupportedTypeConversionForDatabaseValue() public function testNotSupportedTypeConversionForDatabaseValue()
{ {
$this->assertNull($this->type->convertToDatabaseValue(new \stdClass(), $this->platform)); $this->expectException(ConversionException::class);
$this->type->convertToDatabaseValue(new \stdClass(), $this->platform);
} }
public function testNullConversionForPHPValue() public function testNullConversionForPHPValue()

37
src/Symfony/Bridge/Doctrine/Tests/Types/UuidTypeTest.php
View File

@ -44,7 +44,7 @@ final class UuidTypeTest extends TestCase
$this->type = Type::getType('uuid'); $this->type = Type::getType('uuid');
} }
public function testUuidConvertsToDatabaseValue(): void public function testUuidConvertsToDatabaseValue()
{ {
$uuid = Uuid::fromString(self::DUMMY_UUID); $uuid = Uuid::fromString(self::DUMMY_UUID);
@ -54,7 +54,7 @@ final class UuidTypeTest extends TestCase
$this->assertEquals($expected, $actual); $this->assertEquals($expected, $actual);
} }
public function testUuidInterfaceConvertsToDatabaseValue(): void public function testUuidInterfaceConvertsToDatabaseValue()
{ {
$uuid = $this->createMock(AbstractUid::class); $uuid = $this->createMock(AbstractUid::class);
@ -68,31 +68,26 @@ final class UuidTypeTest extends TestCase
$this->assertEquals('foo', $actual); $this->assertEquals('foo', $actual);
} }
public function testUuidStringConvertsToDatabaseValue(): void public function testNotSupportedUuidStringConversionToDatabaseValue()
{
$actual = $this->type->convertToDatabaseValue(self::DUMMY_UUID, $this->platform);
$this->assertEquals(self::DUMMY_UUID, $actual);
}
public function testInvalidUuidConversionForDatabaseValue(): void
{ {
$this->expectException(ConversionException::class); $this->expectException(ConversionException::class);
$this->type->convertToDatabaseValue('abcdefg', $this->platform); $this->type->convertToDatabaseValue(self::DUMMY_UUID, $this->platform);
} }
public function testNotSupportedTypeConversionForDatabaseValue() public function testNotSupportedTypeConversionForDatabaseValue()
{ {
$this->assertNull($this->type->convertToDatabaseValue(new \stdClass(), $this->platform)); $this->expectException(ConversionException::class);
$this->type->convertToDatabaseValue(new \stdClass(), $this->platform);
} }
public function testNullConversionForDatabaseValue(): void public function testNullConversionForDatabaseValue()
{ {
$this->assertNull($this->type->convertToDatabaseValue(null, $this->platform)); $this->assertNull($this->type->convertToDatabaseValue(null, $this->platform));
} }
public function testUuidInterfaceConvertsToPHPValue(): void public function testUuidInterfaceConvertsToPHPValue()
{ {
$uuid = $this->createMock(AbstractUid::class); $uuid = $this->createMock(AbstractUid::class);
$actual = $this->type->convertToPHPValue($uuid, $this->platform); $actual = $this->type->convertToPHPValue($uuid, $this->platform);
@ -100,7 +95,7 @@ final class UuidTypeTest extends TestCase
$this->assertSame($uuid, $actual); $this->assertSame($uuid, $actual);
} }
public function testUuidConvertsToPHPValue(): void public function testUuidConvertsToPHPValue()
{ {
$uuid = $this->type->convertToPHPValue(self::DUMMY_UUID, $this->platform); $uuid = $this->type->convertToPHPValue(self::DUMMY_UUID, $this->platform);
@ -108,36 +103,36 @@ final class UuidTypeTest extends TestCase
$this->assertEquals(self::DUMMY_UUID, $uuid->__toString()); $this->assertEquals(self::DUMMY_UUID, $uuid->__toString());
} }
public function testInvalidUuidConversionForPHPValue(): void public function testInvalidUuidConversionForPHPValue()
{ {
$this->expectException(ConversionException::class); $this->expectException(ConversionException::class);
$this->type->convertToPHPValue('abcdefg', $this->platform); $this->type->convertToPHPValue('abcdefg', $this->platform);
} }
public function testNullConversionForPHPValue(): void public function testNullConversionForPHPValue()
{ {
$this->assertNull($this->type->convertToPHPValue(null, $this->platform)); $this->assertNull($this->type->convertToPHPValue(null, $this->platform));
} }
public function testReturnValueIfUuidForPHPValue(): void public function testReturnValueIfUuidForPHPValue()
{ {
$uuid = Uuid::v4(); $uuid = Uuid::v4();
$this->assertSame($uuid, $this->type->convertToPHPValue($uuid, $this->platform)); $this->assertSame($uuid, $this->type->convertToPHPValue($uuid, $this->platform));
} }
public function testGetName(): void public function testGetName()
{ {
$this->assertEquals('uuid', $this->type->getName()); $this->assertEquals('uuid', $this->type->getName());
} }
public function testGetGuidTypeDeclarationSQL(): void public function testGetGuidTypeDeclarationSQL()
{ {
$this->assertEquals('DUMMYVARCHAR()', $this->type->getSqlDeclaration(['length' => 36], $this->platform)); $this->assertEquals('DUMMYVARCHAR()', $this->type->getSqlDeclaration(['length' => 36], $this->platform));
} }
public function testRequiresSQLCommentHint(): void public function testRequiresSQLCommentHint()
{ {
$this->assertTrue($this->type->requiresSQLCommentHint($this->platform)); $this->assertTrue($this->type->requiresSQLCommentHint($this->platform));
} }

44
src/Symfony/Bridge/Doctrine/Types/AbstractBinaryUidType.php
View File

@ -13,21 +13,19 @@ namespace Symfony\Bridge\Doctrine\Types;
use Doctrine\DBAL\Platforms\AbstractPlatform; use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\ConversionException; use Doctrine\DBAL\Types\ConversionException;
use Doctrine\DBAL\Types\GuidType; use Doctrine\DBAL\Types\Type;
use Symfony\Component\Uid\AbstractUid; use Symfony\Component\Uid\AbstractUid;
abstract class AbstractBinaryUidType extends GuidType abstract class AbstractBinaryUidType extends Type
{ {
abstract protected function getUidClass(): string; abstract protected function getUidClass(): string;
public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform): string public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform): string
{ {
return $platform->getBinaryTypeDeclarationSQL( return $platform->getBinaryTypeDeclarationSQL([
[ 'length' => '16',
'length' => '16', 'fixed' => true,
'fixed' => true, ]);
]
);
} }
/** /**
@ -37,21 +35,19 @@ abstract class AbstractBinaryUidType extends GuidType
*/ */
public function convertToPHPValue($value, AbstractPlatform $platform): ?AbstractUid public function convertToPHPValue($value, AbstractPlatform $platform): ?AbstractUid
{ {
if (null === $value || '' === $value) { if ($value instanceof AbstractUid || null === $value) {
return null;
}
if ($value instanceof AbstractUid) {
return $value; return $value;
} }
try { if (!\is_string($value)) {
$uuid = $this->getUidClass()::fromString($value); throw ConversionException::conversionFailedInvalidType($value, $this->getName(), ['null', 'string', AbstractUid::class]);
} catch (\InvalidArgumentException $e) {
throw ConversionException::conversionFailed($value, $this->getName());
} }
return $uuid; try {
return $this->getUidClass()::fromString($value);
} catch (\InvalidArgumentException $e) {
throw ConversionException::conversionFailed($value, $this->getName(), $e);
}
} }
/** /**
@ -61,23 +57,15 @@ abstract class AbstractBinaryUidType extends GuidType
*/ */
public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string
{ {
if (null === $value || '' === $value) {
return null;
}
if ($value instanceof AbstractUid) { if ($value instanceof AbstractUid) {
return $value->toBinary(); return $value->toBinary();
} }
if (!\is_string($value) && !(\is_object($value) && method_exists($value, '__toString'))) { if (null === $value) {
return null; return null;
} }
try { throw ConversionException::conversionFailedInvalidType($value, $this->getName(), ['null', AbstractUid::class]);
return $this->getUidClass()::fromString((string) $value)->toBinary();
} catch (\InvalidArgumentException $e) {
throw ConversionException::conversionFailed($value, $this->getName());
}
} }
/** /**

48
src/Symfony/Bridge/Doctrine/Types/AbstractUidType.php
View File

@ -13,13 +13,21 @@ namespace Symfony\Bridge\Doctrine\Types;
use Doctrine\DBAL\Platforms\AbstractPlatform; use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\ConversionException; use Doctrine\DBAL\Types\ConversionException;
use Doctrine\DBAL\Types\GuidType; use Doctrine\DBAL\Types\Type;
use Symfony\Component\Uid\AbstractUid; use Symfony\Component\Uid\AbstractUid;
abstract class AbstractUidType extends GuidType abstract class AbstractUidType extends Type
{ {
abstract protected function getUidClass(): string; abstract protected function getUidClass(): string;
/**
* {@inheritdoc}
*/
public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
{
return $platform->getGuidTypeDeclarationSQL($column);
}
/** /**
* {@inheritdoc} * {@inheritdoc}
* *
@ -27,21 +35,19 @@ abstract class AbstractUidType extends GuidType
*/ */
public function convertToPHPValue($value, AbstractPlatform $platform): ?AbstractUid public function convertToPHPValue($value, AbstractPlatform $platform): ?AbstractUid
{ {
if (null === $value || '' === $value) { if ($value instanceof AbstractUid || null === $value) {
return null;
}
if ($value instanceof AbstractUid) {
return $value; return $value;
} }
try { if (!\is_string($value)) {
$uuid = $this->getUidClass()::fromString($value); throw ConversionException::conversionFailedInvalidType($value, $this->getName(), ['null', 'string', AbstractUid::class]);
} catch (\InvalidArgumentException $e) {
throw ConversionException::conversionFailed($value, $this->getName());
} }
return $uuid; try {
return $this->getUidClass()::fromString($value);
} catch (\InvalidArgumentException $e) {
throw ConversionException::conversionFailed($value, $this->getName(), $e);
}
} }
/** /**
@ -51,29 +57,15 @@ abstract class AbstractUidType extends GuidType
*/ */
public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string
{ {
if (null === $value || '' === $value) {
return null;
}
if ($value instanceof AbstractUid) { if ($value instanceof AbstractUid) {
return $value->toRfc4122(); return $value->toRfc4122();
} }
if (!\is_string($value) && !(\is_object($value) && method_exists($value, '__toString'))) { if (null === $value) {
return null; return null;
} }
if ($this->getUidClass()::isValid((string) $value)) { throw ConversionException::conversionFailedInvalidType($value, $this->getName(), ['null', AbstractUid::class]);
try {
$uuid = $this->getUidClass()::fromString($value);
return $uuid->toRfc4122();
} catch (\InvalidArgumentException $e) {
throw ConversionException::conversionFailed($value, $this->getName());
}
}
throw ConversionException::conversionFailed($value, $this->getName());
} }
/** /**