rsinger86/django-lifecycle

Hooks not effecting rows when called using create_or_update in django 4.2

Opened this issue · 2 comments

from dataclasses import dataclass
from typing import List, Optional

from django_lifecycle import BEFORE_CREATE, BEFORE_SAVE, BEFORE_UPDATE, LifecycleModel
from django_lifecycle.decorators import HookConfig


@dataclass(order=False)
class UpdateFieldsHookConfig(HookConfig):
    # Is only required for a single hook,
    update_fields: Optional[List[str]] = None


update_fields_hook = UpdateFieldsHookConfig


class UpdateFieldsLifecycleModel(LifecycleModel):
    class Meta:
        abstract = True

    def save(self, *args, **kwargs):
        skip_hooks = kwargs.pop("skip_hooks", False)
        save = super().save

        if skip_hooks:
            save(*args, **kwargs)
            return

        # unsure about next line, if running twice is OK, but ought to be...
        self._clear_watched_fk_model_cache()
        is_new = self._state.adding

        if "update_fields" in kwargs:

            if is_new:
                methods = self._get_hooked_methods(BEFORE_CREATE, **kwargs)
            else:
                methods = self._get_hooked_methods(BEFORE_UPDATE, **kwargs)

            methods += self._get_hooked_methods(BEFORE_SAVE, **kwargs)
            fields = set()
            for hooked_method in methods:
                for hook in hooked_method.method._hooked:
                    if hook.update_fields:
                        fields = fields.union(set(hook.update_fields))
            kwargs["update_fields"] = list(set(kwargs["update_fields"]).union(fields))

        save(*args, **kwargs)

usage: same as before but you gotta swtich to our mixin and hook decorator, just add update_fields to the hook params
@update_fields_hook(BEFORE_CREATE, update_fields=["anchor_status"])


My basic fix for it. caused by this change in django https://docs.djangoproject.com/en/4.2/releases/4.2/#setting-update-fields-in-model-save-may-now-be-required which makes it so only fields which are being passed into the update kwags, so if you're updating a field based on an updated field the code will run, but not change in the db. This code above works, but maybe there's better solutions, if I'm able to get a PR approved.

Thanks for reporting this bug! 🐞

That could be a solution, but I don't like having to specify which fields are going to be updated in the decorator because in your hooked methods you may or may not update fields.

One option could be to automagically check which fields have changed (via has_changed) and add them to update_fields, but this may not be for everyone.

Another option could be to have a variable in the model to make the developer responsible of adding which fields have updated?

In your code:

class MyModel(LifecycleModel):
    title = models.CharField(max_length=50)
    slug = models.SlugField(max_length=50)

    @hook(BEFORE_CREATE)
    def set_slug_from_title(self):
        self.slug = slugify(title)
        self._lifecycle_update_fields.add("slug")

and in LifecycleModelMixin:

class LifecycleModelMixin:
    def save(*args, **kwargs):
        # ...
        if "update_fields" in kwargs:
            kwargs["update_fields"] = list(set(kwargs["update_fields"]).union(self._lifecycle_update_fields))
        
        super(*args, **kwargs)
        # ...

Maybe both options, configurable via a setting?

What do you think that it could be the best option?

One option could be to automagically check which fields have changed (via has_changed) and add them to update_fields, but this may not be for everyone.

This might be the least painful way, but I do see some dangers with this, as if someone is explicitly trying to save only a certain field, then this will add all changed fields, which would be very hard to debug why.

self._lifecycle_update_fields.add("slug")

Not sure if I see this as a better choice then having it in the hook itself... or is your worry about having it in the hook that it will be required to be listed in every hook if there's multiple decorators on a method?

But this does seem to be a fairly good way to resolve this issue, so long as its highlighted in the docs.

Maybe both options, configurable via a setting?

This seems to be the best option! 💯 by default it can be off (to not cause unexpected side effects to other peoples code), and have it be a setting (either on the model or global level). Or another mixin LifecycleUpdateFieldsMixin which auto adds the fields. 🤔

Main points would be:

  • Clearly noted in the docs that something is required to be done in the case of create_or_update in dj >=4.2
  • By default no magic updating update_fields
  • Settable per model or global