Support for mixed multipart forms

[mzpqnxow]

@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 existing ArgumentsMixin::arguments method which, in practice, is applied as a decorator after being mixed in to the Blueprint class

Note, 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 users

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

Originally 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 project require statement at some point. I would also like to not meed to worry about occasionally checking if any sync is needed from upstream

Thanks for your help on this @lafrech, regardless of where it goes from here

[mzpqnxow]

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.

[lafrech]

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.

[mzpqnxow]

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:

[fredericwyss]

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.

[mzpqnxow]

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 😀