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

UUIDField validation does not catch AttributeError #3679

Closed
nplatias opened this issue Nov 27, 2015 · 17 comments
Closed

UUIDField validation does not catch AttributeError #3679

nplatias opened this issue Nov 27, 2015 · 17 comments
Labels
Milestone

Comments

@nplatias
Copy link

nplatias commented Nov 27, 2015

I am using a UUIDField in a serializer as such:

    class ExampleSerializer(serializers.Serializer):
        some_id = serializers.UUIDField(required = False)


    class ExampleView(APIView):

        def post(self, request):
            example_serializer = ExampleSerializer(data = request.data)
            example_serializer.is_valid()

If I make a request to this view with the parameter some_id mapped to a list, e.g. {"some_id": [1, 2, 3]}, I get an AttributeError: list object has no attribute replace, even though the expected behavior is that the exception is caught by is_valid and its return value is False (since a list is clearly not valid input to a UUIDField)

The AttributeError is thrown by Python's uuid.py (I am on Python 3.4.3) at the following line of UUID's __init__ method:

    if hex is not None:
        hex = hex.replace('urn:', '').replace('uuid:', '')

I believe the issue lies in the to_internal_value method of UUIDField when it calls the UUID constructor to construct the field from the input data (code from https://github.com/tomchristie/django-rest-framework/blob/a8deb380ff70b5df8d3c62c9be981b60e363c7f9/rest_framework/fields.py#L767):

def to_internal_value(self, data):
    if not isinstance(data, uuid.UUID):
        try:
            if isinstance(data, six.integer_types):
                return uuid.UUID(int=data)
            else:
                return uuid.UUID(hex=data)
        except (ValueError, TypeError):
            self.fail('invalid', value=data)
    return data

Shouldn't AttributeError also be caught here before calling self.fail (to register validation failure)?
I an on djangorestframework version 3.3.1

Thanks for taking a look!

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 27, 2015

Looks to me like we should ensure data is a string when calling UUID(hex=data).

@tomchristie
Copy link
Member

tomchristie commented Nov 27, 2015

Yes. Binary string prob also okay.

@tomchristie
Copy link
Member

tomchristie commented Nov 27, 2015

And then only catch ValueError

@tomchristie
Copy link
Member

tomchristie commented Nov 27, 2015

Alt: push upstream and ensure Django raises a TypeError in this case (it probably should, really)

@tomchristie
Copy link
Member

tomchristie commented Nov 27, 2015

We may want to resolve both in REST framework, and upstream in Django.

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 27, 2015

Yes. Binary string prob also okay.

No, it's not. There's a bytes argument for binary.

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 27, 2015

@tomchristie
Copy link
Member

tomchristie commented Nov 27, 2015

Got it!

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 27, 2015

Alt: push upstream and ensure Django raises a TypeError in this case (it probably should, really)

Correct me but uuid doesn't seem to be using Django's field for that one right ?

@tomchristie
Copy link
Member

tomchristie commented Nov 27, 2015

Right yup, ignore my upstream comments! :p

@sloria
Copy link
Contributor

sloria commented May 19, 2017

@xordoquy @tomchristie Sorry for commenting on an old issue, but I'm curious: why aren't bytes values deserialized with the bytes argument to UUID? There is a PR on marshmallow doing just that, and I'd like to know if there are side effects I'm not seeing before I merge that. Am I missing something?

@xordoquy
Copy link
Collaborator

xordoquy commented May 19, 2017

@sloria I can't think of a case where we're get bytes as input to serializer. This issue was about deserialization while yours seems to be about serialization.

@sloria
Copy link
Contributor

sloria commented May 19, 2017

The marshmallow PR affects both serialization and deserialization, since both call the same method which does the logic for handling different types:

        if isinstance(value, uuid.UUID):
            return value
        try:
            if isinstance(value, bytes) and len(value) == 16:
                return uuid.UUID(bytes=value)
            else:
                return uuid.UUID(value)
        except (ValueError, AttributeError):
            self.fail('invalid_uuid')

Is there a reason not to handle bytes as input?

@sloria
Copy link
Contributor

sloria commented Jul 16, 2017

@tomchristie @xordoquy Any thoughts on my comment above? Would it make sense to deserialize bytes strings by passing the bytes argument to UUID?

@xordoquy
Copy link
Collaborator

xordoquy commented Jul 16, 2017

@sloria I don't think the parser would provide bytes. This is mostly why I'm curious about a use case where that would be required.

@sloria
Copy link
Contributor

sloria commented Jul 16, 2017

The user who sent a PR to marshmallow said he was storing bytes in the database. Could this use case arise in Django as well?

@foresmac
Copy link

foresmac commented Oct 13, 2017

Yeah, I'm trying to sort out a custom MySQLUUIDField now for this very reason. For ease of implementation, Django uses CHAR(32) for UUIDs on everything that doesn't have a native UUID type (which AFAIK is only Postgres). However, all the documentation and samples I can find recommend using BINARY(16) on MySQL since it uses much less data and the index should be somewhat more optimized (though InnoDB prefers that PKs be mostly sequential, but I digress). As such, it makes sense to accept bytes I think.

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

No branches or pull requests

5 participants