Create new response on each HTTPException #7918
Conversation
This is a limited backport of aio-libs#5197 and aio-libs#3462
| resp = Response( | ||
| status=exc.status, reason=exc.reason, text=exc.text, headers=exc.headers | ||
| ) | ||
| resp._cookies = exc._cookies |
There was a problem hiding this comment.
I have a strong feeling this code is rather fiddly and probably shouldn't be touched..
There was a problem hiding this comment.
I pulled it directly from https://github.com/aio-libs/aiohttp/pull/5197/files#diff-2126b277e07e3fdd05e7a81da456accf24e5515a46c78c48a79db4eb166043e4R463
I'm also more than happy to close this and give up the optimization for 404s until 4.x
There was a problem hiding this comment.
Yeah, that PR is making the exceptions compatible with cookies. As they are still Responses in 3.x we don't need those changes. I also recall needing to fix a problem in this code, and remember it being complex due to the Response/HTTPException thing, so I'd suggest just leaving this one.
There was a problem hiding this comment.
so I'd suggest just leaving this one.
Let's close this than. We can always come back to it if the performance of the 404 turns out to be a problem but I don't think it will
There was a problem hiding this comment.
To expand on why we need to keep this for backwards-compatibility, we have, for example, this error middleware example:
https://demos.aiohttp.org/en/latest/tutorial.html#middlewares
You'll see this catches HTTPException in order to customise error responses. Older examples would have checked the type of the returned response, as these weren't raised as exceptions previously.
So, in v4 they are exceptions only, but in 3.x they must still be a hybrid Response, otherwise those old middlewares etc. that users wrote will break badly.
| @property | ||
| def status(self) -> int: | ||
| return self.status_code | ||
|
|
||
| @property | ||
| def headers(self) -> "CIMultiDict[str]": | ||
| return self._headers |
There was a problem hiding this comment.
Also, this should already be present from Response. It was added in the linked PR due to it being separated from Response.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.10 #7918 +/- ##
=======================================
Coverage 97.35% 97.35%
=======================================
Files 108 108
Lines 32459 32467 +8
Branches 3849 3851 +2
=======================================
+ Hits 31600 31608 +8
Misses 655 655
Partials 204 204
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
What do these changes do?
This is a limited backport of #5197 and #3462
Are there changes in behavior for the user?
Related issue number
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.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.