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

Deprecate body argument for http web excepions #3420

Merged
merged 2 commits into from
Dec 2, 2018

Conversation

asvetlov
Copy link
Member

@asvetlov asvetlov commented Dec 2, 2018

Fixes #3385

@codecov-io
Copy link

Codecov Report

Merging #3420 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3420      +/-   ##
==========================================
+ Coverage   97.95%   97.95%   +<.01%     
==========================================
  Files          44       44              
  Lines        8543     8546       +3     
  Branches     1383     1384       +1     
==========================================
+ Hits         8368     8371       +3     
  Misses         74       74              
  Partials      101      101
Impacted Files Coverage Δ
aiohttp/web_exceptions.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1618fe6...7c424dd. Read the comment docs.

@asvetlov asvetlov merged commit 92cc0ac into master Dec 2, 2018
@asvetlov asvetlov deleted the deprecate-exception-body branch December 2, 2018 15:24
@eirnym
Copy link

eirnym commented May 11, 2019

Does it mean, that I can't send back a binary here? I have a requirement to communicate using binary protocol and this change is breaking my ability to use exceptions

@kxepal
Copy link
Member

kxepal commented May 11, 2019

HTTP is text protocol and exceptions aka HTTP errors are still about text error messages since humans will read them. Probably, you use protocol in wrong way if you need to communicate by binaries on errors.

Still, it doesn't breaks anything for you if you switch from HTTPException to regular Response - that's would be correct usage, though quite specific. Do you need HTTP protocol at all if you speak binary?

@eirnym
Copy link

eirnym commented May 11, 2019

I can send any data after the headers. The protocol itself doesn't specify, that ALL data is text. it specifies, that headers part IS text. If you disagree, please, point the to the RFC where it is clearly written.

Beside my case, please, take take image URL and see how image is transmitted. The headers are TEXT and body is binary.

yes, I have a requirement to communicate using ProtoBuf instead of JSON over HTTP.

@kxepal
Copy link
Member

kxepal commented May 11, 2019

If you disagree, please, point the to the RFC where it is clearly written.

Just translate what HTTP means.

yes, I have a requirement to communicate using ProtoBuf instead of JSON over HTTP.

Do you need HTTP response statuses (which are set by exception type) after that? I guess no.

@eirnym
Copy link

eirnym commented May 11, 2019

Just translate what HTTP means.

it is the best joke I've ever heard about HTTP. If HTTP was really text-only with no binary allowed in the body, some HTTP features wouldn't work, such a binary file transferring, CONNECT method and many more. Please, open the RFC and find me the place where it's clearly written. You also may point me to an implementation of web-server such as Nginx where it converts binary data of an image using base64, or curl/wget which converts binary back to bytes.

Do you need HTTP response statuses (which are set by exception type) after that? I guess no.

Yes, I do. This helps the client code do distinct which request was failed before looking into contents

@eirnym
Copy link

eirnym commented May 11, 2019

This is some random post on SO on this topic

@kxepal
Copy link
Member

kxepal commented May 11, 2019

I'm glad you liked it 😂 Eventually, RFC is on your side: http body may accept any octet aka 8-bit characters, so just limiting to text here is a bit limitation.

But, from point of your protocol, I guess, you don't need to deal with any HTTP bits at all, just use it as transport and handle metadata from your binary data - that's a bit more solid way since you can change HTTP with any else protocol without issues.

@kxepal
Copy link
Member

kxepal commented May 11, 2019

Hm...that's quite a question.

@asvetlov How we should behave in this case: we have a REST service (remember, REST is not about JSON) which accepts some binary data among the others (let's say it's the same application/protobuf) and have to respond with the similar one. Quite strange to reply back with the language different from you questioned one. So we have to repond via binary reply. Currently, aiohttp supports both Response and HTTPException instances to generate responses. Once exceptions are cannot carry binary data, they becomes quite off the road and we have to stay with Response and use return instead of raise.

May be we can push this story to the end and leave only one way to generate response from a server?

@asvetlov
Copy link
Member Author

Let's continue the conversation in open issue: #3757

@eirnym
Copy link

eirnym commented May 12, 2019

@kxepal In my case I have whole underlying protocol in Protobuf, not only responses.

@lock lock bot added the outdated label May 20, 2020
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bot:chronographer:provided There is a change note present in this PR bug enhancement good first issue Good for newcomers outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable body parameter in HTTPException constructor
5 participants