[Django] #25902: app names with underscore conflict with many-to-many tables
Django
----------------------------------------------+--------------------
Reporter: amosonn | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer (models, ORM) | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
If you have an app called X with a model called Y with a m2m field called
Z, the default table name for the m2m is X_Y_Z.
If you have an app called X_Y with a model called Z, the default table
name for the model is also X_Y_Z.
If you have both, django breaks when creating the tables.
See a broken sample project at:
--
Ticket URL: <https://code.djangoproject.com/ticket/25902>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: amosonn | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by amosonn):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
> If you have an app called X with a model called Y with a m2m field called
> Z, the default table name for the m2m is X_Y_Z.
> If you have an app called X_Y with a model called Z, the default table
> name for the model is also X_Y_Z.
> If you have both, django breaks when creating the tables.
> See a broken sample project at:
New description:
If you have an app called X with a model called Y with a m2m field called
Z, the default table name for the m2m is X_Y_Z.
If you have an app called X_Y with a model called Z, the default table
name for the model is also X_Y_Z.
If you have both, django breaks when creating the tables.
See a broken sample project at:
https://github.com/amosonn/check_underscore
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25902#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: amosonn | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Uncategorized => New feature
* component: Database layer (models, ORM) => Core (System checks)
* version: 1.9 => master
* stage: Unreviewed => Accepted
Comment:
I guess we might be able to add a system check for these sort of clashes,
but it might be a non-trivial amount of work for what I assume isn't a
very common case. Also, such validation would need to account for the
possibility that a user does want two models to use the same database
table. Probably `Meta.db_table` would be set by the user in such cases. Do
you have any other ideas? #12810 is another ticket about checks for
clashing many-to-many tables.
--
Ticket URL: <https://code.djangoproject.com/ticket/25902#comment:2>
Django
--------------------------------------+------------------------------------
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by amosonn):
I also noticed the other bug, but thought the behaviour django exhibits
there is more pleasant: it raises a clean, pythonic, internal error, and
does so when generating the migrations. In this case, it generates the
migrations silently, and then fails with a db error when applying them.
I'm unsure, but I think that a user who wants two models for the same
table, currently won't be able to do so without manually editing the
migrations, running one of the CreateModel-s (but not the other) with
SeparateDatabaseAndState - otherwise the second one will attempt creating
the table a second time, and the same db error will be raised.
So, what I think is that perhaps the system check is something that should
be run at makemigrations time, and issue a warning such as: "The
automatically generated migrations have two models ($names) with the same
database table, and will break when run. Consider giving those models an
explicit db_table. If you are modifying the migrations manually, you can
ignore this error." This covers both the case I described, in which the
user will go and give one of them (presumably the m2m) a name, and the
case you described, in which the user knows that he's doing something
strange, and will get a reminder that this has to be reflected in
migrations. It also covers a third case, in which the exact same app name
appears as two different submodules in the project, and which might
actually be more probable than both our previous cases.
I think such a check should not be too costy; perhaps I can add it, maybe
with some guidance as to where is the optimal place for it to reside.
I finally add that my original thought about this was that the problem was
with the default django table names. I still think that a table name of
shoppingmall_parking is better than shopping_mall_parking for the model
shopping_mall.Parking - just as in model names, the separation between
words using camelcasing is removed, so should be the separation between
words using underscores in app names. It is true that this doesn't cover
all cases, for instance some one with both x_y and xy as app names, but
that is even more uncommon.
--
Ticket URL: <https://code.djangoproject.com/ticket/25902#comment:3>
Django
--------------------------------------+------------------------------------
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I think the argument for having this check outside of the migrations
framework is that if you are combining existing third-party apps, you
won't necessarily be generating migrations for them. The check might go in
`django/django/db/models/base.py` (see the `check()` method). There would
need to be a global registry of tables names.
We likely can't change the default table names that Django generates for
backwards compatibility reasons.
--
Ticket URL: <https://code.djangoproject.com/ticket/25902#comment:4>
Django
--------------------------------------+------------------------------------
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25902#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: Amos Onn | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (by Leonardo Costa):
This looks resolved by #12810 (fixed in `0bce2f102c`). Reproduced on
current main with two apps (`appx.Y` m2m field `z`, and `appx_y.Z`)
matching the linked sample project — `django-admin check` now raises:
{{{
appx.Y.z: (fields.E340) The field's intermediary table 'appx_y_z'
clashes with the table name of 'appx_y.Z'.
}}}
Plain model-vs-model clashes are also covered separately by `models.E028`.
Between the two I can't find a gap, suggest closing as a duplicate of
#12810.
--
Ticket URL: <https://code.djangoproject.com/ticket/25902#comment:6>
Django
Reporter: Amos Onn | Owner: nobody
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by Jacob Walls):
* summary: Add system check for project-wide database table name conflicts
=>
Add test for system check for project-wide database table name
conflicts
* type: New feature => Cleanup/optimization
Comment:
I think that's right, the fix for #12810 appears to have handled not just
explicit `db_table=...` but also the default table names. But I don't see
a test in 0bce2f102c5734d0c7ff1ebf0b10a316d58ed5ce making that explicit
(they all use `db_table`, IIUC). Would you like to convert your manual
test case to a PR? You should be able to set `app_label` when setting up
the models.
--
Ticket URL: <https://code.djangoproject.com/ticket/25902#comment:7>
Django
--------------------------------------+------------------------------------
Reporter: Amos Onn | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (System checks) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (by OsmnvAslan):
Leonardo, if you are not planning to open the PR, I have the test ready
and can do it. Following comment:6, it exercises the default table names
only (no `db_table` anywhere): app `defer` with model `Regress` and a
`ManyToManyField` `foos` (intermediary table `defer_regress_foos`), and
app `defer_regress` with model `Foos` (table `defer_regress_foos`). It
asserts `fields.E340` is raised and fails if `_check_table_uniqueness()`
is disabled.
Branch for preview:
https://github.com/django/django/compare/main...OsmnvAslan:django:ticket_25902_m2m_default_table_clash_test
I'll wait a few days before opening the PR in case you want to take it.
--
Ticket URL: <https://code.djangoproject.com/ticket/25902#comment:8>
Django
Reporter: Amos Onn | Owner:
Type: | ArashMohtarami
Cleanup/optimization | Status: assigned
Component: Core (System | Version: dev
checks) |
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by ArashMohtarami):
* has_patch: 0 => 1
* owner: nobody => ArashMohtarami
* status: new => assigned
Comment:
Claiming this ticket and submitting regression-test coverage in the linked
PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/25902#comment:9>
Django
Reporter: Amos Onn | Owner: (none)
Component: Core (System checks) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by ArashMohtarami):
* has_patch: 1 => 0
* owner: ArashMohtarami => (none)
* status: assigned => new
Comment:
I missed comment 8 and had already prepared a duplicate regression test.
I’m unclaiming the ticket and closing my PR so the existing work can
proceed. Sorry for the noise.
--
Ticket URL: <https://code.djangoproject.com/ticket/25902#comment:10>
Django
Reporter: Amos Onn | Owner:
Component: Core (System | Version: dev
checks) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by OsmnvAslan):
* owner: (none) => OsmnvAslan
Comment:
--
Ticket URL: <https://code.djangoproject.com/ticket/25902#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Amos Onn | Owner:
Type: | OsmnvAslan
Cleanup/optimization | Status: assigned
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by OsmnvAslan):
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/22053
--
Ticket URL: <https://code.djangoproject.com/ticket/25902#comment:12>