Support for pickle

[RikoSmith]

Hi!

I wonder if it's possible to implement support for pickle. Currently dynamically created "Sealed" classes do not allow queryset to be pickled. Pickle does not support dynamically generated types in general. Although there are workarounds for it, implementing them may be quite difficult depending on the situation.

[charettes]

PRs welcome but is there a reason you can't make sure that the queryset you are using subclasses SealableQueryset directly to avoid the dynamic class creation in the first place? This should ensure they a pickleable.

[charettes]

I think I get what you mean, you are referring to the pickle-ability of related querysets?

--- a/tests/test_query.py
+++ b/tests/test_query.py
@@ -1,3 +1,4 @@
+import pickle
 import warnings

 from django.apps import apps
@@ -504,6 +505,13 @@ def test_sealed_prefetch_reverse_generic_many_to_one_results(self):
             self.assertEqual(list(results[0].nicknames.all()), [self.nickname])
             self.assertEqual(list(results[1].nicknames.all()), [other_nickname])

+    def test_related_sealed_pickleability(self):
+        location = Location.objects.prefetch_related("climates").seal().get()
+        climates_dump = pickle.dumps(location.climates.all())
+        climates = pickle.loads(climates_dump)
+        with self.assertNumQueries(0):
+            self.assertEqual(list(climates)[0], self.climate)
+

 class SealableQuerySetInteractionTests(SimpleTestCase):
     def test_values_seal_disallowed(self):

[RikoSmith]

Thanks for the reply! Yes, even if queryset class of the custom manager is set to SealableQueryset explicitly, dynamic classes are created for related querysets (which are not SealableQueryset) where managers (i.e. BaseManagers) are assigned by django itself. At least that's what I think it's happening, I might be wrong though - still learning inner workings of django. The solution may require setting the default manager/queryset as SealableQueryset for every model that might be a related field for the target model. That is if we want to avoid the creation of dynamic classes and complaints from pickle.

The other solution might be to embrace dynamic classes and make them 'pickleable' by adding __reduce__ method (tuple version) to them. It should, in theory, give information to pickle and avoid the lookup of dynamic classes inside the module.

[charettes]

I've started to play with __reduce__ but it seems that it's not picked up for types as I was hoping to simply do

diff --git a/seal/descriptors.py b/seal/descriptors.py
index d2f7e2b..2785fdf 100644
--- a/seal/descriptors.py
+++ b/seal/descriptors.py
@@ -1,5 +1,5 @@
 import warnings
-from functools import lru_cache
+from functools import partial, lru_cache

 from django.contrib.contenttypes.fields import (
     GenericForeignKey,
@@ -26,7 +26,13 @@ def _bare_repr(instance):
     return "<%s instance>" % instance.__class__.__name__

-class _SealedRelatedQuerySet(QuerySet):
+class _SealedRelatedQuerySetType(type(QuerySet)):
+    def __reduce__(self):
+        return _sealed_related_queryset_type_factory, (self._unsealed_class,)
+
+
+class _SealedRelatedQuerySet(QuerySet, metaclass=_SealedRelatedQuerySetType):
     """
     QuerySet that prevents any fetching from taking place on its current form.

It seems that object.__reduce__ doesn't account metaclass overrides so I'll have to have a top level function that creates the class from the factory as well as its instance.

[RikoSmith]

Thanks for looking into this issue and for the quick replies! I agree, it's more complicated than it may seem at first. I am on the verge of giving up on pickle and substituting it with dill for now, which should support serializing dynamic classes out of the box (as many suggest).

[charettes]

@RikoSmith let me know if #72 addresses your issue. If it's the case I should be able to release 1.5.1 with the issue solved tomorrow. Cheers!

[RikoSmith]

Thanks a lot! I'll test it today!

[RikoSmith]

Tested and now it works! Thank you for the fix! :rocket:

[charettes]

Fixed in 1.5.1