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

Configurable Serialization #63

Merged

Conversation

northernSage
Copy link
Member

@northernSage northernSage commented Aug 28, 2021

Some initial work following #11

  • Implement default serializers for each cache type
  • Move serializers into separate module
  • Extract common serialization logic to BaseSerializer
  • add deprecation warning for RedisCache.dump_object and RedisCache.load_object
  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.
  • Add new entry to changelog

@northernSage northernSage added the new feature Implementation of or related to a new feature label Aug 28, 2021
@northernSage northernSage self-assigned this Aug 28, 2021
@northernSage
Copy link
Member Author

In case anyone has any suggestions now would be a good time to let me know while this is still a draft PR, being actively worked on 😉

@northernSage
Copy link
Member Author

northernSage commented Sep 5, 2021

Quick update

  • If we instantiate FilesystemCache and then overwrite a previously set serializer class attribute (that is what I tried first), management files that are created during instantiation will be serialized with the default serializer and later attempts of deserialization will use the custom serializer which causes UB or just errors out. I'm currently avoiding that by passing the custom serializer to the cache class constructor so it's used from the start even for management files. I'm doing the same thing for all other cache classes to keep it uniform.

  • FilesystemCache is a bit different in how it handles timeout values. All cache entries will be stored in their own file along with their timeout value (in the form: <timeout><entry_value>) so the serializer must account for that since every FilesystemCache.get call will first read the timeout to check if the entry has expired and only then will read the actual value of the entry. I still haven't decided if this is something that needs to change in FilesystemCache or if the user should implement their serializer around this as I did with the SillySerializer in the tests.

@northernSage northernSage marked this pull request as ready for review September 11, 2021 19:39
@northernSage northernSage linked an issue Nov 4, 2021 that may be closed by this pull request
@northernSage northernSage merged commit 75b5f2f into pallets-eco:main Nov 8, 2021
@northernSage northernSage deleted the configurable-serialization branch November 8, 2021 20:55
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new feature Implementation of or related to a new feature
Development

Successfully merging this pull request may close these issues.

configurable serializer
1 participant