Fix None UUID ForeignKey serialization by carltongibson · Pull Request #3936 · encode/django-rest-framework (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation8 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

carltongibson

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

@carltongibson

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

@carltongibson

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

Just milestoning this to keep it on the radar.

@xordoquy

@carltongibson

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

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

@xordoquy

Fix None UUID ForeignKey serialization

@xordoquy

This was referenced

Mar 9, 2017

2 participants

@carltongibson @xordoquy