From 85881488bb804d10c72685c82f904bb191e6c982 Mon Sep 17 00:00:00 2001 From: Owen Niles Date: Sat, 3 Aug 2019 18:50:28 +0000 Subject: [PATCH] Modified `serializers.ChoiceField` behavior convert `display_name` --> `key` during deserialization and `key` --> `display_name` during serialization. Perhaps I misunderstand the intended behavior of `fields.ChoiceField`, but I noticed recently that the although every `ChoiceField` instance stores the full list of choices with which it is initialized (a list of pairs of `key` and `display_name`), I couldn't figure out when it ever uses a choice's `display_name`. As a result, I found myself manually converting user input values, which matched choice `display_name`s, to the underlying data values that match the choice `key`s. This commit modifies the behavior of `ChoiceField` instances so that they automatically deserialize user input into the corresponding underlying data value and do the reverse for serialization. --- rest_framework/fields.py | 20 ++++++++++------- tests/test_fields.py | 39 +++++++++++++++++----------------- tests/test_model_serializer.py | 4 ++-- tests/test_validation.py | 4 ++-- 4 files changed, 36 insertions(+), 31 deletions(-) diff --git a/rest_framework/fields.py b/rest_framework/fields.py index f5f0b632e..dfcd0893f 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -130,7 +130,7 @@ def to_choices_dict(choices): """ Convert choices into key/value dicts. - to_choices_dict([1]) -> {1: 1} + to_choices_dict([1]) -> {1: '1'} to_choices_dict([(1, '1st'), (2, '2nd')]) -> {1: '1st', 2: '2nd'} to_choices_dict([('Group', ((1, '1st'), 2))]) -> {'Group': {1: '1st', 2: '2'}} """ @@ -142,7 +142,7 @@ def to_choices_dict(choices): for choice in choices: if not isinstance(choice, (list, tuple)): # single choice - ret[choice] = choice + ret[choice] = str(choice) else: key, value = choice if isinstance(value, (list, tuple)): @@ -150,7 +150,7 @@ def to_choices_dict(choices): ret[key] = to_choices_dict(value) else: # paired choice (key, display value) - ret[key] = value + ret[key] = str(value) return ret @@ -1421,7 +1421,7 @@ class ChoiceField(Field): def to_representation(self, value): if value in ('', None): return value - return self.choice_strings_to_values.get(str(value), value) + return self._choices.get(value, str(value)) def iter_options(self): """ @@ -1440,11 +1440,15 @@ class ChoiceField(Field): self.grouped_choices = to_choices_dict(choices) self._choices = flatten_choices_dict(self.grouped_choices) - # Map the string representation of choices to the underlying value. - # Allows us to deal with eg. integer choices while supporting either - # integer or string input, but still get the correct datatype out. + # `self._choices` is a dictionary that maps the data value of a + # particular choice to its display representation. Here, we want to + # reverse that dictionary so that when a user supplies a choice's + # display value as input, we can look up the underlying data value to + # which we should deserialize it. All of the values in the `OrderedDict` + # returned by `to_choices_dict` should be strings, so `display` should + # always be a string. self.choice_strings_to_values = { - str(key): key for key in self.choices + display_name: key for key, display_name in self.choices.items() } choices = property(_get_choices, _set_choices) diff --git a/tests/test_fields.py b/tests/test_fields.py index 7c495cd63..18687860f 100644 --- a/tests/test_fields.py +++ b/tests/test_fields.py @@ -953,12 +953,13 @@ class TestFilePathField(FieldValues): """ valid_inputs = { - __file__: __file__, + os.path.basename(__file__): os.path.abspath(__file__), } invalid_inputs = { 'wrong_path': ['"wrong_path" is not a valid path choice.'] } outputs = { + os.path.abspath(__file__): os.path.basename(__file__), } field = serializers.FilePathField( path=os.path.abspath(os.path.dirname(__file__)) @@ -1559,15 +1560,15 @@ class TestChoiceField(FieldValues): Valid and invalid values for `ChoiceField`. """ valid_inputs = { - 'poor': 'poor', - 'medium': 'medium', - 'good': 'good', + 'Poor quality': 'poor', + 'Medium quality': 'medium', + 'Good quality': 'good', } invalid_inputs = { 'amazing': ['"amazing" is not a valid choice.'] } outputs = { - 'good': 'good', + 'good': 'Good quality', '': '', 'amazing': 'amazing', } @@ -1658,16 +1659,16 @@ class TestChoiceFieldWithType(FieldValues): instead of a char type. """ valid_inputs = { - '1': 1, - 3: 3, + 'Poor quality': 1, + 'Good quality': 3, } invalid_inputs = { 5: ['"5" is not a valid choice.'], 'abc': ['"abc" is not a valid choice.'] } outputs = { - '1': 1, - 1: 1 + '1': '1', + 1: 'Poor quality', } field = serializers.ChoiceField( choices=[ @@ -1703,15 +1704,15 @@ class TestChoiceFieldWithGroupedChoices(FieldValues): choices, rather than a list of pairs of (`value`, `description`). """ valid_inputs = { - 'poor': 'poor', - 'medium': 'medium', - 'good': 'good', + 'Poor quality': 'poor', + 'Medium quality': 'medium', + 'Good quality': 'good', } invalid_inputs = { 'awful': ['"awful" is not a valid choice.'] } outputs = { - 'good': 'good' + 'good': 'Good quality' } field = serializers.ChoiceField( choices=[ @@ -1733,15 +1734,15 @@ class TestChoiceFieldWithMixedChoices(FieldValues): grouped. """ valid_inputs = { - 'poor': 'poor', + 'Poor quality': 'poor', 'medium': 'medium', - 'good': 'good', + 'Good quality': 'good', } invalid_inputs = { 'awful': ['"awful" is not a valid choice.'] } outputs = { - 'good': 'good' + 'good': 'Good quality' } field = serializers.ChoiceField( choices=[ @@ -1763,12 +1764,12 @@ class TestMultipleChoiceField(FieldValues): """ valid_inputs = { (): set(), - ('aircon',): {'aircon'}, - ('aircon', 'manual'): {'aircon', 'manual'}, + ('AirCon',): {'aircon'}, + ('AirCon', 'Manual drive'): {'aircon', 'manual'}, } invalid_inputs = { 'abc': ['Expected a list of items but got type "str".'], - ('aircon', 'incorrect'): ['"incorrect" is not a valid choice.'] + ('AirCon', 'incorrect'): ['"incorrect" is not a valid choice.'] } outputs = [ (['aircon', 'manual', 'incorrect'], {'aircon', 'manual', 'incorrect'}) diff --git a/tests/test_model_serializer.py b/tests/test_model_serializer.py index 21ec82347..040758c79 100644 --- a/tests/test_model_serializer.py +++ b/tests/test_model_serializer.py @@ -107,7 +107,7 @@ class Issue3674ChildModel(models.Model): class UniqueChoiceModel(models.Model): CHOICES = ( ('choice1', 'choice 1'), - ('choice2', 'choice 1'), + ('choice2', 'choice 2'), ) name = models.CharField(max_length=254, unique=True, choices=CHOICES) @@ -1196,7 +1196,7 @@ class Test5004UniqueChoiceField(TestCase): fields = '__all__' UniqueChoiceModel.objects.create(name='choice1') - serializer = TestUniqueChoiceSerializer(data={'name': 'choice1'}) + serializer = TestUniqueChoiceSerializer(data={'name': 'choice 1'}) assert not serializer.is_valid() assert serializer.errors == {'name': ['unique choice model with this name already exists.']} diff --git a/tests/test_validation.py b/tests/test_validation.py index 6e00b48c2..b5ff873a0 100644 --- a/tests/test_validation.py +++ b/tests/test_validation.py @@ -196,7 +196,7 @@ class TestChoiceFieldChoicesValidate(TestCase): Make sure a value for choices works as expected. """ f = serializers.ChoiceField(choices=self.CHOICES) - value = self.CHOICES[0][0] + value = self.CHOICES[0][1] try: f.to_internal_value(value) except serializers.ValidationError: @@ -218,7 +218,7 @@ class TestChoiceFieldChoicesValidate(TestCase): Make sure a nested value for choices works as expected. """ f = serializers.ChoiceField(choices=self.CHOICES_NESTED) - value = self.CHOICES_NESTED[0][1][0][0] + value = self.CHOICES_NESTED[0][1][0][1] try: f.to_internal_value(value) except serializers.ValidationError: