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

Fix #246 -- Add support for deepcopy #265

Merged
merged 1 commit into from
Mar 19, 2022
Merged

Conversation

vchrisb
Copy link

@vchrisb vchrisb commented Nov 17, 2021

this PR should fix #246

With Django 2.2 being EOL in April 2022, deepcopy support is only fixed for Django 3+

@codecov
Copy link

codecov bot commented Nov 17, 2021

Codecov Report

Merging #265 (312f73f) into master (90cf8de) will increase coverage by 0.15%.
The diff coverage is 100.00%.

❗ Current head 312f73f differs from pull request most recent head ff55581. Consider uploading reports for the commit ff55581 to get more accurate results

@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
+ Coverage   97.40%   97.56%   +0.15%     
==========================================
  Files           5        5              
  Lines         270      287      +17     
==========================================
+ Hits          263      280      +17     
  Misses          7        7              
Impacted Files Coverage Δ
stdimage/models.py 96.90% <100.00%> (+0.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90cf8de...ff55581. Read the comment docs.

@vchrisb vchrisb changed the title Deepcopy support for Django 3.2 Deepcopy support for Django 3+ Nov 17, 2021
@codingjoe
Copy link
Owner

Thanks, @vchrisb, to be honest. I am going to need a minute to review this. Django does a lot of magic here to avoid issues with the file objects when coping them. Copying the whole dict seems to break this again. Would you be so kind as to share a little more information about the exception and the error you are seeing in your setup in the issue description? That would help me a lot to identify the best solution. Thanks, Joe

@vchrisb
Copy link
Author

vchrisb commented Nov 23, 2021

There is no exception itself while executing setUpTestData() but the deepcopied objects used in the test cases are missing the variations. And therefore testcases trying to access the variations will fail.

From my understanding, deepcopy is using the attributes of the StdImageFieldFile Class to generate the list of attributes to be copied. But due to the variations being dynamic, deepcopy does not know that these need to be copied.
By adding __getstate__, called by deepcopy, it does return the list of attributes for the instance, rather for the class.
Actually deepcopy is using pickle, which is calling __getstate__. https://docs.python.org/3/library/pickle.html#pickle-inst

You can look at provided test case and run it without the addition of __getstate__.

To further clarify, it looks like deepcopy never worked with django-stdimages on any Django Version, but only the changes to setUpTestData() in Django 3.2 surfaced the issue.

I don't know why deepcopy still fails with django 2.2, but with it being EOL very soon, I did not further look into it.

@vchrisb
Copy link
Author

vchrisb commented Nov 23, 2021

oh, self.__dict__copy() is actually not needed. Returning it directly is also fine:

    def __getstate__(self):
        return self.__dict__

and maybe relevant:
https://github.com/django/django/blob/main/django/db/models/base.py#L571

Copy link
Owner

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @vchrisb, for the explanation and for your endurance. I just did a quick review and noticed that Django has some intriguing behavior in the super class.

stdimage/models.py Outdated Show resolved Hide resolved
@vchrisb
Copy link
Author

vchrisb commented Mar 14, 2022

@codingjoe I spend some time on it again...
Would the following be ok for the __getstate__ ?

    def __getstate__(self):
        state = super().__getstate__()
        for variation_name in self.field.variations:
            variation = getattr(self, variation_name)
            variation_state = variation.__getstate__()
            state[variation_name] = variation_state
        return state

Still struggle with unpickle using __setstate___ though.
I did not yet find a way to unpickle the variations back to ImageFieldFiles.

@vchrisb vchrisb requested a review from codingjoe March 15, 2022 19:40
@codingjoe codingjoe changed the title Deepcopy support for Django 3+ Fix #246 -- Add support for deepcopy Mar 19, 2022
Copy link
Owner

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you, @vchrisb. This looks excellent! 🥇

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

Successfully merging this pull request may close these issues.

Django 3.2 setUpTestData uses copy.deepcopy()
2 participants