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

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

Open
davidism opened this issue Jun 7, 2018 · 15 comments
Open

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

davidism opened this issue Jun 7, 2018 · 15 comments
Labels
refactoring Does the same thing but in a different way

Comments

@davidism
Copy link
Member

davidism commented Jun 7, 2018

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

davidism commented Jun 7, 2018

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

davidism commented Jun 7, 2018

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

davidism commented Jun 7, 2018

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

This comment has been minimized.

@davidism

This comment has been minimized.

@RemiZOffAlex

This comment has been minimized.

@azmeuk
Copy link
Member

azmeuk commented May 16, 2020

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

@davidism
Copy link
Member Author

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

davidism commented May 17, 2020

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) - field.process_formdata should still be called if formdata is empty #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 - field.process_formdata should still be called if formdata is empty #280 and StringField shouldn't blank data during process_formdata. #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
Copy link
Member Author

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

davidism commented May 17, 2020

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

Erm, I guess useful points are...

  • If you're going to have more than one POST form action associated with the same URL, you really want to be using something like a submit button to populate an action="<some action name"> field, to determine which form is being submitted.
  • Fields are always required, unless they have a default. Tho "required" just means some value must be included, even if it is None or "". We run validation for all values, including "" and None - it's the responsibility of the field to handle validating those cases, rather than the responsibility of the containing schema/form/thingumy.
  • All fields have an optional allow_null. If it is set to True, then we also set default=null. Fields coerce "" values to None if set.
  • For CharFields, we have both allow_null or allow_blank and options, and coerce inputs one way or the other. If allow_blank is set then we also set default="".
  • For booleans, we force users to explicitly add a default=False.

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

@davidism
Copy link
Member Author

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

@sblondon
Copy link
Contributor

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

  • Currently, it's possible to instanciate a Form without filling the formadata or obj parameters. I don't understand what's the point: if none of them are not provided, what is the goal of Form().validate() method? I wonder if it would help to change these optional parameters to force one of them to be filled. If both are None, an Exception would be raised.

  • Missing parameter should be accepted only if the Field accepts it. I think there could be a new parameter to define if it's ok to not post it. By default, it would be forbidden (like Tom Christie does with allow_null=False parameter), except for RadioField and CheckBoxField because the browsers don't send the value if unchecked. In case of not send, the value should be set to the default parameter. (allow_missing or missing_ok are better parameter names IMO.) However there would be a problem with PATCH: a missing field is ok but the default value is not what we want, it need to be known that the value was not filled. Using an boolean attribute on Field could be a solution (field.sent?).

  • Default parameter should provide a compatible type to the formatted data: for example, an IntegerField should provide a default value like 0, an UUIDField should provide a UUID instance (or an equivalent object according to duck typing), etc.

@WolfgangFahl
Copy link

WolfgangFahl commented Mar 23, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Does the same thing but in a different way
Development

No branches or pull requests

6 participants