jazzband/django-model-utils

Field tracker previous should return default for new instances

Opened this issue · 0 comments

Field tracker documentation says this:

Returns None when the model instance isn’t saved yet.

And also this:

The has_changed method relies on previous to determine whether a field’s values has changed.

The resulting behavior is fine for fields that accept and default to None, but as soon as you're wanting to track fields that have a custom default, or only accept blank, you'll get odd results particularly from has_changed.

For example on name = models.TextField(blank=True) on a new instance, previous will return None, while the current value will be "", which means that when has_changed is called, it'll return True, because "" != None

This creates goitchas that require the developer to be mindful of whether the instance is new, and do additional checks in that case, by returning the default value in previous, the result of has_changed is more reliable for unsaved instances.

Environment

  • Django Model Utils version: 4.2.0
  • Django version: 3.2.12
  • Python version: 3.8.12

Code examples

This example extension covers everything that I need right now – it might get hairy in the case of a callable default; but this results in the behavior I expected and covers things like bools that default to True and blank=True, null=False fields:

from django.db.models import NOT_PROVIDED

import model_utils


class FieldInstanceTracker(model_utils.tracker.FieldInstanceTracker):
    def previous(self, attr):
        result = super().previous(attr)

        if self.instance._state.adding and result is None:
            field = self.instance._meta.get_field(attr)

            if field.default is not NOT_PROVIDED:
                result = field.default
            elif field.blank and not field.null:
                result = ""

        return result


class FieldTracker(model_utils.tracker.FieldTracker):
    tracker_class = FieldInstanceTracker

I don't care so much that previous is inaccurate, that kind of makes sense for new instances, but given previous is then used for has_changed you can get lots of false positives, especially when migrating from ad-hoc __init__ based solves.