Potentially unplanned regression regarding ChoiceField and Enums · encode/django-rest-framework · Discussion #9388 (original) (raw)

We're upgrading from 3.12.4 to 3.15.1 as part of a Django 3.2 to 4.2 upgrade. I believe there was a regression caused by this PR https://github.com/encode/django-rest-framework/pull/8955/files but I'm not sure if it is known/expected.

We have the following approximate setup:

class Status(Enum):
    unverified = 0
    verified = 1

    @classmethod
    def as_choices(cls):
        return [
            (status, status.name)
            for status in [
                cls.unverified,
                cls.pending,
                cls.verified,
                cls.rejected,
                cls.rejected_unable_to_verify,
                cls.rejected_prohibited_content,
                cls.age_gate_required,
            ]
        ]

class MySerializer(serializers.Serializer):
    status = serializers.ChoiceField(
        choices=Status.as_choices(),
    )

The integers are the keys used internally, but the client passed the key of the enum to the view ({"status": "unverified"}). Previously this was working but the PR above broke it in favor of the opposite (passing thechoice.value instead of the str(choice)). There seemed to be some concern around regressions and testing that didn't look like they were addressed in the PR, so I'm wondering if this regression is expected or if this is something that should be fixed.

Thanks in advance!

You must be logged in to vote

We just got hit by the exact same issue and end up with the same question of what is actually the expectation here. To make the matter worst, in our case the enum value is a callable, thus it's impossible to change our FE to send that value, we do need to send the key so that we get the related enum instance back.

You must be logged in to vote

2 replies

Probably not helpful, but our stopgap solution was to copy the old ChoiceField's code into our project, name it UnsupportedChoiceField and switch our serializers over to that until I get an answer here. At least to get us through the 4.2 rollout.

Went almost the same route but by subclassing it:

class EnumChoiceField(serializers.ChoiceField):
    def _set_choices(self, choices):
        super()._set_choices(choices)
        self.choice_strings_to_values = {
            str(key.name.lower()) if isinstance(key, Enum) else str(key): key for key in self.choices
        }

    choices = property(serializers.ChoiceField._get_choices, _set_choices)

Still would like it properly supported as it was before

It would be great if you can come with a proper fix to get rid of the regression!

You must be logged in to vote

2 replies

The first question is that while it does regress, and that's something we need, we are not sure if it's actually desired behavior. I did read all the discussion, pr, commit leading to that change and my understating is that they previously behavior was wrong even if we were happy using it. But still, not sure the new behavior make sense with a complex enum like our case and I believe the OP case is also legitimate.

Reading the django doc about optionField doesn't make it clearer to me.

Now let say we do agree it's a regression and desired behavior, I think a good solution could be to include both value and key in the string map conversion, that way both could be used from client side. There are some edge case as values could contain duplicate (we actually have one in our case) but maybe it's not an issue either from drf perspective.

If we do agree, I'd be happy to contribute a pr for it with above solution or other suggestions.

I will happily review your solution

Add me to the list of people thinking that this is a regression and a breaking change. We have a subclass of the ChoiceField, defined as:

from rest_framework import serializers

class MyChoiceField(serializers.ChoiceField):
    def to_representation(self, instance):
        ret = super().to_representation(instance)
        return self.grouped_choices[ret]

We also have an enum with string values:

from enum import Enum

class DisabledCrossFieldMetricMessageEnum(Enum):
    GENERIC = "Field not compatible with cross-fields"
    INCOMPATIBLE_TAGS = "Field not compatible with tag rule"

    @classmethod
    def choices(cls):
        return [(i, i.value) for i in cls]

In djangorestframework 3.14, this returned "Field not compatible with cross-fields"

disabled_for_cross_field_message = MyChoiceField(
    choices=DisabledCrossFieldMetricMessageEnum.choices(), allow_null=True
)
rep = disabled_for_cross_field_message.to_representation(
    "DisabledCrossFieldMetricMessageEnum.GENERIC"
)
print(rep)

On djangorestframework 3.15, the same code raises a KeyError (KeyError: 'Field not compatible with cross-fields'). However, if we use the value as the key, it succeeds. But it would require us knowing the data we're trying to pull out of the field, so it's not very useful:

disabled_for_cross_field_message = MyChoiceField(
    choices=DisabledCrossFieldMetricMessageEnum.choices(), allow_null=True
)
rep = disabled_for_cross_field_message.to_representation(
    "Field not compatible with cross-fields"
)
print(rep)

Surely this can't be the intended behavior.

You must be logged in to vote

0 replies