From 51358603bba352e46d56310125d68430342ed26a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 20 Nov 2017 18:23:57 +0100 Subject: [PATCH 1/2] allow creating PRIMARY KEY AUTOINCREMENT fields for sqlite If a PRIMARY KEY field is defined in sqlite without autoincrement then keys might be reused when rows are deleted, explicitly settings autoincrement will prevent this. Autoincrement is not enabled by default due to additional overhead introduced with autoincrement bookkeeping and should only be enabled when the additional uniqueness is required. --- .../DBAL/Platforms/SqlitePlatform.php | 9 ++- .../Schema/SqliteSchemaManagerTest.php | 59 +++++++++++++++++++ 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php b/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php index 8d8630a5378..d261cb6baa9 100644 --- a/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php +++ b/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php @@ -307,9 +307,9 @@ public function getTimeTypeDeclarationSQL(array $fieldDeclaration) */ protected function _getCommonIntegerTypeDeclarationSQL(array $columnDef) { - // sqlite autoincrement is implicit for integer PKs, but not when the field is unsigned + // sqlite autoincrement is only possible for the primary key if ( ! empty($columnDef['autoincrement'])) { - return ''; + return ' PRIMARY KEY AUTOINCREMENT'; } return ! empty($columnDef['unsigned']) ? ' UNSIGNED' : ''; @@ -345,7 +345,10 @@ protected function _getCreateTableSQL($name, array $columns, array $options = [] if (isset($options['primary']) && ! empty($options['primary'])) { $keyColumns = array_unique(array_values($options['primary'])); - $queryFields.= ', PRIMARY KEY('.implode(', ', $keyColumns).')'; + // the primary key is already defined in the column definition for auto increment fields + if (count($keyColumns) > 1 || !$columns[$keyColumns[0]]['autoincrement']) { + $queryFields .= ', PRIMARY KEY(' . implode(', ', $keyColumns) . ')'; + } } if (isset($options['foreignKeys'])) { diff --git a/tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php b/tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php index 3699fb7c7ae..aad50cb6585 100644 --- a/tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php +++ b/tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php @@ -232,4 +232,63 @@ public function getDiffListIntegerAutoincrementTableColumnsData() array('bigint', true, true), ); } + + /** + * @group DBAL-2921 + */ + public function testPrimaryKeyAutoIncrement() + { + $table = new Schema\Table('test_pk_auto_increment'); + $table->addColumn('id', 'integer', [ + 'autoincrement' => true + ]); + $table->addColumn('text', 'text'); + $table->setPrimaryKey(['id']); + $this->_sm->dropAndCreateTable($table); + + $this->_conn->insert('test_pk_auto_increment', ['text' => '1']); + $this->_conn->insert('test_pk_auto_increment', ['text' => '2']); + $this->_conn->insert('test_pk_auto_increment', ['text' => '3']); + + $query = $this->_conn->query('SELECT id FROM test_pk_auto_increment WHERE text = "3"'); + $query->execute(); + $lastUsedIdBeforeDelete = (int)$query->fetchColumn(); + + $this->_conn->query('DELETE FROM test_pk_auto_increment'); + + $this->_conn->insert('test_pk_auto_increment', ['text' => '4']); + + $query = $this->_conn->query('SELECT id FROM test_pk_auto_increment WHERE text = "4"'); + $query->execute(); + $lastUsedIdAfterDelete = (int)$query->fetchColumn(); + + $this->assertGreaterThan($lastUsedIdBeforeDelete, $lastUsedIdAfterDelete); + } + + /** + * @group DBAL-2921 + */ + public function testPrimaryKeyNoAutoIncrement() + { + $table = new Schema\Table('test_pk_auto_increment'); + $table->addColumn('id', 'integer'); + $table->addColumn('text', 'text'); + $table->setPrimaryKey(['id']); + $this->_sm->dropAndCreateTable($table); + + $this->_conn->insert('test_pk_auto_increment', ['text' => '1']); + $this->_conn->insert('test_pk_auto_increment', ['text' => '2']); + $this->_conn->insert('test_pk_auto_increment', ['text' => '3']); + + $this->_conn->query('DELETE FROM test_pk_auto_increment'); + + $this->_conn->insert('test_pk_auto_increment', ['text' => '4']); + + $query = $this->_conn->query('SELECT id FROM test_pk_auto_increment WHERE text = "4"'); + $query->execute(); + $lastUsedIdAfterDelete = (int)$query->fetchColumn(); + + // with an empty table, non autoincrement rowid is always 1 + $this->assertEquals(1, $lastUsedIdAfterDelete); + } } From 245bfb79827e0e15a40ba56727c6dd55ba1bc0aa Mon Sep 17 00:00:00 2001 From: Timo Bakx Date: Fri, 11 May 2018 18:00:30 +0200 Subject: [PATCH 2/2] Updated sqlite unit tests --- .../DBAL/Platforms/SqlitePlatform.php | 31 ++++++++++---- .../SchemaManagerFunctionalTestCase.php | 28 +++++++++++++ .../Schema/SqliteSchemaManagerTest.php | 40 ++----------------- .../DBAL/Platforms/SqlitePlatformTest.php | 36 ++++++++--------- 4 files changed, 73 insertions(+), 62 deletions(-) diff --git a/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php b/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php index d261cb6baa9..60b6905a9c6 100644 --- a/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php +++ b/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php @@ -343,13 +343,7 @@ protected function _getCreateTableSQL($name, array $columns, array $options = [] } } - if (isset($options['primary']) && ! empty($options['primary'])) { - $keyColumns = array_unique(array_values($options['primary'])); - // the primary key is already defined in the column definition for auto increment fields - if (count($keyColumns) > 1 || !$columns[$keyColumns[0]]['autoincrement']) { - $queryFields .= ', PRIMARY KEY(' . implode(', ', $keyColumns) . ')'; - } - } + $queryFields .= $this->getNonAutoincrementPrimaryKeyDefinition($columns, $options); if (isset($options['foreignKeys'])) { foreach ($options['foreignKeys'] as $foreignKey) { @@ -378,6 +372,29 @@ protected function _getCreateTableSQL($name, array $columns, array $options = [] return $query; } + /** + * Generate a PRIMARY KEY definition if no autoincrement value is used + * + * @param string[] $columns + * @param mixed[] $options + */ + private function getNonAutoincrementPrimaryKeyDefinition(array $columns, array $options) : string + { + if (empty($options['primary'])) { + return ''; + } + + $keyColumns = array_unique(array_values($options['primary'])); + + foreach ($keyColumns as $keyColumn) { + if (isset($columns[$keyColumn]['autoincrement']) && ! empty($columns[$keyColumn]['autoincrement'])) { + return ''; + } + } + + return ', PRIMARY KEY(' . implode(', ', $keyColumns) . ')'; + } + /** * {@inheritDoc} */ diff --git a/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php b/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php index 37ce2bf7524..6c5329a0bab 100644 --- a/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php +++ b/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php @@ -1489,4 +1489,32 @@ function (Sequence $sequence) use ($sequenceName) : bool { self::assertFalse($tableDiff); } + + /** + * @group DBAL-2921 + */ + public function testPrimaryKeyAutoIncrement() + { + $table = new Table('test_pk_auto_increment'); + $table->addColumn('id', 'integer', ['autoincrement' => true]); + $table->addColumn('text', 'string'); + $table->setPrimaryKey(['id']); + $this->_sm->dropAndCreateTable($table); + + $this->_conn->insert('test_pk_auto_increment', ['text' => '1']); + + $query = $this->_conn->query('SELECT id FROM test_pk_auto_increment WHERE text = \'1\''); + $query->execute(); + $lastUsedIdBeforeDelete = (int) $query->fetchColumn(); + + $this->_conn->query('DELETE FROM test_pk_auto_increment'); + + $this->_conn->insert('test_pk_auto_increment', ['text' => '2']); + + $query = $this->_conn->query('SELECT id FROM test_pk_auto_increment WHERE text = \'2\''); + $query->execute(); + $lastUsedIdAfterDelete = (int) $query->fetchColumn(); + + $this->assertGreaterThan($lastUsedIdBeforeDelete, $lastUsedIdAfterDelete); + } } diff --git a/tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php b/tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php index aad50cb6585..14efbff3cce 100644 --- a/tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php +++ b/tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php @@ -233,38 +233,6 @@ public function getDiffListIntegerAutoincrementTableColumnsData() ); } - /** - * @group DBAL-2921 - */ - public function testPrimaryKeyAutoIncrement() - { - $table = new Schema\Table('test_pk_auto_increment'); - $table->addColumn('id', 'integer', [ - 'autoincrement' => true - ]); - $table->addColumn('text', 'text'); - $table->setPrimaryKey(['id']); - $this->_sm->dropAndCreateTable($table); - - $this->_conn->insert('test_pk_auto_increment', ['text' => '1']); - $this->_conn->insert('test_pk_auto_increment', ['text' => '2']); - $this->_conn->insert('test_pk_auto_increment', ['text' => '3']); - - $query = $this->_conn->query('SELECT id FROM test_pk_auto_increment WHERE text = "3"'); - $query->execute(); - $lastUsedIdBeforeDelete = (int)$query->fetchColumn(); - - $this->_conn->query('DELETE FROM test_pk_auto_increment'); - - $this->_conn->insert('test_pk_auto_increment', ['text' => '4']); - - $query = $this->_conn->query('SELECT id FROM test_pk_auto_increment WHERE text = "4"'); - $query->execute(); - $lastUsedIdAfterDelete = (int)$query->fetchColumn(); - - $this->assertGreaterThan($lastUsedIdBeforeDelete, $lastUsedIdAfterDelete); - } - /** * @group DBAL-2921 */ @@ -277,16 +245,14 @@ public function testPrimaryKeyNoAutoIncrement() $this->_sm->dropAndCreateTable($table); $this->_conn->insert('test_pk_auto_increment', ['text' => '1']); - $this->_conn->insert('test_pk_auto_increment', ['text' => '2']); - $this->_conn->insert('test_pk_auto_increment', ['text' => '3']); $this->_conn->query('DELETE FROM test_pk_auto_increment'); - $this->_conn->insert('test_pk_auto_increment', ['text' => '4']); + $this->_conn->insert('test_pk_auto_increment', ['text' => '2']); - $query = $this->_conn->query('SELECT id FROM test_pk_auto_increment WHERE text = "4"'); + $query = $this->_conn->query('SELECT id FROM test_pk_auto_increment WHERE text = "2"'); $query->execute(); - $lastUsedIdAfterDelete = (int)$query->fetchColumn(); + $lastUsedIdAfterDelete = (int) $query->fetchColumn(); // with an empty table, non autoincrement rowid is always 1 $this->assertEquals(1, $lastUsedIdAfterDelete); diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SqlitePlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SqlitePlatformTest.php index 69e33420141..151b6cd9993 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/SqlitePlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/SqlitePlatformTest.php @@ -19,7 +19,7 @@ public function createPlatform() public function getGenerateTableSql() { - return 'CREATE TABLE test (id INTEGER NOT NULL, test VARCHAR(255) DEFAULT NULL, PRIMARY KEY(id))'; + return 'CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, test VARCHAR(255) DEFAULT NULL)'; } public function getGenerateTableWithMultiColumnUniqueIndexSql() @@ -65,7 +65,7 @@ public function testPrefersIdentityColumns() public function testIgnoresUnsignedIntegerDeclarationForAutoIncrementalIntegers() { self::assertSame( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getIntegerTypeDeclarationSQL(array('autoincrement' => true, 'unsigned' => true)) ); } @@ -81,11 +81,11 @@ public function testGeneratesTypeDeclarationForTinyIntegers() $this->_platform->getTinyIntTypeDeclarationSQL(array()) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getTinyIntTypeDeclarationSQL(array('autoincrement' => true)) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getTinyIntTypeDeclarationSQL( array('autoincrement' => true, 'primary' => true)) ); @@ -110,15 +110,15 @@ public function testGeneratesTypeDeclarationForSmallIntegers() $this->_platform->getSmallIntTypeDeclarationSQL(array()) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getSmallIntTypeDeclarationSQL(array('autoincrement' => true)) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getTinyIntTypeDeclarationSQL(array('autoincrement' => true, 'unsigned' => true)) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getSmallIntTypeDeclarationSQL( array('autoincrement' => true, 'primary' => true)) ); @@ -143,15 +143,15 @@ public function testGeneratesTypeDeclarationForMediumIntegers() $this->_platform->getMediumIntTypeDeclarationSQL(array()) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getMediumIntTypeDeclarationSQL(array('autoincrement' => true)) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getMediumIntTypeDeclarationSQL(array('autoincrement' => true, 'unsigned' => true)) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getMediumIntTypeDeclarationSQL( array('autoincrement' => true, 'primary' => true)) ); @@ -172,15 +172,15 @@ public function testGeneratesTypeDeclarationForIntegers() $this->_platform->getIntegerTypeDeclarationSQL(array()) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getIntegerTypeDeclarationSQL(array('autoincrement' => true)) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getIntegerTypeDeclarationSQL(array('autoincrement' => true, 'unsigned' => true)) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getIntegerTypeDeclarationSQL( array('autoincrement' => true, 'primary' => true)) ); @@ -205,15 +205,15 @@ public function testGeneratesTypeDeclarationForBigIntegers() $this->_platform->getBigIntTypeDeclarationSQL(array()) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getBigIntTypeDeclarationSQL(array('autoincrement' => true)) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getBigIntTypeDeclarationSQL(array('autoincrement' => true, 'unsigned' => true)) ); self::assertEquals( - 'INTEGER', + 'INTEGER PRIMARY KEY AUTOINCREMENT', $this->_platform->getBigIntTypeDeclarationSQL( array('autoincrement' => true, 'primary' => true)) ); @@ -300,7 +300,7 @@ public function getGenerateAlterTableSql() return array( "CREATE TEMPORARY TABLE __temp__mytable AS SELECT id, bar, bloo FROM mytable", "DROP TABLE mytable", - "CREATE TABLE mytable (id INTEGER NOT NULL, baz VARCHAR(255) DEFAULT 'def' NOT NULL, bloo BOOLEAN DEFAULT '0' NOT NULL, quota INTEGER DEFAULT NULL, PRIMARY KEY(id))", + "CREATE TABLE mytable (id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, baz VARCHAR(255) DEFAULT 'def' NOT NULL, bloo BOOLEAN DEFAULT '0' NOT NULL, quota INTEGER DEFAULT NULL)", "INSERT INTO mytable (id, baz, bloo) SELECT id, bar, bloo FROM __temp__mytable", "DROP TABLE __temp__mytable", "ALTER TABLE mytable RENAME TO userlist", @@ -318,7 +318,7 @@ public function testGenerateTableSqlShouldNotAutoQuotePrimaryKey() $createTableSQL = $this->_platform->getCreateTableSQL($table); self::assertEquals( - 'CREATE TABLE test ("like" INTEGER NOT NULL, PRIMARY KEY("like"))', + 'CREATE TABLE test ("like" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL)', $createTableSQL[0] ); }