When setting location to `files`, custom field's `_deserialize` and `_validated` methods were not called

[greyli]

When setting location to `files`, custom field's `_deserialize` and `_validated` methods were not called

[sirosen]

Are you sure that the difference is between your form_and_files location and the files location? When I test out your custom field and location, it seems to me that it's working correctly.

Here's a runnable example script

import io

import flask
import marshmallow as ma
from webargs.multidictproxy import MultiDictProxy
from webargs.flaskparser import use_args, parser
from werkzeug.datastructures import FileStorage


@parser.location_loader("form_and_files")
def load_form_and_files(request, schema):
    form_and_files_data = request.files.copy()
    form_and_files_data.update(request.form)
    return MultiDictProxy(form_and_files_data, schema)


class FileField(ma.fields.Field):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.metadata["type"] = "string"
        self.metadata["format"] = "binary"

    default_error_messages = {"invalid": "Not a valid file."}

    def _deserialize(self, value, attr, data, **kwargs):
        print("in _deserialize")
        if not isinstance(value, FileStorage):
            raise self.make_error("invalid")
        return value


class UploadSchema(ma.Schema):
    photo = FileField()


app = flask.Flask(__name__)


@app.route("/alpha", methods=["POST"])
@use_args(UploadSchema, location="files")
def alpha(args):
    print(args)
    return {}


@app.route("/beta", methods=["POST"])
@use_args(UploadSchema, location="form_and_files")
def beta(args):
    print(args)
    return {}


with app.test_client() as client:
    print("---test /alpha")
    response = client.post(
        "/alpha",
        data={"photo": (io.BytesIO(b"file contents"), "test.txt")},
    )
    print("---test /beta")
    response = client.post(
        "/beta",
        data={"photo": (io.BytesIO(b"file contents"), "test.txt")},
    )

And this is the output I see:

---test /alpha
in _deserialize
{'photo': <FileStorage: 'test.txt' ('text/plain')>}
---test /beta
in _deserialize
{'photo': <FileStorage: 'test.txt' ('text/plain')>}

The /alpha route uses files and the /beta route uses form_and_files, defined just as you have it.

If _deserialize isn't being called, I think something else must be going on.
My versions:

$ pip freeze | grep -i -E 'webargs|flask'
Flask==2.1.2
webargs==8.1.0

[greyli]

Thanks for the help. I forgot to mention that it did call the _deserialize if the value is a file. While if the value is not a file, then the _deserialize will be skipped:

with app.test_client() as client:
    print("---test /alpha")
    response = client.post(
        "/alpha",
        data={"photo": "string"},  # <--
    )
    print("---test /beta")
    response = client.post(
        "/beta",
        data={"photo": (io.BytesIO(b"file contents"), "test.txt")},
    )

But I need the _deserialize to be called so that I can raise an error if the user doesn't pass a file to this field.

[sirosen]

If I test that same demo app with a non-file post for the "file field", I'm seeing the _deserialize method get called as expected.

i.e. With the above, if I add this snippet:

with app.test_client() as client:
    response = client.post("/beta", data={"photo": "foo"})
    print(response)

I see <WrapperTestResponse streamed [422 UNPROCESSABLE ENTITY]> added to the output, meaning that the desired ValidationError was raised.

---

So far I've not been able to reproduce this issue, so I'm inclined to think this is a usage error. Perhaps somewhere you've used location="files" by mistake.

If webargs or marshmallow is doing something wrong or unexpected, we need more information. I think the next best step is to try to create a complete and minimal reproduction similar to the sample script I shared, since that way we can run the same code and verify the behavior. Something which might help is to set required=True on the field(s) you want to test, since that eliminates the possibility of them being completely absent from the data.

[sirosen]

A moment after I wrote

Perhaps somewhere you've used location="files" by mistake.

I realized that this may be the cause of the issue, and I'm rereading the original.

