#13513 closed (fixed)
_collect_sub_objects() does not take the database into the account
Reported by: | gavoja | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Keywords: | ||
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The implementation of delete()
seem to be designed to remove all the related objects from the database that was passed via using argument, as the line below states:
delete_objects(seen_objs, using)
The only problem is that the related object collection that takes place befor this call, does not take the database into the account. The proposed patch solves this problem.
I also found a couple occurences of direct _collect_sub_objects() call outside the base.py
which I haven't check tho, but the patch should be backward compatible.
Attachments (2)
Change History (7)
by , 15 years ago
Attachment: | collect_sub_objects.patch added |
---|
comment:1 by , 15 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
I'm not sure I see what this patch is trying to achieve. The delete query must be executed on a write-enabled database, but you should be able to retrieve the list of objects to be deleted from any read-enabled database. The only way I can see that this might fail is if your routing rules aren't consistent - that is, if the db_for_read() and db_for_write() don't return compatible databases, but in this case, you have bigger problems. I can't see any obvious reason that the deleted object set needs to be retrieved from a write database.
Of course, I could be wrong, but you don't provide a test case that validates under what conditions this problem would manifest itself.
Closing wontfix; If you can provide a test case under which deletion is problematic -- even a description of a set of circumstances -- please reopen.
by , 15 years ago
Attachment: | test_proj.zip added |
---|
Sample application with a test case. Run on trunk revision 13188.
comment:2 by , 15 years ago
Resolution: | wontfix |
---|---|
Status: | closed → reopened |
Sorry for not being too specific. We use sort of sharding, where we store records in different databases depending on some external conditions. I have prepared a sample application with a test that causes DatabaseError, which I believe should not be an expected behavior for this case.
comment:3 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
comment:4 by , 15 years ago
milestone: | → 1.2 |
---|---|
Triage Stage: | Unreviewed → Accepted |
To explain the discrepancy between the proposed patch and the committed patch; it isn't necessary to pass the database down as an argument - it can (and should) be derived from the relation with related objects. This was being correctly handled for all relation types *except* m2m relations, which have an m2m table; because the m2m table wasn't being queried using one of the descriptors on the related objects that would force database assignment, the query was being allocated to the default database. The solution is to do a write database lookup using the source object as a hint, and force the query on the through object onto that database. This is analogous to what is done in the m2m descriptors.
Proposed patch.