Re: [Django] #4102: Allow UPDATE of only specific fields in model.save()

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin | Owner: cgrady
Grady <cgrady@…> | Status: new
Type: New | Component: Database layer
feature | (models, ORM)
Milestone: | Severity: Normal
Version: SVN | Keywords: update fields sql
Resolution: | row table modified
Triage Stage: Accepted | Has patch: 1
Needs documentation: 1 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by ramiro):

* keywords: => update fields sql row table modified
* needs_better_patch: 1 => 0
* easy: => 0

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

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin | Owner: cgrady
Grady <cgrady@…> | Status: new
Type: New | Component: Database layer
feature | (models, ORM)
Milestone: | Severity: Normal
Version: SVN | Keywords: update fields sql
Resolution: | row table modified
Triage Stage: Accepted | Has patch: 1
Needs documentation: 1 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0

UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by jlorieau):

* ui_ux: => 0

Comment:

There are a few bugs in this feature that are fixed with my patch
(4102_Django1.3_JL20110610.path).

Field pre_save methods, like the {{{DateTime}}} auto_now, can change the
value of a field just before a save, and these changes are not included in
the modified_attrs list. I've made modifications to account for changes in
field values by pre_save, and I've included a test to account for this
change.

I've made a few other changes, which improve and fix bugs in this new
feature:

1. In the {{{__setattr__}}} method (base.py, line 370), I retrieve the
{{{_modified_attrs}}} list using a setdefault method on the object
{{{__dict__}}}. This is needed for situations when a model's
{{{__init__}}} has been overloaded and attribute access occurs before the
parent {{{__init__}}} is called.

2. I've moved the {{{_reset_modified_attrs}}} call in the save_base to
line 580 (base.py, line 580). Some Field pre_save methods, including some
that I've developed, need access to the {{{_modified_attrs}}} list before
it is cleared.

In addition, I have a few suggestions that I believe would enhance this
feature.

I propose that a 'save_all' flag be included in the save argument list. It
could be used to revert to the original behavior to save all fields at
once. Furthermore, a setting in the settings.py could change the default
behavior of the model to save only modified fields or all.

The new feature could be extended with a new signal named
{{{TrackModification}}} or {{{TrackChange}}}. On object creation,
modification, or deletion, a signal would be emitted with a list of the
modified attributes--these would, of course, have to include fields
modified by {{{pre_save}}}. Alternatively, this change could be integrated
into the {{{post_save}}} signal. I propose this as a feature because it
would be relatively easy to implement, and very useful to have.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:60>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin | Owner: cgrady
Grady <cgrady@…> | Status: new
Type: New | Component: Database layer
feature | (models, ORM)
Milestone: | Severity: Normal
Version: SVN | Keywords: update fields sql
Resolution: | row table modified
Triage Stage: Accepted | Has patch: 1
Needs documentation: 1 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------

Comment (by jlorieau):

Replying to [comment:60 jlorieau]:

> The new feature could be extended with a new signal named
{{{TrackModification}}} or {{{TrackChange}}}. On object creation,
modification, or deletion

Just to clarify on this point, I am proposing to emit a signal only when
{{{save}}} (and possibly {{{__init__}}} and {{{delete}}}) is called, and
not on every invocation of {{{__setattr__}}}. Basically, we could include
the {{{_modified_attrs}}} information to existing signals (post_save and
post_init) for free, making them much more powerful--for example, in
invalidating a cached attribute only when that attribute actually changes.

This would be a new ticket that builds on this feature, if this feature is
approved.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:61>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin | Owner: cgrady
Grady <cgrady@…> | Status: new
Type: New | Component: Database layer
feature | (models, ORM)
Milestone: | Severity: Normal
Version: SVN | Keywords: update fields sql
Resolution: | row table modified
Triage Stage: Accepted | Has patch: 1
Needs documentation: 1 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------

Changes (by kmike):

* cc: kmike84@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:62>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin | Owner: cgrady
Grady <cgrady@…> | Status: new
Type: New | Component: Database layer
feature | (models, ORM)
Milestone: | Severity: Normal
Version: SVN | Keywords: update fields sql
Resolution: | row table modified
Triage Stage: Accepted | Has patch: 1
Needs documentation: 1 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------

Changes (by sfllaw):

* cc: sfllaw@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:63>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin | Owner: cgrady
Grady <cgrady@…> | Status: new
Type: New | Component: Database layer
feature | (models, ORM)
Milestone: | Severity: Normal
Version: SVN | Keywords: update fields sql
Resolution: | row table modified
Triage Stage: Accepted | Has patch: 1
Needs documentation: 1 | Needs tests: 0

