Skip to content

Commit

Permalink
Fix backward compatibility with pickles before v22.2.0
Browse files Browse the repository at this point in the history
Unfortunately we're using `django-redis` to cache some `attrs` instances
and the change to make `__getstate__` and `__setstate__` use `dict`
values instead of `tuple` values prevents reading any values from the
cache when updating to `attrs>=22.2.0`. We get an `AttributeError`
raised for all attributes.

This happens because the tuple of values stored in the original pickle
is checked for the name of the attribute and so none of the attributes
are set on unpickle.

To address this I've made a change so that `__setstate__` will still be
able to handle values pickled as `tuple`s allowing existing pickles to
be unpickled.

The test just contains a pre-pickled value due to the complexities
around tying to mock methods on a slotted as documented[^1].

I realise that documentation also mentions that we should "think twice"
before using pickle, but sometimes those decisions predate our
involvement in a project! In addition, pretty much all of Django's cache
implementations are built around using pickle by default. This change
will allow an easier path to upgrade by unpickling and repickling all
values in the cache.

Regression in 89a890a.

[^1]: https://www.attrs.org/en/latest/glossary.html#term-slotted-classes
  • Loading branch information
ngnpope committed Jan 13, 2023
1 parent 46053d7 commit 3cdb31d
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 3 deletions.
1 change: 1 addition & 0 deletions changelog.d/1085.change.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Restored ability to unpickle instances pickled before to v22.2.0.
12 changes: 9 additions & 3 deletions src/attr/_make.py
Original file line number Diff line number Diff line change
Expand Up @@ -934,9 +934,15 @@ def slots_setstate(self, state):
Automatically created by attrs.
"""
__bound_setattr = _obj_setattr.__get__(self)
for name in state_attr_names:
if name in state:
__bound_setattr(name, state[name])
if isinstance(state, tuple):
# Backward compatibility with attrs instances pickled with
# attrs versions before v22.2.0 which stored tuples.
for name, value in zip(state_attr_names, state):
__bound_setattr(name, value)
else:
for name in state_attr_names:
if name in state:
__bound_setattr(name, state[name])

# The hash code cache is not included when the object is
# serialized, but it still needs to be initialized to None to
Expand Down
18 changes: 18 additions & 0 deletions tests/test_slots.py
Original file line number Diff line number Diff line change
Expand Up @@ -803,3 +803,21 @@ class NEW_A:
assert new_a.b == 2
assert new_a.c == 3
assert not hasattr(new_a, "d")


@pytest.mark.parametrize("cls", [A])
def test_slots_unpickle_is_backward_compatible(cls, frozen):
"""
Ensure object pickled before v22.2.0 can still be unpickled.
"""
a = cls(1, 2, 3)

a_pickled = (
b"\x80\x04\x95&\x00\x00\x00\x00\x00\x00\x00\x8c\x10"
+ a.__module__.encode()
+ b"\x94\x8c\x01A\x94\x93\x94)\x81\x94K\x01K\x02K\x03\x87\x94b."
)

a_unpickled = pickle.loads(a_pickled)

assert a_unpickled == a

0 comments on commit 3cdb31d

Please sign in to comment.