Opened 11 years ago

Closed 5 years ago

Last modified 5 years ago

#20098 closed New feature (fixed)

Add validation for models with the same db_table

Reported by: carsten.klein@… Owned by: Sanyam Khurana
Component: Core (System checks) Version: dev
Severity: Normal Keywords:
Cc: bmispelon@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no 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@… 11 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@… 11 years ago.
20098-support.patch (2.6 KB ) - added by carsten.klein@… 11 years ago.
20098.2.patch (3.6 KB ) - added by carsten.klein@… 11 years ago.
Correct handling of unmanaged models and duplicate db_table declaration
20098-Support.patch (3.4 KB ) - added by carsten.klein@… 11 years ago.
Adds test case for unmanaged model classes

Download all attachments as: .zip

Change History (21)

comment:1 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

by carsten.klein@…, 11 years ago

Attachment: patch20098 added

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

comment:2 by Baptiste Mispelon, 11 years ago

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 by Keryn Knight <django@…>, 11 years ago

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 by carsten.klein@…, 11 years ago

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.

by carsten.klein@…, 11 years ago

Attachment: 20098.patch added

comment:5 by anonymous, 11 years ago

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

in reply to:  4 comment:6 by carsten.klein@…, 11 years ago

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 by Baptiste Mispelon, 11 years ago

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.

by carsten.klein@…, 11 years ago

Attachment: 20098-support.patch added

comment:8 by carsten.klein@…, 11 years ago

Included the newly added apps for backing the test cases.

comment:9 by carsten.klein@…, 11 years ago

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.

in reply to:  3 comment:10 by carsten.klein@…, 11 years ago

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!

by carsten.klein@…, 11 years ago

Attachment: 20098.2.patch added

Correct handling of unmanaged models and duplicate db_table declaration

by carsten.klein@…, 11 years ago

Attachment: 20098-Support.patch added

Adds test case for unmanaged model classes

comment:11 by Tim Graham, 9 years ago

Component: Database layer (models, ORM)Core (System checks)
Summary: Django validate command fails to detect that multiple models declare the same db_tableAdd validation for models with the same db_table
Type: Cleanup/optimizationNew feature
Version: 1.5master

comment:12 by Sanyam Khurana, 5 years ago

Owner: changed from nobody to Sanyam Khurana
Status: newassigned

There is a lot of regression in applying the patch cleanly which revolve mostly around the test cases within Django. I'm working to modify the last version of the patch, cleanly apply it to the master branch and fix test cases.

PR will be up soon.

comment:13 by Simon Charette, 5 years ago

Needs tests: unset
Triage Stage: AcceptedReady for checkin

Not sure if it's worth adding a co-author given the final patch is quite different from the original one here.

comment:14 by Sanyam Khurana, 5 years ago

Hello Tim,

I've addressed your reviews on the patch. Can you please have a look at this when you've some time?

Please let me know if there are any more changes needed.

Thanks!

comment:15 by Tim Graham <timograham@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 5d25804:

Fixed #20098 -- Added a check for model Meta.db_table collisions.

comment:16 by Claude Paroz, 5 years ago

Note that a regression was spotted in #30673.

Note: See TracTickets for help on using tickets.
Back to Top