dcramer / django-uuidfield

A UUIDField for Django
BSD 3-Clause "New" or "Revised" License
262 stars 115 forks source link

Fixed #46 and #49: get_db_prep_value() returns arbitrary UUID hex for badly-formatted ... #50

Open lingxiaoyang opened 9 years ago

lingxiaoyang commented 9 years ago

...UUIDs, and uniformly returns hex representation

Throwing ValueError or TypeError as discussed in #46 is not consistent with how Django ORM interface, which raises ObjectDoesNotExist for get and evaluates to empty list for filter. It suggests, method get_db_prep_value() should return a non-existing value, where an arbitrary value is the best candidate.

ksonbol commented 9 years ago

Sorry, I should have commented here instead of https://github.com/dcramer/django-uuidfield/issues/49. I don't understand how raising a ValueError is not consistent with the Django ORM interface. Could you explain? And how is the fact that the UUID field is an auto field related to this?

lingxiaoyang commented 9 years ago

@ksonbol get_db_prep_value() itself is not expected to raise an exception. This method links Python interface of object.get(), object.filter() and object.save() to underlying database value, not user provided query/update parameter to underlying database value, which is handled by Django form or Django REST framework serializers. Querying SomeModel.objects.filter(boolean_field=255), for example, will be treated as boolean_field=True instead of throwing an error.

I was wrong about the auto field... the test test_raises_exception expects an IntegrityError because there is no null=True set on ManualUUIDField. null=True should be supported and get_db_prep_value should not return a null value when the UUID is invalid.

ksonbol commented 9 years ago

Calling SomeModel.objects.get(id="string") will cause get_prep_value to raise a ValueError. So I think it's normal to raise a ValueError there, and maybe I should move the code to get_prep_value since it is not directly related to a specific database.

lingxiaoyang commented 9 years ago

@ksonbol Yes it does. Django's way of handling invalid value is to write URL regex that excludes all non-integers from getting into the ORM interface by returning 404. URL exclusion may be too complex for the case of UUID, but it can work.

Actually disputation on designing interfaces is the most tricky issue as it is mostly a matter of coding taste ;)

dcramer commented 9 years ago

I pulled in https://github.com/dcramer/django-uuidfield/pull/48 as it was more concise in resolving one of these issues. If you want to rebase to cover the other explicit issue I'll take a look.