As per comment there, this seems similar to#2641.

@carltongibson Just to confirm, do you know if this change is acceptable? If so, could you let me know what I need to do get it in a ready state? I could look into#2641 and check whether the PRs conflict.

Hey@mdentremont — short answer, No I don't know. :-)

What we've got are two related issues (no PKs on unsaved instances) popping up at the same time with two different fixes.

At first glance the fix here looks fine to me — and it's tested — so, is it the behaviour we want and if so have we caught all the places?

#2641 does the same as your change here, basically adding aif obj.pk is None: ... guard clause at the begging of existing logic. — I'm not sure if doing that 3 (or more if there are other cases) times in the same filesmells or not. — Maybe it's okay, but I just flagged it to make sure we make the right change here.

In terms of getting this ready... If you looked at#2641 and thought it through that would help. Is it a related issue that your use-case would bump into coming from the other direction? Is there an obvious test for it? Does your patch fix it already?

Clear enough? Sound reasonable?

My comment is to this line only:

if obj.pk is None:

When you write as this, are you sure there will always be a 'pk' in 'obj'?

From issue#2638, you can see it's not the case for the other place. Please just confirm this. I'm not sure whether this is an issue here.

@zhigang to solidify this, we could also ensure obj has a pk attribute before checking if pk is none?

@carltongibson I could test my change to check whether it also covers#2641, in which case we could close the other PR.

It's so odd that both of these PRs were created withing a day of each other, when this has been present since at least 2.4, if not earlier

@mdentremont if this pull-request could cover issue#2638 , I'm OK to discard#2641 .

@zhigang I've given this a test, and it allows me to serialize an unsaved model, where the HyperlinkedRelatedField is in place.

#2638 doesn't have enough code for me to test the use case; I was hoping maybe you could give it a shot with my changes?

My mistake,field_to_native was removed in 3.0. It looks like not change was required forRelatedField (although the added test is nice to ensure this does not regress)

I've removed the field_to_native change from my commit, it should not have been added in the first place.

@mdentremont : your patch doesn't cover the case in issue#2638 from my test.

This looks good to me. I'll make a further review but believe this should be going in, in it's current state.
Nice work!

I've reviewed#2638. The issue there was specifically about creating the URL for a theurl on aHyperlinkedModelSerializer. I've closed that given that a work around was already available usingvalidated_data, rather thandata for@zhigang's example.

So that means this is good to go. Thanks for the input!