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

Add support for namespaces in APIRoot. #2354

Closed
wants to merge 1 commit into from

Conversation

rockymeza
Copy link
Contributor

This pull request supercedes #2333, #2350 and is in response to #2351.

Problem

The DefaultRouter API Root does not support namespaced URLs.

How this PR attempts to resolve it

@tomchristie recommended this

The API root view can inspect the incoming request, determine the namespace if there is one on the request object, and modify the view names accordingly if so.

So that's how this pull request attempts to resolve the issue.

Things I'm not satisfied with about this PR

The implementation of get_namespace is more complicated than it should be, but only because it has to support tests in two ways:

  1. The default Django request handlers add a resolver_match object to the request, but the tests don't provide that
  2. Some tests send requests to the APIRoot using a fake request with the url '/', which triggers a Resolver404.

Were it not for those two problems, the implementation would simply be namespace = request.resolver_match.namespace.

Thank you for your time and patience with this series of pull requests.

@tomchristie
Copy link
Member

General approach is spot-on. Some minor review and clean up to follow, but otherwise grand.

@tomchristie
Copy link
Member

Closes #2351.

@tomchristie
Copy link
Member

but the tests don't provide that

Assume that'll be due to calling the view function directly rather than using 'client.get'.

The elegant way to handle this would prob be to have a 'resolver_match' attribute on our request objects that tries accessing the HTTPRequest resolver_match and falls back to reverse on AttributeError.

@rockymeza
Copy link
Contributor Author

@tomchristie, you were right about why resolver_match wasn't showing up. It actually leads to another problem that I mentioned in #2359.

That Listless test actually makes the get_namespace code a bit more complicated. I will submit an additional patch with the simplified code after fixing the Listless test.

@rockymeza
Copy link
Contributor Author

I have added the patch as a second commit. Let me know if you have any questions.

"""
Attempt to retrieve the namespace of the current router.
"""
try:
Copy link
Member

Choose a reason for hiding this comment

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

If you create a 'get_resolver_match' function for this and put it in 'compat.py' then we can keep versions branching isolated to that module, and make it easier for us to notice that it needs cleaning up when we drop 1.4 support.

@rockymeza
Copy link
Contributor Author

@tomchristie, thank you for the good suggestions. I have added them and I squashed my commits. Let me know if there is anything else.

@tomchristie
Copy link
Member

Closed via efa5942 which is based on your work - thanks!

@rockymeza
Copy link
Contributor Author

thanks @tomchristie

@rockymeza
Copy link
Contributor Author

thanks for your help too @thedrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants