Form data support #185

Michae1Weiss · 2021-12-13T13:00:16Z

Hi everyone!

This PR shows an example of support for uploading files via Swagger.

Usage example with werkzeug.datastructures.FileStorage (Flask):

from werkzeug.datastructures import FileStorage
from pydantic import BaseModel
from spectree import models

class File(BaseModel):
    uid: str
    file: models.BaseFile

@api.validate(form_data=File)
def with_file():
    file_dict = request.context.form_data.file.dict()
    file = FileStorage(**file_dict)
    return jsonify()
...

Issues:

If uploading files using the Falcon framework plugin - content_length is always 0.

Questions:

How to get content_length from falcon.cyutil.reader.BufferedReader object?
Initially SpecTree.validate only accepts json parameter to define request body. Would it be correct to separate request body schemas for multipart/form-data and application/json content types with different parameters (json and from_data) like I did or is there a cleaner way to do it?

For example, the 'drf-yasg' module uses a generic request_body parameter for both content types.
To define the content type 'drf-yasg' parses the view class object and if a view include only form parsers - swagger will support file upload.
drf-yasg implementation

Wouldn't it make sense to make a generic request_body parameter like in 'drf-yasg' and understand expected content type of the request body by some flag for example?

… fixes.

…ixes

kemingy · 2021-12-14T02:28:14Z

tests/test_plugin_falcon.py

+import falcon
+


Never been used?

kemingy · 2021-12-14T02:32:48Z

spectree/spec.py

                query = func.__annotations__.get("query", query)
                nonlocal json
                json = func.__annotations__.get("json", json)
+                nonlocal form_data
+                form_data = func.__annotations__.get("form_data", form_data)
                nonlocal headers
                headers = func.__annotations__.get("headers", headers)
                nonlocal cookies
                cookies = func.__annotations__.get("cookies", cookies)


Can combine all the nonlocal in one line.

kemingy · 2021-12-14T03:54:54Z

spectree/spec.py

+        if json and form_data:
+            # TODO: how to resolve conflict between json and form_data?
+            # TODO: Maybe replace 'form_data' with 'request_body' as it is done in 'drf-yasg'?
+            raise ValueError("You should provide either 'json' or 'form_data'.")


It's OK to raise exceptions here. Explicit is better than implicit.

I agree that combining these two parameters should be better. But I'm not sure if we should make some breaking changes now.

kemingy · 2021-12-14T16:35:49Z

spectree/plugins/falcon_plugin.py

+                # TODO: add support for other media types?
+                # TODO: OR handle unsupported media types?


Refs: https://www.iana.org/assignments/media-types/media-types.xhtml

How about the following?

font

video

message

model

I wonder what's the major difference among these types for file handlers?

kemingy · 2021-12-14T16:36:29Z

spectree/plugins/falcon_plugin.py

+                        # "content_length": len(part.data),  # FIXME: raises error if file is too big
+                        # sadly, BufferedReader don't have 'tell' method
+                        "content_length": 0,  # TODO: replace placeholder with something that will work
+                        "content_type": part.content_type,
+                        "stream": part.stream.read(),


Since we already consume the stream buffer, why not just use part.data to access the data and content length?

I'm not sure if it's possible to keep the buffer and let the user consume it later.

kemingy · 2021-12-14T16:37:07Z

spectree/plugins/falcon_plugin.py

@@ -182,12 +182,32 @@ def request_validation(self, req, query, json, headers, cookies):
            media = None
        if json:
            req.context.json = json.parse_obj(media)
+        elif form_data and media is not None:
+            data = {}
+            for part in media:  # TODO: add support for falcon 2.0 (media is always None for 'multipart/form-data')


BTW, async falcon should also be in the TODO list.

kemingy · 2021-12-14T16:50:19Z

