Opened 13 years ago

Closed 12 years ago

#16173 closed Cleanup/optimization (wontfix)

manager.get on foreign key fields (related fields)

Reported by: nitinbhide@… Owned by: Tobias McNulty
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: manager foreignkey
Cc: fhahn Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description (last modified by Aymeric Augustin)

__get__ on ForeignKey field uses manager to query the values.

The code is as given below:

            # If the related manager indicates that it should be used for
            # related fields, respect that.
            rel_mgr = self.field.rel.to._default_manager
            db = router.db_for_read(self.field.rel.to, instance=instance)            
            if getattr(rel_mgr, 'use_for_related_fields', False):
                rel_obj = rel_mgr.using(db).get(**params)


Check the last line rel_obj = rel_mgr.using(db).get(**params) for using rel_mgr with multiple databases. rel_mgr.using(db) returns a queryset. If developer has written a custom manager class for which get is overriden, then custom managers get function will never get called. This results in surprises to the developer. I think this line should changed to:

    rel_obj = rel_mgr.db_manager(db).get(**params)

Attachments (2)

unittest.diff (2.6 KB ) - added by Silvan Spross 13 years ago.
tried to create a unittest for this
patch16173.diff (1.9 KB ) - added by fhahn 12 years ago.
updated patch to r17591

Download all attachments as: .zip

Change History (11)

comment:1 by Aymeric Augustin, 13 years ago

Description: modified (diff)
UI/UX: unset

Fixed wiki formatting.

comment:2 by Stephen Burrows, 13 years ago

Easy pickings: set
Keywords: manager foreignkey added
Needs tests: set
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization
Version: 1.3SVN

I often find that if I want to provide a custom manager method, I also need a custom queryset... but this does seem reasonable.

by Silvan Spross, 13 years ago

Attachment: unittest.diff added

tried to create a unittest for this

comment:3 by Silvan Spross, 13 years ago

i tried to proof that and it does not work. maybe the way i'm testing it is wrong :)

comment:4 by nitinbhide@…, 13 years ago

I think you are testing it wrong. As per my understanding, a1.reporter call in

self.assertRaises(a1.reporter, SuccessfulExecutedException) 

will result in a call get function of Reporter's _default_manager and not to get function of ArticleWithDummyManager's default_manager.

comment:5 by Malcolm Tredinnick, 13 years ago

The test looks like it's going along the right lines. I would suggest to not be so fancy with subclassed models (forcing us all to remember the default manager inheritance rules, which are very tricky). Just make a single model with a relation and explicit default manager. Simpler is better for tests like this so that you're only testing related managers, not related managers and default manager in inherited situtations.

comment:6 by Tobias McNulty, 13 years ago

Owner: changed from nobody to Tobias McNulty
Status: newassigned

by fhahn, 12 years ago

Attachment: patch16173.diff added

updated patch to r17591

comment:7 by fhahn, 12 years ago

Cc: fhahn added
Needs tests: unset

I've updated the patch to work with r17591 and the test should work correctly.

But the manager's get uses Manager.get_queryset() now and I'm not sure if returning a Manager instance (rel_mgr.db_manager(db)) instead of a Queryset (rel_mgr.using(db)) is acceptable.

comment:8 by nitinbhide@…, 12 years ago

'But the manager's get uses Manager.get_queryset() now and I'm not sure if returning a Manager instance (rel_mgr.db_manager(db)) instead of a Queryset (rel_mgr.using(db)) is acceptable'.

I am not sure if correctly understood this comment. I am not suggesting any change in Manager.get and of course returning Manager instance instead of queryset will be a wrong decision.

The change is required in get function of class ReverseSingleRelatedObjectDescriptor.

# If the related manager indicates that it should be used for
# related fields, respect that.
rel_mgr = self.field.rel.to._default_manager
db = router.db_for_read(self.field.rel.to, instance=instance)
if getattr(rel_mgr, 'use_for_related_fields', False):
    rel_obj = rel_mgr.using(db).get(**params)
else:
    rel_obj = QuerySet(self.field.rel.to).using(db).get(**params)

Check the line,

rel_obj = rel_mgr.using(db).get(**params). 

Assume that rel_mgr is a 'custom manager' derived from model.Manager and it overrides get function. In such cases, the custom Manager's derived get function will not get called. Because using(db) will return a queryset while rel_mgr.db_manager(db) will return a 'Manager'. Logically rel_mgr.db_manager(db).get() makes more sense to me. Also I think changes passes all the existing test cases as well. ReverseSingleRelatedObjectDescriptor.get function is still returning an object and there is no change in model.Manager.get() function.

in reply to:  8 comment:9 by Carl Meyer, 12 years ago

Patch needs improvement: set
Resolution: wontfix
Status: assignedclosed

Replying to nitinbhide@…:

Check the line,

rel_obj = rel_mgr.using(db).get(**params). 

This line of code no longer exists in trunk Django, the code has been rearranged in a way that makes this change more difficult (ReverseSingleRelatedObjectDescriptor now has a get_query_set method, which would have to be changed to get_manager).

I'm not convinced that it's reasonable to even expect the related manager's get() method to be called here; get() is one of the many Manager methods that is simply a pass-through to QuerySet, and I think it would be a mistake for Django to try to start providing guarantees of when the pass-through Manager method is called vs the same method on the Manager's queryset. If you want to override reliably, you need to override the lower-level get_query_set method on the Manager and provide a custom QuerySet class with your get functionality.

Closing wontfix.

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