[Django] #33333: Models with a BinaryField fail to deepcopy
Django
-------------------------------------+-------------------------------------
Reporter: Adam | Owner: nobody
Zimmerman |
Type: Bug | Status: new
Component: Database | Version: 3.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
While using Postgres, models with a BinaryField fail to deepcopy, due to
Django returning these field values as `memoryview` objects. This causes
issues when creating instances in `setUpTestData()` during tests.
The specific errors seem to be fairly inconsistent and hard to reproduce.
But often the exception complains about a missing `_state` field, or that
`_state` has no attribute `db`.
I've currently worked around this by implementing a mixin for models where
the `__getstate__()` method converts any `memoryview`s to `bytes`. But I
don't know if that's the correct solution for Django generally.
--
Ticket URL: <https://code.djangoproject.com/ticket/33333>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam Zimmerman):
For anyone who finds this ticket and wants to work around it before it's
fixed in Django, here's the mixin I'm currently using:
{{{#!python
class ContainsBinaryField:
"""A mixin for model classes that contain BinaryField fields
Django 3.2 has started using deepcopy() to isolate changes to data
created in
setUpTestData(). This seems to cause issues with models that have a
BinaryField in
them. This implements the __getstate__() method in a way that converts
any
memoryview data to bytes, which seems to fix the issue."""
def __getstate__(self):
state = super().__getstate__()
for key, value in state.items():
if isinstance(value, memoryview):
state[key] = bytes(value)
return state
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:1>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
* severity: Normal => Release blocker
Comment:
Thanks for the report! Regression in
3cf80d3fcf7446afdde16a2be515c423f720e54d. I was able to reproduce this
issue on PostgreSQL with the following test:
{{{#!python
class MyTests(TestCase):
@classmethod
def setUpTestData(cls):
MyModel.objects.create(binary=b'y')
cls.my_binary = MyModel.objects.get()
def test_1(self):
self.my_binary.binary = b'z'
self.my_binary.save()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:2>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Feels like the issue should be solved at the model level because while it
break `setupTestData` (3cf80d3fcf7446afdde16a2be515c423f720e54d) it also
breaks any form of `QuerySet` or `Model` pickling
{{{#!python
MyModel.objects.create(binary=b'y')
pickle.dumps(MyModel.objects.all())
}}}
This can be reproduced by adding a `BinaryField` to
[https://github.com/django/django/blob/ca9872905559026af82000e46cde6f7dedc897b6/tests/queryset_pickle/models.py#L48
models] in the test `queryset_pickle` apps.
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:3>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
My first instinct would be to have `BinaryField.from_db_value` use
`force_bytes` to store `bytes` instead of `memoryview` in `_state` or
introduce a hook to allow `Field` subclass to define a hook using during
serialization and have the model machinery
To me that's more of a long standing issue with `BinaryField` and less of
a regression cause by 3cf80d3fcf7446afdde16a2be515c423f720e54d.
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:4>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Simon, your diagnostic might be right, but I think it's late in the 4.0
release cycle to make such changes now, also considering changing
`memoryview` to `bytes` might have serious memory implications not plainly
visible in tests.
What about writing a workaround for `setuptestData` in 4.0, and make a
more proper and thorough fix in 4.1?
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:5>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Given we're pretty close to the 4.0 release date I'm not against including
a limited hack for `setUpTestData` that special cases `Model` and
`QuerySet` instances.
It does feel weird that we're treating this as a release blocker now
thought given it has been broken since `BinaryField` was introduced and
`setUpTestData`
[https://github.com/django/django/commit/3cf80d3fcf7446afdde16a2be515c423f720e54d
#diff-
7833da5b45a68d00834388d97dd5c4413e3796497c7bc5e0cc2621b08a2d0df1R1108-R1120
has been emitting deprecation warnings about it since Django 3.2].
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:6>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
We have also a ticket about `BinaryField` inconsistency, see #27813.
> What about writing a workaround for setuptestData in 4.0 ...
For me it is a 3.2 release blocker.
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:7>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
> considering changing memoryview to bytes might have serious memory
implications not plainly visible in tests.
Fixing #27813 would be somewhat of a breaking change on PostgreSQL as a
different type would be returned but I doubt it would have memory
implications in the sense of elevated memory usage.
In the end `memoryview` is a pointer to a sequence of bytes but these
bytes have to live somewhere when returned from the database backend and
made available to the client. Returning them proxied by a `memoryview`
instance or not should not make a significant difference in memory usage
beyond the size of the `memoryview` instance itself.
---
Mariusz, how do you want to proceed here? I have to admit I'm not sure how
to navigate the current state of things.
Should we submit a patch meant to be backported to 3.2 and 4.0 in
`setUpTestData` that solely deals with this particular edge case of models
and queryset with a `BinaryField` on Postgres to circumvent #27813? What
if we get other reports of users who ignored the deprecation warning for
other non-pickleable objects assigned during `setUpTestData`?
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:8>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
> Should we submit a patch meant to be backported to 3.2 and 4.0 in
setUpTestData that solely deals with this particular edge case of models
and queryset with a BinaryField on Postgres to circumvent #27813?
Yes, it should be feasible by juggling with `__getstate__()/__reduce__()`
hooks 🤔 I think we should treat this as a release blocker because
`BinaryField` is built-in into Django and
3cf80d3fcf7446afdde16a2be515c423f720e54d broke previously working tests.
> What if we get other reports of users who ignored the deprecation
warning for other non-pickleable objects assigned during setUpTestData?
That would not be a release blocker, IMO, unless any other built-in is
non-pickleable.
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:9>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Mariusz Felisiak
* status: new => assigned
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/15144 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:10>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: Mariusz
| Felisiak
Component: Database layer | Version: 3.2
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"2c7846d992ca512d36a73f518205015c88ed088c" 2c7846d]:
{{{
#!CommitTicketReference repository=""
revision="2c7846d992ca512d36a73f518205015c88ed088c"
Fixed #33333 -- Fixed setUpTestData() crash with models.BinaryField on
PostgreSQL.
This makes models.BinaryField pickleable on PostgreSQL.
Regression in 3cf80d3fcf7446afdde16a2be515c423f720e54d.
Thanks Adam Zimmerman for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:11>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"2c20883cb0df8c56ecbb9a093fc4252410140b59" 2c20883]:
{{{
#!CommitTicketReference repository=""
revision="2c20883cb0df8c56ecbb9a093fc4252410140b59"
[4.0.x] Fixed #33333 -- Fixed setUpTestData() crash with
models.BinaryField on PostgreSQL.
This makes models.BinaryField pickleable on PostgreSQL.
Regression in 3cf80d3fcf7446afdde16a2be515c423f720e54d.
Thanks Adam Zimmerman for the report.
Backport of 2c7846d992ca512d36a73f518205015c88ed088c from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:12>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"cb724ef6c0fd71cd5bdbdc27c02f4da0f7e2ad08" cb724ef6]:
{{{
#!CommitTicketReference repository=""
revision="cb724ef6c0fd71cd5bdbdc27c02f4da0f7e2ad08"
[3.2.x] Fixed #33333 -- Fixed setUpTestData() crash with
models.BinaryField on PostgreSQL.
This makes models.BinaryField pickleable on PostgreSQL.
Regression in 3cf80d3fcf7446afdde16a2be515c423f720e54d.
Thanks Adam Zimmerman for the report.
Backport of 2c7846d992ca512d36a73f518205015c88ed088c from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:13>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"d3a64bea51676fcf8a0ae593cf7b103939e12c87" d3a64be]:
{{{
#!CommitTicketReference repository=""
revision="d3a64bea51676fcf8a0ae593cf7b103939e12c87"
Refs #33333 -- Fixed
PickleabilityTestCase.test_annotation_with_callable_default() crash on
Oracle.
Grouping by LOBs is not allowed on Oracle. This moves a binary field to
a separate model.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:14>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"7bde53a7ae7db5b97bcc3ef8eed2a7195cc0f258" 7bde53a]:
{{{
#!CommitTicketReference repository=""
revision="7bde53a7ae7db5b97bcc3ef8eed2a7195cc0f258"
[4.0.x] Refs #33333 -- Fixed
PickleabilityTestCase.test_annotation_with_callable_default() crash on
Oracle.
Grouping by LOBs is not allowed on Oracle. This moves a binary field to
a separate model.
Backport of d3a64bea51676fcf8a0ae593cf7b103939e12c87 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:15>
Django
PostgreSQL.
-------------------------------------+-------------------------------------
Reporter: Adam Zimmerman | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"6014b812e2608eae1b0950eed8a5891cadc6ca77" 6014b812]:
{{{
#!CommitTicketReference repository=""
revision="6014b812e2608eae1b0950eed8a5891cadc6ca77"
[3.2.x] Refs #33333 -- Fixed
PickleabilityTestCase.test_annotation_with_callable_default() crash on
Oracle.
Grouping by LOBs is not allowed on Oracle. This moves a binary field to
a separate model.
Backport of d3a64bea51676fcf8a0ae593cf7b103939e12c87 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33333#comment:16>