Support source='some_method' for HyperlinkedRelatedField.

[delinhabit]

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!

[tomchristie]

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]

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]

Isn't my example a valid use case?

[tomchristie]

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

Yup - I'd agree that this is valid.

[tomchristie]

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]

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

[delinhabit]

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

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]

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

[tomchristie]

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

[tomchristie]

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]

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

[tomchristie]

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.