Opened 2 years ago

Last modified 4 weeks ago

#20098 new New feature

Add validation for models with the same db_table

Reported by: carsten.klein@… Owned by: nobody
Component: Core (System checks) Version: master
Severity: Normal Keywords:
Cc: bmispelon@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When declaring two models using a custom db_table setting in the Meta class, I found out that django failed to detect that the two models declared the same db_table and subsequently it will fail on syncdb when trying to create the same table twice.

This, of course, was introduced by copy&paste, but at least django should report this on validate instead of failing when trying to syncdb.

Attachments (5)

patch20098 (816 bytes) - added by carsten.klein@… 2 years ago.
Proposed patch to django.db.models.options.py that might resolve the issue (tested)
20098.patch (3.0 KB) - added by carsten.klein@… 2 years ago.
20098-support.patch (2.6 KB) - added by carsten.klein@… 2 years ago.
20098.2.patch (3.6 KB) - added by carsten.klein@… 2 years ago.
Correct handling of unmanaged models and duplicate db_table declaration
20098-Support.patch (3.4 KB) - added by carsten.klein@… 2 years ago.
Adds test case for unmanaged model classes

Download all attachments as: .zip

Change History (16)

comment:1 Changed 2 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/optimization

Changed 2 years ago by carsten.klein@…

Proposed patch to django.db.models.options.py that might resolve the issue (tested)

comment:2 Changed 2 years ago by bmispelon

  • Cc bmispelon@… added
  • Has patch set
  • Needs tests set

This seems like a good idea but it needs to have tests in order for it to be merged.

From what I can tell, the tests for the validate command go in tests/admin_scripts/tests.py: https://github.com/django/django/blob/master/tests/admin_scripts/tests.py#L1046

Also, consider adding a .diff extension to your patch so that trac can use syntax highlighting on it.

Thanks.

comment:3 follow-up: Changed 2 years ago by Keryn Knight <django@…>

It is worth bearing in mind that there are valid use cases for encountering duplicate table names, such as when using proxy models, or unmanaged models. The provided patch doesn't seem to account for that, but I'm not familiar enough with the surrounding code to know whether it is already handled.

comment:4 follow-up: Changed 2 years ago by carsten.klein@…

Thanks for pointing this out. I included a revised version of the patch including also two test cases, one that tests against a single application declaring duplicate db tables and one that tests against multiple applications declaring duplicate db tables.

Changed 2 years ago by carsten.klein@…

comment:5 Changed 2 years ago by anonymous

As proxies reuse the actual model the newly provided patch should be sufficient.

comment:6 in reply to: ↑ 4 Changed 2 years ago by carsten.klein@…

Replying to carsten.klein@…:

Thanks for pointing this out. I included a revised version of the patch including also two test cases, one that tests against a single application declaring duplicate db tables and one that tests against multiple applications declaring duplicate db tables.

As far as unmanaged models go, these should not be using the ModelBase meta class then, or would they?

comment:7 Changed 2 years ago by bmispelon

There's a typo in the added comment:

Lookup table for table names introduced in order to prevent that users from declaring the same table twice

Regarding unmanaged and proxy models, I'm not familiar enough with the code to tell if they are handled with this patch but one surefire way to tell would be to add some tests that use them.

Changed 2 years ago by carsten.klein@…

comment:8 Changed 2 years ago by carsten.klein@…

Included the newly added apps for backing the test cases.

comment:9 Changed 2 years ago by carsten.klein@…

As a side effect this should also detect copy&paste errors such as duplicating a model class and failing to rename it and updating its Meta accordingly.

comment:10 in reply to: ↑ 3 Changed 2 years ago by carsten.klein@…

Replying to Keryn Knight <django@…>:

It is worth bearing in mind that there are valid use cases for encountering duplicate table names, such as when using proxy models, or unmanaged models. The provided patch doesn't seem to account for that, but I'm not familiar enough with the surrounding code to know whether it is already handled.

Just had a look at the documentation on unmanaged models. It seems that this is a special case that needs to be dealt with. I will add a guard to the patch and a test case that also includes unmanaged models.

Thanks for pointing this out!

Changed 2 years ago by carsten.klein@…

Correct handling of unmanaged models and duplicate db_table declaration

Changed 2 years ago by carsten.klein@…

Adds test case for unmanaged model classes

comment:11 Changed 4 weeks ago by timgraham

  • Component changed from Database layer (models, ORM) to Core (System checks)
  • Summary changed from Django validate command fails to detect that multiple models declare the same db_table to Add validation for models with the same db_table
  • Type changed from Cleanup/optimization to New feature
  • Version changed from 1.5 to master
Note: See TracTickets for help on using tickets.
Back to Top