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

plain text http responses to errors #3483

Merged
merged 12 commits into from
Jan 7, 2019
Merged

plain text http responses to errors #3483

merged 12 commits into from
Jan 7, 2019

Conversation

samuelcolvin
Copy link
Member

@samuelcolvin samuelcolvin commented Jan 4, 2019

What do these changes do?

When using curl or httpie, error responses should not be html.

Are there changes in behavior for the user?

Yes, errors should be plain text not html when text/html is not in the "browsers" Accept header.

Currently this is failing due to an existing error which I inadvertently uncovered. I'll comment on the code below. I'll add CHANGES etc. once this is fixed.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

LGTM!

@samuelcolvin
Copy link
Member Author

Maybe we should change the content-type to text/plain for the case of status != 500 line 553?

Doesn't make sense for this to be html I would guess.

@asvetlov
Copy link
Member

asvetlov commented Jan 4, 2019

Maybe we should change the content-type to text/plain for the case of status != 500 line 553?

Doesn't make sense for this to be html I would guess.

Agree, makes sense.
Please do

aiohttp/web_protocol.py Outdated Show resolved Hide resolved
aiohttp/web_protocol.py Outdated Show resolved Hide resolved
aiohttp/web_protocol.py Outdated Show resolved Hide resolved
aiohttp/web_protocol.py Outdated Show resolved Hide resolved
aiohttp/web_protocol.py Outdated Show resolved Hide resolved
@samuelcolvin
Copy link
Member Author

Should all be changed as per requests.

msg = "<h1>500 Internal Server Error</h1>"
ct = 'text/plain'
if status == HTTPStatus.INTERNAL_SERVER_ERROR:
title = '500 Internal Server Error'
Copy link
Member

@webknjaz webknjaz Jan 4, 2019

Choose a reason for hiding this comment

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

Suggested change
title = '500 Internal Server Error'
title = ' '.join((str(HTTPStatus.INTERNAL_SERVER_ERROR.value), HTTPStatus.INTERNAL_SERVER_ERROR.phrase))

Copy link
Member

Choose a reason for hiding this comment

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

In [1]: import http                                                                                                                                                                            

In [3]: http.HTTPStatus.INTERNAL_SERVER_ERROR                                                                                                                                                  
Out[3]: <HTTPStatus.INTERNAL_SERVER_ERROR: 500>

In [5]: http.HTTPStatus.INTERNAL_SERVER_ERROR.phrase                                                                                                                                           
Out[5]: 'Internal Server Error'

In [6]: http.HTTPStatus.INTERNAL_SERVER_ERROR.value                                                                                                                                            
Out[6]: 500

In [8]: http.HTTPStatus.INTERNAL_SERVER_ERROR.description                                                                                                                                      
Out[8]: 'Server got itself in trouble'

Copy link
Member

Choose a reason for hiding this comment

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

@webknjaz honestly the original (a plain string) looks more readable for me.

Copy link
Member

Choose a reason for hiding this comment

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

But I can live with any variant, too negligeable reason for contestion.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. I'd probably go for

Suggested change
title = '500 Internal Server Error'
title = str(HTTPStatus.INTERNAL_SERVER_ERROR.value) + ' ' + HTTPStatus.INTERNAL_SERVER_ERROR.phrase

Copy link
Member

Choose a reason for hiding this comment

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

would it be better readable?

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit surprised that http.HTTPStatus doesn't have a method to construct a status string. IMHO it should be smth like title = http.HTTPStatus.INTERNAL_SERVER_ERROR.to_status_string()

Copy link
Member

Choose a reason for hiding this comment

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

HTTP reason is gone in htttp/2 BTW.
I suspect that very many Web frameworks have own copies of these constants.
+0 for adding to_status_string() to CPython, at least I don't want to make a Pull Request (but you can count me on the PR review).

aiohttp/web_protocol.py Outdated Show resolved Hide resolved
tests/test_web_server.py Outdated Show resolved Hide resolved
Co-Authored-By: samuelcolvin <samcolvin@gmail.com>
@samuelcolvin
Copy link
Member Author

I've left my laptop at work so will fix on Monday.

@webknjaz webknjaz self-assigned this Jan 4, 2019
@samuelcolvin samuelcolvin self-assigned this Jan 7, 2019
@samuelcolvin
Copy link
Member Author

Should now be fixed.

I've gone back on @webknjaz previous request and removed Server got itself in trouble when we have a traceback.

Having thought about this "Server got itself in trouble" is a completely meaningless sentence designed as something to say when we have no information; it adds no further information to casual users or application developers. If we're showing the traceback it is unnecessary and only makes the output more verbose and harder to read. This maintains the previous behaviour.

I hope this is ok.

@codecov-io
Copy link

codecov-io commented Jan 7, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3483      +/-   ##
==========================================
+ Coverage   97.92%   97.93%   +<.01%     
==========================================
  Files          44       44              
  Lines        8567     8560       -7     
  Branches     1383     1380       -3     
==========================================
- Hits         8389     8383       -6     
+ Misses         74       72       -2     
- Partials      104      105       +1
Impacted Files Coverage Δ
aiohttp/test_utils.py 99.35% <ø> (ø) ⬆️
aiohttp/web_protocol.py 92.35% <100%> (+0.15%) ⬆️
aiohttp/streams.py 98.72% <0%> (-0.51%) ⬇️
aiohttp/web_response.py 98.19% <0%> (-0.23%) ⬇️
aiohttp/payload.py 98.6% <0%> (-0.04%) ⬇️
aiohttp/client_reqrep.py 97.43% <0%> (-0.01%) ⬇️
aiohttp/multipart.py 96.09% <0%> (+0.62%) ⬆️

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 4c7bc9f...d76270a. Read the comment docs.

@samuelcolvin samuelcolvin removed their assignment Jan 7, 2019
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

LGTM

@webknjaz
Copy link
Member

webknjaz commented Jan 7, 2019

I think this can also be backported

@samuelcolvin
Copy link
Member Author

I think this can also be backported

ok, do we have an automated way of doing this or should I create a pull request against 3.5 manually?

@webknjaz
Copy link
Member

webknjaz commented Jan 7, 2019

There's cherry-picker, which you can use manually. No bot doing it currently.

@asvetlov asvetlov merged commit b42a23b into aio-libs:master Jan 7, 2019
@asvetlov
Copy link
Member

asvetlov commented Jan 7, 2019

Thanks!

asvetlov pushed a commit that referenced this pull request Jan 8, 2019
(cherry picked from commit b42a23b)

Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@asvetlov
Copy link
Member

asvetlov commented Jan 8, 2019

Backport PR: #3499

asvetlov added a commit that referenced this pull request Jan 8, 2019
(cherry picked from commit b42a23b)

Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@lock
Copy link

lock bot commented Jan 8, 2020

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Jan 8, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 8, 2020
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 8, 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 outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants