[Django] #30083: Model instance state not correctly set when initializing a model with from

Django

unread,

Jan 7, 2019, 10:39:05 AM1/7/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with
from_db
-------------------------------------+-------------------------------------
Reporter: Tirzono | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 2.1
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
When loading a model instance through `from_db`, it first makes a new
instance and after sets the state:

{{{#!python
@classmethod
def from_db(cls, db, field_names, values):
if len(values) != len(cls._meta.concrete_fields):
values_iter = iter(values)
values = [
next(values_iter) if f.attname in field_names else
DEFERRED
for f in cls._meta.concrete_fields
]
new = cls(*values)
new._state.adding = False
new._state.db = db
return new
}}}

This means that during the class initialization, the model instance state
is equal to `_state.adding = True` and `_state.db = None`. This can cause
troubles with related queries in the pre-/post-init methods. For example:

{{{#!python
class Parent(models.Model):
pass

class Child(models.Model):
parent = models.ForeignKey(Parent)

def foo(instance, **kwargs)
print(instance._state.adding)
print(instance._state.db)

# It could be that we initialize the model while it does not exist in
the
# database, so we check whether `instance.parent` exists first.
if getattr(instance, 'parent', None):
# Since we try to access instance.parent, it passes the `instance`
# as hint to the router, but `instance._state.db` is `None`, so it
defaults
# to the `DEFAULT_DB_ALIAS`.
assert instance._state.db == instance.parent._state.db

signals.post_init.connect(foo, sender=Child)
}}}

Trying to fetch an instance of `Child`, will now return:

{{{#!python
>>> child = Child.objects.using('second').get(pk=1)
True
None
AssertionError
}}}

The current behavior indicates that one should never do related queries in
the `pre_init` or `post_init` hooks, because it cannot be guaranteed that
the related queries are done on the same database you're retrieving the
instance from. Currently there's also no way of getting to know the on
which database the query was done for in the `pre_init` and `post_init`
hooks because the state is set after those signals are fired.

One related issue is #18532, which is only talking about the
`_state.adding` flag. There's a suggested solution over there, but I'm not
sure if that's the best way to go. I think there should be at least a note
in the Django documentation at the `pre_init` and `post_init` hooks saying
that related queries should be avoided, because it cannot be guaranteed
that they will be run on the same database you're getting the instance
from.

--
Ticket URL: <https://code.djangoproject.com/ticket/30083>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Jan 8, 2019, 10:17:32 AM1/8/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Model.from_db()

-------------------------------------+-------------------------------------
Reporter: Tirzono | Owner: nobody
Type: Bug | Status: new

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:1>

Django

unread,

Jan 9, 2019, 3:30:34 AM1/9/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Model.from_db()
-------------------------------------+-------------------------------------
Reporter: Tirzono | Owner: Nasir
| Hussain
Type: Bug | Status: assigned

Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Nasir Hussain):

* status: new => assigned
* owner: nobody => Nasir Hussain

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:2>

Django

unread,

Jan 9, 2019, 3:54:09 AM1/9/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Model.from_db()
-------------------------------------+-------------------------------------
Reporter: Tirzono | Owner: (none)
Type: Bug | Status: new

Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Nasir Hussain):

* status: assigned => new
* owner: Nasir Hussain => (none)

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:3>

Django

unread,

Jan 20, 2019, 5:29:10 AM1/20/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Model.from_db()
-------------------------------------+-------------------------------------
Reporter: Sebastiaan ten Pas | Owner: Nasir

| Hussain
Type: Bug | Status: assigned

Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Nasir Hussain):

* owner: nobody => Nasir Hussain

* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:2>

Django

unread,

Jan 21, 2019, 7:40:21 AM1/21/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Nasir Hussain):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:3>

Django

unread,

Feb 20, 2019, 12:09:20 AM2/20/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* needs_better_patch: 0 => 1

Comment:

