Fixed #32124 -- Added per-view opt-out for APPEND_SLASH behavior.

[carltongibson]

No description provided.

[carltongibson]

Hey @jdufresne — This is ref your #13134 for ticket-31747.

It's just the first part but the idea is to allow individual views to opt-out of APPEND_SLASH. This is for people who want the URL normalisation (so they don't want to just set APPEND_SLASH = False) but also have a sensitive_view that they want to exclude.

Phase 2 would then be to combine it with your patch in #13134, to allow an sensitive_admin_view to equally be hidden from a staff-member that wasn't meant to be allow to discover the URL, via enumerating admin URLs. (Non-staff members already handled by your patch.) For situations where that's not sufficient, I'd say folks just have to set APPEND_SLASH = False.

Hopefully this kind of thing would satisfy all the considerations. What do you think? Thanks!

[carltongibson]

With _should_append_slash I had in mind do_not_call_in_templates but I realize that's not got an underscore. So we can rename (or use a decorator, or...)

[carltongibson]

cc. @adamchainz this was the adjustment I was thinking about that we were talking about last week.

The return value for is_valid_path() would be -> Union[ResolverMatch, Literal[False]]. It's crying out to be an Optional[ResolverMatch], but that would be a bigger refactoring.

Anyhoo...

[carltongibson]

Updated with a new decorator based approach.

[adamchainz]

LGTM

[smithdc1]

Most examples I've seen would decorate this function with @wraps(view_func) then return wrapped_view?

[carltongibson]

🤷‍♀️ You say potato, I say potato (Please supply accents.) 😀

[ngnpope]

I'm not sure I really agree with this. The whole point of decorators is that they change the behaviour of the existing function. Wrapping the function here with another doesn't have the "side effect" of assigning a simple attribute to that original function, but then the original function is (strictly speaking) no longer in scope having been replaced by the wrapped function. Therefore it is unlikely anyone would use it anyway.

This is really just adding an extra level of indirection by wrapping in an additional function which then forces us to use functools.wraps() to fix things up. IMO the following is straightforward and achieves the same goal:

def no_append_slash(func):
    func.should_append_slash = False
    return func

[ngnpope]

Note that there are similar decorators that add attributes to functions in the works here and here that are not using this approach.

[felixxm]

Note that there are decorators that are using the same approach, e.g. csrf_exempt() 😉

[ngnpope]

Yes, but I have to ask why? I get that there is a side effect, but how much of a problem is this? I can potentially see the problem if you do the following:

def view(request):
    ...

wrapped_view = no_append_slash(wrapped_view)

# Now both ``view`` and ``wrapped_view`` have ``should_append_slash`` set which
# means using ``view`` might not do what you expect.

But typically you're going to do the following:

@no_append_slash
def view(request):
    ...

Which would be no different to assigning the attribute directly after the function is created if not using a wrapper function:

def view(request):
    ...
view.should_append_slash = False

I was concerned that there may be an issue if you wanted to do something like the following, but it turns out that functools.wraps() also copies attributes other than __name__ and __doc__ so this works fine:

@csrf_exempt
@no_append_slash
def view(request):
    ...

But it also effectively translates to having to call three functions, the outer two which have no benefit other than to have copies of all of the attributes of the functions within them. This has some call overhead and, if there are many decorated functions, excess memory use to store all these additional function objects and extra attributes as they bubble up.

[felixxm]

IMO supporting a typical usage is not enough in this case, because no_append_slash should be used to avoid enumerations, so it's security-related, like csrf_exempt. It must be bulletproof, without any side effects.

[adamchainz]

I had a squint at this too, but didn't comment. But I agree with Nick - there isn't much of a reason to have an intermediate function, especially since decorators in Django aren't consistent. Mostly I see this as adding overhead and complicating debugging with an extra layer in stack traces.

[carltongibson]

So here's the question, would you remove the same usage from csrf_exempt? (Would you take that risk?)

[carltongibson]

I think the decorator strategy is orthogonal to this issue. Maybe there's a case for adjusting styles, or being consistent, but if so, we should address separately, where we can give it due consideration.

2¢: Sometimes, sure, you don't use wraps, but I don't see it's addition as an issue. (TBH don't really buy any of the call overhead, memory use, or stack trace points…)

[felixxm]

@carltongibson Thanks 👍

[felixxm]

Note that there are decorators that are using the same approach, e.g. csrf_exempt() 😉