Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[DBAL-423] Optimize non-native GUID type declaration #465

Closed
wants to merge 2 commits into from

4 participants

@deeky666
Collaborator

Currently non-native GUID type is mapped to VARCHAR(255) which is not effective as GUID always has a fixed length of 36 characters. Therefore it should be mapped to CHAR(36) instead.

@doctrinebot
Collaborator

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-731

We use Jira to track the state of pull requests and the versions they got
included in.

@beberlei
Owner

This is a BC break

@deeky666
Collaborator
@beberlei
Owner

Because it requires database migrations for users that already use the GUID type to avoid having outstanding schema updates.

@deeky666
Collaborator
@beberlei
Owner

We only fix issues on comparator and try to introduce new features so that the schema doesnt need updates. We cant fix this here, same as we cannot change the Object and ArrayTypes to encode their serialize input with base64 in the database to prevent problems with th enullbytes.

@deeky666
Collaborator

@beberlei IMO this is NOT a BC break because it will not generate useless update statements when using the schema tool. The reason for this is that it does not change the Doctrine\DBAL\Schema\Column definition and therefore the comparator will not detect any changes. It will only have an effect if either a new guid type column is created, or an exisiting column is changed to a guid type column.

@beberlei
Owner

@deeky666 Not sure tbh, it still starts suggestion db changes, which sucks :(

@deeky666
Collaborator

@beberlei Did you try that actually? I'm pretty sure that it does NOT affect existing schemas and will NOT generate changes with the schema tool for the reasons I explained earlier. Or am I getting you wrong here?

@deeky666
Collaborator

@beberlei okay rebased this PR and fixed the comparator to NOT detect schema changes for GUID type columns. The added tests prove this, length and fixed options are ignored by the comparator now for GUID type columns so CHAR(36) vs. VARCHAR(255) will be the same for GUID now (preserves BC). Have another thought about this?

@Ocramius Ocramius self-assigned this
@Ocramius Ocramius closed this in c8b58d9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
5 lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
@@ -297,7 +297,7 @@ public function getBinaryTypeDeclarationSQL(array $field)
/**
* Returns the SQL snippet to declare a GUID/UUID field.
*
- * By default this maps directly to a VARCHAR and only maps to more
+ * By default this maps directly to a CHAR(36) and only maps to more
* special datatypes when the underlying databases support this datatype.
*
* @param array $field
@@ -306,6 +306,9 @@ public function getBinaryTypeDeclarationSQL(array $field)
*/
public function getGuidTypeDeclarationSQL(array $field)
{
+ $field['length'] = 36;
+ $field['fixed'] = true;
+
return $this->getVarcharTypeDeclarationSQL($field);
}
View
4 lib/Doctrine/DBAL/Schema/Comparator.php
@@ -399,7 +399,9 @@ public function diffColumn(Column $column1, Column $column2)
$changedProperties[] = 'default';
}
- if ($properties1['type'] instanceof Types\StringType || $properties1['type'] instanceof Types\BinaryType) {
+ if (($properties1['type'] instanceof Types\StringType && ! $properties1['type'] instanceof Types\GuidType) ||
+ $properties1['type'] instanceof Types\BinaryType
+ ) {
// check if value of length is set at all, default value assumed otherwise.
$length1 = $properties1['length'] ?: 255;
$length2 = $properties2['length'] ?: 255;
View
20 tests/Doctrine/Tests/DBAL/Functional/Schema/MySqlSchemaManagerTest.php
@@ -259,4 +259,24 @@ public function testListLobTypeColumns()
$platform->getBlobTypeDeclarationSQL($onlineColumns['col_longblob']->toArray())
);
}
+
+ /**
+ * @group DBAL-423
+ */
+ public function testDiffListGuidTableColumn()
+ {
+ $offlineTable = new Table('list_guid_table_column');
+ $offlineTable->addColumn('col_guid', 'guid');
+
+ $this->_sm->dropAndCreateTable($offlineTable);
+
+ $onlineTable = $this->_sm->listTableDetails('list_guid_table_column');
+
+ $comparator = new Comparator();
+
+ $this->assertFalse(
+ $comparator->diffTable($offlineTable, $onlineTable),
+ "No differences should be detected with the offline vs online schema."
+ );
+ }
}
View
8 tests/Doctrine/Tests/DBAL/Platforms/AbstractMySQLPlatformTestCase.php
@@ -596,4 +596,12 @@ protected function getQuotedAlterTableChangeColumnLengthSQL()
"CHANGE `select` `select` VARCHAR(255) NOT NULL COMMENT 'Reserved keyword 3'"
);
}
+
+ /**
+ * @group DBAL-423
+ */
+ public function testReturnsGuidTypeDeclarationSQL()
+ {
+ $this->assertSame('CHAR(36)', $this->_platform->getGuidTypeDeclarationSQL(array()));
+ }
}
View
10 tests/Doctrine/Tests/DBAL/Platforms/AbstractPlatformTestCase.php
@@ -946,4 +946,14 @@ public function testQuoteStringLiteral()
$this->_platform->quoteStringLiteral($c)
);
}
+
+ /**
+ * @group DBAL-423
+ *
+ * @expectedException \Doctrine\DBAL\DBALException
+ */
+ public function testReturnsGuidTypeDeclarationSQL()
+ {
+ $this->_platform->getGuidTypeDeclarationSQL(array());
+ }
}
View
8 tests/Doctrine/Tests/DBAL/Platforms/AbstractPostgreSqlPlatformTestCase.php
@@ -664,4 +664,12 @@ public function testGetNullCommentOnColumnSQL()
$this->_platform->getCommentOnColumnSQL('mytable', 'id', null)
);
}
+
+ /**
+ * @group DBAL-423
+ */
+ public function testReturnsGuidTypeDeclarationSQL()
+ {
+ $this->assertSame('UUID', $this->_platform->getGuidTypeDeclarationSQL(array()));
+ }
}
View
14 tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php
@@ -255,16 +255,16 @@ public function testModifyLimitQueryWithExtraLongQuery()
$query.= 'WHERE (table1.column1 = table2.column2) AND (table1.column1 = table3.column3) AND (table1.column1 = table4.column4) AND (table1.column1 = table5.column5) AND (table1.column1 = table6.column6) AND (table1.column1 = table7.column7) AND (table1.column1 = table8.column8) AND (table2.column2 = table3.column3) AND (table2.column2 = table4.column4) AND (table2.column2 = table5.column5) AND (table2.column2 = table6.column6) ';
$query.= 'AND (table2.column2 = table7.column7) AND (table2.column2 = table8.column8) AND (table3.column3 = table4.column4) AND (table3.column3 = table5.column5) AND (table3.column3 = table6.column6) AND (table3.column3 = table7.column7) AND (table3.column3 = table8.column8) AND (table4.column4 = table5.column5) AND (table4.column4 = table6.column6) AND (table4.column4 = table7.column7) AND (table4.column4 = table8.column8) ';
$query.= 'AND (table5.column5 = table6.column6) AND (table5.column5 = table7.column7) AND (table5.column5 = table8.column8) AND (table6.column6 = table7.column7) AND (table6.column6 = table8.column8) AND (table7.column7 = table8.column8)';
-
+
$sql = $this->_platform->modifyLimitQuery($query, 10);
-
+
$expected = 'SELECT * FROM (SELECT table1.column1, table2.column2, table3.column3, table4.column4, table5.column5, table6.column6, table7.column7, table8.column8, ROW_NUMBER() OVER (ORDER BY (SELECT 0)) AS doctrine_rownum ';
$expected.= 'FROM table1, table2, table3, table4, table5, table6, table7, table8 ';
$expected.= 'WHERE (table1.column1 = table2.column2) AND (table1.column1 = table3.column3) AND (table1.column1 = table4.column4) AND (table1.column1 = table5.column5) AND (table1.column1 = table6.column6) AND (table1.column1 = table7.column7) AND (table1.column1 = table8.column8) AND (table2.column2 = table3.column3) AND (table2.column2 = table4.column4) ';
$expected.= 'AND (table2.column2 = table5.column5) AND (table2.column2 = table6.column6) AND (table2.column2 = table7.column7) AND (table2.column2 = table8.column8) AND (table3.column3 = table4.column4) AND (table3.column3 = table5.column5) AND (table3.column3 = table6.column6) AND (table3.column3 = table7.column7) AND (table3.column3 = table8.column8) AND (table4.column4 = table5.column5) AND (table4.column4 = table6.column6) ';
$expected.= 'AND (table4.column4 = table7.column7) AND (table4.column4 = table8.column8) AND (table5.column5 = table6.column6) AND (table5.column5 = table7.column7) AND (table5.column5 = table8.column8) AND (table6.column6 = table7.column7) AND (table6.column6 = table8.column8) AND (table7.column7 = table8.column8)) ';
$expected.= 'AS doctrine_tbl WHERE doctrine_rownum BETWEEN 1 AND 10';
-
+
$this->assertEquals($expected, $sql);
}
@@ -920,4 +920,12 @@ protected function getQuotedAlterTableRenameIndexInSchemaSQL()
"EXEC sp_RENAME N'[schema].[table].[foo]', N'[bar]', N'INDEX'",
);
}
+
+ /**
+ * @group DBAL-423
+ */
+ public function testReturnsGuidTypeDeclarationSQL()
+ {
+ $this->assertSame('UNIQUEIDENTIFIER', $this->_platform->getGuidTypeDeclarationSQL(array()));
+ }
}
View
8 tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php
@@ -463,4 +463,12 @@ protected function getQuotedAlterTableRenameIndexInSchemaSQL()
'RENAME INDEX "schema"."foo" TO "bar"',
);
}
+
+ /**
+ * @group DBAL-423
+ */
+ public function testReturnsGuidTypeDeclarationSQL()
+ {
+ $this->assertSame('CHAR(36)', $this->_platform->getGuidTypeDeclarationSQL(array()));
+ }
}
View
8 tests/Doctrine/Tests/DBAL/Platforms/OraclePlatformTest.php
@@ -484,4 +484,12 @@ protected function getQuotedAlterTableRenameIndexInSchemaSQL()
'ALTER INDEX "schema"."foo" RENAME TO "bar"',
);
}
+
+ /**
+ * @group DBAL-423
+ */
+ public function testReturnsGuidTypeDeclarationSQL()
+ {
+ $this->assertSame('CHAR(36)', $this->_platform->getGuidTypeDeclarationSQL(array()));
+ }
}
View
8 tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php
@@ -855,4 +855,12 @@ protected function getQuotedAlterTableRenameIndexInSchemaSQL()
'ALTER INDEX "foo" ON "schema"."table" RENAME TO "bar"',
);
}
+
+ /**
+ * @group DBAL-423
+ */
+ public function testReturnsGuidTypeDeclarationSQL()
+ {
+ $this->assertSame('UNIQUEIDENTIFIER', $this->_platform->getGuidTypeDeclarationSQL(array()));
+ }
}
View
8 tests/Doctrine/Tests/DBAL/Platforms/SqlitePlatformTest.php
@@ -552,4 +552,12 @@ public function testQuotesAlterTableRenameIndexInSchema()
'when used with schemas.'
);
}
+
+ /**
+ * @group DBAL-423
+ */
+ public function testReturnsGuidTypeDeclarationSQL()
+ {
+ $this->assertSame('CHAR(36)', $this->_platform->getGuidTypeDeclarationSQL(array()));
+ }
}
View
15 tests/Doctrine/Tests/DBAL/Schema/ComparatorTest.php
@@ -1107,4 +1107,19 @@ public function testComparesNamespaces()
$this->assertEquals($expected, $comparator->compare($fromSchema, $toSchema));
}
+
+ public function testCompareGuidColumns()
+ {
+ $comparator = new Comparator();
+
+ $column1 = new Column('foo', Type::getType('guid'), array('comment' => 'GUID 1'));
+ $column2 = new Column(
+ 'foo',
+ Type::getType('guid'),
+ array('notnull' => false, 'length' => '36', 'fixed' => true, 'default' => 'NEWID()', 'comment' => 'GUID 2.')
+ );
+
+ $this->assertEquals(array('notnull', 'default', 'comment'), $comparator->diffColumn($column1, $column2));
+ $this->assertEquals(array('notnull', 'default', 'comment'), $comparator->diffColumn($column2, $column1));
+ }
}
Something went wrong with that request. Please try again.