I left comments for improvements on the PR. After a read through of #18532
I'm still not convinced the addition in complexity is worth it. I never
add to use init signals though but performing queries in receivers seems
like a recipe for disaster.

The suggested implementation will also have the side effect of exposing
instances with a `._state` attribute in `pre_init` signal while it wasn't
the case before. If we wanted to maintain backward compatibility it could
be fired from `Model.__new__` through which is probably where it should
belong in the first place.

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:4>

Django

unread,

Jul 17, 2019, 2:30:52 PM7/17/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Nasir Hussain):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:5>

Django

unread,

Jul 19, 2019, 6:07:19 AM7/19/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Model.from_db()
-------------------------------------+-------------------------------------
Reporter: Sebastiaan ten Pas | Owner: Nasir
| Hussain

Type: Bug | Status: closed

Component: Database layer | Version: 2.1
(models, ORM) |

Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by felixxm):

* status: assigned => closed
* resolution: => wontfix

Comment:

When we take into account performance doubts, `__new__()` tricky,
additional complexity, and that we can encourage bad practices to perform
queries in receivers of `pre_init` or `post_init` signals (hidden N+1
queries on every queryset iteration), I think we shouldn't fix this (see
also [https://code.djangoproject.com/ticket/18532#comment:5 Anssi's
comment] and [https://code.djangoproject.com/ticket/30083#comment:4
Simon's comment]). It's just not worth it. I'm going to add notes to the
signal documentation.

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:6>

Django

unread,

Jul 19, 2019, 10:06:38 AM7/19/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"a2e1c17f193f5017e1f6fac7d860f1f9e34d7892" a2e1c17f]:
{{{
#!CommitTicketReference repository=""
revision="a2e1c17f193f5017e1f6fac7d860f1f9e34d7892"
Refs #30083 -- Clarified database state of instances in signals.pre_init
docs.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:7>

Django

unread,

Jul 19, 2019, 10:06:39 AM7/19/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"fc1182af01c391ce33d7fcf51c756829c6a11d5b" fc1182af]:
{{{
#!CommitTicketReference repository=""
revision="fc1182af01c391ce33d7fcf51c756829c6a11d5b"
Refs #30083 -- Added a warning about performing queries in pre/post_init
receivers.

Thanks Carlton Gibson the review.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:8>

Django

unread,

Jul 19, 2019, 10:08:30 AM7/19/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"506f800eadea2ab8c9d90f00278b4b03f3a9b771" 506f800e]:
{{{
#!CommitTicketReference repository=""
revision="506f800eadea2ab8c9d90f00278b4b03f3a9b771"
[2.2.x] Refs #30083 -- Added a warning about performing queries in
pre/post_init receivers.

Thanks Carlton Gibson the review.

Backport of fc1182af01c391ce33d7fcf51c756829c6a11d5b from master
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:10>

Django

unread,

Jul 19, 2019, 10:08:30 AM7/19/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"fa3ae446d938455b47fc978cdfecb956b1238812" fa3ae44]:
{{{
#!CommitTicketReference repository=""
revision="fa3ae446d938455b47fc978cdfecb956b1238812"
[2.2.x] Refs #30083 -- Clarified database state of instances in
signals.pre_init docs.

Backport of a2e1c17f193f5017e1f6fac7d860f1f9e34d7892 from master
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:9>

Django

unread,

Jul 19, 2019, 3:00:58 PM7/19/19

to django-...@googlegroups.com

#30083: Model instance state not correctly set when initializing a model with

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sebastiaan ten Pas):

Thanks for the PR Mariusz Felisiak and your initial PR Mariusz Felisiak. I
would prefer to have the code change, as I still consider it a bug, but I
can live with only the documentation change for now.

--
Ticket URL: <https://code.djangoproject.com/ticket/30083#comment:11>

Reply all

Reply to author

Forward

[Django] #30083: Model instance state not correctly set when initializing a model with from_db

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django