is_valid as property

[Sven R. Kunze]

Good afternoon,

I would like to follow up on https://groups.google.com/forum/#!searchin/django-developers/is_authenticated$20as$20property%7Csort:relevance/django-developers/7k6Z8JxKH5Q/RoNKv66xcAMJ with regards to Form.is_valid.

It's is not as security relevant as is_authenticated but the remaining arguments of "is_authenticated as property" still hold. I also suggest the usage of CallableBool as a good temporary backwards-compatibility measure.

Best

Sven

[Tim Graham]

I don't think the same argument applies because no one (I hope) is calling form.is_valid() in templates.

[Sven R. Kunze]

Am Donnerstag, 29. September 2016 15:52:00 UTC+2 schrieb Tim Graham:

I don't think the same argument applies because no one (I hope) is calling form.is_valid() in templates.

Could you elaborate why this only plays a role with templates?

So, as it turns out I ran grep on our code and easily found a place where the programmer missed the "()" after is_valid. Regarding templates, I found no occurrences of "is_valid" nor "has_changed" in our templates so far but this might have other reasons as we don't have a very long history of templates until recently. Additionally, what's so wrong about using either function in templates?

[Tim Graham]

#1. Regarding templates, one of the arguments for the previous change was:

Django 1.8 worsens the situation significantly:

    {% if request.user.is_authenticated %}

does the right thing in a Django template but is a security vulnerability in a Jinja2 template!

#2. There was an inconsistency with the is_staff, is_active, is_superuser attributes and is_anonymous, is_authenticated as methods. I'm not sure what the inconsistency is with forms. Yes, there's an Form.is_bound property and an is_multipart() method but I don't see a need to convert all is_*() methods to properties.

In my mind, the security ramifications were the main reason for the previous change, and I don't see those concerns here. Changing Form.is_valid() to a property seems like it would cause much more disruption across the Django ecosystem than the gain would be worth.

[Sven R. Kunze]

Am Donnerstag, 29. September 2016 18:55:37 UTC+2 schrieb Tim Graham:

#1. Regarding templates, one of the arguments for the previous change was:

Django 1.8 worsens the situation significantly:

    {% if request.user.is_authenticated %}

does the right thing in a Django template but is a security vulnerability in a Jinja2 template!

Ah, yes. I for one think that this is just a minor issue as changing the underlying template engine is not done on a regular basis. But that might just be me.

If it contributes as an argument, why not. I don't see anything wrong with using is_valid/has_changed in templates especially when we need to display field errors.

 

#2. There was an inconsistency with the is_staff, is_active, is_superuser attributes and is_anonymous, is_authenticated as methods. I'm not sure what the inconsistency is with forms. Yes, there's an Form.is_bound property and an is_multipart() method but I don't see a need to convert all is_*() methods to properties.

In my mind, the security ramifications were the main reason for the previous change, and I don't see those concerns here. Changing Form.is_valid() to a property seems like it would cause much more disruption across the Django ecosystem than the gain would be worth.

It's always the combination of several issues. Here are mine:

#1 Inconsistency

For newbies that's the hardest part; especially when now compared to is_authenticated and other boolean attributes/properties even within Django.

#2 Wasted Effort

Also on a regular basis; which is quite problematic for commercial projects. Focusing on the topic at hand is more important than such technical details.

The mentioned dev who missed the "()" is one of our most experienced devs and that mentioned piece of code is only 3 years old.

#3 "Errors should never pass silently."

Which they do if you write:

if form.is_valid:

    # will always be executed

    # as it is always true

#4 Django devs are not only Django devs

They are Python devs as well and usually Django as a single lib is not enough. Especially Python devs expect things to work the same also across most libs such as boolean attributes/properties.

You might not find them worth enough to warrant the trouble. I for one do, hence the post. But it's not me to decide. :)

Some related thought: relating Django's past lifetime with its projected future lifetime which still lies ahead, Django is still in its infancy. So, I guess that any change no matter how "disruptive" for the sake of consistency will eventually pay out. Latter generations will be grateful not to distinguish between situations where they need "()" and where they don't.

[Aymeric Augustin]

Hello,

On 29 Sep 2016, at 19:57, Sven R. Kunze <srk...@gmail.com> wrote:

#3 "Errors should never pass silently."

Which they do if you write:

if form.is_valid:

    # will always be executed

    # as it is always true

This is perhaps the strongest argument in this discussion but I’m not convinced it’s strong enough to make the change.

It’s weaker than the inconsistency that appeared between `{% if request.user.is_authenticated %}` in Django templates and `{% if request.user.is_authenticated() %}` in Jinja2 templates when Django started supporting both natively. The root cause of that inconsistency was Django’s auto-calling of callable in templates. This factor doesn’t exist here.

Writing `if some_callable:` instead of `if some_callable()` will cause the issue described here for any callable whose result makes sense in a boolean context. It’s always possible to build a security vulnerabilities with this behavior, by putting something security sensitive in the `if` or `else` block.

Given that virtually anything can be evaluated in a boolean context in Python and in other dynamic languages such as JavaScript, I don’t know how to draw a line between what should be a property and what should be a method. For example, I don’t think we want to make QuerySet.exists a magic CallableBoolean, do we?

Generally speaking, properties are expected to be cheap and methods can be expensive. In my opinion, for lack of a better guideline, we should stick to this one. `is_valid()` falls into the expensive category, and for this reason it should remain a method.

-- 

Aymeric.

[Sjoerd Job Postmus]

Replacing functions with properties will create a lot of churn in the people using the mentioned functions.

Another approach might be to wrap these functions in a class with `__call__` and `__bool__` both calling the underlying function. Or `__bool__` raising an error? [1]

I myself am a fan of errors here.

[1] and __nonzero__.

--
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-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/e36d0f0a-fee8-4340-9d8a-24402dd55b76%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Aymeric Augustin]

On 29 Sep 2016, at 20:26, Sjoerd Job Postmus <sjoe...@sjec.nl> wrote:

> Replacing functions with properties will create a lot of churn in the people using the mentioned functions.
>
> Another approach might be to wrap these functions in a class with `__call__` and `__bool__` both calling the underlying function. Or `__bool__` raising an error? [1]

That’s how the change was handled for is_authenticated/anonymous. However (IIRC) the plan is to only support property access after the deprecation completes.

I’m afraid that supporting both styles in the long run would be confusing.

--
Aymeric.

[Sven R. Kunze]

Hi Aymeric,

thanks for your ideas.

Am Donnerstag, 29. September 2016 20:26:54 UTC+2 schrieb Aymeric Augustin:

Hello,

On 29 Sep 2016, at 19:57, Sven R. Kunze <srk...@gmail.com> wrote:

#3 "Errors should never pass silently."

Which they do if you write:

if form.is_valid:

    # will always be executed

    # as it is always true

This is perhaps the strongest argument in this discussion but I’m not convinced it’s strong enough to make the change.

It’s weaker than the inconsistency that appeared between `{% if request.user.is_authenticated %}` in Django templates and `{% if request.user.is_authenticated() %}` in Jinja2 templates when Django started supporting both natively. The root cause of that inconsistency was Django’s auto-calling of callable in templates. This factor doesn’t exist here.

That could depend on the perspective which argument is stronger or weaker. But independently, I still miss the point why {% if form.is_valid %} cannot be a problem in Jinja2?

 

Writing `if some_callable:` instead of `if some_callable()` will cause the issue described here for any callable whose result makes sense in a boolean context. It’s always possible to build a security vulnerabilities with this behavior, by putting something security sensitive in the `if` or `else` block.

Given that virtually anything can be evaluated in a boolean context in Python and in other dynamic languages such as JavaScript, I don’t know how to draw a line between what should be a property and what should be a method. For example, I don’t think we want to make QuerySet.exists a magic CallableBoolean, do we?

Now, that you mentioned it....  *seeking the "new topic" button* ;)

Basically, the same arguments would apply there as well.

Generally speaking, properties are expected to be cheap and methods can be expensive. In my opinion, for lack of a better guideline, we should stick to this one. `is_valid()` falls into the expensive category, and for this reason it should remain a method.

Oh, I've never heard of this guideline. I just thought that properties should be used to reduce visual noise (such as parentheses and "get_" or "set_" prefixes.) or to replace a static attribute with a dynamic one. Cheap vs. expensive never played a role so far (at least where I worked). If that's relevant, we use "cached_property".

Regards,

Sven

[Sjoerd Job Postmus]

Thinking of an alternative route:

