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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lingxiaoyang
Copy link

...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.

…ed UUIDs, and uniformly returns hex representation
@ksonbol
Copy link
Contributor

ksonbol commented Dec 10, 2014

Sorry, I should have commented here instead of #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
Copy link
Author

@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.

@lingxiaoyang lingxiaoyang changed the title Fixed #46 and #49: get_db_prep_value() returns None for badly-formatted ... Fixed #46 and #49: get_db_prep_value() returns arbitrary UUID hex for badly-formatted ... Dec 10, 2014
@ksonbol
Copy link
Contributor

ksonbol commented Dec 10, 2014

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
Copy link
Author

@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
Copy link
Owner

dcramer commented Jan 19, 2015

I pulled in #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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants