Skip to content

Fix None UUID ForeignKey serialization #3915

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.

Sign up for GitHub

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

Closed
carltongibson wants to merge 6 commits into from
Closed

Fix None UUID ForeignKey serialization #3915

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ab3f9cf
Fix None UUID ForeignKey serialization
Feb 9, 2016
50782ec
Remove EOL Django 1.7
Feb 9, 2016
665f434
Update release notes.
Feb 10, 2016
14e52ca
Test deserialising data including `None` fk
Feb 10, 2016
363bc4f
Skip UUID tests for Django 1.7
Feb 10, 2016
793d03a
Revert "Remove EOL Django 1.7"
Feb 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix None UUID ForeignKey serialization
  • Loading branch information
Carlton Gibson committed Feb 9, 2016
commit ab3f9cf6e0f5b30f2256a59526481159bb8205d4
2 changes: 2 additions & 0 deletions rest_framework/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,8 @@ def to_internal_value(self, data):
return data

def to_representation(self, value):
if value is None:
return None
Copy link
Contributor

@lovelydinosaur lovelydinosaur Feb 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The None case is handled as part of the regular serialization machinery - shouldn't need to be handling them on a per-field basis. (Tho perhaps something else wonky in this case?)

Copy link
Collaborator Author

@carltongibson carltongibson Feb 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the first commit here: carltongibson@ab3f9cf

(Before getting distracted with 1.7)

The test fails without this change. If this is the wrong fix, then where should it go? (I'm happy to dig but you probably Just Know™)

Copy link
Collaborator Author

@carltongibson carltongibson Feb 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here PrimaryKeyRelatedField calls self.pk_field.to_representation(value.pk).

When that's a UUIDField we hit this block:

        if self.uuid_format == 'hex_verbose':
            return str(value)

When value is None, without the fix, that's equivalent to:

>>> str(None)
'None'

which is the observed output.

Copy link
Contributor

@lovelydinosaur lovelydinosaur Feb 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/serializers.py#L461-L472

Copy link
Contributor

@lovelydinosaur lovelydinosaur Feb 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question is why that's not catching the None case. Are we always failing to do so for related fields, or is there something different in this case?

Copy link
Collaborator Author

@carltongibson carltongibson Feb 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Good link. I see it.

I'll have a look later.

(Do you have any thoughts on keeping/dropping Django 1.7 here? Happy to keep it but not sure how, since the if ...VERSION fixes didn't work...?)

Copy link
Contributor

@xordoquy xordoquy Feb 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the 3.4 line dropping the 1.7.
I would feel better if that was part of a different PR.

Copy link
Collaborator Author

@carltongibson carltongibson Feb 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to make the tests pass... — I could do the 1.7 one first...

if self.uuid_format == 'hex_verbose':
return str(value)
else:
Expand Down
15 changes: 15 additions & 0 deletions tests/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from __future__ import unicode_literals

import uuid

from django.db import models
from django.utils.translation import ugettext_lazy as _

Expand Down Expand Up @@ -46,6 +48,11 @@ class ForeignKeyTarget(RESTFrameworkModel):
name = models.CharField(max_length=100)


class UUIDForeignKeyTarget(RESTFrameworkModel):
uuid = models.UUIDField(primary_key=True, default=uuid.uuid4)
name = models.CharField(max_length=100)


class ForeignKeySource(RESTFrameworkModel):
name = models.CharField(max_length=100)
target = models.ForeignKey(ForeignKeyTarget, related_name='sources',
Expand All @@ -62,6 +69,14 @@ class NullableForeignKeySource(RESTFrameworkModel):
on_delete=models.CASCADE)


class NullableUUIDForeignKeySource(RESTFrameworkModel):
name = models.CharField(max_length=100)
target = models.ForeignKey(ForeignKeyTarget, null=True, blank=True,
related_name='nullable_sources',
verbose_name='Optional target object',
on_delete=models.CASCADE)


# OneToOne
class OneToOneTarget(RESTFrameworkModel):
name = models.CharField(max_length=100)
Expand Down
21 changes: 20 additions & 1 deletion tests/test_relations_pk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
from rest_framework import serializers
from tests.models import (
ForeignKeySource, ForeignKeyTarget, ManyToManySource, ManyToManyTarget,
NullableForeignKeySource, NullableOneToOneSource, OneToOneTarget
NullableForeignKeySource, NullableOneToOneSource,
NullableUUIDForeignKeySource, OneToOneTarget, UUIDForeignKeyTarget
)


Expand Down Expand Up @@ -43,6 +44,18 @@ class Meta:
fields = ('id', 'name', 'target')


# Nullable UUIDForeignKey
class NullableUUIDForeignKeySourceSerializer(serializers.ModelSerializer):
target = serializers.PrimaryKeyRelatedField(
pk_field=serializers.UUIDField(),
queryset=UUIDForeignKeyTarget.objects.all(),
allow_empty=True)

class Meta:
model = NullableUUIDForeignKeySource
fields = ('id', 'name', 'target')


# Nullable OneToOne
class NullableOneToOneTargetSerializer(serializers.ModelSerializer):
class Meta:
Expand Down Expand Up @@ -432,6 +445,12 @@ def test_foreign_key_update_with_valid_emptystring(self):
]
self.assertEqual(serializer.data, expected)

def test_null_uuid_foreign_key_serializes_as_none(self):
source = NullableUUIDForeignKeySource(name='Source')
serializer = NullableUUIDForeignKeySourceSerializer(source)
data = serializer.data
self.assertEqual(data["target"], None)


class PKNullableOneToOneTests(TestCase):
def setUp(self):
Expand Down