[BUG] _previous_revision_id is gone forever

[iterlace]

💕 First of all, I want to thank you for such a great Pydantic-based library. I don't even want to imagine using any other ODM with embedded data types, validation mechanisms, etc.

Describe the bug When I migrated from 1.18.0 => 1.22.0, some of my tests started failing for no reason. Debugging showed that the cause was beanie.odm.utils.parsing.parse_obj.

When Model.model_validate(data) is called against a Model instance (but not a dict), it returns the same Python object. But parse_obj doesn't seem to expect that, and still executes save_state_swap_revision(result) -> item._swap_revision().

How to Reproduce

from tests.odm.models import DocumentWithRevisionTurnedOn, LockWithRevision, WindowWithRevision

async def test_revision_id(mocker):
    lock = LockWithRevision(k=1)
    await lock.insert()

    lock_rev_id = lock.revision_id

    original_swap_revision = lock._swap_revision
    lock._swap_revision = mocker.Mock(wraps=original_swap_revision)

    window = WindowWithRevision(x=0, y=0, lock=lock)

    await window.insert()
    assert lock._swap_revision.call_count == 0  # False. It's 1.
    assert lock.revision_id == lock_rev_id  # False

We could still initialize WindowWithRevision with lock=lock.ref() (and everything would work great), but what's the point? beanie.odm.utils.encoder.Encoder properly encodes Document instances into Link on save, so there's no reason to do it manually.

But that's not the problem that pushed me to write this issue. The resulting problem is far worse. Let's take another example, when LockWithRevision is fetched from the database.

from tests.odm.models import DocumentWithRevisionTurnedOn, LockWithRevision, WindowWithRevision

async def test_revision_id(mocker):
    # Preparation for the test
    _lock = LockWithRevision(k=1)
    await _lock.insert()

    # Wrap LockWithRevision._swap_revision with a calls counter
    original_swap_revision = LockWithRevision._swap_revision
    swap_revision_count = 0

    def wrapper(self):
        nonlocal swap_revision_count
        if isinstance(self, LockWithRevision) and self.id == _lock.id:
            swap_revision_count += 1
        return original_swap_revision(self)

    LockWithRevision._swap_revision = wrapper

    # Preparation finished. There is the test case:
    lock = await LockWithRevision.get(_lock.id)
    window = WindowWithRevision(x=0, y=0, lock=lock)

    await window.insert()

    assert swap_revision_count == 0  # False. Do you expect 1? No, it's 2.

    # time  _previous_revision_id  revision_id
    # 0     None                   a       -> here happened the first _swap_revision
    # 1     a                      b       -> here happened the second _swap_revision
    # 2     b                      c
    # Original revision_id ("a") is gone forever.

How did we get TWO calls to LockWithRevision._swap_revision?

1. The first one was triggered when the LockWithRevision been initializing (1->2->3).
2. The second one was triggered by beanie.odm.utils.parsing.parse_obj during WindowWithRevision(x=0, y=0, lock=lock) .

What does it mean for us? We can no longer push LockWithRevision to the DB, because _previous_revision_id no longer contains a real revision_id from the database, and Document.update will always update 0 rows.

Expected behavior Document.revision_id is changed only when it is fetched from the DB. _Actually, I would like to have the revision_id changed directly in the update/replace/... methods, but not in get. So that two identical objects, fetched from the db, would be absolutely similar on Document level and comparable with == operator. This is another pain point I wanted to mention. As of now, we can do nothing but compare .model_dump'ed dicts with revision_id excluded. That's a hell of an inconvenience._

Final words I really hope this issue could be resolved in the upcoming release. Thank you!

[roman-right]

Hi! Thank you for the catch. I'll check and fix it this/next week.

[iterlace]

Hey :) Any updates here?

[roman-right]

Hi @iterlace , Sorry for the late reply. Thank you very much for the detailed investigation. Could you please try this: https://github.com/roman-right/beanie/pull/797 I reworked revisions - there are no swaps more. But probably I didn't cover some corner cases.