Opened 4 years ago

Closed 2 years ago

#31846 closed New feature (duplicate)

MySQL inspectdb doesn't handle composite keys

Reported by: François Poulain Owned by:
Component: Core (Management commands) Version: 3.0
Severity: Normal Keywords:
Cc: Adam Johnson Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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?

Attachments (1)

inspectdb.zip (10.0 KB ) - added by Carlton Gibson 4 years ago.
Project using inspectdb on DB table with non-PK id field.

Download all attachments as: .zip

Change History (11)

by Carlton Gibson, 4 years ago

Attachment: inspectdb.zip added

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

comment:1 by Carlton Gibson, 4 years ago

Resolution: worksforme
Status: newclosed

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.

comment:2 by François Poulain, 4 years ago

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.

comment:3 by Carlton Gibson, 4 years ago

Cc: Adam Johnson added
Resolution: worksforme
Status: closednew

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

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

comment:4 by Adam Johnson, 4 years ago

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.

comment:5 by Carlton Gibson, 4 years ago

Summary: inspectdb doesn't handle tables with "id" field and without primary keyMySQL inspectdb doesn't handle composite keys
Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

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.

comment:6 by Mariusz Felisiak, 4 years ago

Has patch: unset

comment:7 by François Poulain, 4 years ago

Owner: changed from nobody to François Poulain
Status: newassigned

comment:8 by François Poulain, 4 years ago

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).

comment:9 by Mariusz Felisiak, 4 years ago

Owner: François Poulain removed
Status: assignednew

comment:10 by Mariusz Felisiak, 2 years ago

Resolution: duplicate
Status: newclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top