Repository: MechanicalSoup/MechanicalSoup

Issue:
Respect enctype specified in forms
MechanicalSoup currently ignores the enctype specified for a form when submitting it, instead using the default used by requests (which is application/x-www-form-urlencoded unless you specify "files" to upload).  I'd like it to behave more like a browser by respecting the given enctype (apparently you should specify "files" to requests to upload form fields that aren't files as multipart/form-data - which is the case I'm concerned with, making sure to submit as multipart/form-data when that's what the form specifies as its enctype).  This is for testing web applications which behave differently when you submit with the wrong enctype (concretely, the Python cgi module defaults to ignoring fields with empty values for application/x-www-form-urlencoded but not for multipart/form-data, so any application using that may behave differently for submissions with the wrong enctype).


Discussion:
moy: OK, I think the code is now ready to be merged. There are multiple features in one (see the bullet list in the commit message), but they are too intricate to be easily split into separate commits. I still split the tests major modification and the actual feature, so that the second commit documents what changes in the tests.
moy: > Is this change worth the complexity that it adds?

Note that most of the complexity is in tests, and testing all cases "with file input, without / possible enctype" is a good thing IMHO regardless of the actual behavior of MechanicalSoup.

> I am happy with MechanicalSoup wrapping Requests functionality rather than mimicking what browsers do at any cost

There are several things in this PR:

1. only "multipart/form-data" enctype allow file sending

2. forms without file inputs can now be sent as "multipart/form-data"
    if specified.

3. "application/x-www-form-urlencoded" is the default when enctype is wrong
    or not specified

Actually, 1. and 3. are relevant features for people using MechanicalSoup to test their website, as they will miss obvious bugs otherwise (missing `enctype` in `<form ...>`). Honestly, I have no idea why things are done this way, if I were inventing the web today I'd switch to `multipart/form-data` whenever there's a file input, but that's not what browsers do.

