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

TimoBakx · 2018-05-11T16:06:07Z

Q	A
Type	bug
BC Break	yes?
Fixed issues	#1861, #2426

Summary

These are the updated unit tests for PR #2921.

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.

morozov · 2018-05-11T17:12:45Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php

+    /**
+     * @group DBAL-2921
+     */
+    public function testPrimaryKeyAutoIncrement()


@TimoBakx thank you for the patch.

There doesn't seem to be anything SQLite-specific in this test which is good. Could you try moving it to a platform-agnostic test case (e.g. SchemaManagerFunctionalTestCase) to make sure it works the same on all platforms which support autoincrement fields?

Good idea. It took a bit of tweaking, but it now runs (and passes) on all tests.

morozov · 2018-05-11T17:13:56Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php

+
+        $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']);


If one record is enough to test that autoincrement works in fact, please leave one out of three.

Good point. I removed two lines in this test and in testPrimaryKeyNoAutoIncrement.

morozov · 2018-05-11T17:19:01Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php

+    /**
+     * @group DBAL-2921
+     */
+    public function testPrimaryKeyNoAutoIncrement()


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

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.

TimoBakx · 2018-05-11T18:35:46Z

@morozov Thank you for your review.

I made some changes. Could you please review again?
Thanks in advance.

morozov · 2018-05-11T18:57:28Z

lib/Doctrine/DBAL/Platforms/SqlitePlatform.php

-            $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) . ')';


Do we still need to use two syntaxes? The PRIMARY KEY(x, y, z) looks more generic since it can describe multiple fields. Given that we take this case into account, should we use just the generic syntax.

This $columns[$keyColumns[0]] doesn't look good. What if the key consists of two columns where the second one has autoincrement?

Using this syntax will revert the behavior back to the original, where ID's are reused if you remove the entry with the highest ID in the table. See #1861, #2426 and #2921 for more info.

I'll look into an alternative way to check which of the syntaxes should be used.

If that's the case, can we leave the AUTOINCREMENT keyword with the field and define the PK later? Otherwise, it's unclear if it's possible and how it will be handled if one of the columns is autoincremented but the PK consists of two columns.

Unfortunately, it only works when the field definition is INTEGER PRIMARY KEY AUTOINCREMENT as a whole. In that specific order.

Still, we need to figure out how this is going to be handled if at all possible:

one of the columns is autoincremented but the PK consists of two columns.

The 0 in count($keyColumns) > 1 || ! $columns[$keyColumns[0]]['autoincrement'] looks like a magic constant.

I agree. I'll run some tests to see if I can improve that.

I've made some improvements. This should cover it.

morozov · 2018-05-11T20:15:28Z

lib/Doctrine/DBAL/Platforms/SqlitePlatform.php