Patch needs improvement: 1 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):

* cc: anssi.kaariainen@… (added)
* needs_better_patch: 0 => 1

Comment:

Some quick notes:
- I don't think this works when using deferred field loading
- The _modified_attrs should go to the model._state, not directly to the
model
- The _state.adding should maybe play a part in this. What about: if we
are adding, then every field has changed by default? Maybe this is not so
hot idea, but could be worth exploring.
- There is a trick one can use to speed up model `__init__`:
{{{
# Pseudocode

class Model():
def __setattr__(args):
...
_my_setattr = __setattr__

def __init__(self, ...):
# Check if inheriting class has defined a __setattr__ method
if self._my_setattr == self.__setattr__:
# If not, we can skip our own __setattr__ and use
# directly the super __setattr__
_set = super(Model, self).__setattr__
else:
# We need to call the __setattr__ defined in inheriting class
# (backwards compatibility).
_set = self.__setattr__
...
for field, val in izip(fields, args):
_set(self, field.attname, val)
...

}}}

This way if there is no overriding `__setattr__` there is almost no speed
loss in Model's `__init__`. On the other hand this is a dirty trick that
could easily bite...

Also a check could be done to guard against overriding `__setattr__` doing
direct `__dict__` manipulation and not calling our `__setattr__`. On
Django startup create a instance of every Model class and see if a
modification registers to _modified_attrs. If not, raise an Exception.
Python documentation says that `__setattr__` should call the base
`__setattr__` for new style classes, but it is a safe bet that there are
users who do not do that.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:64>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin | Owner: cgrady
Grady <cgrady@…> | Status: new
Type: New | Component: Database layer
feature | (models, ORM)
Milestone: | Severity: Normal
Version: SVN | Keywords: update fields sql
Resolution: | row table modified
Triage Stage: Accepted | Has patch: 1
Needs documentation: 1 | Needs tests: 0
Patch needs improvement: 1 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------

Comment (by akaariai):

Correcting myself: Actually, I do think this works with deferred
attributes, the deferred loading of attributes accesses directly the
`__dict__` of the instance, and if the attribute has not been accessed,
save() will not access it. This is actually a big plus to this feature: if
one uses .only() to load just the attributes needed in data manipulation
and then saves the object back, things work as expected. Currently all the
deferred attributes will be fetched before save, one at a time if I am not
mistaken...

On the other hand, there is a minor backwards incompatibility. The
following is valid at the moment (although not necessarily very wise thing
to do):

{{{
# Lets fetch an object and update another object with it's data.
m = M.objects.get(pk=1)
m.pk = 2 # assume pk = 2 is in the DB.
m.save() # Old code would have updated all attributes, new code doesn't.
}}}

This could be fixed by setting all attrs as changed when the pk has
changed.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:65>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------

Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by dtran):

* cc: dtran (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:66>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by Kronuz):

* cc: Kronuz (added)

Comment:

With the current patch in multitable inheritance, when I modify an already
existing object and save it, it only saves data to the parent. This
happens because `_reset_modified_attrs()` is called during the topmost
first parent's save_base() call and further updates are ignored since
`_modified_attrs` no longer have anything after that. I'm attaching a new
patch to fix this.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:67>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by akaariai):

The patch has a fundamental problem. Some field types are mutable in-
place. This means that the check in `__setattr__` will never see a change
if the value is mutated in-place, and hence this approach does not work
for in-place mutable fields. See [https://groups.google.com/group/django-
developers/browse_thread/thread/b028e3c7a366330e/2d2df454bf3d61fc#2d2df454bf3d61fc
this] post for some discussion related to this problem & some ideas how to
solve the problem.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:68>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by mattlong):

After reviewing the history of this ticket, it seems what started as a
simple opt-in feature request of adding a field white-list to the Model's
save function morphed into a complicated dirty flag approach that
obviously has many edge cases and performance implications given that this
ticket has been open for 5 years now. Just compare the very first proposed
patch to the latest to see what I mean.

Unfortunately, this seemed to have happened without much discussion about
the relative merits of each approach. I would prefer the Django ORM leave
it to me to decide which fields I would like to update rather than relying
on it to correctly infer which fields I would like updated. Clearly some
people differently and favor the dirty flag approach for a more hands-off
approach. As such, I propose adding support for both methods so that
developers can choose the right approach for their use case.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:69>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by mattlong):

