Open mzpqnxow opened 11 months ago
As mentioned, the current behavior is exceptionally well documented here
Multipart requests with mixed types (file, form, etc.) are not supported. They can be achieved but the documentation is not correctly generated. arguments decorator can be called multiple times on the same view function but it should not be called with more that one request body location. This limitation is discussed in #46.
Thank you guys for tackling this. I support the use case and in fact I'd be happy to close that long standing issue.
I can't look at it right now. I'd rather make this a modification of arguments
if that works, even if it needs a major version due to a breaking change, as long as all former use cases can be covered.
Other interested users are welcome to try and review.
I shall try to carve out some time to compare with what arguments
does.
Feel free to open a PR to initiate work on this, get more attention and allow review comments.
Thank you guys for tackling this. I support the use case and in fact I'd be happy to close that long standing issue.
And thanks for providing the patient support and discussion, appreciative of the time
I can't look at it right now
I can relate :wink:
I'd rather make this a modification of
arguments
if that works, even if it needs a major version due to a breaking change, as long as all former use cases can be covered.
Gotcha, that was a question I had - would you prefer as a separate class, or "properly" integrated. It would be nice to have it Just Work :tm: with the existing use pattern/docs
I have yet to actually read and digest the current class as well as the subclass makeshift solution, but I will report back with what I see in terms of whether it can slip into place or requires more invasive refactoring
Other interested users are welcome to try and review.
Cheers to this, though I'll try to get to it if I can
I shall try to carve out some time to compare with what
arguments
does.
:+1:
Feel free to open a PR to initiate work on this, get more attention and allow review comments.
:+1:
Thank you guys for tackling this. I support the use case and in fact I'd be happy to close that long standing issue.
And thanks for providing the patient support and discussion, appreciative of the time
I can't look at it right now
I can relate 😉
I'd rather make this a modification of
arguments
if that works, even if it needs a major version due to a breaking change, as long as all former use cases can be covered.Gotcha, that was a question I had - would you prefer as a separate class, or "properly" integrated. It would be nice to have it Just Work ™️ with the existing use pattern/docs
I have yet to actually read and digest the current class as well as the subclass makeshift solution, but I will report back with what I see in terms of whether it can slip into place or requires more invasive refactoring
Other interested users are welcome to try and review.
Cheers to this, though I'll try to get to it if I can
I shall try to carve out some time to compare with what
arguments
does.👍
Feel free to open a PR to initiate work on this, get more attention and allow review comments.
👍
Super interested with the solution, if help is needed, I could contribute.
Thank you guys for tackling this. I support the use case and in fact I'd be happy to close that long standing issue.
And thanks for providing the patient support and discussion, appreciative of the time
I can't look at it right now
I can relate 😉
I'd rather make this a modification of
arguments
if that works, even if it needs a major version due to a breaking change, as long as all former use cases can be covered.Gotcha, that was a question I had - would you prefer as a separate class, or "properly" integrated. It would be nice to have it Just Work ™️ with the existing use pattern/docs
I have yet to actually read and digest the current class as well as the subclass makeshift solution, but I will report back with what I see in terms of whether it can slip into place or requires more invasive refactoring
Other interested users are welcome to try and review.
Cheers to this, though I'll try to get to it if I can
I shall try to carve out some time to compare with what
arguments
does.👍
Feel free to open a PR to initiate work on this, get more attention and allow review comments.
👍
Super interested with the solution, if help is needed, I could contribute.
Despite this being a relatively easy task, I haven't had time to set aside unfortunately. Will definitely take any of the offers that were made to assist 😀
@lafrech if you have a moment, please drop a note here if you support addressing the documented mixed multipart limitation in principle. If you do, I'll prepare a formal PR from the code below that @claudius-kienle contributed, unless he volunteers to do so
No hard feelings either way, and no rush whatsoever. I agree with your initial reaction, that this did not seem to be needing prioritization. It's an edge case, and a pattern that can be worked around easily in most cases
Background
Issue #46 was originally created as an ask for supporting multiple files in multipart requests. Today, multiple files are indeed supported and this is well documented 🙏
At some point, that issue was taken a little off-course with questions about other multipart scenarios- specifically mixed file and variable requests
I created this issue specifically for consideration of changes to the (documented) behavior/limitation for mixed multipart requests
The Problem
Currently, multipart requests that contain both files and variables work (that is, the decorated function gets what it should) but the OpenAPI documentation is not emitted fully. This is well-known and made clear to users, despite it being considered a reasonably rare pattern/edge-case
The Solution
The following was informally contributed by @claudius-kienle via the aforementioned issue
It has been used but not fully reviewed or properly tested
It parses mixed multipart form fields (files and variables) and generates the correct documentation
It can be added to the
ArgumentsMixin
class and then used in place of the existingArgumentsMixin::arguments
method which, in practice, is applied as a decorator after being mixed in to the Blueprint classNote, I did not write this code, but noticed that having it implemented this way (as an additional method, rather than a patch to the existing
arguments
method) allows it to function as "opt-in", which reduces the risk of breaking current usersOriginally posted by @claudius-kienle in https://github.com/marshmallow-code/flask-smorest/issues/46#issuecomment-1353828622
As far as I know, there was no PR created for this. As such, there is no proposed documentation update, no test-cases, no formal or informal code review
However, I would love to get rid of the ugly
git+https://...
from my projectrequire
statement at some point. I would also like to not meed to worry about occasionally checking if any sync is needed from upstreamThanks for your help on this @lafrech, regardless of where it goes from here