A few years ago I noticed some odd behaviour from the library django-model-utils.

https://github.com/jazzband/django-model-utils/issues/520

One of the django model fields it provides acts in a strange way:

class MyModel(models.Model):
    my_field = AutoLastModifiedField()

with freezegun.freezetime(time1):
    instance1 = MyModel()
with freezegun.freezetime(time2):
    instance2 = MyModel()

assert(instance1.my_field != instance2.my_field) # this will fail

The expectation is that as these two instances of models are different the value of the field my_field should be different.

This is related to some form of caching for the default value that happens at the class level.

Varun Juneja explained that this is a feature to have created and modified fields with the same value at instantiation time.

As this is a behaviour that people rely on, there is no easy way to fix this. There is no access to the instance object id to make this cache local to the instance.

Later on Kayvan Zahiri proposed a fix that makes the cache temporary, so it clears after 2 seconds. What I want to discuss today is where this new logic can live.

The fixes

Here are some ideas on how things could be fixed. Each of them have different intents which I feel get lost in translation

1. Kayvan’s proposal

This is what Kayvan proposed at first:

class AutoLastModifiedField(...):
    def _is_auto_stamp(self, current: datetime, fresh: datetime) -> bool:
        if current == fresh:
            return True
        try:
            return abs(current - fresh) <= timedelta(seconds=2)
        except (TypeError, AttributeError):
            return False

   ...

   def pre_save(...):
       current_value = getattr(model_instance, self.attname, value)
       if not self._is_auto_stamp(current_value, value):
           value = current_value

It moves some of the logic to decide if a value is fresh or not to a separate protected member function of AutoLastModifiedField. From a layering perspective it keeps everything within the class but it doesn’t make _is_auto_stamp private, which means that the function is exposed. More importantly, it coerces an assumption into certainty as this is an estimation.

This solution says that determining auto_stamp is a responsibility of the class, and exposes it to the outer world

2. A more functional approach

Let’s look at moving the function away from the class

def are_dt_within_two_seconds(self, first: datetime, second: datetime) -> bool:
    if first == second:
        return True
    try:
        return abs(first - second) <= timedelta(seconds=2)
    except (TypeError, AttributeError):
        return False

class AutoLastModifiedField(...):
   ...

   def pre_save(...):
       current_value = getattr(model_instance, self.attname, value)
       is_auto_stamp = are_dt_within_two_seconds(current_value, value)
       if not is_auto_stamp:
           value = current_value

This variation moves the function outside the class. The function becomes immediately reusable by decoupling the context of the call. The assumption stays inside the pre_save function, and it is now explicit. But the function is still public which polutes the module by having a date function in there.

This solution says that the function is not part of the class, but the assumption is. And that the function belongs to the module interface

2.1 Hiding the function

Adding an underscore to are_dt_within_two_seconds into _are_dt_within_two_seconds makes the function hidden from the perspective of the module. Given that the module is fields.py, this might be a better option. Alternatively, move this function to some date_utils.py module for the same effect.

This variation says that the function is not part of the module and hides the logic from the interface, making the new behaviour 100% an implementation detail of pre_save

2.2 Sharing the assumption outside

Ignoring how visible the function are_dt_within_two_seconds is, there is a second consideration of making the assumption a member of the class:

def _are_dt_within_two_seconds(self, first: datetime, second: datetime) -> bool:
    if first == second:
        return True
    try:
        return abs(first - second) <= timedelta(seconds=2)
    except (TypeError, AttributeError):
        return False

class AutoLastModifiedField(...):
   
   @staticmethod
   def are_two_dt_likely_autogenerated(first, second):
        return _are_dt_within_two_seconds(first, second)

   ...

   def pre_save(...):
       current_value = getattr(model_instance, self.attname, value)
       if not self.are_two_dt_likely_autogenerated(current_value, value):
           value = current_value

This is a hybrid of both solutions that allows the testable function separate of the class but keeps the assumption within the class. It exposes the assumption like [1] but leaves the implementation away from the class and makes the overall thing more verbose.

This variation says that the assumption is responsibility of the class, but the implementation isn’t part of it

Which one is better?

Deciding which variation works best depends on the intent that this new feature has. In this particular case, who owns the implementation and who shares it?

In my opinion the fact that pre_save is using this assumption to decide if the to dt are the same is an implementation detail of pre_save, but the function to calculate is a datetime function and should live elsewhere or be private. To me [2.1] is the optimal solution with my knowledge of django and the intent of the patch.

But I can also see that point of view that the assumption is part of the signature of the class because it is part of what makes this type of field. In that context [1] or [2.2].

[2.2] Is a slightly more verbose but more communicative approach, whereas [1] is more concise as an interface. To me [2.2] has a slight edge because the logic of dates seems not to be part of the field in anyway.

It is a matter of taste.