Feature/url versions

[readevalprint]

This gives the ability to use namespaces in the urls.py and have reverse pick up on them automatically so that all links generated in the api use the current namespace pattern, if any.
From this discussion: https://groups.google.com/forum/?fromgroups=#!topic/django-rest-framework/JBb-k7FTnvA

In this example the default urlpatterns is also aliased to /v1/

url(r'', include(urlpatterns)), # urls are like http://example.com/user/tim
url(r'v1/', include(urlpatterns, namespace='v1')), # urls are like http://example.com/v1/user/tim

It passes currents tests.
Let me know what it would take to get this accepted :)

[carltongibson]

Versioning is something we need to provide. (No one disagrees with that right?)

Every time it comes up on the mailing list there's talk of doing it via Accept headers. This sounds great but (perhaps I'm slow) I can't see how doing it that way is (currently) feasible without rewriting — essentially cut and pasting — whole sections of the GCBVs.

Here we have the basis of a working solution to the versioning problem. Unless similar can be demonstrated using the correct Accept approach we should throw our weight behind this and get it in.

What needs to be done?

I'd be inclined to break it into sub-tickets if possible so we can collaborate. I, for one, will put some cycles into this.

[carltongibson]

Just posting here for ease of reference (whilst planning a "Topics > Versioning your API" page)

This was the example from the mailing list thread:

from app import views_v2
v2_api_urlpatterns = patterns('',
    url(r'^foo/$', views_v2.FooList.as_view(), name='foo-list'),
)

v2_api_urlpatterns += format_suffix_patterns(v2_api_urlpatterns)

So if we did that multiple times we'd do:

urlpatterns = patterns('',
    url(r'', include(v0_api_urlpatterns)),  # Un-versioned. 
    url(r'v1/', include(v1_api_urlpatterns, namespace='v1')),
    url(r'v2/', include(v2_api_urlpatterns, namespace='v2')),
)

[readevalprint]

Looks good, how else could I help out? What are the edge cases where it could break?

[carltongibson]

Ideas:

1. Any new tests for rest_framework.reverse needed? A lot of this is Just Django™ but we're adding a new behaviour here, so surely there's a test case.
2. Any thoughts for the docs?

I think it wold be cool if we could show an absolutely minimal versioned api — or outline in the style of the Quickstart. i.e. one URLConf using the method above pointing to two hello world views in different modules. (And we say, "From here you can include different versions of ... as needed for your application." and so on.)

[carltongibson]

The content of the pull-request strikes me as totally harmless — it's just adding namespace support to rest_framework.reverse — which arguably it should have anyway.

Perhaps we should merge this and open a new ticket for "Documenting how to Version your API"?

@tomchristie It would be good to have your thoughts. Given the tight mapping (in Django) between URLs and views it's hard to see other ways around versioning. — Old code I wrote that went if request.method == "GET": (... and so on) keeps flashing into my mind :) — branching by URL seems cleaner.

[tomchristie]

At least in newer versions of Django I think this information is already stored on the request object?
Little cautious about introducing this as it currently stands as a single view might output a lot of URLs, all of which will now cause URL resolving.

[carltongibson]

Since Django 1.5 we have the ResolverMatch which gives us the namespace.

So we can do:

try: 
    namespace = request.resolver_match.namespace
except AttributeError: # this is right yes? 
    ...

[carltongibson]

Do we always get a request?

I'm just trying to profile this change (and the request.resolver_match.namespace version) against the original and hit an AttributeError here.

'NoneType' object has no attribute 'path'

— Given that the current tests pass but None is a valid value, we need a new test (I think)

[tomchristie]

It would be good to have your thoughts

URL Versioning, accept header versioning, versioning in the request/response content, And not versioning at all are all valid options in different circumstances. It probably would be good to suppport at least URL based versioning out of the box and for sure we need to document the options.

This PR seems to be along sensible lines though will need to mull it over. Also note the inline comment. I might be fine with only supporting on certain versions of Django if there's a limitation there.

[carltongibson]

I've been quiet on this. As ever I got busy. :-)

Before that, I began looking in to it but realised I don't know enough about how app/instance/url namespaces work in Django to be 100% confident about the solution.

I started on the docs but ate whatever time I had not getting very far. (It was basically clear as mud — probably simple enough but not laid out very well in the Django documentation.)

I also started to profile the change — and the suggested alteration using the resolver_match attribute but ran into the error when request is None. That's going to be easy enough to work round but I'm not sure of the desired behaviour. Clearly request=None being in the signature implies we should handle it.

Finally, as I've let it stew, I've come to wonder if we need the namespace at all. Doesn't this work equally?

urlpatterns = patterns('',
    url(r'', include(v0_api_urlpatterns)),  # Un-versioned. 
    url(r'v1/', include(v1_api_urlpatterns)),
    url(r'v2/', include(v2_api_urlpatterns)),
)

(This just adapts the example from the previous discussion.)

Counterpoint: Even if it does, arguably reverse should support name spacing anyway...

[kevin-brown]

I'm pretty sure that not including the namespace but using the same named paths won't work (thinking of a router giving the same names).

Either way, it isn't difficult to include namespace support, and this is definitely a use case that fits larger applications.

