Hi Luke, 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 [snip]
> 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 fix it. 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 situation. 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. Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
