Code

Opened 3 years ago

Last modified 13 months 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

Description

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:

http://stackoverflow.com/questions/3470741/django-forms-integerfield-set-max-value-on-runtime

Attachments (1)

16306.diff (12.1 KB) - added by SmileyChris 3 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years ago by melinath

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 3 years ago by melinath

  • Component changed from Forms to Documentation
  • Easy pickings set
  • Keywords form field documentation added
  • Needs documentation set
  • Needs tests set
  • Resolution invalid deleted
  • Severity changed from Release blocker to Normal
  • Status changed from closed to reopened
  • Summary changed from Validators not set for form field when max_value set in __init__ to Form field documentation documents optional keyword arguments as field attributes.
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.3 to SVN

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 3 years ago by ShawnMilo

  • Component changed from Documentation to Forms
  • Triage Stage changed from Accepted to Design 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 3 years ago by SmileyChris

comment:4 Changed 3 years ago by SmileyChris

  • Easy pickings unset
  • Needs tests unset

How does this look?

comment:5 Changed 13 months ago by aaugustin

  • Status changed from reopened to new

comment:6 Changed 13 months ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

comment:7 Changed 13 months ago by bmispelon

  • 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)

[1] https://github.com/django/django/blob/master/tests/forms_tests/tests/forms.py#L822

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.