Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Jun 13, 2018

Conversation

valtron
Copy link
Contributor

@valtron valtron commented Jun 12, 2018

(This PR is a resubmission of #258.)

Three facets two consider:

  1. New behaviour adds None for empty errors. Maybe it should add [] so users can avoid the extra is not None?
  2. 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.
  3. If no errors, the old behaviour (errors == []) is kept. Ditto above on convenience.

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.']])
Copy link
Contributor

@whb07 whb07 Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@valtron valtron Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@whb07 whb07 Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@valtron valtron Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@whb07 whb07 Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@whb07 whb07 Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@valtron valtron Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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']}]}

Copy link
Contributor

@whb07 whb07 Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@valtron valtron Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@whb07 whb07 Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@whb07 whb07 added invalid tests labels Jun 12, 2018
@davidism davidism removed invalid tests labels Jun 12, 2018
@@ -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:
Copy link
Member

@davidism davidism Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


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)
Copy link
Member

@davidism davidism Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

else:
self.errors.append(None)

if all(x is None for x in self.errors):
Copy link
Member

@davidism davidism Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@davidism
Copy link
Member

@davidism davidism commented Jun 12, 2018

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
Copy link
Contributor

@whb07 whb07 commented Jun 12, 2018

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.

@davidism davidism merged commit 6a401cd into wtforms:master Jun 13, 2018
1 check passed
@whb07
Copy link
Contributor

@whb07 whb07 commented Jun 13, 2018

@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
Copy link
Member

@davidism davidism commented Jun 13, 2018

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
Copy link
Contributor

@whb07 whb07 commented Jun 13, 2018

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
Copy link
Contributor Author

@valtron valtron commented Jun 13, 2018

@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
Copy link
Contributor

@whb07 whb07 commented Jun 13, 2018

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

  1. containers don't keep order
  2. Even if you had an ordered container you wouldn't be able to control the order they come in via Flask etc...
  3. 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
Copy link
Contributor Author

@valtron valtron commented Jun 13, 2018

  1. If FieldList isn't intended to be ordered, it should be renamed. List implies ordering.
  2. 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.
  3. 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
Copy link
Contributor

@whb07 whb07 commented Jun 13, 2018

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
Copy link
Contributor Author

@valtron valtron commented Jun 13, 2018

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.

azmeuk added a commit to azmeuk/wtforms that referenced this issue Apr 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants