# Hook for extending \`update\_fields\` in save() overrides

**URL:** <https://forum.djangoproject.com/t/hook-for-extending-update-fields-in-save-overrides/26040>\
**Category:** ORM\
**Created:** [December 9, 2023, 10:29pm UTC](https://forum.djangoproject.com/t/hook-for-extending-update-fields-in-save-overrides/26040 "2023-12-09T22:29:54Z")\
**Posts on this page:** 7\
**Page:** 1

<div class="post-metadata">

**Author:** ![jacobtylerwalls](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.djangoproject.com/jacobtylerwalls/32/30603_2.png) [@jacobtylerwalls](https://forum.djangoproject.com/u/jacobtylerwalls)\
**Post date:** [December 9, 2023, 10:29pm UTC](https://forum.djangoproject.com/t/hook-for-extending-update-fields-in-save-overrides/26040/1 "2023-12-09T22:29:54Z")

</div>

What do folks think about shipping an `extend_update_fields()` util to make manipulating the `update_fields` argument in in overridden `save()` methods less error-prone?

When upgrading a project to 4.2 I needed this in three or so models. I was in a bit of a hurry, so it took me two pull requests to do it. (The first time I forgot to handle iterables other than sets, because I was overly-focused on writing tests to capture the `update_or_create()` 4.2 behavior change scenario.)

Round 2 looked like this. (I’m just showing my thinking process and how I still missed cases. It’s not supposed to be optimized for performance or anything. You should use [Simon’s suggestion](https://forum.djangoproject.com/t/update-or-create-behavior/25944/4) because it doesn’t mutate the kwargs in place.)

```auto
def extend_update_fields(kwargs, field_name):
    """Update `update_field` inside `kwargs` (if present) in-place with `field_name`."""
    if (update_fields := kwargs.get("update_fields")) is not None:
        # DON'T do this: you shouldn't mutate the input
        if isinstance(update_fields, set):
            update_fields.add(field_name)
        else:
            new = set(update_fields)
            new.add(field_name)
            kwargs["update_fields"] = new

```

This is a lot like Simon’s [suggestion here](https://forum.djangoproject.com/t/update-or-create-behavior/25944/4), other than not eagerly converting to a set and only accepting one new field. EDIT: and mutating the kwargs in place.

**However, both of our implementations missed `*args`**. So the above snippet is _still_ not compliant with the `model.save()` contract, because it doesn’t handle `save(False, False, None, [])`.

Of course, Simon was sketching, not writing production code 😛. But as I’m considering whether to take a _third_ go at this, it struck me – couldn’t we just ship a blessed util for this? Then our advice for auditing overridden `save()` methods would be something like: “scan for assignments to self, and pair them with `extend_update_fields()`”.

---

<div class="post-metadata">

**Author:** ![charettes](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.djangoproject.com/charettes/32/27_2.png) [@charettes](https://forum.djangoproject.com/u/charettes)\
**Post date:** [December 10, 2023, 12:07am UTC](https://forum.djangoproject.com/t/hook-for-extending-update-fields-in-save-overrides/26040/2 "2023-12-10T00:07:03Z")

</div>

Hello @jacobtylerwalls!

What’s the reason to want to avoid creating a new `set` so bad? These objects should normally have only a few members as most models usually don’t have more than 20 fields and usually when you use `update_fields` you want to target a small subset of them a so a new set should have a low memory foot print.

From some local testing it takes about as much time to perform `isinstance(update_fields_list, set)` than to systematically do `set(update_fields)` and it’s also usually bad practice to alter a mutable argument passed by reference as that can unintended side effects. On CPython 3.10.x it appears to be faster with `update_fields: set` than other iterables so it seems like a faster path is taken already for creating shallow copies.

What if the `save` caller was planing to use `update_fields: set` for other purpose after calling `save`? [A similar issue](https://github.com/django/django/commit/978680db224601e3e09f715c82f89f390c63a763) was caught recently with `refresh_from_db`.

With `JSONField` maturing in core without proper `update_fields={'json_field__key'}` support and a long standing desire to entirely replace `auto_now` and friends with fields that use `default` but have a way to opt-in into `update_fields` injection I think we’ll want to revisit how this feature is implemented in the future.

I’m not convinced that a core function is warranted here. I think it might be better to invest efforts in coming up with a generic solution to the problem of derived / generated fields and their interactions with `update_fields`.

---

<div class="post-metadata">

**Author:** ![jacobtylerwalls](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.djangoproject.com/jacobtylerwalls/32/30603_2.png) [@jacobtylerwalls](https://forum.djangoproject.com/u/jacobtylerwalls)\
**Post date:** [December 10, 2023, 3:17am UTC](https://forum.djangoproject.com/t/hook-for-extending-update-fields-in-save-overrides/26040/3 "2023-12-10T03:17:26Z")

</div>

Oh I do prefer your solution! 🙂

I wasn’t saying my snippet was better, sorry–I was just trying to show that what seemed simple at first spiraled out into several further subtle points. (I’ll edit my post to clarify that.) I was just trying to suggest that each time I touched this, I found one more gotcha (i.e. `*args` not handled), and now you’ve helpfully called my attention to one more, re: defending against the caller doing something else with `update_fields` beyond using them in `save()`.

So the journey went something like this:

1. Read the release note or forum discussion and realize you need to patch update\_fields. (In my case, I first ignored it when starting on the upgrade tasks a few months back, as it looked like a situation where if something was wrong, it had been wrong for a while, which would compete with other priorities, but I gave it another look this week, thinking “this should be quick”)
2. You consult “overriding predefined model methods” as linked in the docs; the example uses a set, you write a regression test to build familiarity with what your save method was trying to accomplish, all is groovy
3. You realize later any iterable is accepted, when you discover your own view (e.g. in some other project) sending update\_fields as a list (ouch!)
4. You realize later that `save()` allows positional arguments also, despite the release note saying “keyword argument” (also ouch, but less embarrassing)
5. You realize later altering a mutable argument may cause unwanted side effects

That just seemes like a series of delicate things to get right for every maintainer of a project with custom save()'s, unless they have the cycles to put to a bigger refactor.

Of course nothing beats “slow down and think” 😉 , but just wanted to sound folks out to see whether there was interest in smoothing this path. I am sympathetic to this, though:

> I think it might be better to invest efforts in coming up with a generic solution to the problem of derived / generated fields and their interactions with `update_fields` .

---

<div class="post-metadata">

**Author:** ![jacobtylerwalls](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.djangoproject.com/jacobtylerwalls/32/30603_2.png) [@jacobtylerwalls](https://forum.djangoproject.com/u/jacobtylerwalls)\
**Post date:** [December 10, 2023, 3:59am UTC](https://forum.djangoproject.com/t/hook-for-extending-update-fields-in-save-overrides/26040/4 "2023-12-10T03:59:07Z")

</div>

Another piece was determining what `update_fields` does with empty iterables; not the same as None. I went to the source to find out–it’s not discussed explicitly in the docs.

I might end up with something like this:

```py
### Hypothetical current code ###
class MyModel(models.Model):
    def save(self, *args, **kwargs):
        if altering_field_one:
            self.field_one = 1
        if altering_field_two:
            self.field_two = 2
        super().save(*args, **kwargs)

### Correct version ###
def extend_update_fields(args, kwargs, fields_to_add: set):
    if len(args) >= 4 and (update_fields := args[3]) is not None:
        args[3] = set(update_fields) | fields_to_add
    elif (update_fields := kwargs.get("update_fields")) is not None:
        kwargs["update_fields"] = set(update_fields) | fields_to_add

    return args, kwargs

class MyModel(models.Model):
    def save(self, *args, **kwargs):
        if altering_field_one:
            self.field_one = 1
            extend_update_fields(args, kwargs, {"field_one"})
            args, kwargs = extend_update_fields(args, kwargs)
        elif altering_field_two:
            self.field_two = 2
            extend_update_fields(args, kwargs, {"field_two"})
            args, kwargs = extend_update_fields(args, kwargs)
        super().save(*args, **kwargs)

```

---

<div class="post-metadata">

**Author:** ![adamchainz](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.djangoproject.com/adamchainz/32/26_2.png) [@adamchainz](https://forum.djangoproject.com/u/adamchainz)\
**Post date:** [December 17, 2023, 2:03pm UTC](https://forum.djangoproject.com/t/hook-for-extending-update-fields-in-save-overrides/26040/5 "2023-12-17T14:03:33Z")

</div>

> [@jacobtylerwalls](#):
>
> So the above snippet is _still_ not compliant with the `model.save()` contract, because it doesn’t handle `save(False, False, None, [])`.

I think we could reasonably deprecate positional args for `save()`, and many other Django functions with several arguments where such calls are unclear and hopefully rare.

---

<div class="post-metadata">

**Author:** ![nessita](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.djangoproject.com/nessita/32/12194_2.png) [@nessita](https://forum.djangoproject.com/u/nessita)\
**Post date:** [December 18, 2023, 12:41pm UTC](https://forum.djangoproject.com/t/hook-for-extending-update-fields-in-save-overrides/26040/6 "2023-12-18T12:41:51Z")

</div>

> [@jacobtylerwalls](#):
>
> You realize later any iterable is accepted, when you discover your own view (e.g. in some other project) sending update\_fields as a list (ouch!)

For what is worth, in my head and in all the code bases I have worked with, `update_fields` was always a list. I never stopped thinking why until this post, and while I don’t have a clear answer, there is something about maintaining the order of the added fields to `update_fields` that feels more robust when debugging code/chasing bugs.

I would certainly advice to not assume that most people/code bases are using a `set` (not that I’m saying that this is currently proposed, but I think it’s worth saying it out loud). A quick and naïve search on GitHub returns 2.9K code matches for `update_fields.append` while only 163 code matches for `update_fields.set`.

---

<div class="post-metadata">

**Author:** ![jacobtylerwalls](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.djangoproject.com/jacobtylerwalls/32/30603_2.png) [@jacobtylerwalls](https://forum.djangoproject.com/u/jacobtylerwalls)\
**Post date:** [December 22, 2023, 7:45pm UTC](https://forum.djangoproject.com/t/hook-for-extending-update-fields-in-save-overrides/26040/7 "2023-12-22T19:45:11Z")

</div>

Thanks for sounding this out y’all! Opened a [ticket](https://code.djangoproject.com/ticket/35060) to suggest making save() keyword-only.