-
Notifications
You must be signed in to change notification settings - Fork 73
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
Multipart requests #46
Comments
I think this is a bug/unimplemented feature, although I have not had time to investigate it or report it properly. Form data in OAS3 should be in the I suspect it could be fixed here to also move |
Thanks for the report and investigation, and sorry for the delay. https://swagger.io/docs/specification/describing-request-body/ AFAIU, apispec's mapping is correct for OAS2. When using OAS3, files and form should be sent as However, what we do in flask-rest-api is a bit wrong. In To maintain the OAS version abstraction, we should do the mapping in Here's a PR implementing this: #81. Now, we need to associate a content type to What should the mapping be? files: form: I'm thinking maybe we could modify Guys, what do you think? |
Hi, thanks for looking into this issue. I was actually looking at this again a few days ago. PR #81 makes sense to me, when we later extend it for locations other than json. Flask/webargs don't really care what content-type is set, right, so the cost for supporting more formats is not that high? I would definitely prefer being able to specify the content types. Right now I use Having a default of |
Ho @toonalbers. Thanks for your help on this. Here's a draft implementation: #83. It uses following mapping:
Those are default values than can be overridden in a Next steps:
Note that the content type here is only used for documentation purpose. Using |
Hi @lafrech. Looks good. Though, I'm wondering what a mixed form/file request (such as this) would look like with these default mappings. Would the result be something like this? I have not tried file requests myself, so maybe I'm also missing something in how the schema would be used. "requestBody": {
"content": {
"application/x-www-form-urlencoded": {
"schema": {
"type": "object",
"properties": {
"orderId": { "type": "integer" },
"userId": { "type": "integer" }
} } },
"multipart/form-data": {
"schema": {
"type": "object",
"properties": {
"fileName": {
"type": "string",
"format": "binary"
}
} } }
}
} I think the above would not be correct. So if this is the case, I would prefer just always having I agree with you on not adding more parsers yourself. If anyone should add it by default it would be webargs, like they do with JSON, but there's not much need. |
Yes, I overlooked multipart. In fact, my implementation assumes there is only one request body parameter (only one parameter entered as json, form or files). IIUC, when using multipart, there can be several parameters stuffed into the request, and those are documented as properties of the multipart content schema. I think I understand your point. You're saying that if there is both files and form, form must be part of the multipart as well. We'd need to merge mutipart parameters into the same multipart schema, rather than having 2 distincts multipart descriptions. We could add some logic to check if there are multiple arguments in form/files so as to enforce multipart. It might be easier to give the user the ability (and responsibility) to provide the proper content type. But even then, maybe adding that logic would be useful to provide sensible defaults. |
Ah, so it would not be as simple as I had hoped. Single file uploads and regular forms should work with #83 already, so that is nice. Merging arguments with the same |
Yes. Just to be sure, IIUC, we'd even need to merge arguments with location form and files if they both have the multipart content type. |
Yes, I think you are right. |
I pushed more commits in #83 to add the Feedback welcome. This is almost good to go. Only the multipart case is not handled completely (see above). If it is too complicated to achieve, maybe I'll leave it for later, as a "known issue". |
Hello, I am willing to dive into the issue. |
Thanks @ssfdust. That's interesting. #83 is about correctly documenting resources. I'll limit its scope to that. But of course, it's only useful if this lib doesn't get in the way when it comes to actually parsing a multipart request (not just documenting the resource). So in a further PR, we can do what is needed to allow that: do the necessary changes and either provide a helper or a piece of doc explaining how to do it in user code. I'd need to take a deeper look at your implementation. Meanwhile, what you can do is try the branch in #83 and see what would be needed to correctly document the multipart resource. I think we'd need to merge the different parts location into a single multipart. That would happen in |
I'd like that. In fact, I'd like to achieve json data in multipart/form-data. So I create a decorator for the view function. |
Good news. I finally found some time to sort this out. Turns out multipart could be achieved nicely with very few code in this lib, relying on webargs/apispec features. I merged #83 already (Fix doc of form and files arguments (content type, requestBody in OAS3), allow specifying content types). The remaining work to support and document multipart relies on features in apispec that will be released (hopefully soon) in 3.0. Demonstration in https://github.com/Nobatek/flask-rest-api/tree/document_multipart_files. Here's what it would look like. class MultipartSchema(ma.Schema):
file_1 = Upload()
file_2 = Upload()
@blp.route('/', methods=['POST'])
@blp.arguments(MultipartSchema, location='files')
def func(files):
print(files['file_1'].read().decode())
print(files['file_2'].read().decode())
)
files = {
'file_1': (io.BytesIO('Test 1'.encode()), 'file_1.txt'),
'file_2': (io.BytesIO('Test 2'.encode()), 'file_2.txt'),
}
response = client.post('/test/', data=files) The trick (which could also be seen as a limitation) is to declare a single I still need to properly test mixed content (file + other type in multipart) but I think we're close. As usual, feedback welcome. |
The code in the OP could read: from flask_rest_api.fields import Upload
class MultipartSchema(ma.Schema):
file = Upload()
@bp.route("/update-logo")
class UpdateLogo(MethodView):
@bp.arguments(MultipartSchema, location='files')
@bp.response(code=204)
def post(self):
file = files["logo"]
filename = secure_filename(file.filename)
binary = file.read() |
IIUC, this won't work for file + other data, because when sending a multipart request with file + data, files end up in To actually get the data, one needs to put it in another Schema with |
@ssfdust There's a huge rework going on in webargs and the |
Got it. Sorry for delay, I am really busy these days. |
I've been banging my head against this a little more. Unfortunately, auto-documenting mixed multipart request bodies is a bit complicated. There are issues because for instance @blp.arguments(FilesSchema, location='files', description='Some description')
@blp.arguments(FormSchema, location='form', description='Some other description') This implementation issues make me think that the sanest way to do that would be to require more explicitness from the user. We could add a Things would go relatively smooth if we could pass apispec a schema that would be the aggregation of the different schemas involved ( a single schema inheriting from all). This could avoid duplicating apispec code, or having to pull functions from there to do the job here. To be able to do so, we'd need to add a restriction to I don't think I'll do that for the next release. Once https://github.com/marshmallow-code/flask-smorest/tree/document_multipart_files is merged (almost done, just waiting for apispec 3.0), we'll be able to send files (using multipart), and to properly set a content type. This is already an improvement. We shall add a note in the docs saying mixed multipart is not supported. |
@lafrech Hey, just wanted to check on the status of this last part?
I wonder if there will be a way to do that in a cleaner way? |
@lafrech Is there any solution or workaround available yet? Could you maybe provide an example for uploading a file with additional fields, so that the file can be associated with another entity. Thank you very much! |
Any progress here? i am currently looking for a way how to work with this. |
Can this issue be closed, considering #83 landed a while ago? (Great work, @lafrech : )) |
Not really because the feature is not 100% complete. As I wrote in #46 (comment)
Yet, current implementation allows to upload files in a properly documented way so I don't expect to find the time to spend much effort on this anytime soon. |
I added following decorator. It's super ugly, but parses the form fields and generates the correct documentation: def multipart_arguments(self,
schema,
*,
content_type=None,
required=True,
description=None,
example=None,
examples=None,
**kwargs):
# At this stage, put schema instance in doc dictionary. Il will be
# replaced later on by $ref or json.
parameters = {
'in': 'files',
'required': required,
'schema': schema,
}
if content_type is not None:
parameters['content_type'] = content_type
if example is not None:
parameters['example'] = example
if examples is not None:
parameters['examples'] = examples
if description is not None:
parameters['description'] = description
error_status_code = kwargs.get('error_status_code', self.ARGUMENTS_PARSER.DEFAULT_VALIDATION_STATUS)
def decorator(func):
arg_parser = self.ARGUMENTS_PARSER.use_args(schema, location='files', **kwargs)
@wraps(func)
def wrapper(*f_args, **f_kwargs):
req = self.ARGUMENTS_PARSER.get_default_request()
from werkzeug.datastructures import ImmutableMultiDict
data = []
orig_files = req.files
orig_form = req.form
for key in req.files:
data.append((key, req.files[key]))
for key in req.form:
data.append((key, req.form[key]))
req.form = None
req.files = ImmutableMultiDict(data)
try:
result = arg_parser(func)(*f_args, **f_kwargs)
except Exception as e:
req.files = orig_files
req.form = orig_form
raise e
req.files = orig_files
req.form = orig_form
return result
# Add parameter to parameters list in doc info in function object
# The deepcopy avoids modifying the wrapped function doc
wrapper._apidoc = deepcopy(getattr(wrapper, '_apidoc', {}))
docs = wrapper._apidoc.setdefault('arguments', {})
docs.setdefault('parameters', []).append(parameters)
docs.setdefault('responses', {})[error_status_code] = http.HTTPStatus(error_status_code).name
# Call use_args (from webargs) to inject params in function
return wrapper
return decorator |
Hello All, My code as below:
and getting an error below:
Please help me. Thank you in advance. |
You're missing a parameter in the So you have to replace def post(self): with def post(self, data): |
Please guide me on how may I send additional payload parameters. Also those parameters should be visible to the Swagger-UI |
Hi All,
I am getting only file param nothing else. Please suggest Thanks in advance |
@DineshGuptaa |
Based on the age of the issue (and what seems like a lot of effort from various folks along the way to "fix" this- quotes because I understand it's not necessarily broken as it is a limitation) is it safe to say that this is not going to happen? Perhaps this is generally somewhat of an anti-pattern for a true "restful" API, but there's no clean way around it in my case that doesn't involve adding quite a bit more complexity (breaking a single request into two requests- imagine creating a task with some k/v pairs, then having to PUT to that with the file contents- not ideal for the application or for the user) |
Following up / poking this as it's been a while now
It's not clear to me: is there a workaround to do this that is just a bit clunky, or is there no way to do it without throwing away some key benefits of using flask-smorest? My users have a fixation on Swagger, and currently this issue means it will not have the file dialog for my mixed file/var endpoints- or it won't have the form vars. I want to ditch the docstring annotation-based solution I'm using now- it's bulky, requires being duplicated over and over for similar endpoints and provides no validation- but it allows a friendly Swagger UI with the form fields and the file parameters via a file upload dialog It may not be the most common pattern, but unfortunately, most (almost all) of my endpoints fit it in this case 😔 I think curl is the easiest to succinctly describe it, if it's not obvious what I'm describing:
I don't mean to ignore the docs, which are pretty clear on this, and appreciated:
I also don't mean to ignore the prior discussion on the issue; I've read through it at least 3 times recently and a few times some months ago. But it's still not clear to me if this has been "fixed" or (if not) if it ever will be I really want to use flask-smorest, but because so many of my endpoints are this way, it's not a one-off that I can deal with quickly by manually patching the generated specs, or by manually adding validation in one or two places; I mean, I could, but I'd rather not 😊 Sorry if the question isn't worth answering because nothing has changed, I'm misunderstanding, and/or I'm just rehashing the situation Thanks in advance for any classification or recommendations for workarounds. I suppose I could simply move the non-file parameters to the query string, right? Can flask-smorest validate query string params in a POST? (I'll re-read the docs to answer that one myself) Finally, thanks to the devs for this project, and to all, for the discussion on this issue. I sincerely appreciate it; and great job on the documentation with regard to this limitation. It saved me a ton of time second guessing my code when I first attempted to do this 😆 EDIT: My mistake for not considering the (possibly hacky) possibility of moving the text vars to query string params. I haven't done mixed query string/post body with flask/smorest, but perhaps that's a suitable way to do this? |
Quick answer before I let this go. It took me an important effort back then when implementing current solution with its limitations and now that I've put this aside for a while it would require an effort again just to remember/understand what the problem is. Plus that fact that I see it as a corner case : few users are affected, and this doesn't include myself... not much incentive to solve it. I don't think anything has changed in flask-smorest that would make this simpler to achieve today. Honestly, I didn't take the time to understand your specific use case (but it's nice you expose it here for other users that may be interested or for future reference). But I can at least answer that last one: yes, you may send query args in a POST. No pb. Keeping this open for discussion, people are welcome to help solve this, share workarounds, etc. |
Thanks for taking the time @lafrech, sincerely
That's fair, and I appreciate your initial effort as well as this brief but thoughtful update, even if it's not a commitment to invest a ton of time into "fixing" for my use-case (which I think is actually a slight variation on the one you already spent time unwinding and solving)
I tend to agree with your there, and I don't think it's even subjective; my thinking is that if it was a major/typical problem, there would be a lot more traffic on this issue I'm grateful it still received the thought and effort that it did. I always find myself surprised at how genuinely helpful some projects/individuals are when it comes to this sort of thing. I always prefer a "I/we did what I/we could, it's all that can be done for now, time is finite, priorities are a thing, ..." over being completely ignored, or being made an insincere or careless promise that will never be fulfilled
👌
Yeah, I actually didn't realize that the original issue (I think?) was for multiple independently named multipart files, not a mixture of files and variables. Though are obviously very similar from an HTTP protocol level perspective, obviously it's not necessarily so when it comes to generating the OpenAPI "stuff," as I understand it. I'm not too deeply experienced or familiar with OpenAPI spec files but I did see the difference when I ended up annotated a few endpoints in-line in the docstring with OpenAPI YaML
Ah that's quite helpful and I'm sorry for not having looked into that more deeply some time ago. Perhaps I need to just drop the "all arguments/variables must be in a POST" mentality for these endpoints, and use query params. What I wanted to avoid was making the endpoint path longer by embedding arguments into it, but for some stupid reason I had blocked out the (pretty standard practice) of using simple query parameters for those simple non-file variables. D'oh!
Thanks, would be great to see if anyone else has solved this in isolation and might be willing to contribute whatever they have. I'd be happy to re-base or bring it up to any standards required to merge And for what it's worth, I do intend to submit a PR that aims to better accommodate this (unusual-ish) pattern at some point. When I Have Time ™️ Either way I'll post back whatever I end up using, as you said, to potentially inform anyone else with the issue. The 3 or 4 of us that may be out there using this pattern 😆 Thanks again! |
@mzpqnxow I just used this #46 (comment) And it worked for me. @lafrech The problem with using 2 arguments (1 for files & 1 for body) is that it breaks openapi-client-generator (fails to validate the spec). |
Just noticed that after I posted my message hah I finished testing it a few minutes ago- not full testing, just ensuring my example produced what I expected- and came to report the same. It seems to work for what I need. The Swagger is nice and pretty with the clicky-clicky upload dialog as well as the non-file variables I forked and then committed that single addition, in case anyone wants to try it without manually pasting it into the class in Regardless of where this may go (meaning, whether it will eventually reach a state where it is merged - assuming it's even desired), anyone with this issue that needs to install via
Or, I've used the following in
Thanks again all |
People should just subclass the Blueprint and add the method, you probably won't maintain the fork. |
Ah yeah good point, a fork is a bit clunky and ridiculous when compared with a simple subclass. Thanks for suggesting this, I hadn't even thought about it Regardless, I created #557 as a separate issue, to formally offer the functionality as a PR in principle I'm happy with whatever is decided. I can keep a subclass in my project and not worry about goofy Thanks again, not sure why I jumped right over that as one of the ways to work around this... |
Hello!
I did not found standard way to describe a multipart request. Is it supported?
Currently i had to write something like this to let uploaded files work:
This code allows swagger to render input type="file" with name "logo" and send multipart request when executing request from web interface.
Am i missing something?
Thanks!
The text was updated successfully, but these errors were encountered: