Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stricter type validation for CharField. #4380

Merged
merged 3 commits into from
Aug 10, 2016

Conversation

tomchristie
Copy link
Member

CharField now only accepts string and numeric types, and will raise a validation error on other inputs.

Closes #3394.

@tomchristie tomchristie added this to the 3.4.4 Release milestone Aug 10, 2016
@tomchristie tomchristie changed the title Stricter CharField type validation. Stricter type validation for CharField. Aug 10, 2016
@tomchristie tomchristie merged commit f16e880 into master Aug 10, 2016
@tomchristie tomchristie deleted the stricter-charfield-type-validation branch August 10, 2016 16:22
@michael-k
Copy link
Contributor

Breaking change + minor version update 💥
But now we know about the issue in our code, .i.e. “eg. unclear if booleans should represent as true or True”.

@tomchristie
Copy link
Member Author

tomchristie commented Aug 15, 2016

Noted.

I'd treated it as not a breaking change, given that it'd only be exposed in already broken use-cases. (eg clients posting booleans to a CharField)

Still, it's evidently worth us being more conservative in the future, given the large installed base, now. Thanks for the data-point.

@sergiomb2
Copy link

How I override ? :

    if isinstance(data, bool) or not isinstance(data, six.string_types + six.integer_types + (float,)):

with

    if isinstance(data, bool) or not isinstance(data, six.string_types):

I don't want integers or floats be consider as a valid string .

Thanks

@michael-k
Copy link
Contributor

michael-k commented Jun 13, 2017

@sergiomb2 Derive a new class from CharField. Override to_internal_value by either copying the implementation and changing it, or by first performing your stricter check and then calling the parent's method.

Possibility 1:

class MyCharField(CharField):
    def to_internal_value(self, data):
        if isinstance(data, bool) or not isinstance(data, six.string_types):
            self.fail('invalid')
        value = six.text_type(data)
        return value.strip() if self.trim_whitespace else value

Possibility 2:

class MyCharField(CharField):
    def to_internal_value(self, data):
        if isinstance(data, bool) or not isinstance(data, six.string_types):
            self.fail('invalid')
        return super().to_internal_value(data)  # python 3 syntax

Do not forget to use your field in your serializers.

If you're using the ModelSerializer, you can derive from it and adjust serializer_field_mapping.

@sergiomb2
Copy link

sergiomb2 commented Jun 13, 2017

many thanks, yes I already use Possibility 1 , but Possibility 2 looks to me even better .
yes I already added this code (and to complete the example) :

from rest_framework import serializers 
from rest_framework.serializers import CharField

class CustomSerializer(serializers.ModelSerializer):
    user_name = MyCharField(max_length=45)

If you're using the ModelSerializer, you can derive from it and adjust serializer_field_mapping.

Could you exemplify what you mean ? I think will much more easy to me understand, yes I'd like override all CharFields in the project with MyCharField options , do I need reference all CharField in Serializers class ?

Many thanks for the feedback

@michael-k
Copy link
Contributor

class MyModelSerializer(ModelSerializer):
    serializer_field_mapping = {
        models.AutoField: IntegerField,
        models.BigIntegerField: IntegerField,
        models.BooleanField: BooleanField,
        models.CharField: MyCharField,  # ← see here
        models.CommaSeparatedIntegerField: MyCharField,  # ← see here
        models.DateField: DateField,
        models.DateTimeField: DateTimeField,
        models.DecimalField: DecimalField,
        models.EmailField: EmailField,
        models.Field: ModelField,
        models.FileField: FileField,
        models.FloatField: FloatField,
        models.ImageField: ImageField,
        models.IntegerField: IntegerField,
        models.NullBooleanField: NullBooleanField,
        models.PositiveIntegerField: IntegerField,
        models.PositiveSmallIntegerField: IntegerField,
        models.SlugField: SlugField,
        models.SmallIntegerField: IntegerField,
        models.TextField: MyCharField,  # ← see here
        models.TimeField: TimeField,
        models.URLField: URLField,
        models.GenericIPAddressField: IPAddressField,
        models.FilePathField: FilePathField,
    }

Then use MyModelSerializer instead.

@sergiomb2
Copy link

Thanks , exemplifying a more complete solution :

class CharField_stricter(serializers.CharField):
    def to_internal_value(self, data):
        if isinstance(data, bool) or not isinstance(data, six.string_types):
            self.fail('invalid')
        value = six.text_type(data)
        return value.strip() if self.trim_whitespace else value

class ModelSerializer_stricter(serializers.ModelSerializer):
    serializer_field_mapping = serializers.ModelSerializer.serializer_field_mapping.copy()
    serializer_field_mapping.update({
         models.CharField: CharField_stricter
    })

class FooSerializer(ModelSerializer_stricter):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants