MySQL inspectdb doesn't handle composite keys

[François Poulain]

Hi,

I plugged django on a crappy drupal DB using inspectdb.py. It is intended to be read only. I got models like

class FeedapiStat(models.Model):
    id = models.PositiveIntegerField()
    type = models.CharField(max_length=64)
    timestamp = models.IntegerField()
    time = models.CharField(max_length=20)
    value = models.IntegerField()

And I have been facing with two issues:

• drupal.FeedapiStat: (models.E004) 'id' can only be used as a field name if the field also sets 'primary_key=True'.
• Then I renamed the field and faced with: (models.E007) Field 'id_field' has column name 'id' that is used by another field. HINT: Specify a 'db_column' for the field. This is because the model has no pk, so django automatically try to add one.

I also got some models with a primary key and an id field which is not a pk.

Since there is no way to tell that a model should not have pk, I modified inspectdb as follows:

--- ./venv/lib/python3.7/site-packages/django/core/management/commands/inspectdb.py	2020-08-01 10:28:06.155225992 +0200
+++ ./base/management/commands/inspectdb.py	2020-08-01 10:35:06.677586957 +0200
@@ -101,8 +101,10 @@
                     column_name = row.name
                     is_relation = column_name in relations
 
+                    is_pk = column_name == primary_key_column
+                    has_pk = primary_key_column
                     att_name, params, notes = self.normalize_col_name(
-                        column_name, used_column_names, is_relation)
+                        column_name, used_column_names, is_relation, is_pk, has_pk)
                     extra_params.update(params)
                     comment_notes.extend(notes)
 
@@ -110,7 +112,7 @@
                     column_to_field_name[column_name] = att_name
 
                     # Add primary_key and unique, if necessary.
-                    if column_name == primary_key_column:
+                    if is_pk or (column_name == 'id' and not has_pk):
                         extra_params['primary_key'] = True
                     elif column_name in unique_columns:
                         extra_params['unique'] = True
@@ -173,7 +175,7 @@
                 for meta_line in self.get_meta(table_name, constraints, column_to_field_name, is_view, is_partition):
                     yield meta_line
 
-    def normalize_col_name(self, col_name, used_column_names, is_relation):
+    def normalize_col_name(self, col_name, used_column_names, is_relation, is_pk, has_pk):
         """
         Modify the column name to make it Python-compatible as a field name
         """
@@ -213,6 +215,10 @@
             new_name += '_field'
             field_notes.append('Field renamed because it was a Python reserved word.')
 
+        if new_name == 'id' and not is_pk and has_pk:
+            new_name += '_field'
+            field_notes.append("Field renamed because 'id' can only be used as a field name if the field also sets 'primary_key=True'")
+
         if new_name[0].isdigit():
             new_name = 'number_%s' % new_name
             field_notes.append("Field renamed because it wasn't a valid Python identifier.")

Not sure this is a bug but adding this feature makes inspectdb's behavior more convenient, I guess, because the models.E007 error is not easily understandable and didn't helped me to find the root of the issue.

Moreover, when a model has nor pk neither id field, a specific warning could be raised, since the automatic id creation will mismatch with the db. Maybe, a model instanciated with Meta: managed = False and without pk should always raise a check warning.

How do you think about it?

[Carlton Gibson]

Project using inspectdb on DB table with non-PK id field.

[Carlton Gibson]

OK, I can't reproduce an issue here.

Create table:

CREATE TABLE issue (
     id INT,
     note TEXT
);

Inspect DB:

class Issue(models.Model):
    id = models.IntegerField(blank=True, null=True)
    note = models.TextField(blank=True, null=True)

    class Meta:
        managed = False
        db_table = 'issue'

No issues :

$ ./manage.py check
System check identified no issues (0 silenced).

I've uploaded the sample project for that. If you adjust the DB and then run inspectdb again so that it demonstrates the issue, I'm happy to take another look.

[François Poulain]

Hum. It appears the problem appeared with a mysql table created as this:

  CREATE TABLE `feedapi_stat` (                                                   
    `id` int(10) unsigned NOT NULL DEFAULT 0,                                     
    `type` varchar(64) NOT NULL,                                                  
    `timestamp` int(11) NOT NULL,                                                 
    `time` varchar(20) NOT NULL,                                                  
    `value` int(11) NOT NULL,                                                     
'   KEY `id` (`id`,`type`,`timestamp`,`time`)                                     
  ) ENGINE=MyISAM DEFAULT CHARSET=latin1; 

The KEY is a synonym to PRIMARY KEY , following ​https://mariadb.com/kb/en/create-table/#primary-key-column-option but doesn't seems to be understood as this by connection.introspection. I have no idea if this is a bug or a feature.

[Carlton Gibson]

Thanks for the follow-up — let's reopen to assess.

I'll cc Adam: is this a case we should support?

[Adam Johnson]

This is the old composite keys feature.

Without implementing composite PK's in the Django ORM, I guess we could add the proposed check to inspectdb. We can remap 'id' to 'id_field' (or 'id_field_2' if 'id_field' exists, etc.) if it's not the only field in the PK. The proposed patch seems like a good start.

[Carlton Gibson]

OK, let's accept on that basis. Thanks for the quick response Adam.

François, if you'd like to assign the ticket to yourseld and open a PR with your patch on GitHub, that would be the next step.

[François Poulain]

Hi,
I submitted a PR ( ​https://github.com/django/django/pull/13296 ) but actually, those MySQL subtleties are beyong my knowledge on the topic. I will not advocate on how Django should behave. The primary reason why I opened a bug is because the error models.E007 didn't helped me to understand why this inspection have failed (i.e. produced django-incompatible code).

[Mariusz Felisiak]

As far as I'm aware KEY is not a synonym of PRIMARY KEY it's also not recognized as a primary key:

mysql> CREATE TABLE `feedapi_stat` (                                                   
    ->     `id` int(10) unsigned NOT NULL DEFAULT 0,                                     
    ->     `type` varchar(64) NOT NULL,                                                  
    ->     `timestamp` int(11) NOT NULL,                                                 
    ->     `time` varchar(20) NOT NULL,                                                  
    ->     `value` int(11) NOT NULL,
    ->      KEY `id` (`id`,`type`,`timestamp`,`time`));
mysql>desc feedapi_stat;
+-----------+--------------+------+-----+---------+-------+
| Field     | Type         | Null | Key | Default | Extra |
+-----------+--------------+------+-----+---------+-------+
| id        | int unsigned | NO   | MUL | 0       |       |
| type      | varchar(64)  | NO   |     | NULL    |       |
| timestamp | int          | NO   |     | NULL    |       |
| time      | varchar(20)  | NO   |     | NULL    |       |
| value     | int          | NO   |     | NULL    |       |
+-----------+--------------+------+-----+---------+-------+
mysql> CREATE TABLE `feedapi_stat2` (                                                   
    ->     `id` int(10) unsigned NOT NULL DEFAULT 0,                                     
    ->     `type` varchar(64) NOT NULL,                                                  
    ->     `timestamp` int(11) NOT NULL,                                                 
    ->     `time` varchar(20) NOT NULL,                                                  
    ->     `value` int(11) NOT NULL,
    ->      PRIMARY KEY `id` (`id`,`type`,`timestamp`,`time`));
+-----------+--------------+------+-----+---------+-------+
| Field     | Type         | Null | Key | Default | Extra |
+-----------+--------------+------+-----+---------+-------+
| id        | int unsigned | NO   | PRI | 0       |       |
| type      | varchar(64)  | NO   | PRI | NULL    |       |
| timestamp | int          | NO   | PRI | NULL    |       |
| time      | varchar(20)  | NO   | PRI | NULL    |       |
| value     | int          | NO   |     | NULL    |       |
+-----------+--------------+------+-----+---------+-------+

---

Also, composite primary keys are handled by inspectdb since 295249c901e13ec1703ada5c414cd97aba72f3e9, so IMO we can close it as a duplicate:

$ python manage.py inspectdb
...
class FeedapiStat2(models.Model):
    id = models.PositiveIntegerField(primary_key=True)  # The composite primary key (id, type, timestamp, time) found, that is not supported. The first column is selected.
    type = models.CharField(max_length=64)
    timestamp = models.IntegerField()
    time = models.CharField(max_length=20)
    value = models.IntegerField()

    class Meta:
        managed = False
        db_table = 'feedapi_stat2'
        unique_together = (('id', 'type', 'timestamp', 'time'),)

Duplicate of #32234.