Make view context kwargs available to HyperLinkedIdentityField and HyperLinkedRelatedField #1403

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
3 participants

This makes the view kwargs available to HyperLinkedIdentityField and HyperLinkedRelatedField while still allowing the possibility to override certain parameters.

It should solve the issue raised in #1339

bartvandendriessche added some commits Feb 10, 2014

Remove **extra argument from reverse function
django.core.urlresolvers.reverse doesn't accept a **kwargs attribute
@@ -6,7 +6,7 @@
from django.utils.functional import lazy
-def reverse(viewname, args=None, kwargs=None, request=None, format=None, **extra):
+def reverse(viewname, args=None, kwargs=None, request=None, format=None):
@tomchristie

tomchristie Feb 10, 2014

Collaborator

I don't think this change should really effect the signature here?

@bartvandendriessche

bartvandendriessche Feb 10, 2014

You're right it doesn't. I initially tried a different route for this branch where this change was required.

If you want I'll revert it in a following commit.

Collaborator

tomchristie commented Feb 10, 2014

So, as I understand it, this fix allows you to for example, prefix your URLs with an API version, and still have all hyperlinks apply correctly, right?

Collaborator

tomchristie commented Feb 10, 2014

Note that it looks from travis that your style of dict concatenation isn't supported in python 3 or something similar so you'll need to look into that. Also needs tests and perhaps some additional documentation against HyperlinkedRelatedFields noting that this is supported.

Yeah I noticed the build failing, I'll have a look at the Python 3 issue.

It's possible that this fix will allow people to use versions, but I think proper versioning would require some more work.

I'm using this fix because I'm working on a SaaS app which uses a slug in the url to determine what tenant content should be scoped to.

e.g.:

https://example.com/api/<customer_a_company_name>/documents

A better way to put this is that this makes Views agnostic of their mount point.

Previous to this change, this would work:

# api_documents_router initialisation happens here

 urlpatterns = patterns(
     '',
     url(r'^api-auth/', include('rest_framework.urls', namespace='rest_framework')),
     url(r'^api/documents/', include(api_documents_router.urls)),
)

But this would fail:

# api_documents_router initialisation happens here

 urlpatterns = patterns(
     '',
     url(r'^api-auth/', include('rest_framework.urls', namespace='rest_framework')),
     url(r'^api/(?P<slug>(\w|\-|_)+)/documents/', include(api_documents_router.urls)),
)

Because reverse would not receive the slug, and thus not be able to lookup the proper url. As a DRF user, this felt very unexpected, since the captured slug parameter should be transparent as far as DRF is concerned.

Collaborator

carltongibson commented Feb 11, 2014

Related to #1143. (Note to myself)

Collaborator

tomchristie commented Feb 11, 2014

It's possible that this fix will allow people to use versions, but I think proper versioning would require some more work.

What sort of thing are you thinking?

Haven't given it a lot of thought yet, but the main problem with Versioning is that it means different things to different people.

People usually want to version on either path, accept-header, accept-version-header or a request parameter.

This particular pull request might make it easier to build path-based versioning, because it makes the required 'version' parameter available in the HyperLinked*Field classes.

On its own though, this PR won't magically add versioning support by itself.

Just noticed that this will break drf-nested-routers in the following way:

/<slug>/categories/<category_pk>/items/<pk>

If the ItemSerializer has a Hyperlinked reference to its category, then reverse will fail for category-detail because an extra category_pk kwarg will be present, and django.reverse requires kwargs to exactly match any captured parameters.

Closing this pullrequest since this does not seem to be the low impact change I thought it was.

Collaborator

tomchristie commented Feb 17, 2014

@bartvandendriessche I was going to say something similar. I think there's two options for this: Either allow an explicitly named set of kwargs to be passed through, or else better document how to write custom hyperlinked fields & use them with serializers. I probably prefer the second.

@tomchristie I'd written a custom HyperlinkedModelSerializer, and that worked fine, so I think the documentation for that isn't all that bad.

The reason why I dug in a bit deeper is because it just felt like the default Hyperlinked fields should be agnostic of (and thus work with) whatever Route they are mounted on. That would make them more portable.

I'm going ahead with a different approach right now, but I'll mull over this some more and may revisit the issue at a later date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment