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

Avoid to cache form errors #568

Merged
merged 3 commits into from Apr 19, 2020
Merged

Avoid to cache form errors #568

merged 3 commits into from Apr 19, 2020

Conversation

azmeuk
Copy link
Contributor

@azmeuk azmeuk commented Apr 19, 2020

Caching form errors may cause some strange behaviors when a form errors attribute is accessed during validation, and performance benefits are not obvious.

This should fix #522

@davidism
Copy link
Member

@davidism davidism commented Apr 19, 2020

form.errors needs to be writable so you can add form-level errors.

@azmeuk
Copy link
Contributor Author

@azmeuk azmeuk commented Apr 19, 2020

Interesting. What would be the API then? I can imagine two scenarios:

  • The first one is mimicking the way we add errors in Fields, with myform.errors.append("my error"). The issue here is that myform.errors would behave like a dictionary when reading it, and would behave like a list when writing in it.
  • The second one would be just to allow access to the error dictionary. So for instance myform.errors["my_field"].append("my field error") would append an error in the field my_field. The None key could be used to store the form-level errors (not attached to any peculiar field). For instance myform.errors[None].append("my form error").
    This second scenario feels more consistent, but adding an form-level error seems tedious. We could then add a add_error method that would do this job in both Fields and Forms.

What do you think? I like this second scenario better.

If this API seems OK to you, I will open another PR as I believe it is a different matter than this one. That way we can merge this one?

CHANGES.rst Outdated Show resolved Hide resolved
@davidism
Copy link
Member

@davidism davidism commented Apr 19, 2020

Maybe just make the list of form errors an attribute called form_errors, and add it as the None key for errors. Although I guarantee that would then confuse some users wondering why there's form.errors and form.form_errors. I wouldn't worry about an add_error method for now


It's been a long time since I used form-level errors, maybe it's not actually needed. I'm trying to think of a scenario that I'd use them over just adding the error to a field. I don't think it was ever documented as a feature, so it's probably ok to change the behavior. If someone does report an issue, we can always address it in a bug fix. Given that piece of the documentation you removed that called out when it would be regenerated, I'm thinking form-level errors were just an accident.

I'm still hesitant to remove caching, as users might access form.errors multiple times and not know it builds a new dict in a loop every time. Perhaps there's some hybrid we can use where we cache the dict, and add new keys to it if needed each time it's accessed. Not sure if it's really a performance issue, or if my suggestion would be any faster.

I guess overall I'm fine with the PR as-is, we can always address these things later if someone brings them up.

src/wtforms/form.py Outdated Show resolved Hide resolved
tests/test_form.py Outdated Show resolved Hide resolved
@azmeuk
Copy link
Contributor Author

@azmeuk azmeuk commented Apr 19, 2020

I fixed your suggestions.

About perfs

I'm still hesitant to remove caching, as users might access form.errors multiple times and not know it builds a new dict in a loop every time. Perhaps there's some hybrid we can use where we cache the dict, and add new keys to it if needed each time it's accessed. Not sure if it's really a performance issue, or if my suggestion would be any faster.

I cannot picture a real world case where the caching has significant performance impact:

  • forms have a limited amount of fields (probably a few dozen for large ones)
  • forms have a limited amount of subforms (FormFields in FormFields in FormFields, but that probably won't go very far)
  • I can imagine cases where you wan to access to form.errors a few times, but not a lot of times.

Even in the worst case scenario, think that building the dict won't do a large difference. Say a Form containing 100 FormFields themselve 100 FormFields themselve containing 100 Fields, errors for each possible field. So this is about building a dict with 1M entries. Now let us say you access the dict 10 times, here are the profiling results on my computer:

>>> profile.run('for _ in range(10): {x:x for x in range(10^6)}')                                                                                                                                                                                                                      
         14 function calls in 0.000 seconds

   Ordered by: standard name

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.000    0.000 :0(exec)
        1    0.000    0.000    0.000    0.000 :0(setprofile)
       10    0.000    0.000    0.000    0.000 <string>:1(<dictcomp>)
        1    0.000    0.000    0.000    0.000 <string>:1(<module>)
        1    0.000    0.000    0.000    0.000 profile:0(for _ in range(10): {x:x for x in range(10^6)})
        0    0.000             0.000          profile:0(profiler)

About form-level errors

I totally see cases where form-level errors would be pertinent. For instance when you have each individual field that has a correct value, but the whole form do not make sense.
e.g. 4 fields for a store opening time: morning_opening, morning_closing, afternoon_opening, afternoon_closing. Each field must have a value that is lesser than the following, when it is not the case, there is not obvious faulty field. In my opinion this is a form-level error.

@davidism
Copy link
Member

@davidism davidism commented Apr 19, 2020

Looks good now.

For your example, I'd add validation for each field that its value is after the previous field, so the error would go on the field that didn't validate. I guess it comes down to a matter of preference. Let's just merge this and leave it for now.

@azmeuk azmeuk merged commit 77c9d76 into wtforms:master Apr 19, 2020
1 check passed
@azmeuk
Copy link
Contributor Author

@azmeuk azmeuk commented Apr 19, 2020

I will also backport it in branch 2.2

@azmeuk azmeuk mentioned this pull request Apr 19, 2020
azmeuk added a commit to azmeuk/wtforms that referenced this issue Apr 19, 2020
@azmeuk azmeuk deleted the issue-522 branch Apr 22, 2020
Glandos added a commit to Glandos/ihatemoney that referenced this issue Jun 9, 2021
Glandos added a commit to spiral-project/ihatemoney that referenced this issue Jun 9, 2021
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.

2 participants