Re: URL namespaces

[Tai Lee]

I still think that even though `get_absolute_url` might be disliked these 
days for various reasons, it is very common in the wild and is a perfect 
example of a legitimate need to be able to reverse URLs in code that 
executes in response to a request, but that doesn't have the request passed 
to it as an argument.

So I'd still like to consider the option of adding a default `current_app` 
hint to thread local storage (which has a life cycle of one request), and 
for `reverse()` and therefore also `{% url %}` to use that hint as a 
default. It could still be overridden by explicitly passing `current_app` 
to `reverse()` (and also `{% url %}`).

This could be done with a middleware class, and in fact a quick search 
reveals there are many django snippets and blog posts out where users have 
re-implemented a `ThreadLocalRequestMiddleware` or similar. But I would 
prefer to see it done before middleware is applied to make it non-optional, 
since the value will be read in `reverse()` which can and is often called 
completely outside the request/response cycle.

For any such code that executes outside the request/response cycle, I think 
we could provide a context manager that sets the `current_app` default hint 
on `__enter__` and restores it on `__exit__`.

Django appears to use thread locals already to set the current active 
language. It's been said that is more of a wart than a design pattern to 
follow, but clearly it can be used safely in Django, and just like the 
current active language, having apps that are able to resolve their own 
URLs properly and consistently is an important feature.

Here's a relevant conversation from a couple years 
back: https://groups.google.com/forum/#!topic/django-developers/VVAZMWi76nA

Cheers.
Tai.


On Monday, June 8, 2015 at 5:29:56 AM UTC+10, Marten Kenbeek wrote:
>
> So 2) has been fixed (#24904 and #24906), and I got a PR for 1) (
> https://github.com/django/django/pull/4724). 
>
> About #24127: I see a few options here:
>
>
>1. A context processor or similar that sets `current_app` if it isn't 
>set. By default `current_app` doesn't exist, so there's a good distinction 
>between not set and set to `None`. This would be 100% backwards 
> compatible, 
>but IMO it's not the best solution in the long term. 
>2. Provide a new flag to {% url %} to reverse with current_app set to 
>request.resolver_match.namespace. This feels slightly less awkward than 
> the 
>current method recommended by the docs.
>3. Deprecating a non-existing current_app to force an explicit choice 
>between the old and new behaviour, then add the new behaviour after the 
>deprecation period. This would be coupled with option 1 for ease-of-use. 
>4. A setting? A setting! Yay, another setting...
>5. Document the slight backwards-incompatible change? And provide an 
>easy way to retain the old behaviour, of course. 1 through 4 are all far 
>from ideal, and I don't think there's an easy, backwards-compatible fix 
>here. 
>
> Thoughts?
>
> Op dinsdag 2 juni 2015 14:32:42 UTC+2 schreef Marten Kenbeek:
>>
>> The admin proves problematic. Looking at the uses of `AdminSite.name`, 
>> it's not easily replaced. There are quite a few places that use the name to 
>> reverse urls, but don't have access to the request and the current 
>> namespace. I think the best solution for now is to document that you should 
>> pass `admin.site.urls` directly to `url()`. `include()` does a bunch of 
>> stuff, but none of it is needed in the case of the admin urls. This allows 
>> the new `include()` API to be as intended, but it doesn't put an 
>> unnecessary burden of providing the "correct" namespace for an admin 
>> instance on the end-developer. 
>>
>> In time I'd like to see a method on the AdminSite that returns an actual 
>> resolver object, using the new API. The default URLconf template would 
>> simply look something like this:
>>
>> urlpatterns = [
>> admin.site.get_resolver('^admin/'),  # or 
>> get_resolver(RegexPattern('^admin/')) to be more explicit
>> ]
>>
>> This would solve the namespace issue for the admin, but it would still 
>> allow for only obvious way to set the instance namespace, and keep the 
>> `include()` API clean.
>>
>> Op zaterdag 30 mei 2015 14:28:22 UTC+2 schreef Tai Lee:
>>>
>>> Right. So if I understand correctly, you can avoid problems even when 
>>> installing two different apps into the same app namespace as long as you 
>>> *always* supply `current_app` when reversing URLs? In which case, I don't 
>>> think we need to worry about being able to change the app namespace to 
>>> avoid conflicts (maybe we should even disallow it), and instead just make 
>>> it easier to *always* supply `current_app`, which brings me to my next 
>>> point.
>>>
>>
>> Not providing an option in the API itself is the best we can do to 
>> disallow it. But yes, always setting the current_app would avoid a lot of 
>> problems. https://code.djangopr

Re: URL namespaces

[Marten Kenbeek]

So 2) has been fixed (#24904 and #24906), and I got a PR for 1) 
(https://github.com/django/django/pull/4724). 

About #24127: I see a few options here:


   1. A context processor or similar that sets `current_app` if it isn't 
   set. By default `current_app` doesn't exist, so there's a good distinction 
   between not set and set to `None`. This would be 100% backwards compatible, 
   but IMO it's not the best solution in the long term. 
   2. Provide a new flag to {% url %} to reverse with current_app set to 
   request.resolver_match.namespace. This feels slightly less awkward than the 
   current method recommended by the docs.
   3. Deprecating a non-existing current_app to force an explicit choice 
   between the old and new behaviour, then add the new behaviour after the 
   deprecation period. This would be coupled with option 1 for ease-of-use. 
   4. A setting? A setting! Yay, another setting...
   5. Document the slight backwards-incompatible change? And provide an 
   easy way to retain the old behaviour, of course. 1 through 4 are all far 
   from ideal, and I don't think there's an easy, backwards-compatible fix 
   here. 

Thoughts?

Op dinsdag 2 juni 2015 14:32:42 UTC+2 schreef Marten Kenbeek:
>
> The admin proves problematic. Looking at the uses of `AdminSite.name`, 
> it's not easily replaced. There are quite a few places that use the name to 
> reverse urls, but don't have access to the request and the current 
> namespace. I think the best solution for now is to document that you should 
> pass `admin.site.urls` directly to `url()`. `include()` does a bunch of 
> stuff, but none of it is needed in the case of the admin urls. This allows 
> the new `include()` API to be as intended, but it doesn't put an 
> unnecessary burden of providing the "correct" namespace for an admin 
> instance on the end-developer. 
>
> In time I'd like to see a method on the AdminSite that returns an actual 
> resolver object, using the new API. The default URLconf template would 
> simply look something like this:
>
> urlpatterns = [
> admin.site.get_resolver('^admin/'),  # or 
> get_resolver(RegexPattern('^admin/')) to be more explicit
> ]
>
> This would solve the namespace issue for the admin, but it would still 
> allow for only obvious way to set the instance namespace, and keep the 
> `include()` API clean.
>
> Op zaterdag 30 mei 2015 14:28:22 UTC+2 schreef Tai Lee:
>>
>> Right. So if I understand correctly, you can avoid problems even when 
>> installing two different apps into the same app namespace as long as you 
>> *always* supply `current_app` when reversing URLs? In which case, I don't 
>> think we need to worry about being able to change the app namespace to 
>> avoid conflicts (maybe we should even disallow it), and instead just make 
>> it easier to *always* supply `current_app`, which brings me to my next 
>> point.
>>
>
> Not providing an option in the API itself is the best we can do to 
> disallow it. But yes, always setting the current_app would avoid a lot of 
> problems. https://code.djangoproject.com/ticket/24127 proposes just that. 
>
> I'd take this a step further. In this comment -- 
>> https://code.djangoproject.com/ticket/21927#comment:9 
>> 
>>  
>> -- I made a suggestion that Django set a `current_app` hint in thread local 
>> storage with a middleware class or even before middleware runs, and then 
>> have `reverse()` and `{% url %}` fallback to that default hint ONLY if no 
>> current app is explicitly provided via the current mechanisms, so it should 
>> be a relatively safe change.
>>
>> This should make it much more convenient and possible to reverse URLs 
>> correctly regardless of conflicting app namespaces, even in code that 
>> doesn't have access to the request object. For example, model methods that 
>> are executed in templates and do not have access to a `request` or 
>> `RequestContext` object. As far as I know, 1 thread = 1 request, so using 
>> thread local storage should be safe to provide such a hint for any code 
>> that executes in response to a request. For management commands, it would 
>> still need to be supplied explicitly. What do you think?
>>
>> Cheers.
>> Tai.
>>
>
> I don't think it's necessary to introduce another global state. At the 
> moment there are a few places in the admin that use the name but don't have 
> access to the request, this works fine as it is, and if need be these can 
> be reworked so that the current app is passed down to these methods too. 
> The other place where this would help is `get_absolute_url()`. This only 
> adds to the list of why I don't like that method, and I personally don't 
> find it a compelling argument for introducing a global state for the 
> current app.
>
> I think setting the current_app on request by default is a better solution 
> that works in most 

Re: Making management forms for formsets optional

[Shai Berger]

Semi-devil's-advocate suggestion: Replace the management form with a 
management field, a hidden field with
 
name = "__manage__%s" % (formsetname) 

and 

value="x"

Then instead of taking the number of forms from a field on a management form, 
we can just take the accumulated length of the management field (which would be 
returned as an array). This also offers a better solution to the checkboxes-
only-form problem, I think.

Do we use the management form for anything other than counting forms in the 
formset?

Shai.

On Saturday 06 June 2015 20:11:03 Marc Tamlyn wrote:
> I believe the complaint is mostly that an invalid formset will raise a 500
> whereas an invalid form (should) never. There was a patch to display an
> error if not. My complaint is that this error can only occur if the dev has
> rendered the form by hand missing the management form. It's likely they
> would also miss the non form error output this way and so the end result is
> we display back empty forms to the user with no useful error event though
> they filled in values. This is hard to debug and bad ux - a 500 is better.
> 
> Equally, every time a security crawler posts an empty body to a page
> triggering a 500 is irritating noise. We don't report invalid forms as 500s
> either...
> 
> This brought up the question of whether the management form is needed at
> all.
> 
> Unrelated arguments: It also streamlines testing user formsets massively,
> and significantly reduces the complexity of js to add more forms.
> 
> Marc
> 
> On 6 Jun 2015 17:00, "Florian Apolloner"  wrote:
> > What about instead of trying to guess the forms from the input, just fix
> > the one condition which causes the error and return 0 as totalformcount +
> > an error marker to reraise the error in clean of the modelform?
> > 
> > On Friday, June 5, 2015 at 11:29:21 AM UTC+1, Patryk Zawadzki wrote:
> >> Hi folks,
> >> 
> >> I've talked to Marc about fixing the case where a formset will raise
> >> an uncaught ValidationError when instantiated with a missing or broken
> >> management form. This has caused us great pain when dealing with
> >> vulnerability scanners that tend to POST random crap to each endpoint
> >> they find as it means formsets—unlike forms—are not safe to
> >> instantiate with untrusted data.
> >> 
> >> Now wrapping each and every formset instance in a try/except block and
> >> having to manually handle the fallback case is not fun. But as Marc
> >> pointed out, forgetting a management form is one of the most common
> >> pitfalls people run into when dealing with formsets.
> >> 
> >> Therefore I propose making the management form optional. The only case
> >> that requires an explicit TOTAL_FORMS variable are forms that consist
> >> of nothing but checkboxes (as unchecked checkboxes are not included in
> >> POST data). In other cases (including all ModelFormSets as they always
> >> contain a hidden PK field) we can deduct the number of submitted forms
> >> from the data itself.
> >> 
> >> Ideally I would get rid of the management form entirely and document
> >> that a form that is nothing but checkboxes is an unsupported case and
> >> that a workaround would be to include an extra hidden field. Honza may
> >> or may not kill me for suggesting that.
> >> 
> >> For now, I would like to make the formset optional and document the
> >> cases where you need to include it. If included, it would take
> >> precedence to keep the existing deployments working.
> >> 
> >> Thoughts?
> >> 
> >> --
> >> Patryk Zawadzki
> >> I solve problems.
> >> 
> >  --
> > 
> > You received this message because you are subscribed to the Google Groups
> > "Django developers (Contributions to Django itself)" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to django-developers+unsubscr...@googlegroups.com.
> > To post to this group, send email to django-developers@googlegroups.com.
> > Visit this group at http://groups.google.com/group/django-developers.
> > To view this discussion on the web visit
> > https://groups.google.com/d/msgid/django-developers/de9e5a03-7f14-42e8-bf
> > 76-9405440ecf94%40googlegroups.com
> >  > bf76-9405440ecf94%40googlegroups.com?utm_medium=email&utm_source=footer>
> > .
> > For more options, visit https://groups.google.com/d/optout.