-
-
Notifications
You must be signed in to change notification settings - Fork 758
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 for issue #975 #976
Fix for issue #975 #976
Conversation
Can you add a test? |
Can you help me with that? Schema
Requests
Exception
|
@vasiliy-garenskikh |
The problem was in use this coroutine: But I can't write aiohttp code to imitate HTML file upload form :( Swagger:
But I get 400 status: Do you know rigth way to send "file" html ? |
I haven't checked the codebase for a while, but I can assume that aiohttp files integration is missing Also there are currently few issues regarding multiform data At first glance, I couldn't indentify where exactly the problem is. |
@@ -278,14 +279,17 @@ def get_request(cls, req): | |||
if req.body_exists: | |||
body = yield from req.read() | |||
|
|||
data = yield from req.post() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, this will never work.
If you post the request, whatever that is because important part is to have something in that, so the condition req_body_exists
evaluates to True
, you are reading the entire payload
from aiohttp.web.Request
in line 280 (yield from req.read()
). Later on you are attempting to read the payload
yet again via req.post()
. That's why it fails.
What might have be needed is here is to distinguish between multipart requests and ordinary requests.
I assume that multipart requests might be processed in a way that we extract files and normal fields into separate variables and push'em into ConnexionRequest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
Indeed, you can't read aiohttp.web.Request
payload twice, since it's a StreamReader
object and both methods read from it.
Calling different readers based on the content type sounds like a solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prawn-cake Give it a whirl then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prawn-cake I will give it a try on my own. We're interested in having it fixed as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kornicameister !
Sorry for not being very active here. I'll check the changes
@prawn-cake @vasiliy-garenskikh please check comment I left in a code. I cannot really tell if you noticed that as well, based on your conversation. I am just taking this form top of my head after checking aiohttp codebase |
@prawn-cake @vasiliy-garenskikh I have created PR in my fork. PR -> #987 |
Cool. I left some comments there |
I'm closing this PR for now (as it's pretty old, there are no recent updates, and no tests). |
Fixes #975 .
Changes proposed in this pull request:
files
added to aiohttpConnexionRequest
class