+
+        foreach ($keyColumns as $keyColumn) {
+            if (isset($columns[$keyColumn]['autoincrement']) && ! empty($columns[$keyColumn]['autoincrement'])) {
+                return '';


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.

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...."

I was talking about SQLite itself, not the ORM.

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

Then we're not breaking anything. Sounds good.

morozov · 2018-05-11T20:28:38Z

lib/Doctrine/DBAL/Platforms/SqlitePlatform.php

+     *
+     * @return string
+     */
+    private function getNonAutoincrementPrimaryKeyDefinition(array $columns, array $options)


Please specify return type explicitly instead of an annotation.

morozov · 2018-05-11T20:30:16Z

lib/Doctrine/DBAL/Platforms/SqlitePlatform.php

+     */
+    private function getNonAutoincrementPrimaryKeyDefinition(array $columns, array $options)
+    {
+        if (! isset($options['primary']) || empty($options['primary'])) {


The combination of !isset() || empty() is redundant. It's covered by empty().

TimoBakx · 2018-05-14T05:30:17Z

@morozov Thank you for your reviews, insight and patience.

morozov · 2018-05-14T15:13:09Z

Glad it helped. Thank you for your contribution!

@morozov

Release v2.8.0 [![Build Status](https://travis-ci.org/doctrine/dbal.svg?branch=v2.8.0)](https://travis-ci.org/doctrine/dbal) This is a minor release of Doctrine DBAL that aggregates over 30 fixes and improvements developed over the last 3 months. This release includes all changes of the 2.7.x series, as well as feature additions and improvements that couldn’t land in patch releases. **Backwards Compatibility Breaks** This doesn't contain any intentional Backwards Compatibility (BC) breaks. **Dependency Changes** * The dependency on [doctrine/common](https://github.com/doctrine/common) is removed. DBAL now depends on [doctrine/cache](https://github.com/doctrine/cache) and [doctrine/event-manager](https://github.com/doctrine/event-manager) instead. Please see details in the [UPGRADE.md](UPGRADE.md) documentation. **Deprecations** * The usage of binary fields whose length exceeds the maximum field size on a given platform is deprecated. Please use binary fields of a size which fits all target platforms, or use blob explicitly instead. * The usage of DB-generated UUIDs is deprecated. Their format is inconsistent across supported platforms and therefore the feature is not portable. Use a PHP library (e.g. [ramsey/uuid](https://packagist.org/packages/ramsey/uuid)) to generate UUIDs on the application side. **New features** * Initial support of MySQL 8. * Initial support of PostgreSQL 11. * Ability to evaluate arbitrary SQL expressions via `AbstractPlatform::getDummySelectSQL()`. **Improvements and Fixes** * Improved support of binary fields on Oracle and IBM DB2. * Improved SQL Server configuration capabilities via both `sqlsrv` and `pdo_sqlsrv`. * Improved handling of `AUTOINCREMENT`ed primary keys in SQLite. * Integration tests are run against IBM DB2 on Travis CI. * Code coverage is collected for the Oracle platform on continuousphp. Total issues resolved: **33** **Deprecations:** - [3187: Deprecate usage of binary fields whose length exceeds maximum](doctrine#3187) thanks to @morozov - [3188: Deprecated usage of binary fields whose length exceeds the platform maximum](doctrine#3188) thanks to @morozov - [3192: Added more information to the deprecation notice](doctrine#3192) thanks to @morozov - [3212: Deprecated usage of DB-generated UUIDs](doctrine#3212) thanks to @morozov **New Features:** **Bug Fixes:** - [3149: Introduced binary binding type to support binary parameters on Oracle](doctrine#3149) thanks to @morozov - [3178: Fix incorrect exception thrown from SQLAnywhere16Platform](doctrine#3178) thanks to @Majkl578 - [3044: Functional test for allowing dynamic intervals in date sub/add](doctrine#3044) thanks to @AshleyDawson - [3049: Test failures caused by invalid database connection result in fatal error](doctrine#3049) thanks to @Majkl578 **Improvements:** - [3033: Added support for available DSN parameters for the PDOSqlsrv driver](doctrine#3033) thanks to @aashmelev - [3128: Add MySQL 8 reserved keywords](doctrine#3128) thanks to @mlocati - [3143: initialize sql array into platform files](doctrine#3143) thanks to @AlessandroMinoccheri - [3173: Fix composer branch aliases](doctrine#3173) thanks to @Majkl578 - [3157: When building a limit query, zero offset without a limit should be ignored](doctrine#3157) thanks to @morozov - [3109: Allow to specify arbitrary SQL expression in AbstractPlatform::getDummySelectSQL()](doctrine#3109) thanks to @morozov - [3141: allow creating PRIMARY KEY AUTOINCREMENT fields for sqlite (unit tests)](doctrine#3141) thanks to @TimoBakx - [3180: Import simplified version of Common\Util\Debug for var dumping purposes](doctrine#3180) thanks to @Majkl578 **Documentation Improvements:** - [3117: Added badges for the develop branch in README](doctrine#3117) thanks to @morozov - [3125: Upgrading docs](doctrine#3125) thanks to @jwage - [3144: added improvement type into pull request template](doctrine#3144) thanks to @AlessandroMinoccheri **Code Quality Improvements:** - [3025: Added PHPStan, apply changes for level 3](doctrine#3025) thanks to @Majkl578 - [3200: Php Inspections (EA Ultimate): minor code tweaks](doctrine#3200) thanks to @kalessil - [3204: Fix typo in AbstractPlatform](doctrine#3204) thanks to @Majkl578 - [3205: Ignore OCI-* classes in static analysis (no stubs)](doctrine#3205) thanks to @Majkl578 **Continuous Integration Improvements:** - [3102: Use newer PHPUnit to prevent crashes on failures](doctrine#3102) thanks to @Majkl578 - [3112: Removed hard-coded configuration filenames from the test runner](doctrine#3112) thanks to @morozov - [3133: Travis DB2](doctrine#3133) thanks to @Majkl578, @morozov - [3135: AppVeyor tweaks, retry coverage upload on failure](doctrine#3135) thanks to @Majkl578 - [3137: Workaround for the inability to use a post-PHPUnit script on ContinuousPHP](doctrine#3137) thanks to @morozov - [3151: MSSQL DLL 5.2.0 has been released.](doctrine#3151) thanks to @photodude - [3160: Test against Postgres 11](doctrine#3160) thanks to @Majkl578 **Dependencies** - [3193: DBAL 2.8 needs Common 2.9](doctrine#3193) thanks to @Majkl578 - [3176: Eliminate dependency on doctrine/common](doctrine#3176) thanks to @Majkl578 - [3181: Remove dependency on doctrine/common](doctrine#3181) thanks to @Majkl578 # gpg: Signature made Fri Jul 13 06:02:10 2018 # gpg: using RSA key 374EADAF543AE995 # gpg: Can't check signature: public key not found # Conflicts: # .gitignore # README.md # lib/Doctrine/DBAL/Version.php # tests/appveyor/mssql.sql2008r2sp2.sqlsrv.appveyor.xml # tests/appveyor/mssql.sql2012sp1.sqlsrv.appveyor.xml # tests/appveyor/mssql.sql2017.pdo_sqlsrv.appveyor.xml # tests/appveyor/mssql.sql2017.sqlsrv.appveyor.xml # tests/travis/mariadb.mysqli.travis.xml

TimoBakx changed the title ~~Sqlite pk autoincrement~~ allow creating PRIMARY KEY AUTOINCREMENT fields for sqlite (unit tests) May 11, 2018

TimoBakx mentioned this pull request May 11, 2018

allow creating PRIMARY KEY AUTOINCREMENT fields for sqlite #2921

Closed

morozov reviewed May 11, 2018

View reviewed changes

Updated sqlite unit tests

245bfb7

morozov approved these changes May 11, 2018

View reviewed changes

morozov merged commit 7021bdf into doctrine:master May 13, 2018

TimoBakx deleted the sqlite-pk-autoincrement branch May 14, 2018 06:00

Majkl578 assigned morozov Jun 1, 2018

Majkl578 added Improvement SQLite Schema Management Schema Definition labels Jun 1, 2018

Majkl578 added this to the 2.8.0 milestone Jun 1, 2018

DeepDiver1975 mentioned this pull request Jul 24, 2018

Bump doctrine/dbal from 2.7.1 to 2.8.0 owncloud/core#32065

Merged

danquah mentioned this pull request Aug 15, 2019

doctrine/dbal patch no longer applies os2display/core-bundle#11

Open

flack mentioned this pull request Jun 21, 2021

DBAL-642: Generated IDs are not guaranteed to be unique over the table's lifetime in SQLite #1861

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TimoBakx commented May 11, 2018 •

edited

Loading

morozov May 11, 2018

TimoBakx May 11, 2018

morozov May 11, 2018

TimoBakx May 11, 2018

morozov May 11, 2018

TimoBakx May 11, 2018

TimoBakx commented May 11, 2018

morozov May 11, 2018

TimoBakx May 11, 2018 •

edited

Loading

morozov May 11, 2018

TimoBakx May 11, 2018

morozov May 11, 2018

TimoBakx May 11, 2018

TimoBakx May 11, 2018

morozov May 11, 2018

TimoBakx May 11, 2018

morozov May 11, 2018

TimoBakx May 11, 2018

morozov May 11, 2018

morozov May 11, 2018 •

edited

Loading

morozov May 11, 2018

TimoBakx commented May 14, 2018

morozov commented May 14, 2018

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

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

Conversation

TimoBakx commented May 11, 2018 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoBakx commented May 11, 2018

Choose a reason for hiding this comment

TimoBakx May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoBakx commented May 14, 2018

morozov commented May 14, 2018

TimoBakx commented May 11, 2018 •

edited

Loading

TimoBakx May 11, 2018 •

edited

Loading

morozov May 11, 2018 •

edited

Loading