add support for form data request #225

yedpodtrzitko · 2022-05-28T17:45:43Z

taking over where #185 was left. This is still very much WIP

the previous PR made form and json exclusive (ie. only one can be defined at the time), which I dont think it should be enforced. Here's a use-case where we have right now: we have /login/ endpoint, which can be called either from a legacy web which submits <form>, and it can be also used on a new frontend, which submits JSON payload. As such, one route can have both form and json.
previous PR is adding usage of cgi, which is about to be deprecated, so it is again removed here

spectree/plugins/falcon_plugin.py

yedpodtrzitko · 2022-06-26T14:54:50Z

spectree/plugins/falcon_plugin.py

+            # TODO - possible to pass the BodyPart here?
+            # req_form = {x.name: x for x in req.get_media()}
+            req_form = {x.name: x.stream.read() for x in req.get_media()}
+            req.context.form = form.parse_obj(req_form)


@vytas7 hi, you informed me previously about multipart forms support in Falcon here in this PR. I was looking into it, but I cant make it work in a way I'd be happy with.

Here we're parsing the request data & passing it into Pydantic model for validation, which is then making the data available in a view. I'd like to pass into Pydantic model directly the items returned from get_media() (is it BodyPart right?), but I think their content is getting evaluated while passing through Pydantic model, so when trying to read them again in the view later, it's returning just an empty bytes sequence. Which is what the docstring in get_media() says, but I dont know how to access the cached version of the data. Any advice please?

reading in view:

spectree/tests/test_plugin_falcon.py

Lines 190 to 193 in d6c4e74

# TODO - pass the BodyPart here?

# file_content = form.file.stream.read()

file_content = form.file

resp.media = {"file": file_content.decode("utf-8")}

