Opened 5 years ago

Closed 11 months ago

#17143 closed Bug (fixed)

select_related makes base __init__ unsafe

Reported by: Leo Owned by:
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

For the following models setup:

class A(models.Model):
    foo = models.CharField(max_length=1, default='N')
    
    def __init__(self, *args, **kwargs):
        super(A, self).__init__(*args, **kwargs)
        
        print self.foo
    
class B(A):
    bar = models.CharField(max_length=1, default='X')

The print in in class A is just a proxy for doing something else with the field. My specific use case is that I'm trying to save the field off so I can detect whether it has been changed on save or not - but that doesn't impact the bug.

>>> b = B(foo='Y', bar='Z')
Y
>>> b.save()
>>> A.objects.select_related('b')
Y
N
[<A: A object>]

The second call to A.__init__ is to create the B object, however, it doesn't have all of the fields from the original A so while in A.__init__, self.foo evaluates to the wrong value.

This is either something broken with what fields select_related loads on related objects, or it needs to be documented as a really really nasty gotcha for inherited classes/select_related.

Attachments (2)

17143_test.diff (2.1 KB) - added by calvinspealman 5 years ago.
Turned the example into a test case
17143.diff (3.5 KB) - added by calvinspealman 5 years ago.
Demonstration of fix. needs review and feedback.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 5 years ago by aaugustin

  • Component changed from Database layer (models, ORM) to Documentation
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/optimization

To the best of my knowledge, that's the expected behavior. I agree the docs could be improved. Basically, B carries only the bar field and a one-to-one relation to A.

comment:2 Changed 5 years ago by carljm

  • Component changed from Documentation to Database layer (models, ORM)
  • Type changed from Cleanup/optimization to Bug

I'm not sure I agree, aaugustin. I got further clarification from Leo that this only happens with select_related, not when selecting directly on Bs. IMO that indicates that it is a bug, and might be fixable. I might be wrong; would have to sit down and look at it in more detail to see what a fix would entail. In any case, if Leo or anyone else is interested in looking at a possible fix, I don't want to discourage someone from taking a closer look :-)

Last edited 5 years ago by carljm (previous) (diff)

comment:3 Changed 5 years ago by aaugustin

Indeed, if the problem happens with select_related but not elsewhere, it could be a bug. This needs a test case, for instance with an assertion on the arguments received in __init__.

comment:4 Changed 5 years ago by calvinspealman

I broke this down into a few test conditions and found the report to be accurate. Only through select_related is the default value used, where in all other instantiations of the B the correct value is found.

Changed 5 years ago by calvinspealman

Turned the example into a test case

comment:5 Changed 5 years ago by calvinspealman

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

Changed 5 years ago by calvinspealman

Demonstration of fix. needs review and feedback.

comment:6 Changed 5 years ago by calvinspealman

  • Has patch set

comment:7 Changed 3 years ago by timo

  • Patch needs improvement set

I'm not sure if this is still an issue, but the test in 17143.diff passes as far back as I tested (stable/1.3.x) so it doesn't appear to be a proper regression test for the issue.

comment:8 Changed 11 months ago by timgraham

  • Has patch unset
  • Owner calvinspealman deleted
  • Patch needs improvement unset
  • Status changed from assigned to new

Based on the test case in the description, it looks like this was fixed in Django 1.6 by 1194a9699932088385f9f88869be28a251597f45. The result is now:

>>> A.objects.select_related('b')
Y
Y

However, no tests were added in that patch, so it seems a good idea to try to add some before closing this ticket.

comment:9 Changed 11 months ago by timgraham

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

Actually the tests were added in f51e409a5fb34020e170494320a421503689aea0 -- some didn't pass until the follow up commit.

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