JetBrains / Exposed

Kotlin SQL Framework
http://jetbrains.github.io/Exposed/
Apache License 2.0
8.05k stars 674 forks source link

fix: EXPOSED-405 SQLite bugs: Table with custom ID behaves weirdly in DAO and batchInsert #2119

Closed obabichevjb closed 1 week ago

obabichevjb commented 3 weeks ago

Inside InsertStatement there is a field autoIncColumns with the getter that should return the list of auto increment columns. One of the checks it does is column.columnType is EntityIDColumnType<*> -> !currentDialect.supportsOnlyIdentifiersInGeneratedKeys, but looks like it's a wrong assumption since not every EntityIDColumnType must be autoincrementable (test with custom String primary key demonstrates it)

If I'm right, the main way to create EntityIDColumnType for the user is to use entityId() methods on column, but to make it autoincrement the method cloneWithAutoInc() (autoIncrement()) should be used, what will make column AutoIncColumnType what is also checked inside InsertStatement.

The only test that was affected by removing this check is testFlushNonAutoincEntityWithoutDefaultValue, that test was removed. The name of the test states ...NonAutoincEntity..., but if I'm right the whole test works only because a table AutoIncSharedTable created, and the same name "SharedTable" for both (auto/non-auto increment) tables is chosen. Inside the test two entries are created without specifying id explicitly, what (here I'm not sure) should not work, because it's not autoincrement column... if you know what this test checks, I'd be happy to know about it)

obabichevjb commented 1 week ago

Regarding the testFlushNonAutoincEntityWithoutDefaultValue() test. I talked to the author of the test (@Tapac), and it looks like the most probable reason for creating that test is to check that if the table is created with auto-incrementing column, but inside the table definition that autoincrement is forgotten, autogenerated value will be returned anyway.

But with the changes of the PR it as expected should not work, because the value is not requested to be returned from the database if it's not auto increment column. Before it working only due to a wrong assumption that every entity id is an auto-incremented column and should be returned.