Hi again @yedpodtrzitko!
Yes, Falcon's multipart support is designed for parsing the form on-the-fly, without buffering any data into the memory or temp files behind the user's back (because the former can make you run out of memory, and the latter kill performance, depending on what you're doing).

I'm not familiar with Spectree's codebase, what would you like to send into Pydantic, only the parts serialized as JSON(/YAML/msgpack/etc) and text fields, or also files? Would you like to spool file parts to tempfiles?

In general, if you want to pass a synchronous data structure into Pydantic, you'll need it to create a wrapper to store that data into when parsing.

yedpodtrzitko · 2022-06-26T14:56:19Z

spectree/plugins/falcon_plugin.py

+                res_data = {}
+                async for x in form_data:
+                    res_data[x.name] = x
+                    await x.data  # TODO - how to avoid this?


@vytas7 here in async version I dont know how to avoid passing the object through Pydantic model without awaiting it here. Any advice appreciated.

@yedpodtrzitko It depends what you are trying to achieve. Do you want to buffer whole files in memory? Or do you want to spool them to temp files like some other frameworks do?

Falcon even puts a cap on a maximum amount of data that can be referenced this way (await part.get_data()) in order to avoid surprises such as running out of memory.

Use await part.stream.read() to read the whole part as a bytestring, or await part.stream.pipe(async_file), or read by chunks, and store the result somewhere. You'll probably need to introduce some new object type to hold these attributes.

yedpodtrzitko · 2022-07-22T10:59:18Z

@kemingy how would you feel about making a breaking change and drop support for Falcon 2? It was released 2019 and never got any patch above 2.0.0 (see https://pypi.org/project/falcon/#history ), so it seems unsupported anyway.

The reason why I ask is it looks like it does not support form data. The pipeline right now fails because of get_media() does not exist in Falcon 2, so I tried to use req.media instead, and I got the following error (with Falcon 2)

AssertionError: {"title": "Unsupported media type", "description": "multipart/form-data is an unsupported media type."}

or maybe there's a way how to make it work I dont know about.

kemingy · 2022-07-24T15:48:48Z

Hi @yedpodtrzitko. I need to take some time to check whether it's possible to support this in falcon 2.

My concern is that falcon 2 will be the default one if users type pip install falcon between 2019-04-27 and 2021-04-06 (almost two years). There may be lots of legacy projects using falcon 2. If we are going to deprecate it, we'd better create a release with deprecate warning about falcon 2 before the final deprecation.

yedpodtrzitko · 2022-07-27T03:06:21Z

we'd better create a release with deprecate warning about falcon 2 before the final deprecation.

yes that's the standard practice and under normal circumstances I'd go that way too. However it seems like Falcon 2 is dead in the water - I haven't found any mention about its support anywhere and it didnt get any patch above 2.0.0. So if someone installed it in the timespan you mentioned and they havent upgraded by now, then Spectree's breaking change is the latest problem they could have.

Here's the docs for supported media types in Falcon 2 and Falcon 3. v3 has MEDIA_MULTIPART there, v2 has nothing even remotely close to it.

So I can understand the appeal to do things The Right Way, but I dont think it's worth it in this case. If this will be a breaking change, the next version should be 1.0.0, since breaking change should increase the major version number, right? And for the user who are really stuck with Falcon 2, the branch 0.10.x can still receive bugfixes for a period of time.

kemingy · 2022-07-27T23:50:22Z

So I can understand the appeal to do things The Right Way, but I dont think it's worth it in this case. If this will be a breaking change, the next version should be 1.0.0, since breaking change should increase the major version number, right? And for the user who are really stuck with Falcon 2, the branch 0.10.x can still receive bugfixes for a period of time.

I see. I'm okay with this breaking change in 1.0.0.

Will review drop support for Falcon 2 #239 ASAP

yedpodtrzitko · 2022-07-28T03:15:39Z

I see. I'm okay with this breaking change in 1.0.0.

Ok. Do you feel ready (feature-wise) to have the project bumped to version 1.0? I assume that's how you'd do it anyway, but will there be at least one beta/rc release before having final v1.0.0?

kemingy · 2022-07-28T03:23:05Z

I see. I'm okay with this breaking change in 1.0.0.

Ok. Do you feel ready (feature-wise) to have the project bumped to version 1.0? I assume that's how you'd do it anyway, but will there be at least one beta/rc release before having final v1.0.0?

Yep. I think we can have an RC release before the final v1.0.0.

yedpodtrzitko · 2022-07-29T06:57:59Z

~~Seems like Jinja 3.1+ has some breaking changes, so it needs to be pinned too. Probably in main branch instead of here.~~

edit: this seems to be issue only with Flask 1.x, but then it's up to the project to pin the jinja dependency as needed.

Pypy tests seems to be broken, will take a look later.

…or changes

yedpodtrzitko · 2022-07-30T09:00:16Z

I had to revert some changes from #185 which were making PyPy pipeline fail, but it seems to be all passing now.

I would prefer to not squash the commits, so the work of folks who were working on this previously is properly attributed to them (I squashed it to one commit per each author already).

kemingy · 2022-07-30T15:07:21Z

examples/falcon_demo.py

@@ -101,7 +94,22 @@ def on_post(self, req, resp, source, target):
            resp.media = {"loc": "unknown", "msg": "bad luck", "typ": "random"}
            return
        resp.media = {"label": int(10 * random()), "score": random()}
-        # resp.media = Resp(label=int(10 * random()), score=random())


It's another way to return the response (#212). I guess we can make it more clear in the comments.

kemingy · 2022-08-04T13:25:11Z

So far, most of the code changes look good to me. I'm going to merge this PR(also the original Form data support #185) since it's opened for a very long time.
Really appreciate your contributions @Michae1Weiss @yedpodtrzitko @vytas7
Also, I'll open another issue to keep track of the falcon form data read part. I'm not going to release a stable version until it's well handled.

vytas7 reviewed Jun 13, 2022

View reviewed changes

spectree/plugins/falcon_plugin.py Outdated Show resolved Hide resolved

yedpodtrzitko force-pushed the yed/form-data-support branch 6 times, most recently from b6e257d to 05b0d98 Compare June 26, 2022 14:36

yedpodtrzitko commented Jun 26, 2022

View reviewed changes

yedpodtrzitko mentioned this pull request Jul 22, 2022

fix: use mypy.ignore_missing_imports instead of suppressing them everywhere manually #238

Merged

yedpodtrzitko force-pushed the yed/form-data-support branch 4 times, most recently from a6ce91f to cdd9da1 Compare July 22, 2022 10:35

yedpodtrzitko mentioned this pull request Jul 23, 2022

drop support for Falcon 2 #239

Merged

Added form-data support

4edb22f

yedpodtrzitko force-pushed the yed/form-data-support branch from 61147e3 to a44af02 Compare July 29, 2022 06:18

yedpodtrzitko force-pushed the yed/form-data-support branch 2 times, most recently from 8effa18 to 6decd6e Compare July 30, 2022 08:35

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

f68c8a0

…or changes

yedpodtrzitko force-pushed the yed/form-data-support branch from 6decd6e to 5a2e0df Compare July 30, 2022 08:47

yedpodtrzitko changed the title ~~WIP: form data support~~ add support for form data request Jul 30, 2022

yedpodtrzitko requested a review from kemingy July 30, 2022 09:00

finish form data support

af3fff0

yedpodtrzitko force-pushed the yed/form-data-support branch from 5a2e0df to af3fff0 Compare July 30, 2022 14:01

Merge branch 'master' into yed/form-data-support

feb19e6

kemingy reviewed Aug 1, 2022

View reviewed changes

kemingy approved these changes Aug 4, 2022

View reviewed changes

kemingy merged commit 58f5e8f into 0b01001001:master Aug 4, 2022

This was referenced Aug 4, 2022

improve falcon form data read part #249

Open

Form data support #185

Closed

This was referenced Mar 22, 2023

How to specify mimetype for request body #148

Closed

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for form data request #225

add support for form data request #225

yedpodtrzitko commented May 28, 2022 •

edited

Loading

yedpodtrzitko Jun 26, 2022 •

edited

Loading

vytas7 Jun 27, 2022

yedpodtrzitko Jun 26, 2022

vytas7 Aug 2, 2022 •

edited

Loading

yedpodtrzitko commented Jul 22, 2022 •

edited

Loading

kemingy commented Jul 24, 2022

yedpodtrzitko commented Jul 27, 2022

kemingy commented Jul 27, 2022

yedpodtrzitko commented Jul 28, 2022

kemingy commented Jul 28, 2022

yedpodtrzitko commented Jul 29, 2022 •

edited

Loading

yedpodtrzitko commented Jul 30, 2022

kemingy Jul 30, 2022

kemingy commented Aug 4, 2022

	# TODO - pass the BodyPart here?
	# file_content = form.file.stream.read()
	file_content = form.file
	resp.media = {"file": file_content.decode("utf-8")}

add support for form data request #225

add support for form data request #225

Conversation

yedpodtrzitko commented May 28, 2022 • edited Loading

yedpodtrzitko Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

vytas7 Jun 27, 2022

Choose a reason for hiding this comment

yedpodtrzitko Jun 26, 2022

Choose a reason for hiding this comment

vytas7 Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

yedpodtrzitko commented Jul 22, 2022 • edited Loading

kemingy commented Jul 24, 2022

yedpodtrzitko commented Jul 27, 2022

kemingy commented Jul 27, 2022

yedpodtrzitko commented Jul 28, 2022

kemingy commented Jul 28, 2022

yedpodtrzitko commented Jul 29, 2022 • edited Loading

yedpodtrzitko commented Jul 30, 2022

kemingy Jul 30, 2022

Choose a reason for hiding this comment

kemingy commented Aug 4, 2022

yedpodtrzitko commented May 28, 2022 •

edited

Loading

yedpodtrzitko Jun 26, 2022 •

edited

Loading

vytas7 Aug 2, 2022 •

edited

Loading

yedpodtrzitko commented Jul 22, 2022 •

edited

Loading

yedpodtrzitko commented Jul 29, 2022 •

edited

Loading