Change in #121 breaks subsequent model updates in on_commit=True hooks (infinite recursion)

[mjl]

So here is my use case, simplified:

class Entry(models.Model):

    document = models.BinaryField(blank=True, null=True)
    document_received_ts = models.DateTimeField(null=True)

    @hook(AFTER_UPDATE, when='document', was=None, has_changed=True, on_commit=True)
    def has_received_document(self):
        logger.info("%s received document", self)
        self.document_received_ts = datetime.now()
        self.save()

# … somewhere else …
some_entry.document = b'whatevs'
some_entry.save()

This used to work fine, however since 1.0.1 it runs into a RecursionError: maximum recursion depth exceeded while calling a Python object, calling the above hook over and over again, obviously triggered by the save(). I'm not sure why the above hook is triggered again, since it's an AFTER_UPDATE(on_commit=True), ie. the change has been persisted to the database. It should pick up that document has not changed from None again! However, it obviously does not and recurses until the stack runs out.

I can not just do a save(skip_hooks=True) because there might be other hooks that depend on this one…

My uneducated guess is that django-lifecycle needs to sync its change tracking info before running on_commit hooks?

[EnriqueSoria]

Can you try to update django-lifecycle to the latest version? Maybe #121 fixes it

[mjl]

On 14 Sep 2023, at 18:53, Enrique Soria wrote:

Can you try to update django-lifecycle to the latest version? Maybe #121 fixes it

#121 CAUSED it. It worked in 1.0.0, and broke in 1.0.1.

[mjl]

Here is a minimal testcase

class CommitTrigger(LifecycleModel):
    foo = models.CharField(max_length=10, blank=True, null=True)
    foo_ack = models.BooleanField(default=False)
    t = 0

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.t = 0

    @hook('after_update', when='foo', was=None, has_changed=True, on_commit=True)
    def set_foo_ack(self):
        print(f"set_foo_ack {self.foo=}")
        self.foo_ack = True
        self.save()  # <== this should not trigger the hook b/c foo is not None 
        self.t += 1

from django.test import TestCase, TransactionTestCase
from django.db import transaction


from tests.testapp.models import CommitTrigger


class ModelCommitTestCase(TransactionTestCase):
    def test_trigger_on_commit_1(self):

        instance = CommitTrigger.objects.create()

        self.assertEqual(instance.foo, None)
        self.assertEqual(instance.foo_ack, False)

        with transaction.atomic():
            o = CommitTrigger.objects.get(pk=instance.pk)
            o.foo = 'bar'
            o.save()

        self.assertEqual(o.foo, 'bar')
        self.assertEqual(o.foo_ack, True)

        self.assertEqual(o.t, 1)

which gives

(django-lifecycle) % python manage.py test
Found 52 test(s).
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
...............E..........................E........set_foo_ack self.foo='bar'
set_foo_ack self.foo='bar'
set_foo_ack self.foo='bar'
set_foo_ack self.foo='bar'
[... 1000 more of those ...]
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/copy.py", line 92, in copy
    rv = reductor(4)
         ^^^^^^^^^^^
RecursionError: maximum recursion depth exceeded

[EnriqueSoria]

I need to do some research, but it looks that we'll need to revert #121 because we fixed a gotcha.

PS: thanks for providing a valid test!

[EnriqueSoria]

@mjl For your use case, would it work if you change to BEFORE_UPDATE and remove the save()?

class Entry(models.Model):

    document = models.BinaryField(blank=True, null=True)
    document_received_ts = models.DateTimeField(null=True)

    @hook(BEFORE_UPDATE, when='document', was=None, has_changed=True, on_commit=True)
    def has_received_document(self):
        logger.info("%s received document", self)
        self.document_received_ts = datetime.now()

# … somewhere else …
some_entry.document = b'whatevs'
some_entry.save()

[mjl]

@mjl For your use case, would it work if you change to BEFORE_SAVE and remove the save()?

It would not, I'm afraid. Background info: I use it to model a state machine, and I need to have states persisted to the db before any action on them is triggered (which can change states, of course).

Modifying it to use BEFORE_SAVE would break things, like multiple state changes in a row (the state machine would never be notified of interim states and then complain about invalid transitions), or being sure that some side effect has been performed and comitted, even if something further along fails (something like "create account" -> "charge credit card" -> "send welcome email" -- you would not want to roll back to the first state and charge the card again if sending the welcome mail fails!).

For the moment I just pinned the version required to 1.0.0, so no big deal.

[mjl]

Anything I can do to help track this down?

[EnriqueSoria]

Anything I can do to help track this down?

I need a better understanding on how on_commit works in order to make a decision that works for everyone. I've been testing some approaches but all of them break the expected usage of this library.