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 source='some_method' for HyperlinkedRelatedField. #2690

Merged

Conversation

delinhabit
Copy link
Contributor

@delinhabit delinhabit commented Mar 13, 2015

Currently you cannot use a callable source for a HyperlinkedRelatedField. I added a test to reproduce the issue.

The problem comes from the PKOnlyObject optimization. The value passed to the serialization method (here) is a PKOnlyObject instance on which the pk attribute is the callable configured as the source. Because of that, the URL reversal fails.

Am I trying to do something illegal? I'm happy to fix the issue if we confirm this is a valid case that is failing.

Thanks!

@delinhabit delinhabit changed the title Use callable source for HyperlinkedRelationField Use callable source for HyperlinkedRelatedField Mar 13, 2015
@tomchristie
Copy link
Member

tomchristie commented Mar 13, 2015

Currently you cannot use a callable source for a HyperlinkedRelatedField

Sorry I must be missing something - didn't understand why that'd be something we want/need to support?

@delinhabit
Copy link
Contributor Author

delinhabit commented Mar 13, 2015

A callable source is an allowed argument type for any Field, including HyperlinkedRelatedField. Since the docs are not prohibiting this usage scenario I just assumed that I can use a callable source for a HyperlinkedRelationField like I can use it for other field types.

The use case is simple. Fore example, you have a model Conversation with two foreign keys to a Person model: provider and requester. In the API I want to expose the recipient for the Conversation which is computed using Conversation.get_recipient() based on who created the conversation. The serializer is something along the lines:

class Conversation(models.Model):
    provider = models.ForeignKey(Person)
    requester = models.ForeignKey(Person)
    sender = models.ForeignKey(Person)  # enforced logically to be one of (provider, requester)

    def get_recipient(self):
        if self.sender == self.provider:
            return self.requester
        else:
            return self.provider


class ConversationSerializer(serializers.ModelSerializer):
    sender = serializer.HyperlinkedRelatedField(view_name=...)
    recipient = serializer.HyperlinkedRelatedField(source='get_recipient', view_name=...)

Basically recipient is computed dynamically based on other fields on the instance, but conceptually it's still a relation. I think we should be able to use a HyperlinkedRelatedField for this purpose.

If we don't want to support this we should at least add this to the docs that a related field can be used only on actual relations and not on methods that return a relation.

Thanks!

@delinhabit
Copy link
Contributor Author

delinhabit commented Mar 26, 2015

Isn't my example a valid use case?

@tomchristie
Copy link
Member

tomchristie commented Jun 24, 2015

A callable source is an allowed argument type for any Field, including HyperlinkedRelatedField.

Yup - I'd agree that this is valid.

@tomchristie
Copy link
Member

tomchristie commented Jun 24, 2015

Next steps to progress this issue:

  • Is this the case with all related fields?
  • Determine what changes are required to fix.
  • Assess if this should be a documented constraint or should be resolved.

@tomchristie
Copy link
Member

tomchristie commented Jun 24, 2015

(And apologies for the delay! Getting seriously back on the triaging bandwagon now)

@delinhabit delinhabit force-pushed the hyperlinked-relation-callable-source branch 2 times, most recently from 6b63439 to 16508cf Compare Jul 25, 2015
@delinhabit delinhabit force-pushed the hyperlinked-relation-callable-source branch from 6b54703 to 47a22a5 Compare Jul 25, 2015
@delinhabit
Copy link
Contributor Author

delinhabit commented Jul 25, 2015

@tomchristie I added a test case for PK relations and also a possible fix to show where the problem was introduced (it was working on DRF<3.x)

To answer your questions:

  • Is this the case with all related fields? - No. Only for those that have PKOnly optimizations enabled (PrimaryKeyRelatedField and HyperlinkedRelatedField when lookup field == 'pk')
  • Determine what changes are required to fix. - I have a possible solution, but I don't feel quite pleased with it, though I'm not sure how can it be done differently. The problem is basically that instance.serializable_value() returns a callable for callable sources.
  • Assess if this should be a documented constraint or should be resolved. - I feel that it should be resolved, especially because it was used to work on DRF<3.x, but also because I think it's perfectly valid use case to support (though it might be a rare use case)

@tomchristie
Copy link
Member

tomchristie commented Jul 27, 2015

Okay, seems reasonable enough.

Given the narrow scope of the change, I feel like the test changes (eg changing the test model) are more detrimental than we'd like. I'd suggest that we either: (1) try to limit the test scope so that eg the model they use is contained in the test itself. Or (2) just drop the tests - the code is clearly more correct that it was previously, and even just a code comment of Handle edge case where the relationship source='' argument points to a 'get_relationship() method on the model.` would be sufficient to prevent thoughtless regressions.

Either (1) or (2) would be acceptable to me.

(Motivation for keeping the test scope limited is that we may one day want to refactor and clean up the gnarly relational tests, and right now this'd add more work to achieving that)

@delinhabit delinhabit force-pushed the hyperlinked-relation-callable-source branch from ccf77ca to 0386a01 Compare Jul 27, 2015
@delinhabit
Copy link
Contributor Author

delinhabit commented Jul 27, 2015

@tomchristie I usually prefer tests over code comments, but in this case I just dropped the tests. It felt wrong to duplicate the two related models and have them self-contained in each test case.

I removed the model changes and the tests, and added a code comment on the edge case. Please take another look.

return PKOnlyObject(pk=instance.serializable_value(self.source_attrs[-1]))

# Handle edge case where the relationship `source` argument
# points to a `get_relationship()` method on the model
Copy link
Member

@tomchristie tomchristie Jul 27, 2015

Choose a reason for hiding this comment

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

This comment should probably go instead the "if" block.

@tomchristie
Copy link
Member

tomchristie commented Jul 27, 2015

Great. Minor comments inline, but otherwise good to go.

This level of most-minimal-possible incremental change is absolutely my preferred way to see pull requests. Tiny and self-evidentally correct, sensible changes. :)

@delinhabit
Copy link
Contributor Author

delinhabit commented Jul 27, 2015

I moved the code comments and cleaned-up the whitespace.

@tomchristie tomchristie added this to the 3.1.4 Release milestone Jul 27, 2015
@tomchristie tomchristie changed the title Use callable source for HyperlinkedRelatedField Support source='some_method' for HyperlinkedRelatedField. Jul 27, 2015
@tomchristie
Copy link
Member

tomchristie commented Jul 27, 2015

Perfect.
Updated the title to make the motivation clearer when the release notes get compiled.
To be merged once the final travis run-through completes.

@tomchristie tomchristie modified the milestones: 3.2.0 Release, 3.1.4 Release Jul 30, 2015
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