Repository: python-attrs/attrs
Pull request #1085 (2023-01-13 17:38:10)

Fix backward compatibility with pickles before v22.2.0
# Summary

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 89a890a3262273514619d1881c6a8b0fe26c66ac.

[^1]: https://www.attrs.org/en/latest/glossary.html#term-slotted-classes

# Pull Request Check List

<!--
This is just a friendly reminder about the most common mistakes.
Please make sure that you tick all boxes.
But please read our [contribution guide](https://github.com/python-attrs/attrs/blob/main/.github/CONTRIBUTING.md) at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, **check it anyway** to make it apparent that there's nothing left to do.
If your pull request is a documentation fix or a trivial typo, feel free to delete the whole thing.
-->

- [x] Added **tests** for changed code.
  Our CI fails if coverage is not 100%.
- [x] New features have been added to our [Hypothesis testing strategy](https://github.com/python-attrs/attrs/blob/main/tests/strategies.py).
- [x] Changes or additions to public APIs are reflected in our type stubs (files ending in ``.pyi``).
    - [x] ...and used in the stub test file `tests/typing_example.py`.
    - [x] If they've been added to `attr/__init__.pyi`, they've *also* been re-imported in `attrs/__init__.pyi`.
- [x] Updated **documentation** for changed code.
    - [x] New functions/classes have to be added to `docs/api.rst` by hand.
    - [x] Changes to the signature of `@attr.s()` have to be added by hand too.
    - [x] Changed/added classes/methods/functions have appropriate `versionadded`, `versionchanged`, or `deprecated` [directives](http://www.sphinx-doc.org/en/stable/markup/para.html#directive-versionadded).
          Find the appropriate next version in our [``__init__.py``](https://github.com/python-attrs/attrs/blob/main/src/attr/__init__.py) file.
- [x] Documentation in `.rst` files is written using [semantic newlines](https://rhodesmill.org/brandon/2012/one-sentence-per-line/).
- [x] Changes (and possible deprecations) have news fragments in [`changelog.d`](https://github.com/python-attrs/attrs/blob/main/changelog.d).
- [x] Consider granting [push permissions to the PR branch](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork), so maintainers can fix minor issues themselves without pestering you.

<!--
If you have *any* questions to *any* of the points above, just **submit and ask**!
This checklist is here to *help* you, not to deter you from contributing!
-->


pganssle: Is there any way to add a warning when someone uses the antipattern of persisting pickles across versions? I think a lot of people don't realize that this is a profoundly bad idea. If attrs is going to enable this behavior (even on a best-effort basis), it might be a good idea to at least tell people they are doing something wrong here.
hynek: Cc @djipko for overall assessment 
ngnpope: > Is there any way to add a warning when someone uses the antipattern of persisting pickles across versions?

Across versions - in a general way - sounds difficult as you'd need to store the version in the pickle itself and then you may still suffer from not being able to unpickle in older versions anyway - it'd be backward incompatible. The only viable way would be to store `(<version>, <instance>)` so you could then compare the `<version>` in `__setstate__()` before handling the `<instance>`...

...but then we'd be changing the state _again_ and we'd need to know that the tuple contained a version in the first item rather than being the tuple state from a pre-v22.2.0 version of attrs.

> I think a lot of people don't realize that this is a profoundly bad idea.

This is true. Others of us are saddled with these issues until we have resources to be able to undo decisions made by others in the past.

> If attrs is going to enable this behavior (even on a best-effort basis), it might be a good idea to at least tell people they are doing something wrong here.

This. I think a nice big admonition in the documentation would be a good idea. A [documentation search](https://www.attrs.org/en/latest/search.html?q=pickle) yields very little - one small warning in the slotted classes glossary entry. It may also be worth stating that some cache implementations, e.g. Django's caching framework, make use of pickle serialization by default, which could lead to compatibility issues across versions.

Further, I know that whether this is considered public API is a [possible grey area](https://github.com/python-attrs/attrs/issues/1004#issuecomment-1216645205), but given the fragility it's probably worth mentioning backward incompatible changes to pickle serialization in the release notes in future. That linked comment did also say _"So if you can make it compatible"_ which I would argue didn't happen, hence this PR.
hynek: > Another thought: Do we want to add a `DeprecationWarning` to the `tuple` branch?

I think that's not great because it would warn about exactly **one** version change, as you wrote. Kinda a mix of enough to be annoying, but not enough to solve the problem.

While I agree with Paul that it's not great to pickle across versions, with attrs it shouldn't be a problem and I **bet** people do it and love it and are gonna yell at us if we annoy them (just as they yelled at us about the 3.6 warning).

IOW I agree, but I'm afraid it's too disruptive.

I think I'm fine with the code as it is. I'd just like @djipko to give it a look to ensure we're not breaking something only he knows and then it's ready to merge.
pganssle: > While I agree with Paul that it's not great to pickle across versions, with attrs it shouldn't be a problem and I **bet** people do it and love it and are gonna yell at us if we annoy them (just as they yelled at us about the 3.6 warning).
> 
> IOW I agree, but I'm afraid it's too disruptive.

I mean, I think people shouldn't save pickles to disk *at all*, but :shrug:

FWIW, this issue also has blocked me from upgrading attrs internally for now, but I don't mind when this happens so much because it's a decent way to ferret out programs that are inappropriately using pickle. There are a bunch of other ways that this sort of thing can go wrong in much more subtle ways, so I appreciate when things break a bit more loudly.

Also, I understand the perspective of "people do it and love it", but I suspect that in reality people are neutral about it. They [think pickle is the way you serialize stuff and don't think about it until it breaks](https://blog.ganssle.io/articles/2023/01/attractive-nuisances.html), at which point their reactive "You broke me" emotions kick in. It's kind of a thankless job to be like, "Oops you were doing it wrong sorry pickle exists" but it's almost certainly educational.

That said, in some ways I think you are a kinder person than me and not as inclined to say, "This is for your own good." when breaking private APIs, and I'm not going to lobby *too* hard for just letting this stuff break. I figured this point where we know that someone is relying on cross-version unpickling might be a good place to put a warning (annoying enough to show people they are doing the wrong thing but not difficult to silence or fix, and with no blast radius outside of this particular subset), but it's not a hill I'm interested in dying on (or even just getting grass stains on my knees on).
djipko: I think the change itself is fine and something I considered doing in the first place when I proposed to move to dict. If it's breaking enough people and stopping them from upgrading, even though they are doing something that's known to be "a bad idea" (tm), on the whole, that's a bad thing.

But as demonstrated in the issue I raised, using tuple is inherently unsafe - now everyone's just broken which is, in some ways better than their code doing the wrong thing (even if it's an edge case that doesn't come up as often). 

With this change, everyone is still vulnerable to the original issue though which is not great either. Here's what I propose if @hynek agrees - let's have an issue open for say next version (v23.0.0 or whatever) to __really__ remove tuple then? While I don't doubt this will still break some people even then - most folks would have had enough time to re-process data if there are any stragglers. 

I'd be happy to keep an eye on such issue and put up a fix and land it when safe!
ngnpope: > ...even though they are doing something that's known to be "a bad idea" (tm)...

I know, I know! 🙈 I'm the victim, I promise! 😛 

I'm happy if you want to remove it again in a major version bump - I'd expect nothing less 😄

What this provides is a (few) version(s) where we can read the tuple and dict versions and write the dict version only. That allows for a seamless upgrade of cache values to the new pickle format - via a load-and-dump cycle of the cache - which is not currently possible.

Yes, the original issue is still a problem in a sense, but only as far as reading existing pickles goes. With a load-and-dump upgrade this _potential_ issue will go away for us (although our structure is not currently at risk of changing).

Patch:
diff --git a/changelog.d/1085.change.md b/changelog.d/1085.change.md
new file mode 100644
index 0000000..483fc87
--- /dev/null
+++ b/changelog.d/1085.change.md
@@ -0,0 +1,1 @@
+Restored ability to unpickle instances pickled before 22.2.0.
diff --git a/src/attr/_make.py b/src/attr/_make.py
index 1bb0f50..014b7bc 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -934,9 +934,15 @@ class _ClassBuilder:
             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


Test patch:
diff --git a/tests/test_slots.py b/tests/test_slots.py
index f436b3b..18513ce 100644
--- a/tests/test_slots.py
+++ b/tests/test_slots.py
@@ -754,13 +754,12 @@ class A:
     c = attr.ib()
 
 
-@pytest.mark.parametrize("cls", [A])
-def test_slots_unpickle_after_attr_removed(cls):
+def test_slots_unpickle_after_attr_removed():
     """
     We don't assign attributes we don't have anymore if the class has
     removed it.
     """
-    a = cls(1, 2, 3)
+    a = A(1, 2, 3)
     a_pickled = pickle.dumps(a)
     a_unpickled = pickle.loads(a_pickled)
     assert a_unpickled == a
@@ -778,12 +777,11 @@ def test_slots_unpickle_after_attr_removed(cls):
         assert not hasattr(new_a, "b")
 
 
-@pytest.mark.parametrize("cls", [A])
-def test_slots_unpickle_after_attr_added(cls, frozen):
+def test_slots_unpickle_after_attr_added(frozen):
     """
     We don't assign attribute we haven't had before if the class has one added.
     """
-    a = cls(1, 2, 3)
+    a = A(1, 2, 3)
     a_pickled = pickle.dumps(a)
     a_unpickled = pickle.loads(a_pickled)
 
@@ -803,3 +801,20 @@ def test_slots_unpickle_after_attr_added(cls, frozen):
         assert new_a.b == 2
         assert new_a.c == 3
         assert not hasattr(new_a, "d")
+
+
+def test_slots_unpickle_is_backward_compatible(frozen):
+    """
+    Ensure object pickled before v22.2.0 can still be unpickled.
+    """
+    a = A(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