Serializer validation error on nullable relations

[dustinfarris]

I think this has been danced around before, but I can't remember what the consensus was and Google is not helping me today.

If I have:

class Book(models.Model):
    publisher = models.ForeignKey(Publisher, null=True, blank=True)

class BookSerializer(serializers.ModelSerializer):
    class Meta:
        model = Book

class BookViewSet(viewsets.ViewSet):
    model = Book
    serializer_class = BookSerializer

Then in shell:

>>> from django.test.client import Client
>>> client = Client()
>>> book = Book()
>>> serializer = BookSerializer(book)
>>> serializer.data
{'id': None, 'publisher': None}
>>> response = client.post('/books/', serializer.data)
>>> response.data
{'publisher': ['Incorrect type.  Expected pk value, received str.']}

To make this work I have to manually convert the None value to an empty string (which is hinted at in the documentation under the "required" attribute for relation fields):

>>> response.data.update({'publisher': ''})
>>> response = client.post('/books/', serializer.data)
>>> response.data
{'id': 1, 'publisher': None}

It seems like the first request should not return a 400 given the model is valid, and the data was created using DRF's own serializer.

In case this is intended behavior, I've also inquired on SO for best practices:

http://stackoverflow.com/questions/20681468/how-to-make-foreignkey-play-nice-with-none-in-drf

[tomchristie]

Responded against the S/O question... http://stackoverflow.com/questions/20681468/how-to-make-foreignkey-play-nice-with-none-in-drf

[tomchristie]

Annddd.... updated.

[dustinfarris]

@tomchristie Sorry, I've been editing both SO and this issue for the past couple minutes. Didn't realize I would get a response so fast. Both should be complete now (especially here), I think.

[tomchristie]

Related to #380, which was closed by 02c1b59, fixing this issue for hyperlinked relations. Looks like we're missing the same thing for PK relations (and possibly slug too?)

[frankpape]

I ran into this today. After some stepping around in a debugger, what's happening is pretty clear. APIRequestFactory's _encode_data method calls renderer.render(data) (in my case renderer is MultiPartRenderer), which in turn calls django.test.client.encode_multipart(...).

encode_multipart iterates over data.items(), encoding the None value by calling django.utils.encoding.force_bytes(...) on it, which (eventually) calls the builtin bytes(...), which returns the string 'None'.

So later, when the serializer attempts to deserialize the data from the request, the foreign key field contains a literal 'None'.

A naive fix seems like it would be pretty easy (don't encode a string 'None'), but this is hardly isolated (and it's in core django code), so I'm not sure what the "right" solution would be.

[dustinfarris]

I think coercing None values to empty strings prior to the _encode_data call would work.

[frankpape]

FWIW, this is a bit worse for nullable string fields. For nullable foreign keys, an empty string results in the field getting saved with a null value, which works. For nullable string fields, the empty string gets saved verbatim, and there is no value (that I can see) that the renderer will deserialize as None.

Edit: and of course, it still requires replacing None values with an empty string before encoding, otherwise the string field gets 'None' which is almost certainly worse than it failing.

[dustinfarris]

@frankpape good points. I think nullable string fields might be something we should ignore for now/table for later discussion. Django would certainly not be the first to deal with this nuisance in terms of JSON (de)serialization.

[frankpape]

@dustinfarris fair enough. I only bring it up because they share the root problem, and a solution to the nullable relations issue will likely impact other field types as well.

[dustinfarris]

Ok. I'll submit a PR and see how it goes.

[tomchristie]

Closed by #1441 thanks to @dustinfarris.
Worth rolling a new release?

[dustinfarris]

@tomchristie I'll say yes, but I'm a "release often" kind of guy. Plus, this will benefit several of my own projects so my opinion is biased. :-)

[tomchristie]

Little bust this w/e with fam commitments. Ping this ticket again tues or so if I've not already got around to it by then.

[tomchristie]

s/bust/busy/

[dustinfarris]

Sounds good.

[dustinfarris]

@tomchristie here is your friendly reminder for a version bump.

[tomchristie]

With added sparkles.

💖 ✨ 2.3.13 ✨ 💖

[tomchristie]

Thanks @dustinfarris