Issue: Peewee incorrectly assumes that a column of the form IntegerField(primary_key=True) is an AutoField()
Author: ilyakamens
State: closed
Created: 2020-03-04 14:05:13

- Peewee 3.13.1
- PostgreSQL 9.6.15
- psycopg2 2.8.2

```
class TestModel(db.Model):
    id = IntegerField(primary_key=True)
```

```
>>> TestModel.create_table()
>>> models = generate_models(db.database)
>>> models["testmodel"]._meta.columns["id"]
<AutoField: testmodel.id>
```

```
# \d testmodel;
             Table "public.testmodel"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 id     | integer |           | not null |
Indexes:
    "testmodel_pkey" PRIMARY KEY, btree (id)
```

This may be the wrong spot, but it looks like Peewee only checks for primary key + IntegerField here: https://github.com/coleifer/peewee/blob/master/playhouse/reflection.py#L166-L179

Yeah, Peewee uses a heuristic when introspecting the database and assigns the auto-incrementing integer field when it discovers integer primary keys. Known behavior.

@coleifer - Understood. Would you accept a PR to fix this case?

@ilyakamens - before putting together a patch, how would you go about handling this? My thought was that by far the most common case of using an integer primary key was as an auto-incrementing id. On the other hand, determining if an integer primary-key actually *is* autoincrementing in a cross-db compatible way (without introducing false-negatives, which would be much worse IMO than false-positives), is kinda tricky.

It's been a while since I looked into it, though, so maybe you have some ideas? The main things to keep in mind, as far as I'm concerned, are:

* peewee's reflection has worked this way since the beginning and this is the first issue about this
* it seems much easier to deal with a false-positive than a false-negative. Peewee let's you manually specify the primary key in either case, so i'm kinda pressed to think of when this would even negatively impact someone.
* is there an extremely reliable way to detect autoinc primary keys across the supported dbs? Or conversely, to detect when an integer primary key is definitely not autoinc?

I started working on it already before your reply. 🙂 

> My thought was that by far the most common case of using an integer primary key was as an auto-incrementing id.

Completely agree. However, we use peewee's reflection module for automated tests to ensure our migrations work correctly, and this is causing test failures for `IntegerField(primary_key=True)`, which we have a legitimate use for in our case (won't bore you with the details).

I have a fix with tests passing for PostgreSQL (will look into MySQL later). The tricky one is SQLite, since `PRAGMA` is insufficient to get the data I need. I thought I could update `SqliteDatabase.get_columns()` to use `sqlite_master` to parse out the `AUTOINCREMENT` part of the column via regex, but I'm surprised to not see it for `f11` in the column definitions of [ColTypes](https://github.com/coleifer/peewee/blob/master/tests/reflection.py#L31). Here's what I see from running:

```python
list(self.execute_sql('select * from sqlite_master where tbl_name = \'col_types\''))
```


```[(u'table', u'col_types', u'col_types', 4, u'CREATE TABLE "col_types" ("f11" INTEGER NOT NULL PRIMARY KEY, "f1" INTEGER NOT NULL, "f2" BLOB NOT NULL, "f3" INTEGER NOT NULL, "f4" VARCHAR(50) NOT NULL, "f5" DATE NOT NULL, "f6" DATETIME NOT NULL, "f7" DECIMAL(10, 5) NOT NULL, "f8" REAL NOT NULL, "f9" REAL NOT NULL, "f10" INTEGER NOT NULL, "f12" TEXT NOT NULL, "f13" TIME NOT NULL)'), (u'index', u'col_types_f1', u'col_types', 5, u'CREATE INDEX "col_types_f1" ON "col_types" ("f1")'), (u'index', u'col_types_f10', u'col_types', 6, u'CREATE UNIQUE INDEX "col_types_f10" ON "col_types" ("f10")'), (u'index', u'col_types_f10_f11', u'col_types', 7, u'CREATE UNIQUE INDEX "col_types_f10_f11" ON "col_types" ("f10", "f11")'), (u'index', u'col_types_f11_f8_f13', u'col_types', 8, u'CREATE INDEX "col_types_f11_f8_f13" ON "col_types" ("f11", "f8", "f13")')]```

I understand that a column declared `INTEGER PRIMARY KEY` will autoincrement in SQLite, but IIUC, it still needs the `AUTOINCREMENT` directive to autoincrement over the lifetime of the table (source [here](https://www.sqlite.org/faq.html#q1)).

In PostgreSQL, an `AutoField()` monotonically increases:

```python
>>> t = TestModel.create()
>>> t.id
1
>>> t.delete_instance()
1
>>> t = TestModel.create()
>>> t.id
2
```

So my questions are:
1) Shouldn't peewee add `AUTOINCREMENT` to the column definition for an `AutoField()` in SQLite to have consistent monotonic behavior across DBs?
2) If so, what do you think of my proposed solution? [Here](https://github.com/coleifer/peewee/pull/2126)'s a stub of what I'm proposing.

The reasons for not defaulting sqlite to autoincrement are here: https://www.sqlite.org/autoinc.html

It's available if you want it, but most people don't need it.

Ah. I see. And what about having a better heuristic for PostgreSQL and MySQL and leaving SQLite as is?

Hi @coleifer. After thinking about it more, I would make the case that it's okay to improve the heuristic for MySQL and PostgreSQL and to leave SQLite as-is. My reasoning is that an integer field primary key is effectively an `AutoField()` by definition in SQLite.

I've adjusted [my PR](https://github.com/coleifer/peewee/pull/2126) to reflect these changes and have all tests passing (including new ones I added). Going back to the three things to keep in mind you mentioned earlier:

> * peewee's reflection has worked this way since the beginning and this is the first issue about this

Understood.

> * it seems much easier to deal with a false-positive than a false-negative. Peewee let's you manually specify the primary key in either case, so i'm kinda pressed to think of when this would even negatively impact someone.

I agree. I believe these small changes reduce the chance of a false-positive without increasing the risk of a false-negative. 

> * is there an extremely reliable way to detect autoinc primary keys across the supported dbs? Or conversely, to detect when an integer primary key is definitely not autoinc?

My changes use 1) information that was already available but unused (i.e., `column_default` from `information_schema.columns`), and 2) additional information from `information_schema.columns` (i.e., `extra`). I think this is reliable, but I suppose you'll be the judge of that.

Please take a look when you have a chance and let me know what you think.

Thanks!