Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow creating PRIMARY KEY AUTOINCREMENT fields for sqlite (unit tests) #3141

Merged
merged 2 commits into from
May 13, 2018
Merged

allow creating PRIMARY KEY AUTOINCREMENT fields for sqlite (unit tests) #3141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5135860
allow creating PRIMARY KEY AUTOINCREMENT fields for sqlite
icewind1991 Nov 20, 2017
245bfb7
Updated sqlite unit tests
TimoBakx May 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 26 additions & 6 deletions lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' : '';
Expand Down Expand Up @@ -343,10 +343,7 @@ 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).')';
}
$queryFields .= $this->getNonAutoincrementPrimaryKeyDefinition($columns, $options);

if (isset($options['foreignKeys'])) {
foreach ($options['foreignKeys'] as $foreignKey) {
Expand Down Expand Up @@ -375,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 '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you check if it's technically possible to have a PK which consists of multiple columns one or more of which has AUTOINCREMENT? If yes, then it would be a breaking change since in this case, the PK will consist only declared for the autoincremented column.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's currently not possible. It will result in a MappingException while generating the scheme:
"Single id is not allowed on composite primary key in entity...."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was talking about SQLite itself, not the ORM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can read online it's either a composite key, or a single autoincrement key. The combination seems not possible in SQLite.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we're not breaking anything. Sounds good.

}
}

return ', PRIMARY KEY(' . implode(', ', $keyColumns) . ')';
}

/**
* {@inheritDoc}
*/
Expand Down
28 changes: 28 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
25 changes: 25 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,4 +232,29 @@ public function getDiffListIntegerAutoincrementTableColumnsData()
array('bigint', true, true),
);
}

/**
* @group DBAL-2921
*/
public function testPrimaryKeyNoAutoIncrement()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to cover this case with a test? Is it something DBAL users could/would want to rely on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test was added by the original author to check that the default behavior of SQLite wasn't altered in cases it shouldn't.

{
$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->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();

// with an empty table, non autoincrement rowid is always 1
$this->assertEquals(1, $lastUsedIdAfterDelete);
}
}
36 changes: 18 additions & 18 deletions tests/Doctrine/Tests/DBAL/Platforms/SqlitePlatformTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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))
);
}
Expand All @@ -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))
);
Expand All @@ -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))
);
Expand All @@ -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))
);
Expand All @@ -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))
);
Expand All @@ -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))
);
Expand Down Expand Up @@ -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",
Expand All @@ -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]
);
}
Expand Down