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

Change __getstate__ and __setstate__ to use a dict (#1004) #1009

Merged
merged 1 commit into from Aug 18, 2022

Conversation

djipko
Copy link
Contributor

@djipko djipko commented Aug 16, 2022

Summary

We use a dict instead of a tuple, so that we assign only existing attributes on unpickle, if the class
definition has changed in the meantime.

Resolves #1004

@djipko djipko force-pushed the getstate branch 2 times, most recently from 6ffbcf6 to f91ffe4 Compare August 16, 2022 16:53
@hynek
Copy link
Member

hynek commented Aug 16, 2022

@hynek
Copy link
Member

hynek commented Aug 16, 2022

I think it’s actually an interesting case missing: what are we gonna do, if there’s extra names? By your motivation, we should actually blow up, no?

@djipko
Copy link
Contributor Author

djipko commented Aug 17, 2022

I think it’s actually an interesting case missing: what are we gonna do, if there’s extra names? By your motivation, we should actually blow up, no?

So the case missing is when what we re-hydrate has less attributes than our current class implementation declares, and we have to leave some of them empty. So that's an interesting one. I don't think we should necessarily blow up in every case, as there's some chance calling code can handle this (say, check for None or hasattr). The difference with the reported case and this is that in the former, the calling code stands no chance, it gets a completely wrong value (and why I thought we should fix it).

I think failing early (on unpickling) is tempting here, but might be too aggressive. It is likely that the calling code that doesn't handle this well and tries to access a non existing attribute will blow up anyway. Another potential solution would be to check if we have an .init on the attribute and assign that but even that seems over-reaching as it re-implements parts of initialisation of the class which is clearly skipped when unpickling by design.

At the end of the day, the point of this is to avoid a clear (if a bit obscure :) ) foot gun, and not re-implement versioning on top of pickle. People who actually need and want that can use protobuf or a number of actual solutions to this problem. So I would go with - just skip it (as it would have before). I will add a test

@djipko
Copy link
Contributor Author

djipko commented Aug 17, 2022

$ tox -e coverage-report
coverage-report installed: coverage==6.4.4,tomli==2.0.1
coverage-report run-test-pre: PYTHONHASHSEED='1249099045'
coverage-report run-test: commands[0] | coverage combine
Combined data file .coverage.dubndipanovmacbook1.87338.100213
Combined data file .coverage.dubndipanovmacbook1.87526.556153
coverage-report run-test: commands[1] | coverage report
Name    Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------
---------------------------------------------------
TOTAL    1494      0    723      0   100%

19 files skipped due to complete coverage.
______________________________________________________________________ summary _______________________________________________________________________
  coverage-report: commands succeeded
  congratulations :)

Got this locally now so feeling hopeful 🤞 😅

Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

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

Cool, thanks! I'll write a news fragment.

@Mirandatz
Copy link

I feel like this may be a breaking change. Do objects serialized with the previous version can be deserialized with the new version?

@hynek
Copy link
Member

hynek commented Feb 5, 2023

They cannot. A fix has been added in #1085 that will be in 23.1, but (de)serializing classes across versions is risky (in general, not just the attrs case) so we'd encourage to look into avoiding that.

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.

Current default implementation of __getstate__ and __setstate__ could be made safer
3 participants