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

Shared Types Popped from Shared Map are Empty #94

Merged

Conversation

jbdyn
Copy link
Contributor

@jbdyn jbdyn commented Apr 4, 2024

Hi 👋

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

res = self[key]
del self[key]

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?

Copy link
Collaborator

@davidbrochart davidbrochart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for investigating @jbdyn!
I think pop should not return a shared type, but instead the pure Python object. This means that pop can have a different return value than get, but I think this is fine.
Would you mind doing the same changes for Array.pop?

@jbdyn
Copy link
Contributor Author

jbdyn commented Apr 9, 2024

Thanks for investigating @jbdyn!

❤️

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

Sure, I can do that.

@davidbrochart
Copy link
Collaborator

Thanks @jbdyn !

@davidbrochart davidbrochart merged commit e3cc090 into jupyter-server:main Apr 9, 2024
16 checks passed
@davidbrochart
Copy link
Collaborator

Released in v0.8.18.

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

Successfully merging this pull request may close these issues.

2 participants