* cc: mattlong (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:70>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by akaariai):

Implementing only_fields kwarg for model .save() sounds like a plan. That
would be really useful feature, simple to implement and I think nobody
will give a -1 to that idea.

In ticket #17332 is a patch which should allow (with minor changes)
override of model._state. Also, the `ModelState` does have some methods in
it which allow tracking of dirty field state (of course, implemented in a
custom state class). That way 3rd party projects could pretty easily
implement tracking of dirty field state in the way they wish (hashing,
flags, storing the old values...). Using the ._state information you can
then implement custom .save() which automatically saves only changed
fields. Note that the important parts of the patch are the `ModelState`
changes, and where .update()/clear() are called, rest is irrelevant to
this ticket.

It seems clear that a default behavior of saving only changed fields will
not get into Django core.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:71>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by bitwolaiye):

* cc: bitwolaiye (added)

Comment:

when I use #4102-update_modified_fields_only.diff I meet a question that
item.save() will update all field in the first time, but update specific
changed field in the next.
so I change "self._reset_modified_attrs()" in the end of __init__ not in
the top of __init__. because in __init__ __setattr__ will be called, and
problem is come.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:72>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by inactivist):

* cc: inactivist@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:73>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by niwi):

I have been researching the same, and I agree with akaariai: implementing
only_fields kwarg for model .save() sounds like a plan. Furthermore, it is
simpler and more explicit.

Attached the patch based on the original and modified for the current
version.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:74>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |

Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):

* needs_tests: 0 => 1

Comment:

A couple of design considerations:
- if only fields is given should it imply force_update=True. My take is
yes, as I don't think there will be a use case for only_fields + insert.
Set the field values to NULL instead. In addition only_fields + insert
could lead to some complicated situations. So, maybe the kwarg should
instead be update_fields?
- what if the model does not have fields given in the update_fields
argument? Should it be an error or not. I guess erroring out could be
wise, as otherwise this could hide data loss bugs. This should probably be
done in .save() as a pre-check, and then .save_base just uses the given
set without further checks.
- It might make sense to add the field set to pre/post save signals as
an argument if this is doable from backwards compatibility viewpoint.
- Should a deferred model which is then saved have automatically
update_fields set so that only the non-deferred fields will be saved. I
think currently the fields are loaded from the database and then the
update will update the fields to the just fetched values which does not
make sense.

This should be tested with model inheritance, too. It is a common pain-
point for this kind of feature. The current test doesn't actually seem to
test that the name is updated, so in general more tests needed.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:75>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by niwi):

* cc: niwi@… (added)

Comment:

Interesting considerations. I will perform more tests with your
considerations in mind.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:76>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by niwi):

Here the new patch with more tests and some considerations implemented.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:77>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by akaariai):

Looks good (although I haven't done a full review). Skipping deferred
model handling is OK, it does actually contain some possible issues (load
deferred model, set attributes from a form, save() must work). I think
this could be dealt with, but lets just leave it out for now.

The pre/post save signal should get a new argument. There is some
precedence to adding arguments to signals ("using" in 1.3 for example) so
this should not be impossible to do from backwards compatibility point of
view. The .save() is also documented to take kwargs, so changing save's
signature should be OK, too.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:78>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by niwi):

Here the last patch, with the parameters for signals.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:79>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by akaariai):

The patch looks good to me, I can't spot any serious failures there. Some
corner cases below. I would value other opinions on the issues below.

The patch still lacks the update_fields implies force_update=True part.
Does somebody know of a valid use case for "only fields" behavior for
inserts? My main concern here is that this breaks default values. Another
option (as is done in the patch) is to not imply force_update, but do full
save if the save leads to insert, so that the update_fields is in effect
only for updates. The documentation is currently misleading about the
behavior.

My take on this is to set force_update if update_fields is not None. There
are two main use cases for this feature: performance (in which case you do
want force_update=True), and "update only these fields, leave others as
they are in DB". In the latter case doing an insert would be an error. On
the other hand requiring the user to set the force_update=True is risk
free from Django's perspective.

Should datetime fields with auto_now=True be always included in the fields
list automatically?

If the update_fields is supplied but is empty shouldn't the save be
skipped completely in that case (or if it contains only the PK field)?

And still one more thing: the update_fields is passed to the signal
handler but its type is not known. If it happens to be a list (or any
other mutable container), the signal handler can modify it in place. There
is no safety checks for modifications done there. I am not sure if signal
handlers being able to modify the list is useful or not, but at least the
passed in type should be consistent: either a list or a tuple (actually,
set or immutable set could make more sense).

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:80>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by clay):

* cc: clay@… (removed)

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:81>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by niwi):

Replying to [comment:80 akaariai]:

> The patch looks good to me, I can't spot any serious failures there.
Some corner cases below. I would value other opinions on the issues below.
>
> The patch still lacks the update_fields implies force_update=True part.
Does somebody know of a valid use case for "only fields" behavior for
inserts? My main concern here is that this breaks default values. Another
option (as is done in the patch) is to not imply force_update, but do full
save if the save leads to insert, so that the update_fields is in effect
only for updates. The documentation is currently misleading about the
behavior.

[[BR]]

I've been thinking about this and I think you're right! ``update_fields``
have to implies ``force_update=True``. Although, I agree considerations!

[[BR]]

>
> My take on this is to set force_update if update_fields is not None.
There are two main use cases for this feature: performance (in which case
you do want force_update=True), and "update only these fields, leave
others as they are in DB". In the latter case doing an insert would be an
error. On the other hand requiring the user to set the force_update=True
is risk free from Django's perspective.
>
> Should datetime fields with auto_now=True be always included in the
fields list automatically?

[[BR]]

At this point, I think we should not automatically preselected fields
"DateTimeField" with "auto_now = True". This option is for advanced use,
and in this case one must know what is really saving.

[[BR]]

>
> If the update_fields is supplied but is empty shouldn't the save be
skipped completely in that case (or if it contains only the PK field)?

[[BR]]

If the update_fields is supplied but is empty, now, will have the same
behavior when update_fields is not supplied. In my opinion the behavior is
correct.

[[BR]]

>
> And still one more thing: the update_fields is passed to the signal
handler but its type is not known. If it happens to be a list (or any
other mutable container), the signal handler can modify it in place. There
is no safety checks for modifications done there. I am not sure if signal
handlers being able to modify the list is useful or not, but at least the
passed in type should be consistent: either a list or a tuple (actually,
set or immutable set could make more sense).

[[BR]]

I think it should be a mutable object. Thus, if someone needs it, will
have the ability to automatically add to the list datetime fields with
auto_now = True.

I do not understand what you mean by specifying the type. If the parameter
is None, the signal receives None. If the parameter list/tuple, the signal
receives list/tuple.

Are you thinking about some kind of type checking?

[[BR]]

>
> EDIT: Just noted that there is .delete() calls in the end of the test
methods: no need for that, Django's TestCase will do the cleanup for you.

[[BR]]

I updated the documentation, but my English is not the best in the world.
If you can improve it, I would be very grateful.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:82>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted

Severity: Normal | Needs documentation: 0

Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |

Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):

* needs_docs: 1 => 0
* needs_tests: 1 => 0

Comment:

Replying to [comment:82 niwi]:

> At this point, I think we should not automatically preselected fields
`DateTimeField` with "auto_now = True". This option is for advanced use,
and in this case one must know what is really saving.

Agreed. This is like qs.update() - no addition of auto_now fields there.

> If the update_fields is supplied but is empty, now, will have the same
behavior when update_fields is not supplied. In my opinion the behavior is
correct.

I am not sure about this one. Main concern here is that removing a field
from update_fields can lead to implicit addition of all the model's fields
to the update_fields which feels counter-intuitive.

> I think it should be a mutable object. Thus, if someone needs it, will
have the ability to automatically add to the list datetime fields with
auto_now = True.
>
> I do not understand what you mean by specifying the type. If the
parameter is None, the signal receives None. If the parameter list/tuple,
the signal receives list/tuple.
>
> Are you thinking about some kind of type checking?

The problem is if the user gives update_fields as a tuple then the signal
handler can not mutate it in place - if it is a set you can mutate it with
add - if it is a list then you can append to it. So, I am thinking of
converting the update_fields to a common type for the signal handler.
However not converting can be seen as a feature, too.

> I updated the documentation, but my English is not the best in the
world. If you can improve it, I would be very grateful.

I will see what I can do. The most important thing right now is having
somewhat correct information in correct places, not grammatical accuracy.

If I am not mistaken there are two open items:
1. Should empty, but not None update_fields argument skip save
completely instead of doing a normal save. My feeling is skip the save
here.
2. Should signal handlers be able to mutate the update_fields? It is
possible to leave this up to the caller of .save() - if a list was given
as update_fields, then it is mutable in the signal handler - if a tuple
was given then it isn't. Maybe leaving this as is (but checking that
isinstance(update_fields, (list, tuple))) is a good solution here.

I am leaving the "patch needs improvement" checked on grounds of not
actually testing the patch...

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:83>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: SVN
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by niwi):

I implemented the last considerations. And I added some more tests.

Thanks for your attention. ;)

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:84>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new

Type: New feature | Version: master

Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by niwi):

Here is the latest patch, reviewed by akaariai in the pull request
(https://github.com/django/django/pull/4)

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:85>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by niwi):

Current pull request in one commit
https://github.com/django/django/pull/33

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:86>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by aaugustin):

The code looks generally sane to me.

A few remarks:
* Does the code behave properly with related models -- ie. you can
`save(update_fields=['related'])` rather than
`save(update_fields=['related_id'])`? I haven't seen a test for this.
* There are tests for inherited models, which is a good idea. I'm not
sufficiently familiar with the implementation of the ORM to tell if other
features (like proxy models) are worth testing. If you think they are, a
few more tests would be nice.
* This isn't a minor feature, it deserves a full paragraph in the release
notes.
* `tests/modeltests/update_only_fields/__init__.py` should be totally
empty.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:87>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by niwi):

Replying to [comment:87 aaugustin]:

> The code looks generally sane to me.
>
> A few remarks:
> * Does the code behave properly with related models -- ie. you can
`save(update_fields=['related'])` rather than
`save(update_fields=['related_id'])`? I haven't seen a test for this.

I will check this part and add more tests if necessary.

> * There are tests for inherited models, which is a good idea. I'm not
sufficiently familiar with the implementation of the ORM to tell if other
features (like proxy models) are worth testing. If you think they are, a
few more tests would be nice.

In my opinion, this does not affect these things. Only does the filter
fields that enter the UPDATE statement. The rest of the default behavior
is as if suddenly the model happens to have less fields than actually
have.
In any case, I see to do more tests to be sure.

> * This isn't a minor feature, it deserves a full paragraph in the
release notes.

This part of me is complicated for me. My English is not very good.

> * `tests/modeltests/update_only_fields/__init__.py` should be totally
empty.

;)

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:88>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by niwi):

Changes:
* add tests for proxied models.
* remove all content from tests/modeltests/update_only_fields/__init__.py

I confirm, testcases covers the first point (save related objects) and
works correctly.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:89>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by akaariai):

I just spotted that m2m fields are allowed in the "update_fields" arg. I
guess they have to be forbidden, as this could lead to confusion: "I
defined 'm2m' in the update_fields, but it did not get updated. Whats
going on?". Maybe removing the PK field should be done, too. You can't
actually update the primary key currently, and allowing it in the
update_fields is misleading.

Maybe the right fix is to use something like [field.name for field in
self._meta.fields if not field.primary_key] instead of
get_all_field_names(). Not tested...

I can take care of the above points when committing the patch. Of course,
if you want to do the work I am more than happy to let you do it... :)

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:90>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by niwi):

I tried your suggestion and give the desired results. Now the fields are
no longer allowed m2n. I added the corresponding test.

Regarding github and pull-requests ... always need to be all in a single
commit? actually I'm a bit confused, and I hate to be opening pull-
requests different each time you make a change or commit.

With this change I created a new branch, with the 3 commits I've made for
this issue. And upload the patch here.

Unfortunately, documentation / paragraph in the release notes, I'll leave
to you. I do not feel good ability to write this text.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:91>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by akaariai):

I will take this ticket from here.

We don't have any official guidelines on how to work on github, but here
is an approach I like.
{{{
# Assume you have django/django as "upstream" remote in your local git
repo.
# (if not: git add remote upstream g...@github.com:django/django.git)

# Start work on ticket.
# Work in ticket_4102, make it track upstream/master branch.
git checkout -b ticket_4102 upstream/master
# do some commits:
git add files
git commit -m 'A commit message...'
# check for whitespace errors
git diff --check upstream/master

# Spot some errors, correct files and commit
git add files
git commit -m 'DOH! whitespace error!'

# The above commit should not go into Django's repository, so squash it
# (HEAD~2 refers to last two commits on this branch)
git rebase -i HEAD~2
# pick the original commit ("A commit message..."), squash (s as first
char on line) the "DOH!" commit.
# reword commit messages, usually keep the first message as is, drop the
second message.
# Run (relevant) tests!
# ok, seems the work is ready, push it to your github repo (assumed to be
origin here)
git push origin ticket_4102

# make a pull request in github.
# An annoying core-developer spots a world-ending issue (another
whitespace error).
# And, there have been some commits to the django/django repo too, so you
need to update your patch to head.
# In your local branch ticket_4102 rebase your work to current upstream
master
git fetch upstream
git rebase
# If conflicts above, correct & continue.
# correct & commit the whitespace error, repeat the rebase -i step above
# Run (relevant) tests!

# Now, push again to origin. You have rebased your work. This changes the
commit
# history, and those who based their work on your work will hate you (they
need to
# rebase their work, too). However, this does not matter, as your branch
is a
# "topic branch" and nobody should trust your branch to be stable in
commit history.
# So force push changes:
git push --force origin ticket_4102

# You have your work in a single corrected commit in the original pull
request!
}}}
The above isn't tested.

Commit message guidelines (in short): A summary line of 50 chars.
Separated with an empty line, paragraphs of 70 chars per line detailing
the commit. Limits are soft, so if need be, break the characters per line
rules.

A single commit always? No, separate your work into logical "blocks". For
this ticket, there is just one logical block, so a single commit.

If the above seems confusing or a lot of work to do, here is another
approach: At some point hand over your work to a committer who will finish
it off.

As said, the above isn't official Django workflow, just my approach...

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:92>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: update fields sql | Patch needs improvement: 1
row table modified | UI/UX: 0
Has patch: 1 |
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by niwi):

Thank you very much! It has helped me a lot to understand how git rebase
works.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:93>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: master
Component: Database layer | Resolution:

(models, ORM) | Triage Stage: Ready for
Severity: Normal | checkin
Keywords: update fields sql | Needs documentation: 0
row table modified | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0

Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by akaariai):

* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin

Comment:

[https://github.com/django/django/pull/41 Pull 41] contains an RFC patch -
I will wait for a day or so for final comments on the patch.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:94>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: new
Type: New feature | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Ready for
Severity: Normal | checkin
Keywords: update fields sql | Needs documentation: 0
row table modified | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by russellm):

Replying to [comment:94 akaariai]:

> [https://github.com/django/django/pull/41 Pull 41] contains an RFC patch
- I will wait for a day or so for final comments on the patch.

I've just given the code a quick look over; other than a couple of minor
typos (comments on the diff), it looks good to me. Congratulations to all
involved in killing another 5 year old Django dragon!

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:95>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady

<cgrady@…> | Status: closed

Type: New feature | Version: master

Component: Database layer | Resolution: fixed

(models, ORM) | Triage Stage: Ready for
Severity: Normal | checkin
Keywords: update fields sql | Needs documentation: 0
row table modified | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by Andrei Antoukh):

* status: new => closed
* resolution: => fixed

Comment:

Fixed #4102 -- Allow update of specific fields in model.save()

Added the ability to update only part of the model's fields in
model.save() by introducing a new kwarg "update_fields". Thanks
to all the numerous reviewers and commenters in the ticket
Changeset: 365853da016f242937a657b488514e2f69fa6d82

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:96>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: closed
Type: New feature | Version: master
Component: Database layer | Resolution: fixed
(models, ORM) | Triage Stage: Ready for
Severity: Normal | checkin
Keywords: update fields sql | Needs documentation: 0
row table modified | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by anonymous):

Any interest in addressing the original interest of this ticket again?
[https://github.com/karanlyons/django-save-the-change django-save-the-
changes] is one possible solution. I [https://github.com/karanlyons
/django-save-the-change/issues/1 also opened a ticket] on that project
about the feasibility on porting it to core Django.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:97>

[Django]

#4102: Allow UPDATE of only specific fields in model.save()
-------------------------------------+-------------------------------------
Reporter: Collin Grady | Owner: cgrady
<cgrady@…> | Status: closed
Type: New feature | Version: master
Component: Database layer | Resolution: fixed
(models, ORM) | Triage Stage: Ready for
Severity: Normal | checkin
Keywords: update fields sql | Needs documentation: 0
row table modified | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by russellm):

If, by referring to the "original" interest of the ticket, you're
referring to automatically tracking the 'modified' fields -- then the
answer is probably no. If you read the ticket history, you'll see why we
didn't implement this approach, so unless you've got a new approach to the
implementation, there isn't much to discuss.

--
Ticket URL: <https://code.djangoproject.com/ticket/4102#comment:98>