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 #3915

Closed
wants to merge 6 commits into from

Conversation

carltongibson
Copy link
Collaborator

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.

Verified

This commit was signed with the committer’s verified signature.
casperdcl Casper da Costa-Luis
@carltongibson
Copy link
Collaborator Author

Well the build is failing for Django 1.7 with a weird one:

RuntimeError: populate() isn't reentrant

A quick Google suggests restarting Apache (!?) — It's not one I've seen before. Any ideas?

Django 1.7? :-)

@carltongibson
Copy link
Collaborator Author

Oh... of course... UUIDField isn't available in Django 1.7. (I'll look at that tomorrow)

Django 1.7? :-)

@carltongibson
Copy link
Collaborator Author

Since Django 1.7 is EOL I dropped it from the build.

Happy to re-add and work around it if needed, but IMO it's a good thing to now support.

Let me know.

@xordoquy
Copy link
Collaborator

xordoquy commented Feb 9, 2016

Well, we'll also need to update the release notes for 3.4 if we drop 1.7 for it (I'm fine with this).

@carltongibson
Copy link
Collaborator Author

@xordoquy I updated the release notes.

@carltongibson
Copy link
Collaborator Author

The problem goes both ways. See 944a121: Data setting the UUID FK to None fails validation.

I was hoping the allow_empty on the PK related field would do the job, but it doesn't.

I'm not sure where to put the fix here. Input needed. :-)

Related: #3130

@carltongibson
Copy link
Collaborator Author

OK. I worked it out: the key is allow_null to the parent PrimaryKeyRelatedField.

@xordoquy xordoquy mentioned this pull request Feb 10, 2016
@tomchristie
Copy link
Member

@carltongibson Would it make sense to target this fix at 3.3.3, and have the Django deprecation as a separate PR targeting 3.4.0?

@carltongibson
Copy link
Collaborator Author

Hey @tomchristie — Only issue there is the failure.

RuntimeError: populate() isn't reentrant — what an error message!

It's because we end up importing UUIDField into the test suite.

Is it worth branching on that? You tell me — I don't mind. — What's the easiest way to avoid the issue for now?

@carltongibson
Copy link
Collaborator Author

OK... Let's see how this one goes...

@carltongibson
Copy link
Collaborator Author

OK. Same error.

My version check obviously isn't right. Advice please. 😃

@carltongibson
Copy link
Collaborator Author

As this stands I'd be inclined to reset to carltongibson@14e52ca

If you can point me to an easy way to save Django 1.7 compat for now I can add that, if it's worth it. (Version numbers are cheap and anyone still on 1.7 has more important things to worry about than upgrading DRF...)

@@ -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
Member

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

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

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
Member

Choose a reason for hiding this comment

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

Copy link
Member

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

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
Collaborator

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

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...

@carltongibson
Copy link
Collaborator Author

Replaced by #3936

@carltongibson carltongibson deleted the null-uuid-fk branch August 29, 2017 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants