On Dec 16, 9:47 am, Luke Plant <L.Plant...@cantab.net> wrote:
> For the latter, I think we should use instance._state.adding to reliably
> detect whether a PK has been set or not. This could cause some
> While I was there, I fixed _get_next_or_previous_by_FIELD to use
> self._state.adding, rather than 'not self.pk', for the similar check
> that it does. I also fixed OneToOne fields, and updated ManyToMany
> lookups to check _state.adding rather than check the PK field.
> If people agree that the existing behaviour is a bug and that workflows
> that rely on it are broken, I'll go ahead and commit. Russell closed the
> bug as WONTFIX, but I think based on a misunderstanding (I misunderstood
> the original report in the same way).
This makes me a little bit uneasy. Before we haphazardly sneak
instance._state.adding into more and more places, I think we need to
have a broader discussion about the desired semantics of insert vs
update in the ORM.
Historically, Django has avoided tracking any add-vs-update state on
model instances. Insert-vs-update was (and is) decided at save-time,
by checking whether the PK exists in the DB. This works fine, although
it requires an extra SELECT query at every save.
Other code (in Django, or third-party) that needed to know whether an
instance was "new" would check for "self.pk is None" (including
ManyToManyField, as you note). This idiom is broken with CharField PKs
whose value is set explicitly on new instances before saving, but
apparently that's never been enough of a problem to motivate anyone to
Instance._state.adding is new in 1.2 (in 1.2 it was instance._adding
and only ever monkeypatched onto a model instance by ModelForm; I
changed it to instance._state.adding and encapsulated it inside the
ORM just a few weeks ago in r14612). It was added as part of model-
validation, in order to allow uniqueness-validation of PKs to work
correctly in the very same edge cases (explicitly-set CharField PK)
that generally break the "pk is None" idiom. Currently it is used only
by uniqueness validation checks, nowhere else in the ORM.
It bothers me that we now have two parallel systems in the ORM for
deciding whether an instance is "new," and no real rhyme or reason to
when one is used vs the other. I don't want to exacerbate that
In working on r14612 I spent hours looking for a way to get rid of
instance._adding entirely, but concluded that it was not possible
without breaking uniqueness validation on explicit char PKs.
ISTM that we should either keep the scope of instance._state.adding as
limited as possible (i.e. avoid introducing new uses of it), if we're
satisfied with the status quo and the limitations of "pk is None," or
we should consider fully embracing instance._state.adding as the
endorsed way to determine whether a model instance is new or saved,
and banishing the "pk is None" idiom from Django's codebase. The
latter option could potentially include even changing the behavior of
Model.save() to use instance._state.adding rather than performing the
extra SELECT query, though the backwards-compatibility implications in
edge cases there might be prohibitive.
Instance._state.adding is set to True by default on new model
instances, and set to False anywhere the ORM generates a model
instance from a database query. Using instance._state.adding implies
that we do not support manually creating a model instance and setting
its PK to a value known to exist in the database, as a way to force an
update of that row (doing this already will break uniqueness
validation, unless you also manually tweak instance._state.adding to
False). On the upside, using instance._state.adding provides better
support for explicitly-set PKs than "pk is None," because it doesn't
overload the meaning of the PK field with new/not-new information.
On the whole, I think instance._state.adding has some real advantages
over "pk is None," and I'm even intrigued by the possibility of using
it to avoid the extra SELECT query at save-time. But if we're going to
make this change, let's be fully clear about the change we're making,
and make it consistently across the codebase.