pallets-eco / wtforms

A flexible forms validation and rendering library for Python.
https://wtforms.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.52k stars 397 forks source link

No way to tell that a given form is submitted #402

Open davidism opened 6 years ago

davidism commented 6 years ago

Related to at least #119, #242, #267, #280, #291, #355, #401

We do if formdata is not None before calling process_formdata on each field in a form. This broke the rather common case where users were passing request.form to a form even if the method wasn't POST because request.form isn't None. Previously we checked bool(formdata), which handled this case but caused processing to behave inconsistently depending on what was submitted.

Flask-WTF somewhat handles this by only passing request.form if the method is actually a submit method (POST, PUT, DELETE).

None of the approaches are really correct. Maybe you have a client that always sends some extra data (for example, an auth key). Or you have two forms on the same page, whichever was not submitted will still see the same non-empty form data as the one that was submitted.

f1 = FormA(request.form)  # was submitted, sees data
f2 = FormB(request.form)  # was not submitted, sees same data

Most built-in fields mask this issue because they only change data (which is set from the default / object data first) if formdata.getlist('name') is not empty. However, StringField will set data = '' if nothing was submitted (which #355 changes). And SelectMultipleField and MultipleFileField will overwrite with an empty list because there's no way to distinguish "selected zero values" with "didn't send data" (BooleanField has a similar problem.)

This can also cause weird issues during validation with InputRequired. If there are two forms, and you do something like if f1.validate(): ... elif f2.validate(): ..., and f1 was not submitted, it will show a "input required" error even if a default value was being displayed, which is confusing.

I'm not sure what the solution is here, but am open to discussion. We can break things, I don't care about backwards compatibility for 3.0.

CC @lepture @alanhamlett

davidism commented 6 years ago

This is more broadly important if we want to validate other data sources such as JSON, or PATCH requests, which do not have the same constraints and behaviors as HTML forms.

davidism commented 6 years ago

We could check if any of the form keys match any of the field names before processing. But this is expensive and unnecessary for the most common case of a single form. It's also incorrect for validation, where "no form data" might legitimately be a validation issue.

Alternatively, we could have a Form.is_submitted(formdata) method which just returns True by default, or True if any keys match submit field names and values. Flask-WTF already has this concept, although it's not implemented the way I just described, and it only applies to validation right now, not processing.

davidism commented 6 years ago

The most radical change would be to stop considering different data sources differently. Remove process_formdata and only use process_data. Combine data in layers, like form (incoming) data > keyword args > data dict > object > field defaults and process it once. However, I want to fully understand why it's not done this way (there's an FAQ entry about it and multiple issues have been closed because of it) before we do this.

I'm not super familiar with PATCH requests (or if there's even a standard), but it wouldn't be a full solution for patching because it wouldn't handle merging lists or nested data, only overwriting it completely.

RemiZOffAlex commented 6 years ago

It is logical error. Doc https://wtforms.readthedocs.io/en/stable/forms.html The Form class -> class wtforms.form.Form -> Construction init(formdata=None, obj=None, prefix='', data=None, meta=None, **kwargs):

formdata – Used to pass data coming from the enduser, usually request.POST or equivalent. formdata should be some sort of request-data wrapper which can get multiple parameters from the form input, and values are unicode strings, e.g. a Werkzeug/Django/WebOb MultiDict

MultiDict is not None. MultiDict can be empty dict. Flask used Werkzeug as wrappers standart function.

Example:

@app.route('/url', methods=['GET', 'POST'])
def url_func():
    form = UserForm(request.form, data={'var': 'val'})
    if request.method=='POST':
        ...
        return 'POST request'
    request 'GET request. var={}'.format(form.var.data)

And GET request.form is type MultiDict (or ImmutableMultiDict) and equal empty list (is not None)

davidism commented 6 years ago

See the rest of my post, I address why truth testing instead of none doesn't solve the issue.

RemiZOffAlex commented 6 years ago

My english is bad. Do I need to suggest a solution? Or the solution is already found and will be in the next version?

azmeuk commented 4 years ago

It sounds like this issue should be tackled for 3.0. What do you think?

davidism commented 4 years ago

I would like to get something into 3.0, but as I've said above I don't know what that would look like. I don't think there's going to be a 100% satisfying solution to this.

davidism commented 4 years ago

We need to come up with a policy for data processing and apply it consistently. We need to keep in mind that WTForms is intended first for processing HTML form data, and account for the fact that HTML form data is ambiguous in some cases such as boolean, multi-select, and file fields.

There are multiple types of data:

  1. No data (formdata is None) - #280 started using this instead of bool(formdata) to decide to process data, since it's more accurate in indicating that form data wasn't submitted.
  2. Empty data (bool(formdata)) - This is unreliable if other form data is submitted such as API tokens, or if one form is submitted on a page with multiple forms. Technically this is true of formdata is None too, but I think None is a better indication that this isn't a post request.
  3. Partial data - This isn't supported right now, but would be useful for patch requests or API requests, where only changed data might be submitted. Could do something similar to Marshmallow, where a partial=True flag on the form uses default values if the key is missing. This can come later, but should be kept in mind at least.
  4. Implict defaults - #280 and #355 dealt with this for StringField, where the value might be '' or None depending on what was passed in. I think 3.0 should consistently set values to None unless a value was actually given (for all fields, not just string).
  5. Provided defaults - There is constant confusion that defaults passed in as default= for fields, or obj, data, and kwargs for forms, is only used when no form data at all is provided. If this wasn't the case, there would be no way to submit an empty value for a field if it also had a default. It's also confusing which of obj, data, and kwargs takes precedence. I think the way forward here is better documentation, no behavior changes.

There are multiple places where processing happens:

  1. Form processing - Called automatically when a form instance is created, decides what default data to pass along with form data to each field's processing.
  2. Field processing, default data - This should be better about accepting string or Python data consistently.
  3. Field processing, form data - This should also be better about accepting string or Python data consistently; HTML form data will always be strings, but people keep trying to pass JSON or Python in. Some default filtering to strip surrounding space should probably be added (except for strings, where that should be an option).
  4. Field filters - Seems to be very underdocumented. It's common to modify data in a validator instead, since that API is more well known.
  5. Form validation - Recently formalized form-level validation errors. Other than that just calls out to each validator, collecting validator methods from the form.
  6. Field validation - Some fields implement pre validation. Post validation is also available but not used by any built-in fields. In charge of accounting for errors that were caught during processing. Errors here can either stop validation or just append an error message and keep going.

This post initially started as me collecting all the discussions once again, but turned into a big dump of my current thoughts on behavior of the entire processing stack. The amount of information here might make the situation look worse than it is. WTForms is already fairly consistent and works, but there's a lot of little inconsistencies and ambiguities that can be addressed.

davidism commented 4 years ago

Further down the line, a solution to ambiguous fields could be to render a hidden input with some sentinel value in order to force a submitted form to send a value for the field. For example a boolean field could render a hidden input with "value=unchecked" and then a checkbox with "value=true". If the list of values only contains the sentinel, it is false, otherwise it is true. If the name is not in the form data at all, we now know it was not sent and can safely assume the default. Similar schemes can be used for other field types. I'm not sure exactly how this would work, fields right now only render one input, and hidden fields are usually rendered in one block with form.hidden_field().

davidism commented 4 years ago

@tomchristie Any insight from TypeSystem on handling the ambiguity of missing HTML form data? I couldn't find a discussion of that in its docs.

tomchristie commented 4 years ago

Erm, I guess useful points are...

Don't know if that's any help. 🤷‍♂️

davidism commented 4 years ago

Haha, sorry to drag you into this without warning, I just remembered you had worked on a form library recently, thanks for answering 😅

sblondon commented 4 years ago

(I'm unsure this is useful, but, as WTForms user, this is the way I expect WTForms would behave:

WolfgangFahl commented 2 years ago

For a start it should be possible to use the name and id of a form to be able to work around this missing feature with e.g. some javascript solution that calls the submit button of another form and handles the necessary data.

See also the lengthy discussion in https://stackoverflow.com/questions/18290142/multiple-forms-in-a-single-page-using-flask-and-wtforms