Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33333 closed Bug (fixed)

Models with a BinaryField fail to deepcopy in setUpTestData() on PostgreSQL.

Reported by: Adam Zimmerman Owned by: Mariusz Felisiak
Component: Database layer (models, ORM) Version: 3.2
Severity: Release blocker Keywords:
Cc: Simon Charette Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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 memoryviews to bytes. But I don't know if that's the correct solution for Django generally.

Change History (16)

comment:1 by Adam Zimmerman, 3 years ago

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:

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

comment:2 by Mariusz Felisiak, 3 years ago

Cc: Simon Charette added
Severity: NormalRelease blocker
Summary: Models with a BinaryField fail to deepcopyModels with a BinaryField fail to deepcopy in setUpTestData() on PostgreSQL.

Thanks for the report! Regression in 3cf80d3fcf7446afdde16a2be515c423f720e54d. I was able to reproduce this issue on PostgreSQL with the following test:

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()

comment:3 by Simon Charette, 3 years ago

Triage Stage: UnreviewedAccepted

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

MyModel.objects.create(binary=b'y')
pickle.dumps(MyModel.objects.all())

This can be reproduced by adding a BinaryField to models in the test queryset_pickle apps.

comment:4 by Simon Charette, 3 years ago

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 Model.__getstate__ call it.

To me that's more of a long standing issue with BinaryField and less of a regression caused by 3cf80d3fcf7446afdde16a2be515c423f720e54d.

Last edited 3 years ago by Simon Charette (previous) (diff)

comment:5 by Claude Paroz, 3 years ago

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?

comment:6 by Simon Charette, 3 years ago

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 has been emitting deprecation warnings about it since Django 3.2.

Version 0, edited 3 years ago by Simon Charette (next)

comment:7 by Mariusz Felisiak, 3 years ago

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.

comment:8 by Simon Charette, 3 years ago

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?

comment:9 by Mariusz Felisiak, 3 years ago

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.

comment:10 by Mariusz Felisiak, 3 years ago

Has patch: set
Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

comment:11 by GitHub <noreply@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 2c7846d:

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.

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 2c20883:

[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.

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In cb724ef6:

[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.

comment:14 by GitHub <noreply@…>, 3 years ago

In d3a64be:

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.

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 7bde53a:

[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

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 6014b812:

[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

Note: See TracTickets for help on using tickets.
Back to Top