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

Support serializing unsaved models with related fields. #2637

Merged
merged 1 commit into from Mar 8, 2015
Merged

Support serializing unsaved models with related fields. #2637

merged 1 commit into from Mar 8, 2015

Conversation

mdentremont
Copy link
Contributor

@mdentremont mdentremont commented Mar 4, 2015

  • In both RelatedField and ManyRelatedField, provide an empty return
    when trying to access a relation field if the instance in question has
    no PK (so likely hasn't been inserted yet)
  • Add relevant tests
  • Without these changes, exceptions would be raised when trying to
    serialize the uncreated models as it is impossible to query
    relations without a PK

@tomchristie tomchristie changed the title Add support for serializing models with related fields Add support for serializing unsaved models with related fields Mar 5, 2015
@tomchristie tomchristie changed the title Add support for serializing unsaved models with related fields Support serializing unsaved models with related fields. Mar 5, 2015
@@ -80,6 +80,13 @@ def run_validation(self, data=empty):
data = None
return super(RelatedField, self).run_validation(data)

def field_to_native(self, obj, field_name):
# Without PK there is no way to determine any relations to the object
if obj.pk is None:
Copy link

@ghost ghost Mar 5, 2015

Choose a reason for hiding this comment

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

Please check #2641 to make sure we don't make another mistake here.

@carltongibson
Copy link
Collaborator

carltongibson commented Mar 5, 2015

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

@mdentremont
Copy link
Contributor Author

mdentremont commented Mar 6, 2015

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

@carltongibson
Copy link
Collaborator

carltongibson commented Mar 6, 2015

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 a if 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 file smells 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?

@ghost
Copy link

ghost commented Mar 6, 2015

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.

@mdentremont
Copy link
Contributor Author

mdentremont commented Mar 6, 2015

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

@mdentremont
Copy link
Contributor Author

mdentremont commented Mar 6, 2015

@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

@ghost
Copy link

ghost commented Mar 6, 2015

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

@mdentremont
Copy link
Contributor Author

mdentremont commented Mar 6, 2015

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

@mdentremont
Copy link
Contributor Author

mdentremont commented Mar 6, 2015

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

- In both ManyRelatedField, provide an empty return when trying to
  access a relation field if the instance in question has no PK (so
  likely hasn't been inserted yet)
- Add relevant tests
- Without these changes, exceptions would be raised when trying to
  serialize the uncreated models as it is impossible to query
  relations without a PK
- Add test to ensure RelatedField does not regress as currently 
  supports being serialized with and unsaved model
@mdentremont
Copy link
Contributor Author

mdentremont commented Mar 6, 2015

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

@tomchristie tomchristie added this to the 3.1.1 Release milestone Mar 6, 2015
@ghost
Copy link

ghost commented Mar 6, 2015

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

@tomchristie
Copy link
Member

tomchristie commented Mar 6, 2015

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!

@carltongibson
Copy link
Collaborator

carltongibson commented Mar 8, 2015

I've reviewed #2638. The issue there was specifically about creating the URL for a the url on a HyperlinkedModelSerializer. I've closed that given that a work around was already available using validated_data, rather than data for @zhigang's example.

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

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

Successfully merging this pull request may close these issues.

None yet

3 participants