fields: keep entries order in errors list 2: son of fields: keep entries order in errors list #407

valtron · 2018-06-12T17:47:01Z

(This PR is a resubmission of #258.)

Three facets two consider:

New behaviour adds None for empty errors. Maybe it should add [] so users can avoid the extra is not None?
To localize errors, it's only necessary to add Nones up to the latest error, e.g. ['a', '', 'a'] -> [None, <errors>] instead of [None, <errors>, None]. It might be more convenient to have the error list the same length as the data.
If no errors, the old behaviour (errors == []) is kept. Ditto above on convenience.

@the-tosh

Taken verbatim from @the-tosh.

whb07 · 2018-06-12T18:22:34Z

tests/test_fields.py

+        F = make_form(a=FieldList(self.t))
+        form = F(DummyPostData({'a-0': ['a'], 'a-1': ''}))
+        assert not form.validate()
+        self.assertEqual(form.a.errors, [None, ['This field is required.']])


This doesn't solve the actual issue of enumerating which field within the field enclosure, i.e - 'a-0-user_name'. I thought that was the whole purpose of the PR?

I don't understand what you mean. I'll explain it another way:

Currently, FormList.errors loses information. Take the output [<error>, <error>]: this could have been produced by many inputs: [<invalid>, <invalid>], [<valid>, <invalid>, <invalid>], [<invalid>, <invalid>, <valid>], etc.

Or to put it another way, the indices in the errors don't necessarily match the indices in data. By inserting a dummy value (None or []), the indices can be made to match up, and information isn't lost anymore.

You're not actually doing anything though. The errors themselves are still identical from this project vs. your fork.
Besides that, if I again think what you are saying is correct than you want to have an error be reflected like this:

class IMForm(Form): protocol = SelectField(choices=[('aim', 'AIM'), ('msn', 'MSN')]) username = StringField() class ContactForm(Form): first_name = StringField() last_name = StringField() im_accounts = FieldList(FormField(IMForm)) # for brevity assume data has 2 entries for IMForm # would look like this 'im_accounts-0-protocol' etc.. form = ContactForm(data) form.validate() # is false for now form.errors # should be a dictionary such as {'im_accounts-0-protocol': ['Not a valid choice']}

Thats what I thought you're trying to say. If thats what you're trying to say then I agree it should help and enumerate which field is exactly missing/incorrect. Having said that this PR again is not doing as such. Theres virtually no change in the form.errors and just does more work for no clear reason. Your test also needs to be more clear in what its doing and what should be the exact structure of your modified form.errors dictionary.

I understand: you want form.errors to have the same structure as formdata, I want it to have the same structure as data: { 'first_name': '...', 'im_accounts': [{ 'protocol': '...' }, ...] }.

Currently, Form.errors/Field.errors are data-structured. This PR intended to correct the bug in FieldList.errors under that assumption.

Personally, I prefer data-structured approach. formdata structure, IMO, should only be used on the input side as a convenient "parser" of HTML form data (since HTML doesn't have proper support for nested data).

I don't have a preference for the structure, I'm only mentioning one way of correctly enumerating the associated field that got the error.

Also the current errors are just the way you want them to be. When using FieldList the error will be structured as such:

{ 'first_name': ['This field is required.'], 'im_accounts': [ {'protocol': ['This field is required.'], 'username': ['This field is required.']} ] }

As you can see there is no enumeration of the fields within the errors which is what I am pointing out. I thought thats what you meant. If it isn't, please point out what it is that you wanted if not that?

Either the form.errors or the form.im_accounts.errors that is.

form = ContactForm(data = { 'first_name': "Test", 'last_name': "Test", 'im_accounts': [{ 'protocol': 'aim' }, {}] }) assert not form.validate() print(form.errors) # Before: {'im_accounts': [{'protocol': ['Not a valid choice']}]} # After: {'im_accounts': [None, {'protocol': ['Not a valid choice']}]}

On a clean install Ubuntu 17.10 with a copy of your fork.

class IMForm(Form): protocol = SelectField(choices=[('aim', 'AIM'), ('msn', 'MSN')]) username = StringField() class ContactForm(Form): first_name = StringField() last_name = StringField() im_accounts = FieldList(FormField(IMForm)) data = {} data['first_name'] = 'test_first_name' data['last_name'] = 'test_last_name' data['im_accounts'] = [{'protocol':'gchat', 'username': 'test_username'}] form = ContactForm(data=data) form.validate() form.errors # >> {'im_accounts': [{'protocol': ['Not a valid choice']}]} form.im_accounts.errors # >> [{'protocol': ['Not a valid choice']}]

Different server same Ubuntu 17.10 but this is straight from the pip install -U WTForms

class IMForm(Form): protocol = SelectField(choices=[('aim', 'AIM'), ('msn', 'MSN')]) username = StringField() class ContactForm(Form): first_name = StringField() last_name = StringField() im_accounts = FieldList(FormField(IMForm)) data = {} data['first_name'] = 'test_first_name' data['last_name'] = 'test_last_name' data['im_accounts'] = [{'protocol':'gchat', 'username': 'test_username'}] form = ContactForm(data=data) form.validate() form.errors # >> {'im_accounts': [{'protocol': ['Not a valid choice']}]} form.im_accounts.errors # >> [{'protocol': ['Not a valid choice']}]

As you can see they are identical.

@whb07 The result would be the same in that case, since data['im_accounts'] only contains one item.

Yes I see what you're saying now. But that doesn't do anything to help the user figure out which field in the data is invalid.
{'im_accounts': [{'protocol': ['Not a valid choice']}, None]} thats the dictionary that comes from your changes when passed two im_accounts.

So we are back to guessing what exactly is wrong with the data except that instead of looking at say N fields, I'm still looking at N fields because its not clear which particular data point is invalid. Except now I have a (N - correct-fields) worth of None objects to look at.

davidism · 2018-06-12T20:30:41Z

wtforms/fields/core.py

@@ -940,11 +940,16 @@ def validate(self, form, extra_validators=tuple()):
        for subfield in self.entries:
            if not subfield.validate(form):
                self.errors.append(subfield.errors)
+            else:


Collect the subfield errors unconditionally to ensure that they're all references to the error lists, even when empty.

subfield.validate(form) self.errors.append(subfield.errors)

davidism · 2018-06-12T20:38:07Z

wtforms/fields/core.py


        chain = itertools.chain(self.validators, extra_validators)
        self._run_validation_chain(form, chain)

-        return len(self.errors) == 0
+        return all(e is None for e in self.errors)


This doesn't need to change, bool(self.errors) would still work because the empty subfield errors would be cleared earlier.

davidism · 2018-06-12T20:38:57Z

wtforms/fields/core.py

+            else:
+                self.errors.append(None)
+
+        if all(x is None for x in self.errors):


Use all(x for x in self.errors) since we're referencing the lists instead of None.

davidism · 2018-06-12T20:42:37Z

This change makes sense to me. @whb07 I'm not sure why the tests don't look right on your machine, they appear to be passing on Travis.

whb07 · 2018-06-12T21:04:23Z

I agree with the general goal of this PR. I’m aware for the need to be able to correctly enumerate the fields using FormField especially when doing an AJAX request.

I’ll have to check later what’s the reason as to what exactly that new test is asserting and why it’s passing. But from preliminary basic usage on clean servers for each one as noted above, the actual code as is, is not doing what it’s supposed to.

whb07 · 2018-06-13T00:34:30Z

@davidism how is having a list such as [{'username': ['Not a valid choice']}, None, None, None, None] help enumerate which form/field exactly needs to be addressed? Are the fields kept in order going in and going out? How do you ensure order when its posted ?

davidism · 2018-06-13T00:49:18Z

Hmm, good point, I think there's a related issue open about order of field list fields when not all of them are submitted. Not sure if that's solvable.

whb07 · 2018-06-13T00:53:49Z

I mean it’s up to you, if you can don’t merge this in yet. I’ll see if we can come up with a viable way to enumerate properly. There are ordered dicts and other possible solutions now that python2.7 is EOL.

But as this goes, this does not appear to move FieldList forward. But it’s up to you @davidism

valtron · 2018-06-13T01:06:17Z

@whb07 Do you mean it's possible for data to be [<valid>, <invalid>], but the output to be either [[], <error>] or [<error>, []]? How would this happen? Are you referring to _extract_indices iterating over formdata keys (e.g. a-1 processed before a-0)? Because other than that, everything, as far as I can tell, maintains the order.

Also, I would expect FieldList to maintain order. Otherwise, it might be better to name it UnorderedFieldList :)

whb07 · 2018-06-13T01:19:44Z

I'm saying multiple things that were quickly glossed over:

containers don't keep order
Even if you had an ordered container you wouldn't be able to control the order they come in via Flask etc...
A list filled with None is not very informative. This entire PR's purpose I thought was to help the user better understand which part exactly was invalid in a nested form.

There are ways to better address this, either give them an index or perhaps a hint as to the data that was inside the invalid field. The latter option might be better as all the parts are already there. I'll just submit a PR later to patch this up.

valtron · 2018-06-13T01:43:12Z

If FieldList isn't intended to be ordered, it should be renamed. List implies ordering.
Can you give an example of this? I assume you're talking about using HTML forms+POST, because the order of items in a JSON array wouldn't change. If so, it seems to me wtforms expects the subfields of FieldList to be named {fieldname}-{index} (or more generally, {prefix}{index}), in which case the order information is available and should be used in _extract_indices. I don't see anywhere else things can get re-ordered, but if I'm wrong please show me how that can happen.
The Nones (or []s as per @davidism's changes) themselves aren't informative. However, the presence of these in the list causes the position of the errors to match up with the data. The "index" you say should be given is what this provides, implicitly: the errors at position i correspond to the data at position i.

Maybe I should give more details of what my front-end looks like, though this sort of logic could also exist server side for validation, rendering, etc. Initial page load passes down a JSON description of the form (generated from the wtform), initial data; there's also the errors, which are initially empty, but also get updated from AJAX submissions. These variables go into a render function:

function render_form(field, data, errors) {
    const $err = render_errors(errors);
    let $widget;
    
    if (field.type == 'object') {
        $widget = [];
        for (const k of Object.keys(field.subfields)) {
            const $subwidget = render_form(field.subfields[k], data[k], errors[k]);
            $widget.push(<div>
                <label>{k}</label>
                {$subwidget}
            </div>);
        }
    } else if (field.type == 'list') {
        $widget = [];
        for (let i = 0; i < data.length; ++i) {
            $widget.push(render_form(field.subfield, data[i], errors[i]));
        }
    } else if (field.type == 'text') {
        $widget = <input type="text" value={data}/>;
    } else if (/* other types */) { ... }
    
    return <div>{$err} {$widget}</div>;
}

whb07 · 2018-06-13T02:01:06Z

Again the intent of the PR is correct. There should be better inspection for nested forms. But having a [{field: [error message here]}, None, None, None ] is just as helpful as the current way it is now. I't would be much more useful to hint within the error itself something like name or short_name which is an attribute already available to members under the FieldList.

valtron · 2018-06-13T02:14:45Z

I don't understand why you say the new behaviour is "just as helpful as the current way". I gave this example data:

[<valid>, <invalid>]

Error-wise, pre-patch, this is produced:

[<errors>]

The <errors> might apply to either the first item, or the second, there's no way to tell. Post-patch, you get these errors:

[[], <errors>]

Now it's clear that the first item had no errors, and the second item had errors.

True, if the items containing errors were always a contiguous prefix of the list, as your examples all are, this wouldn't be necessary. You're totally right, in that case, it's unnecessary. However, it's possible that there exist i < j such that item[i] has no errors and item[j] has errors. In these cases, the post-patch is more helpful.

Or is your objection to having a string of []s at the end of the error list? It's true they're not necessary, since a user can assume any i > len(errors) has no errors and get the same information. It's similar to having []s for FormField subfields that have no errors: unnecessary, but harmless.

fields: keep entries order in errors list

94d9348

Taken verbatim from @the-tosh.

whb07 reviewed Jun 12, 2018

View reviewed changes

whb07 added invalid tests Related to tests labels Jun 12, 2018

davidism removed invalid tests Related to tests labels Jun 12, 2018

davidism requested changes Jun 12, 2018

View reviewed changes

add subfield.errors unconditionally and other cleanup

7c2bae3

davidism merged commit 6a401cd into pallets-eco:master Jun 13, 2018

davidism mentioned this pull request Jun 13, 2018

Keep order and number of entries in list of FieldList errors #257

Closed

azmeuk added a commit to azmeuk/wtforms that referenced this pull request Apr 18, 2020

Backported pallets-eco#407

29c41b0

Uh oh!

fields: keep entries order in errors list 2: son of fields: keep entries order in errors list #407

fields: keep entries order in errors list 2: son of fields: keep entries order in errors list #407

Uh oh!

Conversation

valtron commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whb07 Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidism commented Jun 12, 2018

Uh oh!

whb07 commented Jun 12, 2018

Uh oh!

whb07 commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidism commented Jun 13, 2018

Uh oh!

whb07 commented Jun 13, 2018

Uh oh!

valtron commented Jun 13, 2018

Uh oh!

whb07 commented Jun 13, 2018

Uh oh!

valtron commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whb07 commented Jun 13, 2018

Uh oh!

valtron commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

valtron commented Jun 12, 2018 •

edited

Loading

whb07 Jun 12, 2018 •

edited

Loading

whb07 commented Jun 13, 2018 •

edited

Loading

valtron commented Jun 13, 2018 •

edited

Loading

valtron commented Jun 13, 2018 •

edited

Loading