CRUDView does not work with namespaced urlpatterns

LucidDan commented 1 year ago

Perhaps this isn't very high priority for now and could just be documented as a known limitation, and I know you're potentially going to make API changes anyway. But I thought it was worth mentioning.

A simple example here: https://github.com/LucidDan/neapolitan-example - the main branch is a working copy of the tutorial project, slightly altered to use a projects.urls module.

The branch namespaces breaks due to the reverse urls not being valid, because of the addition of app_name = "projects" in the projects.urls module:

Internal Server Error: /project/
Traceback (most recent call last):
  File "/Users/dans/Library/Application Support/pdm/venvs/neapolitan-test-O2zzKXMP-3.11/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dans/Library/Application Support/pdm/venvs/neapolitan-test-O2zzKXMP-3.11/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dans/Library/Application Support/pdm/venvs/neapolitan-test-O2zzKXMP-3.11/lib/python3.11/site-packages/neapolitan/views.py", line 426, in view
    return self.dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dans/Library/Application Support/pdm/venvs/neapolitan-test-O2zzKXMP-3.11/lib/python3.11/site-packages/django/views/generic/base.py", line 143, in dispatch
    return handler(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dans/Library/Application Support/pdm/venvs/neapolitan-test-O2zzKXMP-3.11/lib/python3.11/site-packages/neapolitan/views.py", line 312, in list
    context = self.get_context_data(
              ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dans/Library/Application Support/pdm/venvs/neapolitan-test-O2zzKXMP-3.11/lib/python3.11/site-packages/neapolitan/views.py", line 250, in get_context_data
    kwargs["create_view_url"] = reverse(f"{self.model._meta.model_name}-create")
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dans/Library/Application Support/pdm/venvs/neapolitan-test-O2zzKXMP-3.11/lib/python3.11/site-packages/django/urls/base.py", line 88, in reverse
    return resolver._reverse_with_prefix(view, prefix, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dans/Library/Application Support/pdm/venvs/neapolitan-test-O2zzKXMP-3.11/lib/python3.11/site-packages/django/urls/resolvers.py", line 828, in _reverse_with_prefix
    raise NoReverseMatch(msg)
django.urls.exceptions.NoReverseMatch: Reverse for 'project-create' not found. 'project-create' is not a valid view function or pattern name.

I really like url namespaces and find them extremely useful in large projects, but even the Django implementation itself is a bit magical and complicated to use...I'd totally understand if you decided its better to just say "yeah this doesn't work, make sure you do get_urls() from your main project urls.py" 🤷

carltongibson commented 1 year ago

Yeah...

I basically never use namespaces, since they're a bit of a pain, and real life URL name collisions are quite rare.

Maybe get_urls() could take a namespace. But how would that get passed down to the various places where the url name for reverse is needed? (Would that be worth the price of admission here?)

LucidDan commented 1 year ago

Yeah agreed. I like them, but every time I use them I find myself thinking "you know, I could just name all of these urls explicitly with a prefix instead of a namespace".

In any case, I had noodled on how to support it - I can see both of these being possibly good ways to handle it:

urlpatterns += get_urls(namespace="app_name")

as well as:

class ProjectView(CRUDView):
    namespace = "app_name"
    # ...

I suppose though, it does make a little more sense in get_urls(), because generating the urls is when you'd "decide" the url namespace...but the problem with that is since get_urls() is a classmethod, it can't set the namespace for the CRUDView, which needs to have the namespace stored so it knows how to do its reverse() calls correctly.

It seems to me like maybe the latter approach with the attribute in the CRUDView is the only way that would work in the current API?

carltongibson commented 1 year ago

It's the {% url ... %} usages too…

carltongibson commented 1 year ago

More or less, I think I'd lean to not namespacing unless a proof-of-concept can demonstrate that adding it is not going to cause issues.

Currently we're able to programmatically construct the right view names, like so:

reverse(f"{model_name}-detail", kwargs={"pk": object.pk})

And similar.

We'll need some complexity of different URL patterns maybe, but everywhere and always needing to handle an optional namespace there seems like quite a lot of grief.

Happy to be shown otherwise, but it's not something I'm going to put thought to myself at this stage.

LucidDan commented 1 year ago

Makes sense to me!

carltongibson commented 1 year ago

For reference, there was good discussion about this on fosstodon

https://fosstodon.org/@carlton/110503156769347222

I'm not adverse to looking at something here if it's clean, but I'm not personally going to work on it, so closing for that reason.

carltongibson commented 1 week ago

Reopening this, as it might be feasible now that URL name generation AND reversing lives within the view.

Still not 100% convinced but it's gone from No to Maybe.

carltongibson / neapolitan

CRUDView does not work with namespaced urlpatterns #16