Code

Opened 16 months ago

Last modified 16 months ago

#20098 new Cleanup/optimization

Django validate command fails to detect that multiple models declare the same db_table

Reported by: carsten.klein@… Owned by: nobody
Component: Database layer (models, ORM) Version: 1.5
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@… 16 months ago.
Proposed patch to django.db.models.options.py that might resolve the issue (tested)
20098.patch (3.0 KB) - added by carsten.klein@… 16 months ago.
20098-support.patch (2.6 KB) - added by carsten.klein@… 16 months ago.
20098.2.patch (3.6 KB) - added by carsten.klein@… 16 months ago.
Correct handling of unmanaged models and duplicate db_table declaration
20098-Support.patch (3.4 KB) - added by carsten.klein@… 16 months ago.
Adds test case for unmanaged model classes

Download all attachments as: .zip

Change History (15)

comment:1 Changed 16 months 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 16 months ago by carsten.klein@…

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

comment:2 Changed 16 months 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 16 months 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 16 months 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 16 months ago by carsten.klein@…

comment:5 Changed 16 months ago by anonymous

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

comment:6 in reply to: ↑ 4 Changed 16 months 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 16 months 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 16 months ago by carsten.klein@…

comment:8 Changed 16 months ago by carsten.klein@…

Included the newly added apps for backing the test cases.

comment:9 Changed 16 months 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 16 months 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 16 months ago by carsten.klein@…

Correct handling of unmanaged models and duplicate db_table declaration

Changed 16 months ago by carsten.klein@…

Adds test case for unmanaged model classes

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.