jupyter-server / pycrdt

CRDTs based on Yrs.
https://jupyter-server.github.io/pycrdt
MIT License
42 stars 10 forks source link

Shared Types Popped from Shared Map are Empty #94

Closed jbdyn closed 5 months ago

jbdyn commented 5 months ago

Hi :wave:

I experienced that, when having a shared datatype nested into a shared map, and popping that nested item, the returned item is of the same shared datatype, but empty:

from pycrdt import Doc, Map

ydoc = Doc()
ymap = ydoc["map"] = Map()
print(ymap)
# {}

inner_map = Map({"bar": "baz"})
ymap.update({"foo": inner_map})
print(ymap)
# {"foo":{"bar":"baz"}}

x = ymap.pop("foo")
print(ymap)
# {}
print(x)
# {} <-- expected {"bar":"baz"}

This is also the same for nested Shared Arrays and Shared Text, but not for Subdocs.

After some hours, I found the deletion to happen here https://github.com/jupyter-server/pycrdt/blob/15a47b0628d02a58893d594f31674895429cdda0/python/pycrdt/_map.py#L110-L111

where res is the popped shared type and populated (as expected), but empty after calling del self[key]. According to the Yrs docs, this is expected behavior for Shared Maps:

fn remove([...]) [...] Removing nested shared types

In case when a nested shared type (eg. MapRef, ArrayRef, TextRef) is being removed, all of its contents will also be deleted recursively. A returned value will contain a reference to a current removed shared type (which will be empty due to all of its elements being deleted), not the content prior the removal.

So, we need to copy the content to another object before perfoming the removal with del self[key].

I quickly added tests as well as a check if res is an instance of BaseType, relying on the method to_py to be defined, but maybe a more sophisticated fix with __copy__ and __deepcopy__ implemented might be better?

jbdyn commented 5 months ago

Thanks for investigating @jbdyn!

:heart:

Would you mind doing the same changes for Array.pop?

Sure, I can do that.

davidbrochart commented 5 months ago

Thanks @jbdyn !

davidbrochart commented 5 months ago

Released in v0.8.18.