Item 2. is more a detail, I can't think of any real-life use-case for it, but it shouldn't harm. The [implementation is a bit hacky](https://github.com/MechanicalSoup/MechanicalSoup/pull/290/files#diff-ae060c6024d83a9454f5bfb9144bdef2R233-R241) and does rely on a requests implementation detail, but it's a very local hack, easy enough to remove if it ever becomes a problem. Since it's not a feature many users are likely to rely on, it's also not a huge problem if we introduce this feature and remove it afterwards if requests totally breaks our code.

So I think the benefits of merging the PR are larger than the cost.

Patch:
diff --git a/mechanicalsoup/browser.py b/mechanicalsoup/browser.py
index 35e4047..3979c64 100644
--- a/mechanicalsoup/browser.py
+++ b/mechanicalsoup/browser.py
@@ -155,6 +155,8 @@ class Browser(object):
         # Requests also retains order when encoding form data in 2-tuple lists.
         data = [(k, v) for k, v in data.items()]
 
+        multipart = form.get("enctype", "") == "multipart/form-data"
+
         # Process form tags in the order that they appear on the page,
         # skipping those tags that do not have a name-attribute.
         selector = ",".join("{}[name]".format(i) for i in
@@ -175,18 +177,17 @@ class Browser(object):
                     # browsers use empty string for inputs with missing values
                     value = tag.get("value", "")
 
-                if tag.get("type", "").lower() == "file":
-                    # read http://www.cs.tut.fi/~jkorpela/forms/file.html
-                    # in browsers, file upload only happens if the form
-                    # (or submit button) enctype attribute is set to
-                    # "multipart/form-data". We don't care, simplify.
+                # If the enctype is not multipart, the filename is put in
+                # the form as a text input and the file is not sent.
+                if tag.get("type", "").lower() == "file" and multipart:
                     filename = value
                     if filename != "" and isinstance(filename, string_types):
                         content = open(filename, "rb")
                     else:
                         content = ""
-                    # If value is the empty string, we still pass it for
-                    # consistency with browsers (see #250).
+                    # If value is the empty string, we still pass it
+                    # for consistency with browsers (see
+                    # https://github.com/MechanicalSoup/MechanicalSoup/issues/250).
                     files[name] = (filename, content)
                 else:
                     data.append((name, value))
@@ -223,6 +224,22 @@ class Browser(object):
         else:
             kwargs["data"] = data
 
+        # The following part of the function is here to respect the
+        # enctype specified by the form, i.e. force sending multipart
+        # content. Since Requests doesn't have yet a feature to choose
+        # enctype, we have to use tricks to make it behave as we want
+        # This code will be updated if Requests implements it.
+        if multipart and not files:
+            # Requests will switch to "multipart/form-data" only if
+            # files pass the `if files:` test, so in this case we use
+            # a modified dict that passes the if test even if empty.
+            class DictThatReturnsTrue(dict):
+                def __bool__(self):
+                    return True
+                __nonzero__ = __bool__
+
+            files = DictThatReturnsTrue()
+
         return self.session.request(method, url, files=files, **kwargs)
 
     def submit(self, form, url=None, **kwargs):


Test patch:
diff --git a/tests/test_browser.py b/tests/test_browser.py
index 860f224..e16ec7f 100644
--- a/tests/test_browser.py
+++ b/tests/test_browser.py
@@ -81,54 +81,95 @@ def test__request(httpbin):
         "Content-Type"]
 
 
-@pytest.mark.parametrize('expected_content, set_value', [
-    (b":-)", True),
-    (b"", False)
+valid_enctypes_file_submit = {"multipart/form-data": True,
+                              "application/x-www-form-urlencoded": False
+                              }
+
+default_enctype = "application/x-www-form-urlencoded"
+
+
+@pytest.mark.parametrize("file_field", [
+  """<input name="pic" type="file" />""",
+  ""])
+@pytest.mark.parametrize("submit_file", [
+    True,
+    False
+])
+@pytest.mark.parametrize("enctype", [
+  pytest.param("multipart/form-data"),
+  pytest.param("application/x-www-form-urlencoded"),
+  pytest.param("Invalid enctype")
 ])
-def test__request_file(httpbin, expected_content, set_value):
+def test_enctype_and_file_submit(httpbin, enctype, submit_file, file_field):
+    # test if enctype is respected when specified
+    # and if files are processed correctly
     form_html = """
-    <form method="post" action="{}/post">
-      <input name="pic" type="file" />
+    <form method="post" action="{}/post" enctype="{}">
+      <input name="in" value="test" />
+      {}
     </form>
-    """.format(httpbin.url)
+    """.format(httpbin.url, enctype, file_field)
     form = BeautifulSoup(form_html, "lxml").form
 
-    if set_value:
+    valid_enctype = (enctype in valid_enctypes_file_submit and
+                     valid_enctypes_file_submit[enctype])
+    expected_content = b""  # default
+    if submit_file and file_field:
         # create a temporary file for testing file upload
+        file_content = b":-)"
         pic_filedescriptor, pic_path = tempfile.mkstemp()
-        os.write(pic_filedescriptor, expected_content)
+        os.write(pic_filedescriptor, file_content)
         os.close(pic_filedescriptor)
-
+        if valid_enctype:
+            # Correct encoding => send the content
+            expected_content = file_content
+        else:
+            # Encoding doesn't allow sending the content, we expect
+            # the filename as a normal text field.
+            expected_content = pic_path.encode()
         form.find("input", {"name": "pic"})["value"] = pic_path
 
     browser = mechanicalsoup.Browser()
     response = browser._request(form)
 
-    # Check that only "files" includes a "pic" keyword in the response
+    if enctype not in valid_enctypes_file_submit:
+        expected_enctype = default_enctype
+    else:
+        expected_enctype = enctype
+    assert expected_enctype in response.request.headers["Content-Type"]
+
+    resp = response.json()
+    assert resp["form"]["in"] == "test"
+
     found = False
-    for key, value in response.json().items():
-        if key == "files":
-            if "pic" in value:
-                if value["pic"].encode() == expected_content:
-                    # If pic is found twice, an error will occur
-                    assert not found
-                    found = True
-        # One would expect to find "pic" in files, but as of writing,
-        # httpbin puts it in form when the filename is empty:
-        elif key == "form":
+    found_in = None
+
+    for key, value in resp.items():
+        if value:
             if "pic" in value:
-                if value["pic"].encode() == expected_content:
-                    assert not found
-                    found = True
-                    assert b"filename=\"\"" in response.request.body
+                content = value["pic"].encode()
+                assert not found
+                assert key in ("files", "form")
+                found = True
+                found_in = key
+            if key == "files" and not valid_enctype:
+                assert not value
+
+    assert found == bool(file_field)
+    if file_field:
+        assert content == expected_content
+
+        if valid_enctype:
+            assert found_in == "files"
+            if submit_file:
+                assert ("filename=\"" + pic_path + "\""
+                        ).encode() in response.request.body
+            else:
+                assert b"filename=\"\"" in response.request.body
         else:
-            assert (value is None) or ("pic" not in value)
-
-    assert found
-    assert "multipart/form-data" in response.request.headers["Content-Type"]
+            assert found_in == "form"
 
-    # In case we created a file for upload, we need to close & delete it
-    if set_value:
+    if submit_file and file_field:
         os.remove(pic_path)
 
 
diff --git a/tests/test_stateful_browser.py b/tests/test_stateful_browser.py
index 97147ae..f581d46 100644
--- a/tests/test_stateful_browser.py
+++ b/tests/test_stateful_browser.py
@@ -339,8 +339,10 @@ def test_upload_file(httpbin):
                     ("first file content", "second file content"))
 
     # The form doesn't have a type=file field, but the target action
-    # does show it => add the fields ourselves.
+    # does show it => add the fields ourselves, and add enctype too.
     browser.select_form()
+    browser._StatefulBrowser__state.form.form[
+      "enctype"] = "multipart/form-data"
     browser.new_control("file", "first", path1)
     browser.new_control("file", "second", "")
     browser["second"] = path2