Opened 8 months ago

Closed 3 months ago

#22951 closed Bug (fixed)

Adding 'deconstruct' method breaks serialization of type

Reported by: Sam Hartsfield <samh.public+django@…> Owned by: jambonrose
Component: Migrations Version: 1.7-rc-1
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

A custom field may take a type as as argument (for example, an Enum type). Normally, class references are one of the things the migrations framework can serialize by default (https://docs.djangoproject.com/en/dev/topics/migrations/#serializing-values). However, we probably also want to serialize the value, otherwise we get an error (see ValueEnum in the example).

To serialize the value, we add a deconstruct method (in the example of ValueEnum2, this uses the deconstructible decorator). I would expect that, after adding this, the field would work. However, django.db.migrations.autodetector.deep_deconstruct tries to call deconstruct on the class.

ValueEnum3 is an example of a workaround, which should probably not be required.

I would expect Django to either check whether something is a type before calling deconstruct, or to not call deconstruct if it is an instance method (e.g. you might want to be able to also add a deconstruct classmethod to customize deconstruction of the class).

# Requires Python 3.4 or the 'enum34' package
import enum
import types
from django.db import models
from django.utils.deconstruct import deconstructible
from django.utils.six import with_metaclass


class SimpleEnumField(with_metaclass(models.SubfieldBase, models.IntegerField)):
    def __init__(self, enum_type, **kwargs):
        self.enum = enum_type
        choices = [(e.value, e.name) for e in enum_type]
        super(SimpleEnumField, self).__init__(choices=choices, **kwargs)

    def deconstruct(self):
        name, path, args, kwargs = super(SimpleEnumField, self).deconstruct()
        kwargs['enum_type'] = self.enum
        kwargs.pop('choices', None)
        return name, path, args, kwargs

    def get_prep_value(self, value):
        return value.value

    def to_python(self, value):
        return self.enum(value)


class ValueEnum(enum.Enum):
    """
    Django will be able to serialize the type, but not the instance::

        ValueError: Cannot serialize: <ValueEnum.A: 0>

    """
    A = 0
    B = 1


@deconstructible
class ValueEnum2(enum.Enum):
    """
    The deconstructible decorator allows Django to serialize the instances,
    but breaks serialization of the type--it will try to call deconstruct
    on the type, but it will fail because it is an instance method::

        TypeError: deconstruct() missing 1 required positional argument: 'obj'

    """
    A = 0
    B = 1


def deconstruct_enum(self):
    return (
        '%s.%s' % (self.__class__.__module__, self.__class__.__name__),
        (self.value,),
        {}
    )


class ValueEnum3(enum.Enum):
    """
    Workaround: if we add the deconstruct method to the instances, but not
    to the type, then Django will do the right thing.
    """
    A = 0
    B = 1

    def __init__(self, value):
        self.deconstruct = types.MethodType(deconstruct_enum, self)


class Foo(models.Model):
    # This field works
    field1 = SimpleEnumField(ValueEnum)

    # ValueError: Cannot serialize: <ValueEnum.A: 0>
    # field2 = SimpleEnumField(ValueEnum, default=ValueEnum.A)

    # TypeError: deconstruct() missing 1 required positional argument: 'obj'
    # field3 = SimpleEnumField(ValueEnum2, default=ValueEnum2.A)

    field4 = SimpleEnumField(ValueEnum3, default=ValueEnum3.A)

Change History (9)

comment:1 Changed 7 months ago by charettes

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 months ago by andrewgodwin

  • Easy pickings set

This should be an easy change to deep_deconstruct to change "doesn't have deconstruct: return" to "doesn't have deconstruct or is a type: return".

comment:3 Changed 6 months ago by jambonrose

  • Owner changed from nobody to jambonrose
  • Status changed from new to assigned

comment:4 Changed 6 months ago by jambonrose

I have just issued pull-request 3177 to fix the issue.

comment:5 Changed 6 months ago by collinanderson

  • Has patch set

comment:6 Changed 6 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 4680d25df23875bced0f17d361782bda80ce79d6:

Fixed #22951 -- Checked for types during deep_deconstruct migration serialization process.

Thanks Sam Hartsfield for the report.

comment:7 Changed 6 months ago by Tim Graham <timograham@…>

In 27e7972e63c6f7d6555a724a3b356b4e08b0fefa:

[1.7.x] Fixed #22951 -- Checked for types during deep_deconstruct migration serializ

Thanks Sam Hartsfield for the report.

Backport of 4680d25df2 from master

comment:8 Changed 3 months ago by gavinwahl

  • Resolution fixed deleted
  • Status changed from closed to new

I don't think this was fixed everywhere it needed to be -- the migration writer is still calling deconstruct on types here: https://github.com/django/django/blob/master/django/db/migrations/writer.py#L334

Traceback (most recent call last):
  File "./manage.py", line 11, in <module>
    execute_from_command_line(sys.argv)
  File "django/django/core/management/__init__.py", line 385, in execute_from_command_line
    utility.execute()
  File "django/core/management/__init__.py", line 377, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "django/core/management/base.py", line 338, in execute
    output = self.handle(*args, **options)
  File "django/core/management/commands/makemigrations.py", line 124, in handle
    self.write_migration_files(changes)
  File "django/core/management/commands/makemigrations.py", line 152, in write_migration_files
    migration_string = writer.as_string()
  File "django/db/migrations/writer.py", line 131, in as_string
    operation_string, operation_imports = OperationWriter(operation).serialize()
  File "django/db/migrations/writer.py", line 88, in serialize
    arg_string, arg_imports = MigrationWriter.serialize(arg_value)
  File "django/db/migrations/writer.py", line 331, in serialize
    return cls.serialize_deconstructed(path, args, kwargs)
  File "django/db/migrations/writer.py", line 239, in serialize_deconstructed
    arg_string, arg_imports = cls.serialize(arg)
  File "django/db/migrations/writer.py", line 334, in serialize
    return cls.serialize_deconstructed(*value.deconstruct())
TypeError: unbound method deconstruct() must be called with FooEnum instance as first argument (got nothing instead)

comment:9 Changed 3 months ago by timgraham

  • Resolution set to fixed
  • Status changed from new to closed

Could you please open a new ticket? The fix for this one was already released. Thanks!

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