Opened 14 years ago

Closed 14 years ago

Last modified 13 years ago

#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)

collect_sub_objects.patch (2.5 KB ) - added by gavoja 14 years ago.
Proposed patch.
test_proj.zip (3.9 KB ) - added by gavoja 14 years ago.
Sample application with a test case. Run on trunk revision 13188.

Download all attachments as: .zip

Change History (7)

by gavoja, 14 years ago

Attachment: collect_sub_objects.patch added

Proposed patch.

comment:1 by Russell Keith-Magee, 14 years ago

Resolution: wontfix
Status: newclosed

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 gavoja, 14 years ago

Attachment: test_proj.zip added

Sample application with a test case. Run on trunk revision 13188.

comment:2 by gavoja, 14 years ago

Resolution: wontfix
Status: closedreopened

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 Russell Keith-Magee, 14 years ago

Resolution: fixed
Status: reopenedclosed

(In [13232]) Fixed #13513 -- Ensured that queries collecting deleted objects are issued on the right database, especially when dealing with m2m intermediate tables. Thanks to gavoja for the report.

comment:4 by Russell Keith-Magee, 14 years ago

milestone: 1.2
Triage Stage: UnreviewedAccepted

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.

comment:5 by Jacob, 13 years ago

milestone: 1.2

Milestone 1.2 deleted

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