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

Make UUIDs valid if input is string representation or 16 byte rep #625

Closed
wants to merge 3 commits into
base: dev
from

Conversation

Projects
None yet
2 participants
@JeffBerger
Contributor

JeffBerger commented May 11, 2017

My database contains UUIDs in a 16 byte representation and not the string representation. The python UUID library does not automatically detect the difference if passed as the first argument, byte representations must be passed into the bytes kwarg of the constructor.

This PR is to allow marshmallow to accept both the normal string representation and the byte representation.

In python 2.x bytes is the same type as string so for a valid string representation this will pass, so an additional check is made on the length being 12. For python 3.x a string representation will be a different type so the first statement will catch it.

TypeError is not needed to be caught
Because the if statement is and only string/byte objects will get to
the length statement, and since these objects have a length then
we don't have to worry about catching exceptions by calling len on
an object that does not have that implemented.
@sloria

This comment has been minimized.

Member

sloria commented May 19, 2017

Thanks @JeffBerger for this PR. I think this is the correct behavior. I looked into what djangorestframework does, and it looks like they only pass hex (https://github.com/encode/django-rest-framework/blob/1e9e1a5bfebaa7bb1ceb5dcb546b7bbd83c209ba/rest_framework/fields.py#L836-L837). I'm unsure if this was an oversight, but the discussion here suggests that the authors were at least aware of the bytes argument. I'd like to hear a response on my comment here before proceeding with this.

@JeffBerger

This comment has been minimized.

Contributor

JeffBerger commented May 19, 2017

Sounds great - thanks for getting back to me on this, I'll start watching the other thread as well.

@JeffBerger

This comment has been minimized.

Contributor

JeffBerger commented Dec 5, 2017

@sloria - I am just checking in about getting this merged. I looked at the reference and it seemed that it was given the ok in a comment in October on the django-rest-framework thread

@sloria

This comment has been minimized.

Member

sloria commented Dec 10, 2017

@JeffBerger The DRF thread has been silent, but I think we can move forward with this as I think it's the correct behavior. Would you be up for adding a test for this?

Also, don't forget to add yourself to AUTHORS.rst.

Thanks!

@JeffBerger

This comment has been minimized.

Contributor

JeffBerger commented Dec 12, 2017

Sure I'll make some tests, I'll see if I can get to it before this holiday break but if not I'll get back to you in the new year. Cheers!

@sloria

This comment has been minimized.

Member

sloria commented Mar 18, 2018

Just checking in-- @JeffBerger do you still plan to add tests to this? If not, just let us know so that someone else can pick it up.

@sloria

This comment has been minimized.

Member

sloria commented May 16, 2018

I picked this up in #816.

@sloria sloria closed this May 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment