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

CharField should not accept collections as valid input #3394

Closed
wants to merge 1 commit into from

Conversation

sloria
Copy link
Contributor

@sloria sloria commented Sep 12, 2015

This issue came up on the marshmallow issue tracker (marshmallow-code/marshmallow#231), so I thought I'd pass it along here.

It seems incorrect for CharField to take numbers and collections as valid input.

Code to reproduce

from django.conf import settings
settings.configure(INSTALLED_APPS=['rest_framework'])
import django
django.setup()
from rest_framework import serializers as ser
from rest_framework.exceptions import ValidationError

field = ser.CharField()

inputs = (
    42,
    42.0,
    {},
    []
)

for val in inputs:
    try:
        field.run_validation(val)
    except ValidationError as err:
        print(err.detail[0])

Expected

Validation fails with messages like "{} is not a valid string.".

Actual

Numbers and collections pass validation.

@sloria
Copy link
Contributor Author

sloria commented Sep 12, 2015

I'm not quite sure why the py3x-django18 builds are failing; I can look into it further if you decide this change is a good idea.

@tomchristie
Copy link
Member

I don't think I mind us coercing numbers to string representations. Collection types should prob fail tho.

@jpadilla
Copy link
Member

I second @tomchristie

@sloria
Copy link
Contributor Author

sloria commented Sep 12, 2015

What is the rationale for allowing numbers? Any time a client passes a number to a string field, it is most likely due to user or client error and should be treated as such. The client should be responsible for "stringifying" any numerical input.

@sloria
Copy link
Contributor Author

sloria commented Sep 12, 2015

^ Same goes for booleans passed to a string field.

@kevin-brown
Copy link
Member

I agree with triggering an error when a collection is passed in as a string.

What is the rationale for allowing numbers?

I'm -0 on not allowing numbers mostly because that's expected functionality at the moment and changing this would break backwards compatibility. We even had a test to ensure that numbers were coerced properly.

It's also useful to note that there may be renders out there which try their best to coerce string input to the right type, so numbers would become a numeric type (decimal, probably) instead of staying as strings. We allow for the reverse case (numbers passed in as strings to number-type fields), so I don't understand why we would want to disallow this.

@sloria
Copy link
Contributor Author

sloria commented Sep 13, 2015

We allow for the reverse case (numbers passed in as strings to number-type fields), so I don't understand why we would want to disallow this.

Indeed, most of simple fields accept strings as input. The fields' deserialization logic, however, is responsible for deciding which string inputs are valid (e.g. Boolean accepts "true" but not "glue"). That logic is also responsible for deciding which Python types are valid (e.g. Dict only accepts dict; Boolean doesn't accept dict).

I lean slightly toward disallowing numbers as inputs to String. That said, I don't feel that strongly about it, and it's probably not worth breaking backwards compat for this change. Let me know how to proceed.

@tomchristie
Copy link
Member

At least they should be treated as individually. The case for one pull request is unambiguous, the case for the other less so.

@rowanseymour
Copy link
Contributor

I'd like to see this be configurable at least. I would imagine passing anything other than a string to a CharField is usually indicative of user error.

@tomchristie
Copy link
Member

I would imagine passing anything other than a string to a CharField is usually indicative of user error.

Possibly, tho "Generous on input, strict on output" could also be a valid rule of thumb for us to use here.

@tomchristie tomchristie added this to the 3.4.4 Release milestone Aug 10, 2016
@tomchristie tomchristie changed the title CharField should not accept numbers and collections as valid input CharField should not accept collections as valid input Aug 10, 2016
@tomchristie
Copy link
Member

Retitling to reflect the resolution.

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.

5 participants