It seems to me that the default `method.__bool__` is undesirable in Jinja2 templates. I do not know Jinja2 well enough, but maybe they could benefit from a patch where `if`-statements give a warning/error when the expression is a callable (with the default `FunctionType.__bool__`?

This would solve the issue not just for the methods you mention, but more in general.

[Or maybe Python itself should have that warning/error?]

Does that make sense?

--

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-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/ecd30665-f698-4199-8fc3-d75ca66557d1%40googlegroups.com.

[Aymeric Augustin]

On 29 Sep 2016, at 21:09, Sven R. Kunze <srk...@gmail.com> wrote:

But independently, I still miss the point why {% if form.is_valid %} cannot be a problem in Jinja2?

If you’re writing that kind code in templates, you have much, much bigger problems than what we’re discussing here!

-- 

Aymeric.

[Shai Berger]

Hi Sven,

I'm completely with Aymeric on this one. The parentheses are not just noise,
they're a strong hint about the price of computation expected, and also about
the expectation of exceptions popping up. Properties hiding complex code are
downright misleading in my book, and "is_valid()", which may call user code,
certainly qualifies as complex.

Have fun,
Shai.

[Michal Petrucha]

Hi everybody,

I also agree with Aymeric and Shai on this, which is why I want to
bring up the elephant in the room: Form.errors. This is a property
which triggers full validation of the form on first access. What are
the chances of replacing Form.errors with a method?

Or maybe making it raise an error if it's accessed before the form has
been validated by calling an explicit method?

Cheers,

Michal

[Sven R. Kunze]

I took a sample of 4 of my colleagues and asked them what the problem with expressions like {% if form.is_valid %} is. They have no idea. They said "it might not make sense in some cases but when it makes sense, it doesn't look very terrible me".

So, what's the real problem here?

Cheers,

Sven

PS: as said, we don't use it yet but we shouldn't it should be noted somewhere as it

[Sven R. Kunze]

Am Donnerstag, 29. September 2016 21:25:44 UTC+2 schrieb Sjoerd Job Postmus:

Thinking of an alternative route:

It seems to me that the default `method.__bool__` is undesirable in Jinja2 templates. I do not know Jinja2 well enough, but maybe they could benefit from a patch where `if`-statements give a warning/error when the expression is a callable (with the default `FunctionType.__bool__`?

This would solve the issue not just for the methods you mention, but more in general.

[Or maybe Python itself should have that warning/error?]

Does that make sense?

I don't know if it is possible for Python to warn about this. I create an issue on python-ideas.

But your idea seems interesting (at least from a development point of view).

[Aymeric Augustin]

Hello Sven,

On 11 Oct 2016, at 14:07, Sven R. Kunze <srk...@gmail.com> wrote:

I took a sample of 4 of my colleagues and asked them what the problem with expressions like {% if form.is_valid %} is. They have no idea. They said "it might not make sense in some cases but when it makes sense, it doesn't look very terrible me”.

Can you show a non-contrived example, predating this discussion, of a situation where this pattern make sense?

So, what's the real problem here?

The real problem is that templates aren’t the correct place to implement business logic such as form validation.

-- 

Aymeric.

[Sven R. Kunze]

https://mail.python.org/pipermail/python-ideas/2016-October/042782.html 

[Sven R. Kunze]

Am Dienstag, 11. Oktober 2016 14:16:50 UTC+2 schrieb Aymeric Augustin:

Hello Sven,

On 11 Oct 2016, at 14:07, Sven R. Kunze <srk...@gmail.com> wrote:

I took a sample of 4 of my colleagues and asked them what the problem with expressions like {% if form.is_valid %} is. They have no idea. They said "it might not make sense in some cases but when it makes sense, it doesn't look very terrible me”.

Can you show a non-contrived example, predating this discussion, of a situation where this pattern make sense?

For displaying some error indicators, changing backgrounds to signal colors (like red), etc. I don't know what counts as a non-contrived example to you.

 

So, what's the real problem here?

The real problem is that templates aren’t the correct place to implement business logic such as form validation.

I did never say "let's do business logic in templates". AFAIK, displaying errors (or preparing for it HTML-structure-wise) requires knowing whether a form is invalid or not.

When there's some logic to do, that's fine. But when some developer requires a simple boolean flag to know whether a form is valid or not, why shouldn't he use form.is_valid? That looks to me like making things more complicated than necessary.

Also, think about GET forms + AJAX which do not POST after redirect at all (e.g. search form + result table in separate AJAX containers). There you will re-use the same template for success and failure.

[ludovic coues]

If I remember correctly, form.is_valid do the actual form validation
and return false in case of error.

In your template, you can use {% if form.errors %} to check if the
form has any error. It's a dict with field as key and list of errors
as value.

> --
> 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-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/django-developers/4ffb3d8c-43fa-4bc1-9762-eae79334af28%40googlegroups.com.

>
> For more options, visit https://groups.google.com/d/optout.

--

Cordialement, Coues Ludovic
+336 148 743 42

[Sven R. Kunze]

Am Dienstag, 11. Oktober 2016 15:03:21 UTC+2 schrieb ludovic coues:

If I remember correctly, form.is_valid do the actual form validation
and return false in case of error.

I don't know what counts as "actual form validation" but form.is_valid is "just" a wrapper on form.errors which is a property. 

cf. https://github.com/django/django/blob/3c447b108ac70757001171f7a4791f493880bf5b/django/forms/forms.py#L164

[ludovic coues]

First time you access form.errors, it will call form.full_clean.

But after looking at the code and not simply reading the doc, I agree
that is_valid should be a property. After the first call to is_valid,
the value won't change, even if a field is set to a non-valid value.

> --
> 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-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/django-developers/bc3d0c3a-024a-4149-afde-6f34c99f53d2%40googlegroups.com.

[Michal Petrucha]

On Tue, Oct 11, 2016 at 04:28:40PM +0200, ludovic coues wrote:
> First time you access form.errors, it will call form.full_clean.
>
> But after looking at the code and not simply reading the doc, I agree
> that is_valid should be a property. After the first call to is_valid,
> the value won't change, even if a field is set to a non-valid value.

Personally, I'd argue that the opposite should be true; is_valid
should remain a method, and instead, we should change errors to no
longer trigger full validation on first access. One possibility which
would make sense to me would be that form.is_valid() could be the only
supported way to trigger full validation, and form.errors could raise
an error if it's accessed before calling is_valid first. The reasons I
see for this change have been articulated pretty clearly by several
people in this thread already...

Of course, this would have to go through a full deprecation period,
since the current behavior of form.errors is documented [0]...

Cheers,

Michal


[0]: https://docs.djangoproject.com/en/1.10/ref/forms/api/#django.forms.Form.errors

[Aymeric Augustin]

On 11 Oct 2016, at 14:52, Sven R. Kunze <srk...@gmail.com> wrote:

I did never say "let's do business logic in templates". AFAIK, displaying errors (or preparing for it HTML-structure-wise) requires knowing whether a form is invalid or not.

The API for displaying errors is form.errors.

If the documentation implies otherwise, please accept my apologies!

-- 

Aymeric.

[ludovic coues]

If we choose to move the call to full_clean from errors to is_valid
with a deprecation period, maybe we could rename is_valid to validate.
is_valid could be kept as a property, raising error if called before
validate like error would be after the change.

2016-10-11 17:50 GMT+02:00 Aymeric Augustin
<aymeric....@polytechnique.org>:

> --
> 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-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/django-developers/810B668F-D4D3-4C39-90CF-7357DF029921%40polytechnique.org.

[Tim Graham]

I don't think requiring every Django project to rewrite their views from:

if form.is_valid():

to

form.validate()
if form.is_valid:

is practical.

About changing form.errors not to validate the form, I'm not sure there's much benefit there either. When writing tests, I often write: self.assertEqual(form.errors, {...}) without calling is_valid().

[Sven R. Kunze]

No problem. I now did read the documentation (https://docs.djangoproject.com/es/1.10/ref/forms/api/) to be 100% sure.

It says: "The primary task of a Form object is to validate data. With a bound Form instance, call the is_valid() method to run validation and return a boolean designating whether the data was valid:" and furthermore "You can access errors without having to call is_valid() first. The form’s data will be validated the first time either you call is_valid() or access errors."

So, the docs are actually pretty clear about what will happen (how to code works), but not so much about the intended usage.

I for one, however, think that this is also not necessary as the names of those properties are pretty clear in their meaning.

[Sven R. Kunze]

I agree with Tim. I don't think it's overly necessary.

FWIW, form.validate actually is form.full_clean.

[Aymeric Augustin]

Hello Sven,

On 12 Oct 2016, at 12:21, Sven R. Kunze <srk...@gmail.com> wrote:

Am Dienstag, 11. Oktober 2016 17:51:20 UTC+2 schrieb Aymeric Augustin:

The API for displaying errors is form.errors.

If the documentation implies otherwise, please accept my apologies!

No problem. I now did read the documentation (https://docs.djangoproject.com/es/1.10/ref/forms/api/) to be 100% sure.

It says: "The primary task of a Form object is to validate data. With a bound Form instance, call the is_valid() method to run validation and return a boolean designating whether the data was valid:" and furthermore "You can access errors without having to call is_valid() first. The form’s data will be validated the first time either you call is_valid() or access errors."

So, the docs are actually pretty clear about what will happen (how to code works), but not so much about the intended usage.

The intended usage is shown here: https://docs.djangoproject.com/en/1.10/topics/forms/#the-view. The document goes on to describe various ways to render errors in template.

Sorry, I overreacted because I failed to imagine other use cases. I withdraw my objections to using is_valid() in templates. While it’s (likely) not intended, it isn’t unrealistic either.

I’m still -0 on changing is_valid to be a property because I still think it’s less appropriate (validation can be expensive) and the benefits are more limited than they were for is_authenticated / is_anonymous.

-- 

Aymeric.