[carltongibson]

On 24 Oct 2013, at 15:31, Kevin Brown [email protected] wrote:

thinking of a router giving the same names

Ah, yes, of course.

My comment was hoping to restart this one.

it isn't difficult to include namespace support

— Cool. The bit I ran out of time on was "app" vs "instance" and such. I didn't have time to get my head round it to make sure the patch was right.

[readevalprint]

Ok I've been seriously thinking about this and just got around to testing it. My worry was 1) what happens when reverse() was used on the shell or in a situation without a request? 2) what happens when it is passed a view with a namespace specified as inreverse("v2:profile")?

So this new commit should cover both. And I switched to request.resolver_match.namespace too.

[kevin-brown]

As it currently stands, all tests are now failing. New tests should probably be added to make sure the namespaces work correctly?

[readevalprint]

Ah I didn't run tests. Huburis...

[readevalprint]

And what should we test now? Using the DRF reverse with a "namspace:app"? and without request objects? and perhaps without a namespace in the urls.py

[readevalprint]

Hey @carltongibson I'm not quite sure I follow. As it currently stands, this pull attempts to use resolver_match which is just resolve(request.path).namespace cached on the request or to directly call resolve(request.path).namespace as a fallback.

[kevin-brown]

i'm pretty sure this will fail if the passed url has a different namespace than the view/request namespace.

[readevalprint]

@kevin-brown could you give an example?

[carltongibson]

@readevalprint I think the idea was simply not to support namespaces if resolver_match is not available on the request. i.e. to fall back to the current behaviour, rather than call resolve(), potentially hundred of times, in a tight loop.

[readevalprint]

@carltongibson That works for me. Let me update the pull.

[rosscdh]

bump. Hey guys thanks for working on this aspect. What is the current status?

[carltongibson]

This is on my list to get finished. (We should be supporting namespaced URLs.)

My guess is the pull as-is would work for the usual cases but we need to draw up a list of test cases where it may break and ensure all of those are good.

(Given that it's new behaviour we could just make sure non-namespaced URLs are okay and put a big "try this, if you have issues let us know" sign on it — but I'd rather cover as much as we can before thinking about that.)

Any thoughts on this thread as to cases that would be worth testing would be welcome.

On 6 Dec 2013, at 09:44, rosscdh [email protected] wrote:

bump. Hey guys thanks for working on this aspect. What is the current status?

—
Reply to this email directly or view it on GitHub.

[tomchristie]

We could drop this logic into the Request class, to transparently support a .resolver_match attribute.

[tomchristie]

@carltongibson Sounds good ✨

I guess we have the following cases:

• non-namespaced request, reversing to namespaced view. (0)
• namespaced request, reversing to non-namespaced view. (1)
• namespaced request, reversing to same namespaced view. (2)
• namespaced request, reversing to a differently namespaced view. (3)

I don't think case (0) needs any work - that ought to already be fine.
In cases (1), (2) and (3) it seems to me that there are always two cases that might be correct - with the request namespace pre-pended and without the request namespace prepended.

I think the right approach might be to use the existing reverse behaviour, and only if that fails to then try prepending any request namespace and trying that.

[carltongibson]

OK. I somehow managed to clear the list. Am looking at this now.

[readevalprint]

oo wow, forgot about this. I'm looking now too

[carltongibson]

Hey Tim — Ok, cool.

The bit I'm thinking about is the minimum needed to bootstrap a unit for reverse — I'm going to take a look at the Django test suite to see what they do there.

Beyond that there's @tomchristie's comments and we should be about done.

[carltongibson]

Right… Okay… As per usual I've been totally snowballed by work. However, this ticket seems a good candidate for the sprint in Cardiff this weekend. (There is one right?) I made this note that might aid that:

The relevant tests to adapt from the Django test suite are in django/tests/urlpatterns_reverse/tests.py.

The key point is to set the urls attribute of the TestCase subclass. (Documented here: https://docs.djangoproject.com/en/1.6/topics/testing/tools/#urlconf-configuration)

If this doesn't get knocked off in Cardiff I'll make the session in the next few weeks to get it done.
And I had such grand plans for Christmas. :-)

C.

[edemocracy]

any news on this?

[carltongibson]

It's still on the list. I need to sit down and write the tests for it. Unless someone jumps in that will be "next free moment".

The patch should more-or-less work. If you want to cherry-pick it and have a play you can do that. Any failing test cases you come up with would also help.

[pikeas]

Bump. Will test the patch, but wondering if this will be in the next DRF release? Any reason it wasn't in the 2.3.13 release from a few weeks ago?

[carltongibson]

Hey @pikeas — this wasn't in the last release because it's not ready yet. At the least it needs tests and docs — but the code probably needs adjusting too. (Having said that it'll probably work, and any real-world feedback would no doubt be handy.)

The main stumbling block is the tests — they're not hard but it's a bit of boilerplate which means a little bit of time setting it up. I've put my hand up to do that but frankly have been snowed under and have had essentially zero time for any OSS — or even much Django for that matter.

I am doing some API work this week so I'll see if I can squeeze it in.

[carltongibson]

Closing in favour of #1495