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

Better catching of error cases in .update and .create. #2202

Closed
brianhv opened this issue Dec 4, 2014 · 12 comments · Fixed by #2215
Closed

Better catching of error cases in .update and .create. #2202

brianhv opened this issue Dec 4, 2014 · 12 comments · Fixed by #2215

Comments

@brianhv
Copy link

brianhv commented Dec 4, 2014

When I do a PUT through the API browser to a view whose serializer contains a source property:

last_name = serializers.CharField(source='user.last_name')

I receive the following error:

Cannot assign "{'first_name': u'first', 'last_name': u'last'}": "ContactInfo.user" must be a "User" instance

I've created a bare-bones django project that reproduces the error. Of particular note are the serializers and the model.

Relevant excerpt of IRC conversation leading to this ticket:

<BrianHV> if you clone the repo, install the stuff in the requirements.txt (just django
          1.6 and DRF 3), set up a user, and use admin to create a ContactInfo for the
          user, you should be able to reproduce
<BrianHV> I'm testing through the API browser, btw.
<kevin-brown> Yeah, I think I know what is happening
<kevin-brown> `validated_data` is being (correctly?) generated as a nested dictionary,
              and that's being assigned to the `user` parameter directly
<kevin-brown> If you can open a ticket explaining that it works in DRF 2.4, that'd be
              helpful
<kevin-brown> In DRF 2.4, it would traverse the relation and assign the properties
              directly
@tomchristie
Copy link
Member

tomchristie commented Dec 4, 2014

Possible that we should make this case raise an explicit error explaining why it can't be dealt with automatically. You need to write an explicit create and/or update method here. Eg.

There isnt any way of automatically handling this case: What are you expecting to happen on create? Create a new user instance? If so how would you determine a username? You probably only want to support updates on this serializer and override create to raise 'NotImplemented' but if that's not the case then you'll need to figure out what it is you want to happen instead.

I've no doubt that 2.x would present some horrible buggy behaviour here.

@tomchristie
Copy link
Member

tomchristie commented Dec 4, 2014

Incidentally what type was the exception you got? Would be useful to know so we can improve the error messaging.

@kevin-brown
Copy link
Member

kevin-brown commented Dec 4, 2014

Incidentally what type was the exception you got?

Pretty sure this is a ValueError.

@tomchristie
Copy link
Member

tomchristie commented Dec 4, 2014

We could expand on the exception handling then

@brianhv
Copy link
Author

brianhv commented Dec 4, 2014

Yep, ValueError.

Environment:


Request Method: POST
Request URL: http://localhost:8765/api/users/1/

Django Version: 1.6.8
Python Version: 2.7.5
Installed Applications:
('django.contrib.admin',
 'django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.messages',
 'django.contrib.staticfiles',
 'rest_framework',
 'drftest.readonly')
Installed Middleware:
('django.contrib.sessions.middleware.SessionMiddleware',
 'django.middleware.common.CommonMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware')


Traceback:
File "/Users/bhv1/irccode/drfvirt/lib/python2.7/site-packages/django/core/handlers/base.py" in get_response
  112.                     response = wrapped_callback(request, *callback_args, **callback_kwargs)
File "/Users/bhv1/irccode/drfvirt/lib/python2.7/site-packages/django/views/decorators/csrf.py" in wrapped_view
  57.         return view_func(*args, **kwargs)
File "/Users/bhv1/irccode/drfvirt/lib/python2.7/site-packages/rest_framework/viewsets.py" in view
  79.             return self.dispatch(request, *args, **kwargs)
File "/Users/bhv1/irccode/drfvirt/lib/python2.7/site-packages/rest_framework/views.py" in dispatch
  406.             response = self.handle_exception(exc)
File "/Users/bhv1/irccode/drfvirt/lib/python2.7/site-packages/rest_framework/views.py" in dispatch
  403.             response = handler(request, *args, **kwargs)
File "/Users/bhv1/irccode/drfvirt/lib/python2.7/site-packages/rest_framework/mixins.py" in update
  68.         self.perform_update(serializer)
File "/Users/bhv1/irccode/drfvirt/lib/python2.7/site-packages/rest_framework/mixins.py" in perform_update
  72.         serializer.save()
File "/Users/bhv1/irccode/drfvirt/lib/python2.7/site-packages/rest_framework/serializers.py" in save
  136.             self.instance = self.update(self.instance, validated_data)
File "/Users/bhv1/irccode/drfvirt/lib/python2.7/site-packages/rest_framework/serializers.py" in update
  669.             setattr(instance, attr, value)
File "/Users/bhv1/irccode/drfvirt/lib/python2.7/site-packages/django/db/models/fields/related.py" in __set__
  339.                                  self.field.name, self.field.rel.to._meta.object_name))

Exception Type: ValueError at /api/users/1/
Exception Value: Cannot assign "{'first_name': u'asdf', 'last_name': u'asdf'}": "ContactInfo.user" must be a "User" instance.

@tomchristie
Copy link
Member

tomchristie commented Dec 4, 2014

How about the following?...

Got a ValueError when calling %s.objects.create(). This may be because you have a field on the serializer class that is is returning the incorrect type for '%s.objects.create(). You probably need to either change the field type, or override the %s.create() method to handle this correctly.\nOriginal exception text was: %s.'

@tomchristie
Copy link
Member

tomchristie commented Dec 4, 2014

Even more comprehensive would be to check for dotted source notation in the same way we do for writable nested fields.

@brianhv
Copy link
Author

brianhv commented Dec 4, 2014

Here's my perspective as a newbie (and assuming TypeError was meant to be ValueError).

My first response to that was similar to my response to the error I got: "Guess I can't do that, then." As I thought about it while finishing breakfast, my response shifted towards: "But if I really wanted to do it maybe that would tell me how if I were more familiar with the framework."

At the moment, though, I've settled on, "Why is it trying to create something when I'm trying to update an existing record? I must really not be doing this the right way."

If those are reasonable things to think in this situation, then that message would have the correct effect.

@tomchristie
Copy link
Member

tomchristie commented Dec 4, 2014

Why is it trying to create something

Oops my error above is incorrect of course - this is not during .objects.create() it's during assignment in update(). Anyways, upshot is that we should look into better error reporting here.

As it happens for "to one" relationships we could try to handle this automatically for updates only but there would still be error cases to deal with there (eg nullable relationship that does not exist) so not sure if we should try to do it automagically, or if we should force the code to be explicit.

@jonathan-golorry
Copy link

jonathan-golorry commented Jan 11, 2017

I noticed this is closed but I'm still getting the exact same error response.

It's also a little annoying that I can't even remove the attribute during validation or creation and then run super().create.

@tomchristie
Copy link
Member

tomchristie commented Jan 11, 2017

I can't even remove the attribute during validation

Override validate() if you want to modify what ends up in .validated_data

@jonathan-golorry
Copy link

jonathan-golorry commented Jan 12, 2017

Yeah, I looked at things a bit closer and realized it was storing the attribute in attrs and validated_data according to the source path, not the name of the attribute. I suppose that makes sense, I just didn't expect it.

I'm still curious as to why I wasn't getting the new exception though.

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

Successfully merging a pull request may close this issue.

4 participants