Skip to content
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

Wrong schema for FileField in model with default parser classes #386

Closed
maun opened this issue Jun 17, 2019 · 8 comments
Closed

Wrong schema for FileField in model with default parser classes #386

maun opened this issue Jun 17, 2019 · 8 comments

Comments

@maun
Copy link

maun commented Jun 17, 2019

When using a FileField in the model, a ModelSerializer, and a ModelViewSet DRF generates a endpoint where the file is uploaded using the MIME type multipart/form-data.

drf-yasg does not recognize this and instead skips the File field in the request but includes the other parameters in the scheme as JSON body parameters.

When setting the parser classes parser_classes = [FormParser, MultiPartParser] on the viewset the correct scheme is generated using form parameters. Unfortunately this is not an option because the update endpoint of the ModelViewSet requires the JSONParser.
Also adding the parameter manually as a form parameter does not work, this error is thrown:

raise SwaggerGenerationError("cannot add form parameters when the request has a request body; "

@daviddavis
Copy link

daviddavis commented Jun 24, 2019

I'm dealing with the same problem. However, it looks like the accepted mimetypes for an endpoint that accepts a file upload is json and multipart/form-data:

(Pdb) request.parsers
[<rest_framework.parsers.JSONParser object at 0x7f2eb3352b00>, <rest_framework.parsers.FormParser object at 0x7f2eb3352b38>, <rest_framework.parsers.MultiPartParser object at 0x7f2eb3352b70>]

I assume that somehow drf-yasg is picking the JSONParser and then excluding the FileField because it can't be submitted via json.

You can customize the parser_classes with the @action decorator but I don't think that it's available for predefined routes like create, update, etc. At least, I get the error: Cannot use the @action decorator on the following methods, as they are existing routes: create when I try to do so.

Perhaps drf-yasg could add an option to @swagger_auto_schema where you could manually set the consumes media type? Or perhaps drf-yasg could set the consumes media type to multipart form if there's a multipart parser and a FileField?

@daviddavis
Copy link

This patch worked for me although I am sure there's a better way to do this:

diff --git a/src/drf_yasg/inspectors/view.py b/src/drf_yasg/inspectors/view.py
index 334c274..c7d8c06 100644
--- a/src/drf_yasg/inspectors/view.py
+++ b/src/drf_yasg/inspectors/view.py
@@ -29,6 +29,12 @@ class SwaggerAutoSchema(ViewInspector):
         consumes = self.get_consumes()
         produces = self.get_produces()
 
+        multipart = ['multipart/form-data', 'application/x-www-form-urlencoded']
+        if 'file' in [param['type'] for param in self.get_request_body_parameters(multipart)]:
+            # automatically set the media type to form data if there's a file
+            # needed due to https://github.com/axnsan12/drf-yasg/issues/386
+            consumes = multipart
+
         body = self.get_request_body_parameters(consumes)
         query = self.get_query_parameters()
         parameters = body + query

@ychab
Copy link
Contributor

ychab commented Aug 7, 2019

👍

No matter if you set manually parser_classes = (MultiPartParser, JSONParser) or parser_classes = (JSONParser, MultiPartParser), the MultiPartParser parser would be ignored... You must set it alone: parser_classes = (MultiPartParser,)

And I found the guilty: https://github.com/axnsan12/drf-yasg/blob/master/src/drf_yasg/utils.py#L380

There is no comment so I'm just wondering why...?? Sure there is a good reason to do this??

Indeed, because by default we have the following parsers:

    'DEFAULT_PARSER_CLASSES': [
        'rest_framework.parsers.JSONParser',
        'rest_framework.parsers.FormParser',
        'rest_framework.parsers.MultiPartParser',
    ],

@see https://github.com/encode/django-rest-framework/blob/master/rest_framework/settings.py#L10

we should always returns just the application/json media type if you don't override the parser classes...
So it means we must always override parser_classes to set it only to MultiPartParser when you have to play with file/images (or at least, returns only form media types). Which also means we have to manage to have only one endpoint in order to override its parsers only (with modelviewset, some other endpoints may require non form media types, aka application/json, like retrieve, list, etc?

Sure not... Maybe a good way would be to allow multiples media types and let' the end-user select it into the drop-down list in Swagger UI for parameters? (no idea for redoc)

But... is that possible? No restriction with the core API?
I'll try to dig into!

@ychab
Copy link
Contributor

ychab commented Aug 7, 2019

I have created a pull request: #436
Not sure if this patch is good so awaiting for a better fix...

@ychab
Copy link
Contributor

ychab commented Aug 12, 2019

Ok bad news, it seens to not be possible to enforce this media content type without having to set it manually... See the merge request update with comments... :/

@axnsan12
Copy link
Owner

I know this response is way late but it may help others who stumble here.

The comments here clarified the problem quite well. The only way to have file upload in swagger is to make the request accept form parameters instead of json. You can use @action on a viewset, the parser_classes class attr, or override the get_parsers method to achieve this.

Perhaps drf-yasg could add an option to @swagger_auto_schema where you could manually set the consumes media type? Or perhaps drf-yasg could set the consumes media type to multipart form if there's a multipart parser and a FileField?

You can subclass SwaggerAutoSchema and implement that logic yourself, maybe using **extra_overrides/self.overrides. Changing the return of get_consumes should be enough.

alles-klar added a commit to alles-klar/django-DefectDojo that referenced this issue Nov 25, 2019
madchap pushed a commit to madchap/django-DefectDojo that referenced this issue Dec 23, 2019
@theoden-dd
Copy link

Why does it ignore the parser_classes decorator on a view set method?

@MatejMijoski
Copy link

MatejMijoski commented Jul 14, 2021

Why does it ignore the parser_classes decorator on a view set method?

I noticed the same thing. The @parser_classes decorator doesn't get picked up by the generator.

dkliban added a commit to dkliban/pulpcore that referenced this issue Jun 6, 2022
The code[0] for special handling of FileFields when generating the OpenAPI schema was
introduced to work around a bug in drf-yasg[1]. This code was then ported when pulpcore
switched to using drf_spectacular for generating OpenAPI. This code is not needed. It
actually causes the OpenAPI schema for Content creation APIs to be different from all
other create operations.

[0] pulp@24b5071#diff-0535b5e0e76e73cdc44991f38a187114c123eb5ba125bda3015ca222a2b18088
[1] axnsan12/drf-yasg#386

closes pulp#2806
dkliban added a commit to dkliban/pulpcore that referenced this issue Jun 6, 2022
The code[0] for special handling of FileFields when generating the OpenAPI schema was
introduced to work around a bug in drf-yasg[1]. This code was then ported when pulpcore
switched to using drf_spectacular for generating OpenAPI. This code is not needed. It
actually causes the OpenAPI schema for Content creation APIs to be different from all
other create operations.

[0] pulp@24b5071#diff-0535b5e0e76e73cdc44991f38a187114c123eb5ba125bda3015ca222a2b18088
[1] axnsan12/drf-yasg#386

closes: pulp#2806
dkliban added a commit to dkliban/pulpcore that referenced this issue Jun 6, 2022
The code[0] for special handling of FileFields when generating the OpenAPI schema was
introduced to work around a bug in drf-yasg[1]. This code was then ported when pulpcore
switched to using drf_spectacular for generating OpenAPI. This code is not needed. It
actually causes the OpenAPI schema for Content creation APIs to be different from all
other create operations.

[0] pulp@24b5071#diff-0535b5e0e76e73cdc44991f38a187114c123eb5ba125bda3015ca222a2b18088
[1] axnsan12/drf-yasg#386

closes: pulp#2806
dkliban added a commit to dkliban/pulpcore that referenced this issue Jun 6, 2022
The code[0] for special handling of FileFields when generating the OpenAPI schema was
introduced to work around a bug in drf-yasg[1]. This code was then ported when pulpcore
switched to using drf_spectacular for generating OpenAPI. This code is not needed. It
actually causes the OpenAPI schema for Content creation APIs to be different from all
other create operations.

[0] pulp@24b5071#diff-0535b5e0e76e73cdc44991f38a187114c123eb5ba125bda3015ca222a2b18088
[1] axnsan12/drf-yasg#386

closes: pulp#2806
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants