Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#13184 closed (fixed)

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

Reported by: Mark Owned by: Mark
Component: Forms Version: master
Severity: Keywords: model validation
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

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?

Attachments (4)

test_form.py (2.8 KB) - added by Mark 4 years ago.
forms_models.diff (1.8 KB) - added by Mark 4 years ago.
tests_forms_models.diff (2.7 KB) - added by Mark 4 years ago.
13184-doc.diff (914 bytes) - added by Honza_Kral 4 years ago.
Alternative solution to the problem - documenting how to handle custom fields' formfield method

Download all attachments as: .zip

Change History (13)

Changed 4 years ago by Mark

comment:1 Changed 4 years ago by russellm

  • Component changed from Database layer (models, ORM) to Forms
  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

comment:2 Changed 4 years ago by Mark

  • Owner changed from nobody to Mark
  • Status changed from new to assigned

comment:3 Changed 4 years ago by Mark

  • Has patch set

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.

Changed 4 years ago by Mark

Changed 4 years ago by Mark

comment:4 Changed 4 years ago by 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.

Changed 4 years ago by Honza_Kral

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

comment:5 Changed 4 years ago by Honza_Kral

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.

comment:6 Changed 4 years ago by 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?

comment:7 Changed 4 years ago by russellm

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.

comment:8 Changed 4 years ago by russellm

  • Resolution set to fixed
  • Status changed from assigned to closed

(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.

comment:9 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.