File upload is too aggressive

[laucia]

from mock import Mock
from bravado_core.param import add_file, Param
from bravado_core.operation import Operation
from bravado_core.spec import Spec
empty_swagger_spec = Spec(spec_dict={})
request = {}
op = Mock(spec=Operation, consumes=['multipart/form-data'])
param_spec = {
    'type': 'file',
    'in': 'formData',
    'name': 'photo'
}
param = Param(empty_swagger_spec, op, param_spec)
add_file(param, None, request)
print request

results in {'files': [('photo', ('photo', None))]}

feels like this should be {} as this results in bravado always trying to upload an empty/inexisting file if it is defined in the spec, which doesn't play well with most bravado clients (Fido and RequestsClient).

[prat0318]

I am wondering if add_file should throw up if value is not of file type instead of just ignoring it (as per the PR).

[laucia]

That's would then also require to refactor construct_params in bravado to handle it

        for param_name, param_value in iteritems(op_kwargs):
            param = current_params.pop(param_name, None)
            if param is None:
                raise SwaggerMappingError(
                    "{0} does not have parameter {1}"
                    .format(self.operation.operation_id, param_name))
            marshal_param(param, param_value, request)

the last line will now raise when no file is given in the op_kwargs instead of "just working".

[bpicolo]

@prat0318 if value is not of file type is a bit less obvious in py3, keep in mind. Make sure it has some .read() for bytes or whatnot is the tell

[prat0318]

@laucia my opinion is that if validate_requests is set, the validation is done here and if it does not throw up when value doesn't "look" like a file then that is a bug, which should be fixed.

the last line will now raise when no file is given in the op_kwargs instead of "just working".

Isn't it an undesired behavior? Why should it just work?

@bpicolo agreed.

[laucia]

So, just to be clear I found out that we bravado was setting some files to None when not provided the hard way. I'm not super sure who (or what) is adding None in the first place, so rather than following the full intrication of bravado_core and bravado client calls, I thought it couldn't hurt to add a guard there which would made bravado usable with files. In the meanwhile I had to rewrite a 1.2 spec and use swaggerpy, which made me sad.

If I find time, I will do a step debugging with a simpler test case reproducing the issue, to know what messes up, and will open a ticket accordingly.

@prat0318 there is no validation for files in validate_schema_object. That could deserve a ticket yes.

[diegodelemos]

what is the status for solving this issue?