Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Validation of forms with value attribute set to an int #14

Open
fhsm opened this Issue May 12, 2010 · 12 comments

Comments

Projects
None yet
5 participants

fhsm commented May 12, 2010

If the value attribute of a dropdown or radio button element is set to an integer and validation of the form containing that element fails the element's state is lost in the redisplayed form.

If the following form was submitted with the textbox left blank and the first radio button selected the form displayed after validation would show an empty textbox and no radio button selected.
simple_form = form.Form(
form.Textbox('test',
form.notnull,
),
form.Radio('example',
[1,2,3,4],
)
)

I think this happens because the form post is u/str and thus the value checks fail. Maybe this could be fixed by converting the values to strings for the test of equality in the render functions for these elements.

243c243
<             if self.value == value: select_p = ' selected="selected"'

---
>             if self.value == str(value): select_p = ' selected="selected"'
266c266
<             if self.value == arg:

---
>             if self.value == str(arg):
Contributor

anandology commented Aug 26, 2010

I don't see a problem with this behavior. I think it is okay to assume that the Dropdown options must be strings.

If we implicitly convert value to string before comparison, we might get into more trouble. Explicit is better than implicit!

fhsm commented Aug 26, 2010

The problem is that ints are already being implicitly converted when the are rendered but the validation logic doesn't acknowledge this. The library allows forms to be defined with int values (ex: form.Radio in the OP) so it should be able to handle their validation, even though they have been converted by the HTML round trip.

Either the validation logic needs to be changed to account for the implicit conversion when the ints described in the form object are returned from HTML or the form object needs to restrict the ability to define ints as values in recognition of the inability to mantain type when rendered.

I don't have a strong opinion about which is better but it's clear that the current behavior is buggy. It should not be possible to define a form that can't keep track of its own state during during validation b/c of datatypes.

Contributor

aaronsw commented Apr 11, 2011

So you want Radio to crash when it gets an int value? That seems reasonable to me.

fhsm commented Apr 13, 2011

I don't have a clear preference for a particular solution (i.e. where you choose to check for stringyness and what action is taken). I do however think that form init needs to stop writing checks that form validation can't cash.

The patch I posted would help validation deliver on init's promise. I think presumptive conversion in the validation layer would work for most people most of the time. It would also create maddeningly subtle errors for someone. On the other hand type checking on form creation and crashing will create (unnecessary) frustrating WTF moments for new users while alerting everyone to validation edge cases. Seen in this light the selection amongst solutions is clearly a philosophical. Are you making Gentoo or Ubuntu?

Given these options I suppose my preference is, as you said, to crash early and clearly.

As an aside I wouldn't implement this by looking for ints in just Radio. The fact is that regardless of what goes in a form field a string is coming back. Any number of combos could demo this problem: floats in textareas, ints in radio buttons, or as was the case for me numpy floats in dropdowns. I suspect anything where obj != obj.str() will do it.

Contributor

pjz commented Jun 18, 2011

I agree that the inconsistency is troublesome and it's better to crash early (with a good error message if possible) rather than help hide bugs.

It looks to me like the bug might not be in Radio as much as it's in net.websafe(), which coerces its argument to unicode ; perhaps it should instead raise a ValueError? This would also get rid of the special-looking check for None at the beginning of the routine. It would become something like:

def websafe(val=u''):

    if isinstance(val, str):
        val = val.decode('utf-8')
    if not isinstance(val, unicode):
        raise ValueError("Cowardlyly refusing to autoconvert to unicode")

    return htmlquote(val)
Contributor

aaronsw commented Jun 20, 2011

       if isinstance(val, str):
           val = val.decode('utf-8')
       if not isinstance(val, unicode):
           raise ValueError("Cowardlyly refusing to autoconvert to unicode")

Yeah, that's probably right. It's really only intended to deal with
the charset issue.

Contributor

aaronsw commented Jun 21, 2011

On second thought (after reviewing the context in aa0a356), I've changed my mind on this change. Isn't websafe used by the template system? Coercing None and integers to quoted strings is pretty widely used and doesn't seem obviously wrong to me. I think crashing on receiving an integer at the Form level is a much better solution.

Contributor

pjz commented Jun 21, 2011

Should it error immediately when passed to Radio() or should it wait until it tries to get render()'d ?

Contributor

aaronsw commented Jun 23, 2011

Immediately, I'd think.

cigumo commented Jul 27, 2011

Another problem related to the original issue (not the websafe discussion) is when filling a form from a database query.

If the table has integers or bigint columns, the storage object returned by any DB query has ints or longs (e.g. 3L), and using form.fill(query_result) will not match the Dropdown and Radio options if they are strings. Conversely if you define the options as integers, they match with form.fill(query_result) but they do not work with form.validates(), as the web.input() only contains strings (cgi.FieldStorage values).

Any ideas on how to solve this without parsing each value manually?

Contributor

pjz commented Oct 26, 2011

I was looking at this again, and I see two options:

  1. do typechecking in the Radio() constructor to check that the args are all strings or tuples/lists of strings. This would fix the issue by making it an error to call radio with non-string arguments to be rendered

  2. explicitly document that we're going to call websafe()/unicode() on args, and do so, so that the validation logic (which currently checks 'self.value == value' ) will work. This could be done either once in the constructor, or at test-time in the aforementioned render() line ('self.value == net.websafe(value)').

Preferences?

cigumo commented Oct 31, 2011

On Tue, Oct 25, 2011 at 10:07:08PM -0700, Paul Jimenez wrote:

I was looking at this again, and I see two options:

  1. do typechecking in the Radio() constructor to check that the
    args are all strings or tuples/lists of strings. This would fix
    the issue by making it an error to call radio with non-string
    arguments to be rendered

  2. explicitly document that we're going to call
    websafe()/unicode() on args, and do so, so that the validation
    logic (which currently checks 'self.value == value' ) will work.
    This could be done either once in the constructor, or at test-time
    in the aforementioned render() line ('self.value ==
    net.websafe(value)').

Preferences?

Hi Paul,

what I ended up doing was converting the non-string values of the
source to string in the web.form.fill method.

Changed this:
def fill(self, source=None, *_kw):
return self.validates(source, _validate=False, *_kw)

To this:
def fill(self, source=None, *_kw):
d = source
cs = dict(map(lambda k: type(d[k]) in [int, long, float] and (k, str(d[k])) or (k, d[k]), list(d)))
return self.validates(cs, _validate=False, *_kw)

Thanks,

Ciro Mondueri

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment