Opened 6 years ago

Closed 6 years ago

Last modified 5 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: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


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 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 6 years ago.
Proposed patch. (3.9 KB) - added by gavoja 6 years ago.
Sample application with a test case. Run on trunk revision 13188.

Download all attachments as: .zip

Change History (7)

Changed 6 years ago by gavoja

Proposed patch.

comment:1 Changed 6 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to wontfix
  • Status changed from new to 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.

Changed 6 years ago by gavoja

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

comment:2 Changed 6 years ago by gavoja

  • Resolution wontfix deleted
  • Status changed from closed to 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 Changed 6 years ago by russellm

  • Resolution set to fixed
  • Status changed from reopened to closed

(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 Changed 6 years ago by russellm

  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to 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.

comment:5 Changed 5 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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