gintas / django-picklefield

A pickled object field for Django
MIT License
180 stars 47 forks source link

v0.3.2 breaks Django 1.8.4 #19

Closed Koed00 closed 9 years ago

Koed00 commented 9 years ago

Since release v0.3.2, tests for my own project with Django 1.8.4 on Python 2.7 and 3.4 fail. Tests with 1.7.10 seem unaffected.

I will have to look at the code, but from my test log it looks like all results return the integer 1, instead of the actual field value.

charettes commented 9 years ago

Please provide a test case to reproduce.

Koed00 commented 9 years ago

First think I found this morning is that v0.3.1 values_list('picklefield') would return undecoded strings. V0.3.2 decodes them. This breaks any code that tries to dbsafe_decode a values_list.

Running some more tests.

Koed00 commented 9 years ago

The problem seems to be that in Django 1.8.4 , the values_list values are nice and decoded. In Django 1.7.10 , the values_list values still return encoded .Which was the old behavior.

Koed00 commented 9 years ago
testmodel = TestModel.objects.first()
testmodel.picklefield = 'hello'
TestModel.objects.filter(pk=testmodel.pk).values_list('picklefield', flat=True)[0]==testmodel.picklefield

In the current version , this is only true for Django 1.8.4 and not for Django 1.7.10 The fix for this used to be to dbsafe_decode the value, but since it's now already decoded with 1.8.4, dbsafe_decode raises an exception.

charettes commented 9 years ago

As of Django 1.8+ you should expect all fields to be automatically decoded when using values(_list)?() as documented in the release notes.

You weren't getting any failures when using django-picklefield==0.3.1 because it was using the deprecated SubfieldBase API that was removed in 9cabb40191e4fda1f5246da46ca1f59dd5bef362.

Since this Django change is definitely an improvement and is here to stay I'll close this ticket.

Koed00 commented 9 years ago

Do you have a suggestion how I run my code on both versions of Django without having to resort to checking for Django versions?

charettes commented 9 years ago

I'm afraid this will really hard to achieve without relying on Django version checking. Can you point me to some code?

Koed00 commented 9 years ago

I agree with you that this is a real improvement and I'm happy someone has taken it upon themselves to keep django-picklefield up to date.

    @staticmethod
    def get_result_group(group_id, failures=False):
        # values + decode is 10 times faster than just list comprehension
        if failures:
            values = Task.objects.filter(group=group_id).values_list('result', flat=True)
        else:
            values = Task.objects.filter(group=group_id).exclude(success=False).values_list('result', flat=True)
        return [dbsafe_decode(t) for t in values]

https://github.com/Koed00/django-q/blob/master/django_q/models.py

After the 0.3.2 update I removed the dbsafe_decode() iteration and that tested just fine. Unfortunately my projects tries to be 1.7 compatible too, so those tests then failed on Travis. I've been thinking about refactoring a version of dbsafe_encode with a try clause in it, but that feels counterproductive. There is no real way of determining if a string is base64 encoded either.

charettes commented 9 years ago

I'd would suggest you create a compat module with the following code and import it wherever you need to do some conditional decoding.

from picklefield import PickledObjectField, dbsafe_decode

if hasattr(PickledObjectField, 'from_db_value'):
    def decode_results(values):
        return values
else:
    def decode_results(values):
        return [dbsafe_decode(v) for v in values]
Koed00 commented 9 years ago

Perfect, thanks a million.

charettes commented 9 years ago

You could then replace your method by:

from .compat import decode_results
...
def get_result_group(group_id, failures=False):
    # values + decode is 10 times faster than just list comprehension
    if failures:
        values = Task.objects.filter(group=group_id).values_list('result', flat=True)
    else:
        values = Task.objects.filter(group=group_id).exclude(success=False).values_list('result', flat=True)
    return decode_results(values)

And simply remove the decode_results compatibly shim when dropping support for Django 1.7.