From fcc8f85664078b8e22dd9f0305203985722e490b Mon Sep 17 00:00:00 2001 From: Vignesh Date: Mon, 11 Sep 2017 20:00:48 +0800 Subject: [PATCH 1/5] first version of fix only for min and max value --- rest_framework/fields.py | 10 +++---- rest_framework/utils/field_mapping.py | 14 +++++---- tests/test_model_serializer.py | 2 +- tests/test_validators.py | 43 ++++++++++++++++++++++++--- 4 files changed, 52 insertions(+), 17 deletions(-) diff --git a/rest_framework/fields.py b/rest_framework/fields.py index 072bbf1b9..800741be5 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -967,8 +967,8 @@ class FloatField(Field): class DecimalField(Field): default_error_messages = { 'invalid': _('A valid number is required.'), - 'max_value': _('Ensure this value is less than or equal to {max_value}.'), - 'min_value': _('Ensure this value is greater than or equal to {min_value}.'), + 'max_value': _('Ensure this value is less than or equal to %(limit_value)s.'), + 'min_value': _('Ensure this value is greater than or equal to %(limit_value)s.'), 'max_digits': _('Ensure that there are no more than {max_digits} digits in total.'), 'max_decimal_places': _('Ensure that there are no more than {max_decimal_places} decimal places.'), 'max_whole_digits': _('Ensure that there are no more than {max_whole_digits} digits before the decimal point.'), @@ -997,11 +997,9 @@ class DecimalField(Field): super(DecimalField, self).__init__(**kwargs) if self.max_value is not None: - message = self.error_messages['max_value'].format(max_value=self.max_value) - self.validators.append(MaxValueValidator(self.max_value, message=message)) + self.validators.append(MaxValueValidator(self.max_value, message=self.error_messages['max_value'])) if self.min_value is not None: - message = self.error_messages['min_value'].format(min_value=self.min_value) - self.validators.append(MinValueValidator(self.min_value, message=message)) + self.validators.append(MinValueValidator(self.min_value, message=self.error_messages['min_value'])) def to_internal_value(self, data): """ diff --git a/rest_framework/utils/field_mapping.py b/rest_framework/utils/field_mapping.py index dff33d8b3..9d4c5761e 100644 --- a/rest_framework/utils/field_mapping.py +++ b/rest_framework/utils/field_mapping.py @@ -127,12 +127,13 @@ def get_field_kwargs(field_name, model_field): else: # Ensure that max_value is passed explicitly as a keyword arg, # rather than as a validator. - max_value = next(( - validator.limit_value for validator in validator_kwarg + max_value, messsage = next(( + (validator.limit_value, validator.message) for validator in validator_kwarg if isinstance(validator, validators.MaxValueValidator) - ), None) + ), (None, None)) if max_value is not None and isinstance(model_field, NUMERIC_FIELD_TYPES): kwargs['max_value'] = max_value + kwargs['error_messages'] = {'max_value': messsage} validator_kwarg = [ validator for validator in validator_kwarg if not isinstance(validator, validators.MaxValueValidator) @@ -140,12 +141,13 @@ def get_field_kwargs(field_name, model_field): # Ensure that min_value is passed explicitly as a keyword arg, # rather than as a validator. - min_value = next(( - validator.limit_value for validator in validator_kwarg + min_value, messsage = next(( + (validator.limit_value, validator.message) for validator in validator_kwarg if isinstance(validator, validators.MinValueValidator) - ), None) + ), (None, None)) if min_value is not None and isinstance(model_field, NUMERIC_FIELD_TYPES): kwargs['min_value'] = min_value + kwargs['error_messages'] = {**kwargs['error_messages'], **{'min_value': messsage}} validator_kwarg = [ validator for validator in validator_kwarg if not isinstance(validator, validators.MinValueValidator) diff --git a/tests/test_model_serializer.py b/tests/test_model_serializer.py index 3411c44b5..975a9f67f 100644 --- a/tests/test_model_serializer.py +++ b/tests/test_model_serializer.py @@ -200,7 +200,7 @@ class TestRegularFieldMappings(TestCase): expected = dedent(""" TestSerializer(): id = IntegerField(label='ID', read_only=True) - value_limit_field = IntegerField(max_value=10, min_value=1) + value_limit_field = IntegerField(error_messages={'max_value': 'Ensure this value is less than or equal to %(limit_value)s.', 'min_value': 'Ensure this value is greater than or equal to %(limit_value)s.'}, max_value=10, min_value=1) length_limit_field = CharField(max_length=12, min_length=3) blank_field = CharField(allow_blank=True, max_length=10, required=False) null_field = IntegerField(allow_null=True, required=False) diff --git a/tests/test_validators.py b/tests/test_validators.py index 62126ddb3..77a7fc6d9 100644 --- a/tests/test_validators.py +++ b/tests/test_validators.py @@ -1,6 +1,7 @@ import datetime import pytest +from django.core.validators import MinValueValidator, MaxValueValidator from django.db import DataError, models from django.test import TestCase @@ -36,7 +37,7 @@ class RelatedModel(models.Model): class RelatedModelSerializer(serializers.ModelSerializer): username = serializers.CharField(source='user.username', - validators=[UniqueValidator(queryset=UniquenessModel.objects.all(), lookup='iexact')]) # NOQA + validators=[UniqueValidator(queryset=UniquenessModel.objects.all(), lookup='iexact')]) # NOQA class Meta: model = RelatedModel @@ -245,10 +246,12 @@ class TestUniquenessTogetherValidation(TestCase): When model fields are not included in a serializer, then uniqueness validators should not be added for that field. """ + class ExcludedFieldSerializer(serializers.ModelSerializer): class Meta: model = UniquenessTogetherModel fields = ('id', 'race_name',) + serializer = ExcludedFieldSerializer() expected = dedent(""" ExcludedFieldSerializer(): @@ -262,6 +265,7 @@ class TestUniquenessTogetherValidation(TestCase): When serializer fields are read only, then uniqueness validators should not be added for that field. """ + class ReadOnlyFieldSerializer(serializers.ModelSerializer): class Meta: model = UniquenessTogetherModel @@ -281,6 +285,7 @@ class TestUniquenessTogetherValidation(TestCase): """ Ensure validators can be explicitly removed.. """ + class NoValidatorsSerializer(serializers.ModelSerializer): class Meta: model = UniquenessTogetherModel @@ -329,6 +334,7 @@ class TestUniquenessTogetherValidation(TestCase): filter_queryset should add value from existing instance attribute if it is not provided in attributes dict """ + class MockQueryset(object): def filter(self, **kwargs): self.called_with = kwargs @@ -411,6 +417,7 @@ class TestUniquenessForDateValidation(TestCase): 'published': datetime.date(2000, 1, 1) } + # Tests for `UniqueForMonthValidator` # ---------------------------------- @@ -427,7 +434,6 @@ class UniqueForMonthSerializer(serializers.ModelSerializer): class UniqueForMonthTests(TestCase): - def setUp(self): self.instance = UniqueForMonthModel.objects.create( slug='existing', published='2017-01-01' @@ -450,6 +456,7 @@ class UniqueForMonthTests(TestCase): 'published': datetime.date(2017, 2, 1) } + # Tests for `UniqueForYearValidator` # ---------------------------------- @@ -466,7 +473,6 @@ class UniqueForYearSerializer(serializers.ModelSerializer): class UniqueForYearTests(TestCase): - def setUp(self): self.instance = UniqueForYearModel.objects.create( slug='existing', published='2017-01-01' @@ -532,23 +538,25 @@ class TestHiddenFieldUniquenessForDateValidation(TestCase): class ValidatorsTests(TestCase): - def test_qs_exists_handles_type_error(self): class TypeErrorQueryset(object): def exists(self): raise TypeError + assert qs_exists(TypeErrorQueryset()) is False def test_qs_exists_handles_value_error(self): class ValueErrorQueryset(object): def exists(self): raise ValueError + assert qs_exists(ValueErrorQueryset()) is False def test_qs_exists_handles_data_error(self): class DataErrorQueryset(object): def exists(self): raise DataError + assert qs_exists(DataErrorQueryset()) is False def test_validator_raises_error_if_not_all_fields_are_provided(self): @@ -563,3 +571,30 @@ class ValidatorsTests(TestCase): date_field='bar') with pytest.raises(NotImplementedError): validator.filter_queryset(attrs=None, queryset=None) + + +class ItemModel(models.Model): + price = models.DecimalField(decimal_places=2, max_digits=10, validators=[MinValueValidator(limit_value=0, message='Price has to be >= 0.'), + MaxValueValidator(limit_value=10, message='Price has to be <= 10.')]) + + +class ItemSerializer(serializers.ModelSerializer): + class Meta: + model = ItemModel + fields = '__all__' + + +class ValidatorMessageTests(TestCase): + def test_min_value_validator_message_is_copied_from_model(self): + data = {'price': -1} + s = ItemSerializer(data=data, partial=True) + s.is_valid() + + assert s.errors['price'] == ['Price has to be >= 0.'] + + def test_max_value_validator_message_is_copied_from_model(self): + data = {'price': 11} + s = ItemSerializer(data=data, partial=True) + s.is_valid() + + assert s.errors['price'] == ['Price has to be <= 10.'] From f9125c6bb71d11223f5bbcba535c8e4666cc36d6 Mon Sep 17 00:00:00 2001 From: Vignesh Date: Sat, 23 Sep 2017 02:15:38 +0800 Subject: [PATCH 2/5] make changes based on review --- rest_framework/utils/field_mapping.py | 8 ++++---- tests/test_model_serializer.py | 3 +++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/rest_framework/utils/field_mapping.py b/rest_framework/utils/field_mapping.py index 9d4c5761e..33d07bace 100644 --- a/rest_framework/utils/field_mapping.py +++ b/rest_framework/utils/field_mapping.py @@ -127,13 +127,13 @@ def get_field_kwargs(field_name, model_field): else: # Ensure that max_value is passed explicitly as a keyword arg, # rather than as a validator. - max_value, messsage = next(( + max_value, message = next(( (validator.limit_value, validator.message) for validator in validator_kwarg if isinstance(validator, validators.MaxValueValidator) ), (None, None)) if max_value is not None and isinstance(model_field, NUMERIC_FIELD_TYPES): kwargs['max_value'] = max_value - kwargs['error_messages'] = {'max_value': messsage} + kwargs['error_messages'] = {'max_value': message} validator_kwarg = [ validator for validator in validator_kwarg if not isinstance(validator, validators.MaxValueValidator) @@ -141,13 +141,13 @@ def get_field_kwargs(field_name, model_field): # Ensure that min_value is passed explicitly as a keyword arg, # rather than as a validator. - min_value, messsage = next(( + min_value, message = next(( (validator.limit_value, validator.message) for validator in validator_kwarg if isinstance(validator, validators.MinValueValidator) ), (None, None)) if min_value is not None and isinstance(model_field, NUMERIC_FIELD_TYPES): kwargs['min_value'] = min_value - kwargs['error_messages'] = {**kwargs['error_messages'], **{'min_value': messsage}} + kwargs.setdefault('error_messages', {}).update(min_value=message) validator_kwarg = [ validator for validator in validator_kwarg if not isinstance(validator, validators.MinValueValidator) diff --git a/tests/test_model_serializer.py b/tests/test_model_serializer.py index 975a9f67f..e5e0fa6f9 100644 --- a/tests/test_model_serializer.py +++ b/tests/test_model_serializer.py @@ -214,6 +214,9 @@ class TestRegularFieldMappings(TestCase): expected = expected.replace( "('red', 'Red'), ('blue', 'Blue'), ('green', 'Green')", "(u'red', u'Red'), (u'blue', u'Blue'), (u'green', u'Green')" + ).replace( + "{'max_value': 'Ensure this value is less than or equal to %(limit_value)s.', 'min_value': 'Ensure this value is greater than or equal to %(limit_value)s.'}", + {'max_value': u'Ensure this value is less than or equal to %(limit_value)s.', 'min_value': u'Ensure this value is greater than or equal to %(limit_value)s.'} ) self.assertEqual(unicode_repr(TestSerializer()), expected) From 633480957a341eb957e9e3db0cc3749b624bc2d3 Mon Sep 17 00:00:00 2001 From: Vignesh Date: Sat, 23 Sep 2017 02:20:07 +0800 Subject: [PATCH 3/5] remove unnecessary code formatting changes --- tests/test_validators.py | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/tests/test_validators.py b/tests/test_validators.py index 77a7fc6d9..42f5635e5 100644 --- a/tests/test_validators.py +++ b/tests/test_validators.py @@ -37,7 +37,7 @@ class RelatedModel(models.Model): class RelatedModelSerializer(serializers.ModelSerializer): username = serializers.CharField(source='user.username', - validators=[UniqueValidator(queryset=UniquenessModel.objects.all(), lookup='iexact')]) # NOQA + validators=[UniqueValidator(queryset=UniquenessModel.objects.all(), lookup='iexact')]) # NOQA class Meta: model = RelatedModel @@ -246,12 +246,10 @@ class TestUniquenessTogetherValidation(TestCase): When model fields are not included in a serializer, then uniqueness validators should not be added for that field. """ - class ExcludedFieldSerializer(serializers.ModelSerializer): class Meta: model = UniquenessTogetherModel fields = ('id', 'race_name',) - serializer = ExcludedFieldSerializer() expected = dedent(""" ExcludedFieldSerializer(): @@ -265,7 +263,6 @@ class TestUniquenessTogetherValidation(TestCase): When serializer fields are read only, then uniqueness validators should not be added for that field. """ - class ReadOnlyFieldSerializer(serializers.ModelSerializer): class Meta: model = UniquenessTogetherModel @@ -285,7 +282,6 @@ class TestUniquenessTogetherValidation(TestCase): """ Ensure validators can be explicitly removed.. """ - class NoValidatorsSerializer(serializers.ModelSerializer): class Meta: model = UniquenessTogetherModel @@ -334,7 +330,6 @@ class TestUniquenessTogetherValidation(TestCase): filter_queryset should add value from existing instance attribute if it is not provided in attributes dict """ - class MockQueryset(object): def filter(self, **kwargs): self.called_with = kwargs @@ -417,7 +412,6 @@ class TestUniquenessForDateValidation(TestCase): 'published': datetime.date(2000, 1, 1) } - # Tests for `UniqueForMonthValidator` # ---------------------------------- @@ -434,6 +428,7 @@ class UniqueForMonthSerializer(serializers.ModelSerializer): class UniqueForMonthTests(TestCase): + def setUp(self): self.instance = UniqueForMonthModel.objects.create( slug='existing', published='2017-01-01' @@ -456,7 +451,6 @@ class UniqueForMonthTests(TestCase): 'published': datetime.date(2017, 2, 1) } - # Tests for `UniqueForYearValidator` # ---------------------------------- @@ -473,6 +467,7 @@ class UniqueForYearSerializer(serializers.ModelSerializer): class UniqueForYearTests(TestCase): + def setUp(self): self.instance = UniqueForYearModel.objects.create( slug='existing', published='2017-01-01' @@ -538,25 +533,23 @@ class TestHiddenFieldUniquenessForDateValidation(TestCase): class ValidatorsTests(TestCase): + def test_qs_exists_handles_type_error(self): class TypeErrorQueryset(object): def exists(self): raise TypeError - assert qs_exists(TypeErrorQueryset()) is False def test_qs_exists_handles_value_error(self): class ValueErrorQueryset(object): def exists(self): raise ValueError - assert qs_exists(ValueErrorQueryset()) is False def test_qs_exists_handles_data_error(self): class DataErrorQueryset(object): def exists(self): raise DataError - assert qs_exists(DataErrorQueryset()) is False def test_validator_raises_error_if_not_all_fields_are_provided(self): From f6e0c5eeb81540444716b1263a1f533663bd3d7b Mon Sep 17 00:00:00 2001 From: Vignesh Date: Sat, 23 Sep 2017 02:37:14 +0800 Subject: [PATCH 4/5] fix broken test --- tests/test_model_serializer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_model_serializer.py b/tests/test_model_serializer.py index e5e0fa6f9..906510f73 100644 --- a/tests/test_model_serializer.py +++ b/tests/test_model_serializer.py @@ -216,7 +216,7 @@ class TestRegularFieldMappings(TestCase): "(u'red', u'Red'), (u'blue', u'Blue'), (u'green', u'Green')" ).replace( "{'max_value': 'Ensure this value is less than or equal to %(limit_value)s.', 'min_value': 'Ensure this value is greater than or equal to %(limit_value)s.'}", - {'max_value': u'Ensure this value is less than or equal to %(limit_value)s.', 'min_value': u'Ensure this value is greater than or equal to %(limit_value)s.'} + "{'max_value': u'Ensure this value is less than or equal to %(limit_value)s.', 'min_value': u'Ensure this value is greater than or equal to %(limit_value)s.'}" ) self.assertEqual(unicode_repr(TestSerializer()), expected) From 667b78d5d265586b7440775dc2ec5603274889e3 Mon Sep 17 00:00:00 2001 From: Vignesh Date: Sat, 23 Sep 2017 02:35:32 +0800 Subject: [PATCH 5/5] WIP Add failing tests for more fields --- tests/test_validators.py | 62 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/tests/test_validators.py b/tests/test_validators.py index 42f5635e5..f0bfdaab1 100644 --- a/tests/test_validators.py +++ b/tests/test_validators.py @@ -1,7 +1,7 @@ import datetime import pytest -from django.core.validators import MinValueValidator, MaxValueValidator +from django.core.validators import MinValueValidator, MaxValueValidator, URLValidator, EmailValidator, MinLengthValidator, MaxLengthValidator from django.db import DataError, models from django.test import TestCase @@ -591,3 +591,63 @@ class ValidatorMessageTests(TestCase): s.is_valid() assert s.errors['price'] == ['Price has to be <= 10.'] + + def test_url_validator_message_is_copied_from_model(self): + class BlogModel(models.Model): + url = models.URLField(validators=[URLValidator(message='This URL is not valid.')]) + + class BlogSerializer(serializers.ModelSerializer): + class Meta: + model = BlogModel + fields = '__all__' + + data = {'url': 'broken url'} + s = BlogSerializer(data=data) + s.is_valid() + + assert s.errors['url'] == ['This URL is not valid.'] + + def test_email_validator_message_is_copied_from_model(self): + class UserModel(models.Model): + email = models.EmailField(validators=[EmailValidator(message='Please enter a valid email.')]) + + class UserSerializer(serializers.ModelSerializer): + class Meta: + model = UserModel + fields = '__all__' + + data = {'email': 'invalid email'} + s = UserSerializer(data=data) + s.is_valid() + + assert s.errors['email'] == ['Please enter a valid email.'] + + def test_min_length_validator_message_is_copied_from_model(self): + class Review(models.Model): + text = models.CharField(max_length=100, validators=[MinLengthValidator(limit_value=5, message='This is too short.')]) + + class ReviewSerializer(serializers.ModelSerializer): + class Meta: + model = Review + fields = '__all__' + + data = {'text': 'Hi'} + s = ReviewSerializer(data=data) + s.is_valid() + + assert s.errors['text'] == ['This is too short.'] + + def test_max_length_validator_message_is_copied_from_model(self): + # Added this test because was expecting is_valid() to be false but it is not. + # Will investigate further + class Post(models.Model): + text = models.CharField(max_length=100, validators=[MaxLengthValidator(limit_value=1, message='This is too long.')]) + + class PostSerializer(serializers.ModelSerializer): + class Meta: + model = Post + fields = '__all__' + + data = {'text': 'A very long text'} + s = PostSerializer(data=data) + assert not s.is_valid()