Code

Opened 3 years ago

Closed 2 years ago

#16173 closed Cleanup/optimization (wontfix)

manager.get on foreign key fields (related fields)

Reported by: nitinbhide@… Owned by: tobias
Component: Database layer (models, ORM) Version: master
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 aaugustin)

__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 sspross 3 years ago.
tried to create a unittest for this
patch16173.diff (1.9 KB) - added by fhahn 2 years ago.
updated patch to r17591

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by aaugustin

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • UI/UX unset

Fixed wiki formatting.

comment:2 Changed 3 years ago by melinath

  • Easy pickings set
  • Keywords manager foreignkey added
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/optimization
  • Version changed from 1.3 to SVN

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

Changed 3 years ago by sspross

tried to create a unittest for this

comment:3 Changed 3 years ago by sspross

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

comment:4 Changed 3 years ago by 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.

comment:5 Changed 3 years ago by mtredinnick

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 Changed 3 years ago by tobias

  • Owner changed from nobody to tobias
  • Status changed from new to assigned

Changed 2 years ago by fhahn

updated patch to r17591

comment:7 Changed 2 years ago by fhahn

  • 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 follow-up: Changed 2 years ago by 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.

comment:9 in reply to: ↑ 8 Changed 2 years ago by carljm

  • Patch needs improvement set
  • Resolution set to wontfix
  • Status changed from assigned to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.