Some tests for the patch in #1143. Finally!

[carltongibson]

Okay, I finally got round to writing the test cases for #1143.

@tomchristie: can you review? — I made sure the current (and new) tests pass but there was talk of reordering the logic. If you comment here I'll adjust.

[kevin-brown]

I think we are missing a test for a request from a namespaced view to a non-namespaced view, which should fail because we just assume that all requests with namespaces point to views with namespaces.

#1143 (comment)

[carltongibson]

Hey @kevin-brown — Do you mean something like:

    request = factory.get('/v2/view')
    url = reverse('view', request=request)
    self.assertEqual(url, 'http://testserver/view')

? This would be the exact opposite of the original intention — it's opposite is the first of the added tests (and it's the one that fails without the patch.) — If that's a requirement I'm not at all sure how to proceed.

If this isn't what you mean, can you clarify? Thanks.

[kevin-brown]

Pulling from #1143 (comment):

namespaced request, reversing to non-namespaced view. (1)
[...]
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.

If you added a second url to the v0 patterns, lets call it other-view, and wanted to reverse to it from within a v1 router, then that would currently fail because it would assume that non-namespaced views are always within the same namespace.

Using the namespace (or not using it, depending on the order) as a fallback was proposed by @tomchristie in his comment to handle this issue.

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.

This would only be needed if our first guess at how to reverse failed.

[carltongibson]

OK. So I add this test (with the appropriate un-namespaced url pattern):

def test_namespaced_request_to_non_namespaced_view_not_in_namespace(self):
    request = factory.get('/v2/view')
    url = reverse('other-view', request=request)
    self.assertEqual(url, 'http://testserver/other-view')

(Sorry about the name — it's getting late)

Sure enough this fails. I then follow the " use the existing reverse behaviour..." thought in reverse.py:

try:
    url = django_reverse(viewname, args=args, kwargs=kwargs,
                          **extra)
except NoReverseMatch:
    # Patch goes here...

But then the main use-case test for #1143 fails: reversing view gets the un-namespaced URL.

Not sure what to say.

[carltongibson]

Ignore it for a while. Always.

I reversed the order of the checks:

try: 
  #patch here
except NoReverseMatch:
  # existing reverse behaviour

With this all the tests pass — the main use-case test for #1143 and reverse('other-view', request=request) — to a non-namespaced view.

The thing you couldn't do here is reverse to a non-namespaced view which has a matching view in the request's namespace — but that hurts to even say, and if you've got that issue you should be using namespaces...

---

If the general approach is considered sound then I'm happy to make whatever tweaks to the code and write up a bit for the docs.

[LilyFoote]

if request:
    try:
        namespace = request.resolver_match.namespace
    except AttributeError:
        try:
            namespace = resolve(request.path).namespace
        except Http404:
            namespace=None

if request and namespace and ':' not in viewname:
    viewname_to_try = '{namespace}:{viewname}'.format(
        namespace=namespace, viewname=viewname)

    try:
        url = django_reverse(
            viewname_to_try, args=args, kwargs=kwargs, **extra)
    except NoReverseMatch:
        url = django_reverse(viewname, args=args, kwargs=kwargs, **extra)
else:
    url = django_reverse(viewname, args=args, kwargs=kwargs, **extra)

[farridav]

Bump. any news on this? I'm trying to use the DefaultRouter with namespaced urls failing test here

This gives me a NoReverseMatch when i go the root api view...

EDIT: replaced my example code with a failing test

[carltongibson]

Hi @farridav — Thanks for the bump.

There seem to be two things going on here. The first is why you're not getting a match — which at first glance I'm not sure about because register takes a ViewSet class — I'm not sure it handles dotted paths to an individual view — but perhaps it does... — If you're routing a single view why not use the standard patterns approach? Also, are your url patterns named with the my_app: prefix? — Does it work if you drop those? (Perhaps the mailing list if it's a usage question.)

The second thing is, "What about this pull request?"

If you're going to make breaking changes to an API and want to sunset the existing version then the approach here allows you to move everything into a new folder, adjust the URLs and keep working. It's both neat — in that it's easy — and gnarly in that you're essentially doing cut and paste — but in want of another (simple) means of versioning I'm broadly in favour.

I have two questions for it:

1. Is this the right implementation of the approach? (Alternatively is there some genuine problem with it?)
2. Should it be the DRF default?

On 1: @Ian-Foote's point is well taken, but are there others? — If the approach is basically sound then I'm happy to tidy it up.

On 2: I'm just not sure. Lets say we said "No". I'd happily put the alternate reverse in a separate package — I need to use it in earnest myself soon enough — but what changes would be needed to DRF to make it quickly swappable?

@tomchristie (and anyone else) care to have a think about it and we can settle this one one way or the other?

[farridav]

Thanks for your reply @carltongibson happy to see such a prompt response 😄 i may have been a little unclear in my example.

...So I've added a failing test here so you can better see what I'm talking about, i had tried to simplify my code snippet, and in doing so i changed it to a lazy lookup of a view rather than directly referencing a viewset (which is what my actual code is doing) .. i hope the test adds some clarity, I will try to re-read your comment over the weekend and look at your second point in more detail.. thanks again

[carltongibson]

Given the success of the Django REST Framework version 3 Kickstarter, I'm going to close this pull request.

Versioning is to be addressed for V3, and in any case I'm still not convinced this is the right approach.

The patch works well enough in itself.

It's worth noting that the benefit of this approach is that there is no need to write branching logic in the view. IMO that's a big win, worth capturing.

[tomchristie]

@carltongibson 👍

[readevalprint]

I've been away but, one year later and this bug bit me again, haha! Just funded the Kickstarter, @tomchristie @carltongibson you guys rock.

[rh0dium]

@tomchristie Can you give us a timeframe when this will be supported - I know you're super busy with all of this 💰 - KIDDING. Seriously good job on the kick-starter. This just appears to be a simple fix for such a PITA problem.

[tomchristie]

Will be considering this as part of the 3.1 milestone.

[carltongibson]

Hi @rh0dium — Can I ask you to explain exactly what the problem is as you see it?

Namespaces are currently supported — if you pass a namespaced view into reverse you get the right URL — see the tests in the pull request — all bar the "Main test for #1143" already pass...

reverse('v1:view', request=request) # This already works.

The patch in #1143 makes some pretty big assumptions. For any non-namespaced view name passed in it prepends the namespace of the current request and reverses to that. As a (cough) hack for copy and paste versioning it's quite effective — I've used it myself — BUT I'm not at all convinced this should be DRF's default behaviour. The longer I looked at this patch the more I thought that "If you want namespaces use them".

So what exactly is it you need? (Just seeking understanding.)

[tomchristie]

I'm not at all convinced this should be DRF's default behaviour.

Also seconding, thanks @carltongibson. But API versioning in general is slated for consideration at 3.1, so that point still stands.