Fix #7271 - SQLite - composite keys wrongly introspected as autoincrement

[fisharebest]

Q	A
Type	bug
Fixed issues	#7271

Summary

In SQLite, integer primary keys are implicitly autoincrement columns, and dbal contains logic for this.

However, this logic fails when a composite primary key contains an integer and also other non-integer columns. The integer column part of the composite PK becomes wrongly flagged as autoincrement.

(A consequence of this is that when calling getAlterSchemaSQL(), the non-integer columns are removed from the primary key. For example, a PK consisting of INTEGER, VARCHAR gets changed to just INTEGER. This causes loss of data as the rows saved to the temporary table cannot be re-inserted.)

This happens because the code filters the existing PK columns by "is integer" instead of filtering the column under consideration by "is integer".

Moving this "is integer" check fixes the issue.

A new test case was added containing three assertions. Two of these assert that the existing functionality isn't broken. The third would fail under the old code, but passes under the new code.

---

NOTE: I have submitted this PR against the 4.5.x branch (as requested by @morozov).
However, the code is the same in the 4.4.x branch, and could be backported if desired.

[morozov]

The fix looks fine to me. I don't think there's a BC break other than "it used to work the other way previously". Maintainers, please speak up if you disagree.

[morozov]

This tests seems to combine three independent tests. Would it make sense to separate them?

[fisharebest]

The three assertions are all testing the same logic, so it seemed clearer (to me) to have them in a single test.

I can split them up if desired, but I prefer them together.

[morozov]

With the current approach, if the first assertion fails, the other two will be skipped, however they shouldn't.

[greg0ire]

From #7271 (comment) (emphasis mine)

You could try making this change and providing a BC impact analysis

[morozov]

From #7271 (comment) (emphasis mine)

You could try making this change and providing a BC impact analysis

When I requested this analysis, I assumed that the complete fix would somehow resemble #6862, which is a BC break. My own assessment of the actual changes in this PR is:

1. Not ignoring non-integer columns in the PK definition is correct: any column can be part of the PK, and there's no promise that the DBAL will ignore them.
2. Setting auto-increment to true only if the column is integer is also correct (this behavior doesn't change, it's just the condition above being moved lower).

I don't see an obvious BC break here, it's just a different more correct behavior, which is more appropriate for a minor release.