Opened 5 years ago

Last modified 4 years ago

#16306 new Bug

Form field documentation documents optional keyword arguments as field attributes.

Reported by: ShawnMilo Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords: form field documentation
Cc: bmispelon@… Triage Stage: Accepted
Has patch: no Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


I've tested this for forms.IntegerField and forms.DecimalField -- setting a max_value when the field is declared works normally, but if set in init (because it's unknown till runtime) no validators are assigned although the max_value value is properly set.

from django import forms

class TestForm(forms.Form):
    amount = forms.IntegerField(max_value = 10)

class TestForm2(forms.Form):
    amount = forms.IntegerField()

    def __init__(self, *args, **kwargs):
        super(TestForm2, self).__init__(*args, **kwargs)
        self.fields['amount'].max_value = 10

x = TestForm({'amount': 12})
print x.is_valid() #returns False
print x.fields['amount'].max_value # returns 10
print x.fields['amount'].validators # [<django.core.validators.MaxValueValidator object at 0x7febd5240610>]
print x.errors #complains about size

x = TestForm2({'amount': 12})
print x.is_valid() #returns True
print x.fields['amount'].max_value #returns 10
print x.fields['amount'].validators # []
print x.errors #nothing

Evidently this bug has existed for some time, because almost a year ago it was discussed on StackOverflow and the suggestion was to just completely replace the field at runtime:

Attachments (1)

16306.diff (12.1 KB) - added by Chris Beaven 5 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 5 years ago by melinath

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: invalid
Status: newclosed

max_value and min_value are documented as arguments to the __init__ method of the DecimalField and IntegerField. They are not documented as attributes on the field instances. The don't even seem to be used internally, so I'm not really sure why they even get set. In any case, this is not a bug, and there are ways to change the field at runtime already in place.

If you want to request this as a new feature, reopen the ticket.

comment:2 Changed 5 years ago by melinath

Component: FormsDocumentation
Easy pickings: set
Keywords: form field documentation added
Needs documentation: set
Needs tests: set
Resolution: invalid
Severity: Release blockerNormal
Status: closedreopened
Summary: Validators not set for form field when max_value set in __init__Form field documentation documents optional keyword arguments as field attributes.
Triage Stage: UnreviewedAccepted
Version: 1.3SVN

Meh. Reread the documentation and although the words say "optional arguments" the examples are indeed attributes. I'm marking this as a documentation bug. I think that a feature request would be valid, but that's a separate issue.

comment:3 Changed 5 years ago by ShawnMilo

Component: DocumentationForms
Triage Stage: AcceptedDesign decision needed

Also, a comment about using one of the following workarounds:

  1. Replace the field completely in the form's init.
  2. Add the field in the form's init (don't have it in the original definition).

Both of these would cause confusion for the maintenance developer: Either a field is appearing 'by magic' or it's not as defined in the form originally. Neither case is desirable.

Changed 5 years ago by Chris Beaven

Attachment: 16306.diff added

comment:4 Changed 5 years ago by Chris Beaven

Easy pickings: unset
Needs tests: unset

How does this look?

comment:5 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:6 Changed 4 years ago by Jacob

Triage Stage: Design decision neededAccepted

comment:7 Changed 4 years ago by Baptiste Mispelon

Cc: bmispelon@… added

I took a closer look at this ticket and investigated several approaches (including the proposed patch)

I came up with 3 approaches on this ticket (the 3rd one being SmileyChris's one), but all of them have some issue:

1) move the creation of validators from __init__() to run_validators()

This approach is the less intrusive one.
It works quite well but breaks some tests in the test suite, which shows there are some potential backwards-compatibility issues.
The root of these issues is code which expects the field instance's validators attribute to be populated before the field has been actually validated [1].
There's also two possibilities with this approach, the difference being whether we append the validators inside run_validators (in which case a MinValueValidator would be placed after any custom one for example), or whether we prepend them (this is more compatible with the existing behavior).

2) Use lazy values

This sort of works, but there are two problems:

  • The validators need to be changed because they can now be passed a None value to validate against.
  • The error messages are broken because "%d" % lazy_int does not work (and as far as I know, that's not something that can be made to work).

3) descriptors (proposed patch)

This approach is quite elegant on paper, and while the patch looks good, I think the proposed solution can't work because of some new features that were added since the patch was proposed.

It's a bit tricky to explain why, but let me try.

Let's consider what happens when creating an IntegerField with a max_value argument:

  • IntegerField(max_value=42) calls IntegerField.__init__
  • The __init__ needs to do two things: set self.min_value to 42, and call Field.__init__.
  • Setting self.min_value = 42 calls ValidatorAttribute.__set__
  • ValidatorAttribute.__set__ requires the field's validators attributes (a list of validators) to exist.
  • This attribute is set in Field.__init__
  • This means that IntegerField.__init__ needs to call Field.__init__ before setting self.min_value
  • However ...
  • Field.__init__ calls self.widget_attrs(), which allows the field to pass custom attributes to the widget.
  • IntegerField.widget_attrs checks the value of self.min_value ...
  • ... which hasn't been set for the reasons stated above.

It might be possible to add some more descriptors to make the whole thing work.
For example, I tried making Field.widget into a property that would only call widget_attrs when being accessed.
This however failed with a weird error message and it started getting hard to trace the problem with all the different layers of descriptors.
All things considered, I'm not sure that adding all these additional layers is worth it.

I think approach 1 is the best one, even at the cost of minor backwards-compatibility.
Field.validators is not really documented and the documentation has no examples
of directly manipulating it.

Additionally, other fields have attributes which could use a similar fix:

  • CharField.max_length
  • charField.min_length
  • DecimalField.max_digits
  • DecimalField.decimal_places
  • RegexField.regex (current solution is based on @property)


Note: See TracTickets for help on using tickets.
Back to Top