Custom fields with __metaclass__ = models.SubfieldBase - ValidationError breaks out of form.is_valid()

[Mark]

According to the docstring of django.db.models.Field, django.db.models.Field.to_python "Converts the input value into the expected Python data type, raising django.core.exceptions.ValidationError if the data can't be converted".

Declaring __metaclass__ = SubfieldBase for the Field subclass is supposed to force the to_python conversion of the said field without having to call clean_fields, which makes sense to do for the fields that are of no meaning to you in their textual/database representation.

Upon using it exactly in the way it is proposed (as illustrated in the attached test case), it appears, that if the validation fails and I raise the django.core.exceptions.ValidationError in to_python, the ValidationError somehow breaks out of the is_valid (where it is supposed to be caught and used to populate form.errors), if __metaclass__ = SubfieldBase is declared in the Field subclass.

Is this a bug in documentation or, perhaps, I am doing something wrong? I would like to stress, though, that I am doing it "by the book", - exactly as it is stated in the documentation without any deviations (or so I think, anyway).

A somewhat unrelated question - how is it supposed to work?

[Russell Keith-Magee]

There's definitely a problem here, but I'm not entirely sure what the solution is.

Line 317 of forms/models.py constructs a new instance using the form data. It assumes that the form data is clean at this point. The exception you are seeing is caused because the form data *isn't* clean - the value "bar" is accepted as form data (since it doesn't violate the default CharField validation conditions). When you try to construct a model instance (essentially invoking instance.field_with_subfieldbase = 'bar'), a validation error is raised. This error isn't caught by any of the form handling code, so it propagates as reported.

When you omit the SubfieldBase, to_python isn't fully installed on the model instance, so instance.field_without_subfieldbase = 'bar' doesn't raise a validation error. However, when self.instance.clean_fields() is invoked on line 321, a validation error *is* raised, because to_python is called directly on the model field.

This implementation without SubfieldBase is clearly wrong, because to_python isn't being invoked on value assignment; you would only see the error if you ran full_clean() on the instance. By analogy, if you assign 'asdf' to an integer field, you get a validation error immediately. IntegerField doesn't have the same problem as your custom field because there is an forms.IntegerField that does clean form data to return an integer.

So, I'm not sure if the error here is:

• An error on your part because you haven't provided a formfield that replicates the field validation logic (in which case, we need to document that custom fields need custom form fields)
• An error on construct_instance, because it should be hiding validation errors that will be caught later by clean_fields
• An error on the way construct_instance is called, because it should be wrapped in a try..except in the same way that clean_fields is.

Whatever the answer, there is something that needs to be addressed for 1.2

[Mark]

I am submitting a patch against forms/models.py (and against tests/regressiontests/forms/models.py), that aims to fix this issue.

It alters construct_instance to behave more like clean_fields of the model instance (so that errors are collected and then raised in a single ValidationError with message_dict populated).

It alters BaseModelForm._post_clean to catch the ValidationError, that might be raised by construct_instance.

It introduces one subtle change to the validation process: the fields that failed validation during the instance construction are excluded from the subsequent call to clean_fields. It is done, because otherwise, the validators will be run against the fields, that are already considered invalid, which can result in extra, unwanted (such as "This field is required"), errors being reported and displayed on the form.

Let me know if this needs any improvements, please.

[jkocherhans]

I need to think about this some more, but here are some quick comments on the possible causes that Russ mentioned.

Replying to russellm:

So, I'm not sure if the error here is:

• An error on your part because you haven't provided a formfield that replicates the field validation logic (in which case, we need to document that custom fields need custom form fields)

I think this might be the right solution. Generally, formfield() shouldn't need to return a form field that replicates validation logic. The model field is responsible for validation. However, in this case, there are pretty strict requirements in to_python. I think formfield() should be expected to return a field whose to_python method will return something that will pass the model field's to_python check, even if the value fails other validation later on. I realize this is just a test case, but the check in to_python here seems like it would be better performed in validate(). Maybe your actual use-case makes sense in to_python though.

• An error on construct_instance, because it should be hiding validation errors that will be caught later by clean_fields

Fixing the problem here seems like a hack to me.

• An error on the way construct_instance is called, because it should be wrapped in a try..except in the same way that clean_fields is.

I don't think constuct_instance was designed to raise ValidationError. Obviously that's happening though.

I'd like to hear Honza's opinion on this. If documentation isn't the answer, I suspect Mark's patch is close to the right approach.

[Honza Král]

Alternative solution to the problem - documenting how to handle custom fields' formfield method

[Honza Král]

The problem is that custom fields that use the SubfieldBase metaclass have very different behavior from rest of built-in fields - their to_python method is called whenever the field is assigned a value - this needs to be so in order for freshly created instances from DB (by the ORM) would have proper data types under instance.field.

The workaround for this particular issue (unhandled ValidationError in construct_instance) would be to have the Field's formfield method return a class that replicates the validation done in to_python. I consider this smaller hack than to wrap calls to save_form_data in try: blocks. I added a patch that implements this in form of a documentation paragraph.

[Mark]

I understand, that I have no say whatsoever on this issue, but wouldn't it be best not to force the user to write additional functionality (custom form field), if it can be avoided? Especially since that functionality will be, basically, duplicated in the to_python of the model field. That is what I had in mind when I was working on this issue. Perhaps, it is possible to solve this in a less hackish (than mine) way, while maintaining the transparency for the framework user?

[Russell Keith-Magee]

On reflection, I can see Honza and Joseph's point. The ValidationError handling in to_python is necessary, but it isn't really intended to be part of a form/input validation system - it's a check to ensure that values manually assigned meet whatever datatype requirements exist for internal storage prior to writing to the database.

Form validation is a separate exercise - it's the process of ensuring that a string input can be successfully converted into a model value. The Form starts with the assumption that the input is a human-entered string; although the to_python method *can* make this assumption, it isn't required to. It's arguable whether a model field to_python() is even require to handle strings at all. Django's builtin fields handle strings for historical reasons, which shouldn't be emulated. They also handle strings for serialization purposes, but in this case, the string support is for machine readable, not human-readable reasons. For example, a DateField can parse a string as a date, but only in ANSI format; a form's DateField will parse many formats, and will even allow for locale specific variations if correctly configured.

Model validation is also a separate exercise; a value can be of the right type, but be out of bounds, incompatible with other model values, or otherwise invalid. Again, these aren't things that the field's to_python should (or even can) check.

So - what we're left with is the idea that form validation should be producing data that is 'model ready', which means that a formfield() method may be required if your model requires input in a specific format. In some cases, this may involve some duplication of logic, but that won't necessarily always be the case; when it is the case, it can be easily factored out into a common location.

[Russell Keith-Magee]

(In [12980]) [1.1.X] Fixed #13184 -- Document the requirement that custom model fields using SubfieldBase should probably implement formfield. Thanks to Mark L. for the report, and to Joseph and Honza for the guidance on the solution.

Backport of r12979 from trunk.

[Jacob]

Milestone 1.2 deleted