Two phase cleaning of models/forms

[Thomas Güttler]

Up to now the validation/cleaning of a attribute can not access the
value of an other attribute.

The work around is to override the clean() method of the form/model.

A team mate and I talked about it, and we asked ourself, if
a two phase cleaning would help here.

Our idea:

- The first clean phase works like the current implementation

- The second clean phase is optional and does nothing except the user implements it.

- In the second clean phase your are allowed to peek into
the results of the first clean phase.
The clean method for the second phase would need to get the other values passed in.


I would like to have this on model level, not only for forms.

Example:

class Issue(models.Model):
state=models.CharField(choices=[('in_progress', 'In progress'), ('wait_until_due_date', 'Wait until due date')])
due_date=models.DateField(null=True)


We want to implement this constraint:

- due date must be set, if the state is "wait_until_due_date".
- due date must not be set, if state is different from "wait_until_due_date".

We know that we can use the clean() method of the model, but a field based solution would
be more preferred.

class Issue(models.Model):
state = models.CharField(choices=[('in_progress', 'In progress'), ('wait_until_due_date', 'Wait until due date')])
due_date = models.DateField(null=True, validators2=[check_state_and_due_date_match])

def check_state_and_due_date_match(due_date, all_fields):
state = all_fields['state']
if due_date:
if state != 'wait_until_due_date':
raise ValidationError('If due date is set, state must be "wait_until_due_date"')
return
if state != 'wait_until_due_date':
raise ValidationError('If no due date is set, state must be "wait_until_due_date")


Open questions: I am unsure if this should be used only for validation, or if it should be possible
to alter values, too.

What do you think?

Other solutions or improvements?

Regards,
Thomas Güttler



--
Thomas Güttler
http://thomas-guettler.de/

[Preston Timmons]

There are times when I've definitely wanted this feature. Particularly, when multiple fields on a form have this type of constraint. Putting all the logic in the clean method gets convoluted.

[Wim Feijen]

Great proposal and I'd love it on forms. I'm not sure about models, though. I always use forms for sanitization. 

[Carl Meyer]

Hi Thomas,

Why?

> class Issue(models.Model):
> state = models.CharField(choices=[('in_progress', 'In progress'),
> ('wait_until_due_date', 'Wait until due date')])
> due_date = models.DateField(null=True,
> validators2=[check_state_and_due_date_match])
>
> def check_state_and_due_date_match(due_date, all_fields):
> state = all_fields['state']
> if due_date:
> if state != 'wait_until_due_date':
> raise ValidationError('If due date is set, state must be
> "wait_until_due_date"')
> return
> if state != 'wait_until_due_date':
> raise ValidationError('If no due date is set, state must be
> "wait_until_due_date")
>
>
> Open questions: I am unsure if this should be used only for validation,
> or if it should be possible
> to alter values, too.
>
> What do you think?
>
> Other solutions or improvements?

I think this is unnecessary complexity to add to the validation process.
There is nothing that this proposal makes possible that isn't already
possible using the clean() method. In cases where the clean() method
becomes too long or convoluted, you can break it up into smaller methods
yourself.

In fact, I would guess that your proposal could be entirely implemented
on top of current Django, by overriding the clean() method to implement
what you've outlined above.

Carl

[Marc Tamlyn]

In particular it is worth noting the addition of form.add_error[1] in Django 1.7 which makes long clean methods much nicer to handle.

[1] https://docs.djangoproject.com/en/1.7/ref/forms/api/#django.forms.Form.add_error

--
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 http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/5512DE42.8000800%40oddbird.net.

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

[Tom Evans]

On Wed, Mar 25, 2015 at 9:37 AM, Thomas Güttler <h...@tbz-pariv.de> wrote:
> Up to now the validation/cleaning of a attribute can not access the
> value of an other attribute.
>
> The work around is to override the clean() method of the form/model.
>
> A team mate and I talked about it, and we asked ourself, if
> a two phase cleaning would help here.
>
> Our idea:
>
> - The first clean phase works like the current implementation
>
> - The second clean phase is optional and does nothing except the user
> implements it.
>
> - In the second clean phase your are allowed to peek into
> the results of the first clean phase.
> The clean method for the second phase would need to get the other values
> passed in.

How is this different from the existing behaviour of clean_FOO(),
followed by clean()? Is that not "clean phase 1" and "clean phase 2"?

You can place each multi field constraint to be tested in to its own
method and call them from clean() if it gets too unwieldy.

Cheers

Tom

[Preston Timmons]

Since I think this can be a useful feature, I'll explain why.

One use case is for validating address forms. We deal with a lot of

them with varying levels of validation based on country, state, zip

code, etc. Sometimes, multiple sets of address fields appear on the

same form. We can't simply reuse the fields without worrying about

disparate validation routines specified on the form in addition to the

fields. This leads to a meticulous set of mixins. It gets the job done,

but I don't think that's great api.

If fields had a second clean method that was called with the form

data, we could do something simpler like:

state1 = StateField(country_field="country1")

state2 = StateField(country_field="country2")

The field would then look like:

StateField():

    def __init__(self, country_field=None):

        self.country_field = country_field

    def clean_full(self, cleaned_data):

        if self.country_field:

            do_something(cleaned_data[country_field])

Yes, clean_FOO() can be made to work. No, it doesn't make reusing

fields with complex validation in multiple forms and contexts easy,

especially if the field names may need to differ in certain forms.

Preston

[Carl Meyer]

Hi Preston,

As I said earlier in the thread, I think it's entirely possible to build
this, in a fully generic and reusable way, on top of current Django core
as a third-party add-on. It could even have the exact API you propose.
You just need a `Form` subclass with a `clean` method that loops through
the fields on the form and calls the `clean_full` method (if it exists)
on each of them.

I think the "frequency of need" vs "added complexity" calculation does
not come out in favor of adding this to Django core, but I don't see any
reason it can't exist as a third-party utility.

I also find it a mild violation of field encapsulation for fields to
know about other fields on the same form. In my mind, the right level of
abstraction for cases like this is usually to build a reusable
`AddressForm`, which does cross-field validation at the form level, and
then reuse that alongside other forms. (In general, I think users of
Django's forms library often don't take enough advantage of the fact
that a single HTML form doesn't need to be a single Django `Form` class.)

Carl

[Preston Timmons]

Yep, I think you're right now. Given existing solutions there's not warrant

enough to add another built-in validation hook.

Preston

[Thomas Güttler]

Am 26.03.2015 um 15:12 schrieb Marc Tamlyn:
> In particular it is worth noting the addition of form.add_error[1] in Django 1.7 which makes long clean methods much
> nicer to handle.
>
> [1] https://docs.djangoproject.com/en/1.7/ref/forms/api/#django.forms.Form.add_error

Yes, you are right. My proposal could be implemented with a small second loop over all fields.
Adding exceptions which get raised in the second run with form.add_error().

Nevertheless I think something like this would be nice to have in django core.

But in this moment, I will use the easy way: Write a loop in my code.

I withdraw the proposal. If someone likes to do it: go ahead, and ask
me and other for feedback :-)

[Thomas Güttler]

Since `field_order` will be in Django 1.9 the second clean loop is hardly needed in my case.

https://github.com/django/django/commit/28986da4ca167ae257abcaf7caea230eca2bcd80

[Raphael Michel]

Hi,

Am Thu, 2 Apr 2015 09:15:42 -0700 (PDT)
schrieb Preston Timmons <preston...@gmail.com>:

> One use case is for validating address forms. We deal with a lot of
> them with varying levels of validation based on country, state, zip
> code, etc. Sometimes, multiple sets of address fields appear on the
> same form. We can't simply reuse the fields without worrying about
> disparate validation routines specified on the form in addition to the
> fields. This leads to a meticulous set of mixins. It gets the job
> done, but I don't think that's great api.

While Thomas' use case (issue due dates that are required for
certain issue states) can be solved using clean() without major
drawbacks, for your use case (re-usable address fields), I'd go for
MultiValueFields[1] -- this logic does not really belong to the form.

Raphael

[1] https://docs.djangoproject.com/en/1.8/ref/forms/fields/#multivaluefield