spectree/plugins/starlette_plugin.py

        request.context = Context(
            query.parse_obj(request.query_params) if query else None,
            json.parse_raw(await request.body() or "{}") if json else None,
+            form_data.parse_obj(data or "{}") if form_data and data else None,


Suggested change

form_data.parse_obj(data or "{}") if form_data and data else None,

form_data.parse_obj(data or {}) if form_data and data else None,

kemingy · 2021-12-14T16:53:35Z

Thanks for your great work!

You can fix the lint by running make format and test with make lint.

…minor changes

…or changes

lgtm-com · 2021-12-20T19:08:58Z

This pull request introduces 1 alert when merging bad2b28 into 0f24b49 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

Michae1Weiss · 2021-12-20T19:22:59Z

@kemingy Thanks for review! I’ve added form-data support for async falcon and implemented various optimizations and fixes based on your comments. It's now time to write some tests for it :)
I will try to write them next week.
I'd be grateful for a second review and feedback. I'm not quite sure about some of the fixes (still need to learn more about asgi and cgi)

kemingy · 2021-12-21T06:26:49Z

Hi, @Michae1Weiss. Thanks for your contribution. It may take me some time to review the code. I will try to give you some feedback by the end of this week.

yihong0618 · 2021-12-21T06:30:40Z

spectree/plugins/starlette_plugin.py

@@ -38,9 +39,27 @@ def register_route(self, app):
            )

    async def request_validation(self, request, query, json, form_data, headers, cookies):
+        data = {}
+        if request.headers.get('content-type', '').split(";")[0] in self.FORM_MIMETYPE:


if get the default '' and split here index[0] will raise an error

Thanks for the feedback!
You're right, splitting an empty string with a None separator returns [] and ''.split()[0] will raise an error.
But in this case split() uses a separator ;. So ''.split(';')[0] will return an empty string
From Python documentation:
Splitting an empty string with a specified separator returns [''].
Refs: https://docs.python.org/3/library/stdtypes.html#str.split

got your point, you are right, cool~

yihong0618 · 2021-12-21T06:33:47Z

spectree/plugins/flask_plugin.py

+                form_dict.update(**request.files.to_dict())
+                if form:
+                    req_form = form_dict
+                elif json:


maybe if here?

elsewhere in this PR there is a condition that will raise exception if both json and form exist, so it seems like they are mutually exclusive

kemingy

Thanks for your great work. Feel free to leave some comments. Especially for the cgi.FileStorage part. I'm not very familiar with it. I'm not sure if I understand your code correctly.

kemingy · 2021-12-25T12:44:40Z

spectree/plugins/starlette_plugin.py

@@ -13,6 +13,7 @@

 class StarlettePlugin(BasePlugin):
    ASYNC = True
+    FORM_MIMETYPE = ("application/x-www-form-urlencoded", "multipart/form-data")


kemingy · 2021-12-27T16:41:48Z

spectree/plugins/flask_plugin.py

        req_headers = request.headers or {}
        req_cookies = request.cookies or {}
+
+        if request.mimetype in self.FORM_MIMETYPE:


Can we assume that the user should provide form instead of json here?

kemingy · 2021-12-27T16:58:32Z

spectree/plugins/falcon_plugin.py

+            req.env.setdefault("QUERY_STRING", "")
+
+            try:
+                _form = cgi.FieldStorage(fp=req.stream, environ=req.env)  # TODO: rename


UNSAFE behavior: req.env. This may cause the leak of the machine information.

kemingy · 2021-12-27T17:00:35Z

spectree/plugins/falcon_plugin.py

+
+            try:
+                _form = cgi.FieldStorage(fp=req.stream, environ=req.env)  # TODO: rename
+            except ValueError:  # Invalid boundary?


Will falcon.Request.bounded_stream be better to avoid this exception?

kemingy · 2021-12-27T17:06:34Z

spectree/plugins/falcon_plugin.py

+            encoding, filename = encoded.split("''")
+            field.filename = filename
+            field.file = BytesIO(field.file.read().encode(encoding))
+        if getattr(field, "filename", False):


I think getattr(field, "filename", False) will always return None since the type of field is always cgi.FieldStorage.

kemingy · 2021-12-27T17:10:02Z

spectree/plugins/falcon_plugin.py

+            return [self.parse_field(subfield) for subfield in field]
+
+        # When file name isn't ascii FieldStorage will not consider it.
+        encoded = field.disposition_options.get("filename*")


Is filename* a regex?

kemingy · 2021-12-27T17:12:57Z

spectree/plugins/falcon_plugin.py

+        # When file name isn't ascii FieldStorage will not consider it.
+        encoded = field.disposition_options.get("filename*")
+        if encoded:
+            encoding, filename = encoded.split("''")


Any example for this one? I'm not sure when will the field.filename contains ''.

yedpodtrzitko · 2022-05-22T05:04:03Z

spectree/plugins/flask_plugin.py

        req_headers = request.headers or {}
        req_cookies = request.cookies or {}
+
+        if request.mimetype in self.FORM_MIMETYPE:
+            body = form or json or None


what is or None for?

yedpodtrzitko · 2022-05-28T15:33:28Z

spectree/plugins/falcon_plugin.py

@@ -1,6 +1,9 @@
+import cgi


cgi is deprecated since Python 3.11

kemingy · 2022-08-04T13:29:36Z

close this because of add support for form data request #225

Mykhailo_Bilyi added 8 commits December 10, 2021 09:48

Added form-data support

35e5333

Added BaseFile model, comments

3e98f4b

Edited falcon_demo, deleted BaseFile's comments, update falcon plugin

827ccd1

Merge with master

f84d316

Added Swagger/Redoc form-data support for flask, updated 'flask_demo'

48b282a

Added multipart/form-data support for starlette, updated demos, minor…

3fd1364

… fixes.

Deleted PR message note.

eb65595

Added compatibility with werkzeug.datastructures.FileStorage, minor f…

e543921

…ixes

kemingy reviewed Dec 14, 2021

View reviewed changes

Michae1Weiss marked this pull request as draft December 15, 2021 07:35

Mykhailo_Bilyi and others added 2 commits December 17, 2021 18:08

Refactored form-data handling logic, falcon-asgi form-data snapshot, …

8b51413

…minor changes

Added multipart/form-data support for falcon-asgi, updated demos, min…

bad2b28

…or changes

yihong0618 reviewed Dec 21, 2021

View reviewed changes

kemingy reviewed Dec 27, 2021

View reviewed changes

kemingy mentioned this pull request May 19, 2022

[BUG] There is no way to create swagger file upload #85

Closed

yedpodtrzitko reviewed May 22, 2022

View reviewed changes

yedpodtrzitko reviewed May 28, 2022

View reviewed changes

spectree/plugins/falcon_plugin.py

@@ -1,6 +1,9 @@

import cgi

Copy link

Collaborator

yedpodtrzitko May 28, 2022 •

edited

Loading

Choose a reason for hiding this comment

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

cgi is deprecated since Python 3.11

yedpodtrzitko mentioned this pull request May 28, 2022

add support for form data request #225

Merged

kemingy closed this Aug 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Form data support #185

Form data support #185

Michae1Weiss commented Dec 13, 2021 •

edited

Loading

kemingy Dec 14, 2021

kemingy Dec 14, 2021

kemingy Dec 14, 2021

kemingy Dec 14, 2021

kemingy Dec 14, 2021

kemingy Dec 14, 2021

kemingy Dec 14, 2021

kemingy commented Dec 14, 2021

lgtm-com bot commented Dec 20, 2021

Michae1Weiss commented Dec 20, 2021

kemingy commented Dec 21, 2021

yihong0618 Dec 21, 2021

Michae1Weiss Dec 21, 2021 •

edited

Loading

yihong0618 Dec 21, 2021

yihong0618 Dec 21, 2021

yedpodtrzitko May 22, 2022

kemingy left a comment

kemingy Dec 25, 2021

kemingy Dec 27, 2021

kemingy Dec 27, 2021

kemingy Dec 27, 2021

kemingy Dec 27, 2021

kemingy Dec 27, 2021

kemingy Dec 27, 2021

yedpodtrzitko May 22, 2022

yedpodtrzitko May 28, 2022 •

edited

Loading

kemingy commented Aug 4, 2022

		# TODO: add support for other media types?
		# TODO: OR handle unsupported media types?

	form_data.parse_obj(data or "{}") if form_data and data else None,
	form_data.parse_obj(data or {}) if form_data and data else None,

Form data support #185

Form data support #185

Conversation

Michae1Weiss commented Dec 13, 2021 • edited Loading

Issues:

Questions:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kemingy commented Dec 14, 2021

lgtm-com bot commented Dec 20, 2021

Michae1Weiss commented Dec 20, 2021

kemingy commented Dec 21, 2021

Choose a reason for hiding this comment

Michae1Weiss Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kemingy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yedpodtrzitko May 28, 2022 • edited Loading

Choose a reason for hiding this comment

kemingy commented Aug 4, 2022

Michae1Weiss commented Dec 13, 2021 •

edited

Loading

Michae1Weiss Dec 21, 2021 •

edited

Loading

yedpodtrzitko May 28, 2022 •

edited

Loading