Fix "bigint" type introspection for PK column with AI for SQLite

[mvorisek]

Q	A
Type	bug
Fixed issues	#6396

Summary

Because SQLite does not support AUTOINCREMENT keyword on any other type than INTEGER [1] [2], bigint type is mapped to INTEGER SQLite database column by DBAL already [3].

As we remap the type on table creation, we need to remap the type also on table introspection. The reason, we remap integer to bigint, is bigint is subtype of integer - PK defined/created originally as bigint must be always introspected as bigint.

[1] https://www.sqlite.org/autoinc.html
[2] https://dbfiddle.uk/yyXvHV-6
[3] https://github.com/doctrine/dbal/blob/3.8.4/src/Platforms/SqlitePlatform.php#L271-L274

[greg0ire]

It would seem that this is going to cause the opposite problem: a primary key with type INTEGER is wrongly going to be introspected as BIGINT, isn't it?

[greg0ire]

I don't understand why $tableColumn['pk'] !== 0 && $tableColumn['pk'] !== '0' should mean this is an auto increment column. Couldn't it be an integer column that is part of the primary key?

[mvorisek]

I coded the "auto increment" detection based on existing https://github.com/doctrine/dbal/blob/b6a65d9ef8/src/Schema/SqliteSchemaManager.php#L295-L319. Is that right?

[greg0ire]

I also noticed that code block, but I don't know why it is right (if it is).

[mvorisek]

It would seem that this is going to cause the opposite problem: a primary key with type INTEGER is wrongly going to be introspected as BIGINT, isn't it?

This is unavoidable because of #5107 and expected - INT is subtype of BIGINT, so BIGINT must be introspected because of https://github.com/doctrine/dbal/blob/3.8.4/src/Platforms/SqlitePlatform.php#L271-L274 transformation done when creating the table.

[remco-pc]

The auto increment are in the sequence table and these are integers probably. You could select from this table to check if there is auto increment.

[mvorisek]

Is it really wanted to do additional query in _getPortableTableColumnDefinition method?

[derrabus]

I'm not comfortable with merging this change into 3.8, tbh.

As you've already noticed, SQLite's implementation of integer PKs is a bit odd. The main issue is that SQLite does not allow you to create BIGINT PKs which is why the DBAL creates an INTEGER PRIMARY KEY column instead which has the value range of a BIGINT.

This also means that when introspecting, we cannot tell the difference between a column that was created by DBAL from a IntegerType and one that was created from a BigIntType. This is why the DBAL guesses one of two possible types. Apparently the wrong one in your case. Your "fix" would simply change that guess to a different one. That guess is not more accurate that the currently implemented one. It's just wrong for a different group of apps.

[remco-pc]

You could also solve this in documentation: say in sqlite primary keys are just ints, not bigints and do nothing about the code or throw an exception when a primary key is not an int in sqlite

[derrabus]

throw an exception when a primary key is not an int in sqlite

We probably don't want this. If someone uses BIGINT PKs on their entity model, e.g. because they're using MySQL in production, they should still be able to deploy their model to SQLite for testing purposes.

[remco-pc]

Yes and testing a big int number will fail

[SenseException]

I think the point was that it shouldn't fail. Prod uses MySQL and a Test environment SQLite. It would break projects if SQLite would suddenly throw an exception. If it would stay that way, I wouldn't expect such a change in 3.8 or 4.0.