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)
Change History (11)
by , 4 years ago
Attachment: | inspectdb.zip added |
---|
comment:1 by , 4 years ago
Resolution: | → worksforme |
---|---|
Status: | new → closed |
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 , 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 , 4 years ago
Cc: | added |
---|---|
Resolution: | worksforme |
Status: | closed → new |
Thanks for the follow-up — let's reopen to assess.
I'll cc Adam: is this a case we should support?
comment:4 by , 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 , 4 years ago
Summary: | inspectdb doesn't handle tables with "id" field and without primary key → MySQL inspectdb doesn't handle composite keys |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → New 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 , 4 years ago
Has patch: | unset |
---|
comment:7 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 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 , 4 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:10 by , 2 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
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.
Project using inspectdb on DB table with non-PK id field.