Opened 12 months ago

Closed 12 months ago

Last modified 12 months 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 Changed 12 months ago 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:

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 Changed 12 months ago by Mariusz Felisiak

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 Changed 12 months ago by Simon Charette

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 Changed 12 months ago 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 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 12 months ago by Simon Charette (previous) (diff)

comment:5 Changed 12 months ago 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?

comment:6 Changed 12 months ago 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 though given it has been broken since BinaryField was introduced and setUpTestData has been emitting deprecation warnings about it since Django 3.2.

Last edited 12 months ago by Simon Charette (previous) (diff)

comment:7 Changed 12 months ago 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.

comment:8 Changed 12 months ago 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?

comment:9 Changed 12 months ago 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.

comment:10 Changed 12 months ago by Mariusz Felisiak

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

comment:11 Changed 12 months ago by GitHub <noreply@…>

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 Changed 12 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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 Changed 12 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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 Changed 12 months ago by GitHub <noreply@…>

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 Changed 12 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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 Changed 12 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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