Skip to content

Ensure changes to the status attribute are written to the transport.#656

Closed
nhoad wants to merge 1 commit intoaio-libs:masterfrom
nhoad:allow-modifying-status
Closed

Ensure changes to the status attribute are written to the transport.#656
nhoad wants to merge 1 commit intoaio-libs:masterfrom
nhoad:allow-modifying-status

Conversation

@nhoad
Copy link
Copy Markdown

@nhoad nhoad commented Dec 4, 2015

I'm writing a proxy with aiohttp, and I need the possibility of modifying the response status code under certain conditions, something that currently isn't possible.

Personally I'm not a big fan for how I've done the handling of the reason of the attribute, but I can't think of a better way to do this.

@asvetlov
Copy link
Copy Markdown
Member

Please elaborate why do you need to modify status code?
What's your use case?

From my perspective http proxy can write different status line if needed without changing aiohttp.
Adding status_line property doesn't make any harm but you can keep formatting code in your application. I feel the need for status_line is very rare.

@nhoad
Copy link
Copy Markdown
Author

nhoad commented Dec 12, 2015

By default the proxy sets the status code to 200, assuming success. It sends a modified version of the request upstream, which may or may not return a 200, at which point the status code to be sent to the client needs to be modified.

Without this patch, the only way to do that is by modifying status_line directly, which is in my opinion much more dangerous.

@asvetlov
Copy link
Copy Markdown
Member

I still believe proxy should get response from upstream and send a copy of it to client.

No need for inplace request/response modification. All work can be done without aiohttp changing.

Even if status_line updating may be convenient for your particular code the use case is not too common for adding such hackery patch.

@nhoad
Copy link
Copy Markdown
Author

nhoad commented Dec 17, 2015

Then as an alternative, modifying the status attribute should be forbidden. It should be a readonly property, to make it clear that you're not supposed to modify it.

@asvetlov
Copy link
Copy Markdown
Member

Converting status to readonly makes sense.

@asvetlov asvetlov mentioned this pull request Dec 26, 2015
@asvetlov
Copy link
Copy Markdown
Member

Fixed by #710

@asvetlov asvetlov closed this Dec 27, 2015
@lock
Copy link
Copy Markdown

lock bot commented Oct 29, 2019

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 Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants