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

v0.3.2 breaks Django 1.8.4 #19

Closed
Koed00 opened this issue Sep 6, 2015 · 11 comments
Closed

v0.3.2 breaks Django 1.8.4 #19

Koed00 opened this issue Sep 6, 2015 · 11 comments

Comments

@Koed00
Copy link

Koed00 commented Sep 6, 2015

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
Copy link
Collaborator

Please provide a test case to reproduce.

@Koed00
Copy link
Author

Koed00 commented Sep 7, 2015

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
Copy link
Author

Koed00 commented Sep 7, 2015

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
Copy link
Author

Koed00 commented Sep 7, 2015

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
Copy link
Collaborator

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 9cabb40.

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

@Koed00
Copy link
Author

Koed00 commented Sep 7, 2015

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
Copy link
Collaborator

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

@Koed00
Copy link
Author

Koed00 commented Sep 7, 2015

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
Copy link
Collaborator

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
Copy link
Author

Koed00 commented Sep 7, 2015

Perfect, thanks a million.

@charettes
Copy link
Collaborator

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.

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

No branches or pull requests

2 participants