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

Fix multiform bug #403

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jingapore
Copy link

This PR fixes the bug where Swagger does not interpreting an API consuming an array of files, as requiring the request to be a multipart-form. Others have reflected this problem, e.g. #177.

To fix this, this PR makes the following changes over 2 commits.

  • First, expand test-suite to account for an array of files. If this test-suite had been expanded, the original codebase would have failed the test.
  • Second, edit swagger.py to fix the bug. This passes the expanded test suite above.

jingapore added 2 commits January 3, 2022 00:09
-expected result is for swagger to interpret the api as consuming only 'multipart/form-data'
-this result will fail in original implementation
Copy link

@gilisland gilisland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@@ -466,7 +466,7 @@ def serialize_operation(self, doc, method):
doc_params = list(doc.get("params", {}).values())
all_params = doc_params + (operation["parameters"] or [])
if all_params and any(p["in"] == "formData" for p in all_params):
if any(p["type"] == "file" for p in all_params):
if any(p["type"] == "file" or (p["type"]=="array" and p["collectionFormat"]=="multi") for p in all_params):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change

@@ -769,6 +769,25 @@ def get(self):
assert "consumes" in op
assert op["consumes"] == ["multipart/form-data"]

def test_parser_parameter_in_files_appended(self, api, client):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@jingapore
Copy link
Author

@SteadBytes @a-luna @j5awry @ziirish can trouble one of you to look thru this PR? Thanks!

@mdylan2
Copy link

mdylan2 commented May 2, 2022

Was wondering if someone could approve this merge request so that uploads through restx work? Been having a tough time

@ziirish ziirish force-pushed the master branch 2 times, most recently from 0db7d06 to 4eaf373 Compare November 1, 2022 17:47
@pbaylies
Copy link

Looks like this got approved and pushed somewhere, but ultimately never got merged?

@gerardoemr
Copy link

Does anyone know if this fix will be merged?

@peter-doggart
Copy link
Contributor

@gerardoemr @pbaylies Thanks for bumping this, I will try and get it merged over the weekend and released if it's something you need!

@gabinguo
Copy link

gabinguo commented May 6, 2023

Someone has updates on this PR? It would be great if it's merged to the main code base..

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

Successfully merging this pull request may close these issues.

9 participants