From 466d4584ae64e73713a56144b6c2128e76e0f5b6 Mon Sep 17 00:00:00 2001 From: Mark Shirley Date: Thu, 3 Jan 2013 23:18:58 +0100 Subject: [PATCH 1/4] Remove duplicate release notes line --- docs/topics/release-notes.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/topics/release-notes.md b/docs/topics/release-notes.md index 3b13f86fb..c073c03be 100644 --- a/docs/topics/release-notes.md +++ b/docs/topics/release-notes.md @@ -22,7 +22,6 @@ Major version numbers (x.0.0) are reserved for project milestones. No major poi * Added `PATCH` support. * Added `RetrieveUpdateAPIView`. -* Relation changes are now persisted in `save` instead of in `.restore_object`. * Remove unused internal `save_m2m` flag on `ModelSerializer.save()`. * Tweak behavior of hyperlinked fields with an explicit format suffix. * Relation changes are now persisted in `.save()` instead of in `.restore_object()`. From 6e9865cb71ff45e90020d3d0dc7c40f20c760d2e Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Thu, 3 Jan 2013 23:17:31 +0000 Subject: [PATCH 2/4] Fix for #446. Note: Also needs applying to other relational types. --- rest_framework/relations.py | 21 +++++++++++++++++++-- rest_framework/tests/fields.py | 18 ++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 rest_framework/tests/fields.py diff --git a/rest_framework/relations.py b/rest_framework/relations.py index 686dcf04e..ae0d3de89 100644 --- a/rest_framework/relations.py +++ b/rest_framework/relations.py @@ -4,6 +4,7 @@ from django import forms from django.forms import widgets from django.forms.models import ModelChoiceIterator from django.utils.encoding import smart_unicode +from django.utils.translation import ugettext_lazy as _ from rest_framework.fields import Field, WritableField from rest_framework.reverse import reverse from urlparse import urlparse @@ -168,6 +169,11 @@ class PrimaryKeyRelatedField(RelatedField): default_read_only = False form_field_class = forms.ChoiceField + default_error_messages = { + 'does_not_exist': _("Invalid pk '%s' - object does not exist."), + 'invalid': _('Invalid value.'), + } + # TODO: Remove these field hacks... def prepare_value(self, obj): return self.to_native(obj.pk) @@ -193,7 +199,10 @@ class PrimaryKeyRelatedField(RelatedField): try: return self.queryset.get(pk=data) except ObjectDoesNotExist: - msg = "Invalid pk '%s' - object does not exist." % smart_unicode(data) + msg = self.error_messages['does_not_exist'] % smart_unicode(data) + raise ValidationError(msg) + except (TypeError, ValueError): + msg = self.error_messages['invalid'] raise ValidationError(msg) def field_to_native(self, obj, field_name): @@ -215,6 +224,11 @@ class ManyPrimaryKeyRelatedField(ManyRelatedField): default_read_only = False form_field_class = forms.MultipleChoiceField + default_error_messages = { + 'does_not_exist': _("Invalid pk '%s' - object does not exist."), + 'invalid': _('Invalid value.'), + } + def prepare_value(self, obj): return self.to_native(obj.pk) @@ -249,7 +263,10 @@ class ManyPrimaryKeyRelatedField(ManyRelatedField): try: return self.queryset.get(pk=data) except ObjectDoesNotExist: - msg = "Invalid pk '%s' - object does not exist." % smart_unicode(data) + msg = self.error_messages['does_not_exist'] % smart_unicode(data) + raise ValidationError(msg) + except (TypeError, ValueError): + msg = self.error_messages['invalid'] raise ValidationError(msg) ### Slug relationships diff --git a/rest_framework/tests/fields.py b/rest_framework/tests/fields.py new file mode 100644 index 000000000..147a9229b --- /dev/null +++ b/rest_framework/tests/fields.py @@ -0,0 +1,18 @@ +from django.db import models +from django.test import TestCase +from rest_framework import serializers + + +class NullModel(models.Model): + pass + + +class FieldTests(TestCase): + def test_pk_related_field_with_empty_string(self): + """ + Regression test for #446 + + https://github.com/tomchristie/django-rest-framework/issues/446 + """ + field = serializers.PrimaryKeyRelatedField(queryset=NullModel.objects.all()) + self.assertRaises(serializers.ValidationError, field.from_native, ('',)) From 4c86fd46d772e1fd3789d9ed2a76b9b92cce0872 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Fri, 4 Jan 2013 13:05:31 +0000 Subject: [PATCH 3/4] Rename module for basic relational field tests --- rest_framework/tests/{fields.py => relations.py} | 4 ++++ 1 file changed, 4 insertions(+) rename rest_framework/tests/{fields.py => relations.py} (91%) diff --git a/rest_framework/tests/fields.py b/rest_framework/tests/relations.py similarity index 91% rename from rest_framework/tests/fields.py rename to rest_framework/tests/relations.py index 147a9229b..108ec473e 100644 --- a/rest_framework/tests/fields.py +++ b/rest_framework/tests/relations.py @@ -1,3 +1,7 @@ +""" +General tests for relational fields. +""" + from django.db import models from django.test import TestCase from rest_framework import serializers From eb14278a3b08247c0aff5b2338a98203b51728c3 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Fri, 4 Jan 2013 13:50:40 +0000 Subject: [PATCH 4/4] Add proper validation for updating relational fields with incorrect types. Fixes #446. --- docs/topics/release-notes.md | 8 ++++-- rest_framework/relations.py | 48 ++++++++++++++++++++++++------- rest_framework/tests/relations.py | 13 ++++++++- 3 files changed, 55 insertions(+), 14 deletions(-) diff --git a/docs/topics/release-notes.md b/docs/topics/release-notes.md index c073c03be..40b65761e 100644 --- a/docs/topics/release-notes.md +++ b/docs/topics/release-notes.md @@ -16,6 +16,10 @@ Major version numbers (x.0.0) are reserved for project milestones. No major poi ## 2.1.x series +### Master + +* Bugfix: Validation errors instead of exceptions when related fields receive incorrect types. + ### 2.1.15 **Date**: 3rd Jan 2013 @@ -36,9 +40,9 @@ Major version numbers (x.0.0) are reserved for project milestones. No major poi * Bugfix: Model fields with `blank=True` are now `required=False` by default. * Bugfix: Nested serializers now support nullable relationships. -**Note**: From 2.1.14 onwards, relational fields move out of the `fields.py` module and into the new `relations.py` module, in order to seperate them from regular data type fields, such as `CharField` and `IntegerField`. +**Note**: From 2.1.14 onwards, relational fields move out of the `fields.py` module and into the new `relations.py` module, in order to separate them from regular data type fields, such as `CharField` and `IntegerField`. -This change will not affect user code, so long as it's following the recommended import style of `from rest_framework import serializers` and refering to fields using the style `serializers.PrimaryKeyRelatedField`. +This change will not affect user code, so long as it's following the recommended import style of `from rest_framework import serializers` and referring to fields using the style `serializers.PrimaryKeyRelatedField`. ### 2.1.13 diff --git a/rest_framework/relations.py b/rest_framework/relations.py index ae0d3de89..fcef42dd7 100644 --- a/rest_framework/relations.py +++ b/rest_framework/relations.py @@ -276,6 +276,11 @@ class SlugRelatedField(RelatedField): default_read_only = False form_field_class = forms.ChoiceField + default_error_messages = { + 'does_not_exist': _("Object with %s=%s does not exist."), + 'invalid': _('Invalid value.'), + } + def __init__(self, *args, **kwargs): self.slug_field = kwargs.pop('slug_field', None) assert self.slug_field, 'slug_field is required' @@ -291,8 +296,11 @@ class SlugRelatedField(RelatedField): try: return self.queryset.get(**{self.slug_field: data}) except ObjectDoesNotExist: - raise ValidationError('Object with %s=%s does not exist.' % + raise ValidationError(self.error_messages['does_not_exist'] % (self.slug_field, unicode(data))) + except (TypeError, ValueError): + msg = self.error_messages['invalid'] + raise ValidationError(msg) class ManySlugRelatedField(ManyRelatedMixin, SlugRelatedField): @@ -311,6 +319,14 @@ class HyperlinkedRelatedField(RelatedField): default_read_only = False form_field_class = forms.ChoiceField + default_error_messages = { + 'no_match': _('Invalid hyperlink - No URL match'), + 'incorrect_match': _('Invalid hyperlink - Incorrect URL match'), + 'configuration_error': _('Invalid hyperlink due to configuration error'), + 'does_not_exist': _("Invalid hyperlink - object does not exist."), + 'invalid': _('Invalid value.'), + } + def __init__(self, *args, **kwargs): try: self.view_name = kwargs.pop('view_name') @@ -347,7 +363,7 @@ class HyperlinkedRelatedField(RelatedField): slug = getattr(obj, self.slug_field, None) if not slug: - raise ValidationError('Could not resolve URL for field using view name "%s"' % view_name) + raise Exception('Could not resolve URL for field using view name "%s"' % view_name) kwargs = {self.slug_url_kwarg: slug} try: @@ -361,7 +377,7 @@ class HyperlinkedRelatedField(RelatedField): except: pass - raise ValidationError('Could not resolve URL for field using view name "%s"' % view_name) + raise Exception('Could not resolve URL for field using view name "%s"' % view_name) def from_native(self, value): # Convert URL -> model instance pk @@ -369,7 +385,13 @@ class HyperlinkedRelatedField(RelatedField): if self.queryset is None: raise Exception('Writable related fields must include a `queryset` argument') - if value.startswith('http:') or value.startswith('https:'): + try: + http_prefix = value.startswith('http:') or value.startswith('https:') + except AttributeError: + msg = self.error_messages['invalid'] + raise ValidationError(msg) + + if http_prefix: # If needed convert absolute URLs to relative path value = urlparse(value).path prefix = get_script_prefix() @@ -379,10 +401,10 @@ class HyperlinkedRelatedField(RelatedField): try: match = resolve(value) except: - raise ValidationError('Invalid hyperlink - No URL match') + raise ValidationError(self.error_messages['no_match']) if match.url_name != self.view_name: - raise ValidationError('Invalid hyperlink - Incorrect URL match') + raise ValidationError(self.error_messages['incorrect_match']) pk = match.kwargs.get(self.pk_url_kwarg, None) slug = match.kwargs.get(self.slug_url_kwarg, None) @@ -394,14 +416,18 @@ class HyperlinkedRelatedField(RelatedField): elif slug is not None: slug_field = self.get_slug_field() queryset = self.queryset.filter(**{slug_field: slug}) - # If none of those are defined, it's an error. + # If none of those are defined, it's probably a configuation error. else: - raise ValidationError('Invalid hyperlink') + raise ValidationError(self.error_messages['configuration_error']) try: obj = queryset.get() except ObjectDoesNotExist: - raise ValidationError('Invalid hyperlink - object does not exist.') + raise ValidationError(self.error_messages['does_not_exist']) + except (TypeError, ValueError): + msg = self.error_messages['invalid'] + raise ValidationError(msg) + return obj @@ -460,7 +486,7 @@ class HyperlinkedIdentityField(Field): slug = getattr(obj, self.slug_field, None) if not slug: - raise ValidationError('Could not resolve URL for field using view name "%s"' % view_name) + raise Exception('Could not resolve URL for field using view name "%s"' % view_name) kwargs = {self.slug_url_kwarg: slug} try: @@ -474,4 +500,4 @@ class HyperlinkedIdentityField(Field): except: pass - raise ValidationError('Could not resolve URL for field using view name "%s"' % view_name) + raise Exception('Could not resolve URL for field using view name "%s"' % view_name) diff --git a/rest_framework/tests/relations.py b/rest_framework/tests/relations.py index 108ec473e..91daea8a6 100644 --- a/rest_framework/tests/relations.py +++ b/rest_framework/tests/relations.py @@ -19,4 +19,15 @@ class FieldTests(TestCase): https://github.com/tomchristie/django-rest-framework/issues/446 """ field = serializers.PrimaryKeyRelatedField(queryset=NullModel.objects.all()) - self.assertRaises(serializers.ValidationError, field.from_native, ('',)) + self.assertRaises(serializers.ValidationError, field.from_native, '') + self.assertRaises(serializers.ValidationError, field.from_native, []) + + def test_hyperlinked_related_field_with_empty_string(self): + field = serializers.HyperlinkedRelatedField(queryset=NullModel.objects.all(), view_name='') + self.assertRaises(serializers.ValidationError, field.from_native, '') + self.assertRaises(serializers.ValidationError, field.from_native, []) + + def test_slug_related_field_with_empty_string(self): + field = serializers.SlugRelatedField(queryset=NullModel.objects.all(), slug_field='pk') + self.assertRaises(serializers.ValidationError, field.from_native, '') + self.assertRaises(serializers.ValidationError, field.from_native, [])