Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Field tracker previous should return default for new instances #523

Open
stevelacey opened this issue Apr 1, 2022 · 0 comments
Open

Field tracker previous should return default for new instances #523

stevelacey opened this issue Apr 1, 2022 · 0 comments

Comments

@stevelacey
Copy link

stevelacey commented Apr 1, 2022

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.

@stevelacey stevelacey changed the title Field tracker previous should return defaults values for new instances Field tracker previous should return default for new instances Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant