Implements pickle support for Page models #5998
Conversation
Manage this branch in SquashTest this branch here: https://pysilverpickle-streamfield-5fefm.squash.io |
|
Simplest test case would be smth like this: |
|
@pySilver - just checking, is this a stream field problem only or all fields in general. The issue referenced and the code changes look like stream fixes only, but the title says page models. Is the intent of this PR for stream fields only? Or are there problems with other field types? |
|
@lb its more broader problem than just StreamField. Basically anything that extends Block wouldn't work without changes I've made in Block class. StreamField in fact is a different story since it's value is converted into StreamValue that needs to have an instruction how to (un)pickle. |
|
@lb- have you had any time to look into that? |
|
@pySilver thanks for your patience, we had a bit of discussion about this within the core team. The important thing there will be testing to ensure everything works as expected both before sending an object to and retrieving the object from the cache. Are you ok to continue work on this and cover both pickle/unpicke scenarios + add unit tests, including for some more complex scenarios such as custom In general though, this appears to be a great start for this PR and a good direction to head in. |
|
@lb- thanks for feedback. Ok, I'll do my best to create some tests, since it's kinda simple. I'll try complete it sometime next week |
|
Thanks |
|
Wow. This was long. I've discovered this is not a fully correct approach. The problem is, it would recreate very expensive block objects many many times during unpickling, so it kills the idea pickling for cache. I've digged into Django sources once again here All the fuss is about the StreamField returning StremValue in it's Making it work look a little bit hackish and I'd love to see your opinion and ideas on that. Basically we need to tie model field with StreamValue returned from The demo code goes below: import json
from django.db.models.fields import _load_field
from wagtail.core.blocks.stream_block import StreamValue
from wagtail.core.fields import StreamField
def stream_value_loader(app_label, model_name, field_name, field_value) -> StreamValue:
"""Returns StreamValue from pickled data"""
stream_field = _load_field(app_label, model_name, field_name)
stream_value = stream_field.stream_block.to_python(field_value)
stream_value._field = stream_field
return stream_value
def stream_value_reducer(self) -> tuple:
"""Reducer to make this class pickleable."""
return (
stream_value_loader,
(
self._field.model._meta.app_label,
self._field.model._meta.object_name,
self._field.name,
self.get_prep_value(),
),
)
def stream_field_to_python(self, value):
"""
Patch for StreamField.to_python
* Adds StreamValue._field reference to associated StreamField so we can unpickle later.
"""
if value is None or value == "":
ret = StreamValue(self.stream_block, [])
ret._field = self
return ret
elif isinstance(value, StreamValue):
ret = value
ret._field = self
return ret
elif isinstance(value, str):
try:
unpacked_value = json.loads(value)
except ValueError:
# value is not valid JSON; most likely, this field was previously a
# rich text field before being migrated to StreamField, and the data
# was left intact in the migration. Return an empty stream instead
# (but keep the raw text available as an attribute, so that it can be
# used to migrate that data to StreamField)
ret = StreamValue(self.stream_block, [], raw_text=value)
ret._field = self
return ret
if unpacked_value is None:
# we get here if value is the literal string 'null'. This should probably
# never happen if the rest of the (de)serialization code is working properly,
# but better to handle it just in case...
ret = StreamValue(self.stream_block, [])
ret._field = self
return ret
ret = self.stream_block.to_python(unpacked_value)
ret._field = self
return ret
else:
# See if it looks like the standard non-smart representation of a
# StreamField value: a list of (block_name, value) tuples
try:
[None for (x, y) in value]
except (TypeError, ValueError):
# Give up trying to make sense of the value
raise TypeError(
"Cannot handle %r (type %r) as a value of StreamField"
% (value, type(value))
)
# Test succeeded, so return as a StreamValue-ified version of that value
ret = StreamValue(self.stream_block, value)
ret._field = self
return ret
def patch_blocks():
"""Patches Wagtail StreamField & StreamValue so it can be pickled"""
StreamValue.__reduce__ = stream_value_reducer
StreamField.to_python = stream_field_to_pythonTo make it more resilient, |
|
@lb- ping :) |
|
haha thanks for the ping @pySilver - I will try to take another look at this soon. I am not the best member of the team to dig into this fully though, I will flag with the core team and see who else can take a look also. |
|
(code comment updated) |
|
Btw, latest 2.10.1 breaks pickling for BaseSettings by adding page_url attribute that cannot be serialized. |
Hi!
This PR misses tests but it will be nice if some maintainer review and maybe adopt this changes into the project.
My PR fixes broken Page model pickle support (django models are pickleable by default). So with this patch you can use popular caching solutions such as beloved django-cacheops (or any other cache battery that needs to cache querysets and/or model instances).
Problem was originally mentioned in #1988