flask.request.files gets populated only with valid file uploads, not other strings.
So if you send data={"photo": "foo"} using a flask test client, flask.request.files is ImmutableMultiDict([]) (an empty multidict). That's a flask/werkzeug behavior which location="files" in webargs therefore inherits.

I'll reread to confirm, but I think we're seeing the same (correct/expected) behavior, but with different expectations about what should happen.

[greyli]

It's a bit weird... If I run the same script with a string as a file, it returns 200 (instead of 422), and _deserialize has not got called.

(venv) ➜  testapispec pip freeze | grep -i -E 'webargs|flask|werkzeug'
Flask==2.1.2
webargs==8.1.0
Werkzeug==2.1.2

(venv) ➜  testapispec cat app.py
import io

import flask
import marshmallow as ma
from webargs.multidictproxy import MultiDictProxy
from webargs.flaskparser import use_args, parser
from werkzeug.datastructures import FileStorage


@parser.location_loader("form_and_files")
def load_form_and_files(request, schema):
    form_and_files_data = request.files.copy()
    form_and_files_data.update(request.form)
    return MultiDictProxy(form_and_files_data, schema)


class FileField(ma.fields.Field):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.metadata["type"] = "string"
        self.metadata["format"] = "binary"

    default_error_messages = {"invalid": "Not a valid file."}

    def _deserialize(self, value, attr, data, **kwargs):
        print("in _deserialize")
        if not isinstance(value, FileStorage):
            raise self.make_error("invalid")
        return value


class UploadSchema(ma.Schema):
    photo = FileField()


app = flask.Flask(__name__)


@app.route("/alpha", methods=["POST"])
@use_args(UploadSchema, location="files")
def alpha(args):
    print(args)
    return {}


@app.route("/beta", methods=["POST"])
@use_args(UploadSchema, location="form_and_files")
def beta(args):
    print(args)
    return {}


with app.test_client() as client:
    print("---test /alpha")
    response = client.post(
        "/alpha",
        # data={"photo": (io.BytesIO(b"file contents"), "test.txt")},
        data={"photo": "test"},
    )
    print(response.status_code)
    print("---test /beta")
    response = client.post(
        "/beta",
        data={"photo": (io.BytesIO(b"file contents"), "test.txt")},
    )

(venv) ➜  testapispec python app.py
---test /alpha
{}
200
---test /beta
in _deserialize
{'photo': <FileStorage: 'test.txt' ('text/plain')>}

---

If I send the post with an API client (i.e. Insomnia) instead of Flask's test client, it still returns 200, and _deserialize still has not got called.

This behavior only happens when I use location="files". It's working properly if I use the custom location form_and_files.

In my opinion, when using location="files", the field's _deserialize/_validated should always get called no matter whether the user sends a file or a string, so I can validate it in the _deserialize/_validated methods.

[greyli]

Ah, I know what you mean. Since the request.files is empty, so it's not possible to trigger the _deserialize on fields. I finally make it work by rewriting the files location:

@parser.location_loader('files')
def load_files(request, schema):
    files_data = request.files

    if not files_data:  # make sure the data is not empty
        files_data = request.form  # the failed parsed file fields goes into request.form
    
    return MultiDictProxy(files_data, schema)

Do you think we should change the default behavior of webargs to make sure the files data is not empty? I just made a PR (#721) to keep this easier to understand. We can continue the discussion in that PR. Thanks!

[sirosen]

@greyli, thanks for putting in the work on the PR, including looking into the problems it would cause and making the decision to self-close! The changes we don't make are as important as the ones which we do make.

Per the closing comment #721, automatically combining files and form would lead to problems. At the very least, it would need to be a major release (9.0), but I'm also not sure it's a good idea.

To offer a potential improvement which we could make, would a form_and_files location be a good addition? I haven't thought about this very hard, but if you're doing this in your library it's possible it would be useful to direct webargs consumers too. We could even define it on the base Parser class, making it available in other frameworks -- although we'd need to think about how it behaves on each framework.