Add deprecation warning when HTTPException is returned instead of raised

[panagiks]

What do these changes do?

Add DeprecationWarning when an HTTPException is returned instead of being raised.

Are there changes in behavior for the user?

If user's handler (view function) returns an HTTPException, a DeprecationWarning will be displayed.

Related issue number

Discussion in #2415

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.bug)
  • 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."

[asvetlov]

Thanks a lot.

What's about the next step?
I suggest adding a new build_response() method to HTTPException for construction a web.Response object from exception instance.

I've tried to replace everything in aiohttp by build_response() usage but figured out that the change is too invasive.
Dropped my attempt to get rid on inheritance from Response in the single step.

But we still could release aiohttp 3.0 with build_response() method and very encourage it.

[asvetlov]

Sorry, travis checks is broken by me.
Will fix it soon.

[panagiks]

[INVALID QUESTION] I will add the build_response() method along with test in a while. Do we need reference to HTTPException instance in Response? (should I instantiate the object as done in HTTPException's constructor or by Response(...))

[panagiks]

I'm not really a fan of the implementation in my last commit (as it includes some code repetition) but there were a few issues with my initial approach:

• If build_response() creates a new Response there is the issue that, at this point, self.body and self.text are already both populated and this fails this check. Similarly content_type is present along with a Content Type header that fails this check.
• Removing the inheritance of Response (so the above values don't get populated during HTTPException initialization) is, as you noted, too invasive, as a few aiohttp components along the way depend on it's behaviour as both Response and Exception.

If there are any suggestions for a better approach I'd be more than happy to implement them :)

[panagiks]

I also thought about returning self in build_response() as self is currently a Response object. But (besides the fact that I'm not sure it would be a good practice since the target is to eventually get rid of the Response dependency) it wouldn't force middlewares to handle the non-duality of the response (i.e. functionalities that depended on response's Exception nature would still work when calling build_response(), although that feature is deprecated through the build_response() interface).

[asvetlov]

Making a new _as_response in constructor looks... ugly, sorry.

Let's go further.
In my mind build_response() should replace returning the HTTPException derived class.
#2521 shows my fault in changing everything at once.

But you can get the main idea : headers, reason, body, text and content_type parameters should be deprecated.

Every class like _HTTPMove (a base for every 3xx redirection response) should override build_response() method.

In the future (aiohttp 4.0) we need to support the only our own parameters like location, link, allowed_methods etc.

Sorry for long message but sure, I've got my point.

[panagiks]

Wouldn't this change deprecate the return web.HTTPException() interface outright?

From #2415 I got the sense that deprecation should come in waves. i.e.:

• in one release add a deprecation warning and a new interface that you will encourage people to use (i.e. build_response())
• in another release fully deprecate the return web.HTTPException() interface

I have no stance/opinion/position on whether the deprecation should come in waves, just the sense that I got from the issue mentioned above.

In any case, I'll look into modeling the changes needed here based on the example you provided.

Sorry for the back-and-forth this pr required and thank you for the guideline :)

[asvetlov]

I suggest merging this PR with deprecation warning only (with proper documentation update) and making a new PR for build_response().

[codecov-io]

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2515      +/-   ##
==========================================
+ Coverage   97.13%   97.13%   +<.01%     
==========================================
  Files          39       39              
  Lines        8065     8068       +3     
  Branches     1414     1415       +1     
==========================================
+ Hits         7834     7837       +3     
  Misses         99       99              
  Partials      132      132

Impacted Files	Coverage Δ
aiohttp/web_protocol.py	88.16% <100%> (+0.11%)	⬆️

---

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 0104753...3387aec. Read the comment docs.

[asvetlov]

thanks!

[lock]

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].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new