manager.get on foreign key fields (related fields)

[nitinbhide@…]

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

[Aymeric Augustin]

Fixed wiki formatting.

[Stephen Burrows]

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

[Silvan Spross]

tried to create a unittest for this

[Silvan Spross]

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

[nitinbhide@…]

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.

[Malcolm Tredinnick]

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.

[fhahn]

updated patch to r17591

[fhahn]

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.

[nitinbhide@…]

'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.

[Carl Meyer]

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.