[Django] #31846: inspectdb doesn't handle tables with "id" field and without primary key
Django
-------------------------------------+-------------------------------------
Reporter: François | Owner: nobody
Poulain |
Type: | Status: new
Uncategorized |
Component: Core | Version: 3.0
(Management commands) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/31846>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Core (Management | Version: 3.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "inspectdb.zip" added.
Project using inspectdb on DB table with non-PK id field.
Django
-------------------------------------+-------------------------------------
Component: Core (Management | Version: 3.0
Severity: Normal | worksforme
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => worksforme
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31846#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: closed
Component: Core (Management | Version: 3.0
commands) | Resolution:
Severity: Normal | worksforme
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31846#comment:2>
Django
-------------------------------------+-------------------------------------
Component: Core (Management | Version: 3.0
commands) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Adam (Chainz) Johnson (added)
* status: closed => new
* resolution: worksforme =>
Comment:
Thanks for the follow-up — let's reopen to assess.
I'll cc Adam: is this a case we should support?
--
Ticket URL: <https://code.djangoproject.com/ticket/31846#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Core (Management | Version: 3.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam (Chainz) 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31846#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: François Poulain | Owner: nobody
Component: Core (Management | Version: 3.0
commands) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Uncategorized => New feature
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31846#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: François Poulain | Owner: nobody
Type: New feature | Status: new
Component: Core (Management | Version: 3.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31846#comment:6>
Django
-------------------------------------+-------------------------------------
| Poulain
Type: New feature | Status: assigned
Component: Core (Management | Version: 3.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => François Poulain
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31846#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: François Poulain | Owner: François
| Poulain
Type: New feature | Status: assigned
Component: Core (Management | Version: 3.0
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/31846#comment:8>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: François Poulain => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/31846#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: François Poulain | Owner: (none)
Component: Core (Management | Version: 3.0
commands) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => duplicate
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31846#comment:10>