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

Fix None UUID ForeignKey serialization #3936

Merged
merged 3 commits into from Mar 22, 2016

Conversation

carltongibson
Copy link
Collaborator

@carltongibson carltongibson commented Feb 16, 2016

Replaces #3915

A nullable foreign key with a UUID primary key on the other end would result in incorrect serialisation as (the string) 'None'.

This PR adds a minimal test case and fix.

@carltongibson
Copy link
Collaborator Author

carltongibson commented Feb 16, 2016

As per @tomchristie's comment this might not be the right fix.

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

and

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?

@carltongibson
Copy link
Collaborator Author

carltongibson commented Feb 18, 2016

Right. Progress.

Slapping a breakpoint in where @tomchristie said reveals the problem:

rest_framework/serializers.py(469)to_representation()
-> if attribute is None:
(Pdb) field
PrimaryKeyRelatedField(allow_null=True, pk_field=UUIDField(), queryset=[])
(Pdb) attribute
<rest_framework.relations.PKOnlyObject object at 0x1122de7d0>
(Pdb) l
464                 except SkipField:
465                     continue
466     
467                 import pdb; pdb.set_trace()
468     
469  ->             if attribute is None:
470                     # We skip `to_representation` for `None` values so that
471                     # fields do not have to explicitly deal with that case.
472                     ret[field.field_name] = None
473                 else:
474                     ret[field.field_name] = field.to_representation(attribute)
(Pdb) 

In this case at least attribute is not None, even though the relation in None. Instead we have a PKOnlyObject object.

@carltongibson
Copy link
Collaborator Author

carltongibson commented Mar 13, 2016

Where a foreign key is null RelatedField.get_attribute will return None is all cases except where use_pk_only_optimization returns True. Only then will the existing attribute is None check give the wrong result.

Thus in 2ef74cf I resolve the pk value before checking for None. This allows the ad hoc code added to UUIDField to be removed, and the added tests still pass. 🎉

I'm happy to adjust the formatting of the A if B else C if you don't like that.

Otherwise I think this is probably the right fix. Thoughts?

@carltongibson carltongibson added this to the 3.4.0 Release milestone Mar 22, 2016
@carltongibson
Copy link
Collaborator Author

carltongibson commented Mar 22, 2016

Just milestoning this to keep it on the radar.

@xordoquy
Copy link
Collaborator

xordoquy commented Mar 22, 2016

@carltongibson does this need a review ?

@carltongibson
Copy link
Collaborator Author

carltongibson commented Mar 22, 2016

I think it's the right fix — but I'd be happy if you or @tomchristie could just think it through... — is there another case we've missed? (Or shall we handle that if/when it comes up)

@xordoquy
Copy link
Collaborator

xordoquy commented Mar 22, 2016

It looks good to me. I don't see other cases.
Let's move on.

On a side note, I'd be interested in discussing the design (ping @tomchristie) esp. why this has to be in the serializer as opposed as @carltongibson's initial proposal - which was to handle that within the field.

xordoquy added a commit that referenced this pull request Mar 22, 2016
Fix None UUID ForeignKey serialization
@xordoquy xordoquy merged commit 0e83063 into encode:master Mar 22, 2016
@xordoquy
Copy link
Collaborator

xordoquy commented Mar 22, 2016

Nice job 👍

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

Successfully merging this pull request may close these issues.

None yet

2 participants