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

BaseRequest: misleading name of has_body attribute #2005

Closed
ku3o opened this issue Jun 23, 2017 · 10 comments
Closed

BaseRequest: misleading name of has_body attribute #2005

ku3o opened this issue Jun 23, 2017 · 10 comments
Labels
Milestone

Comments

@ku3o
Copy link

ku3o commented Jun 23, 2017

Long story short

class BaseRequest has attribute has_body that has an unfortunate name as it checks if there are more bytes to read from its body. And when the body is read it changes its value to False. This fact is not obvious from attribute name neither documentation and it causes confusion and bugs.

Expected behavior

has_body should stay static over a lifetime of a request. Or the attribute name and a related documentation should be more specific about the fact that once a body of a request is read, the value has changed.

Actual behavior

has_body return False if a request has a body but was read by another method such as await request.json().

Steps to reproduce

Simple handler that shows this behavior.

async def handler(request):
    """Simple request handler."""
    #  Let's say request has a JSON body
    request.has_body  # This returns True
    body = await request.json()
    request.has_body  # This returns False

Your environment

Not an environment specific.

@asvetlov
Copy link
Member

Oops. IIRC many ages ago the attribute behavior was exactly that its name means.
But now it's misleading, I agree.
We need deprecate the property and invite a brand new one with better name.
.can_read_body maybe?

@ku3o
Copy link
Author

ku3o commented Jun 23, 2017

Is there a use case that validates the existence of .can_read_body attribute? I like the idea of .has_body which indicates that there is a body in a request. So maybe a solution is to fix the behavior?

@asvetlov
Copy link
Member

asvetlov commented Jun 23, 2017

Sorry, no. I like the name too but aiohttp is open source project.
We don't know all our users and their use cases.
The only safe way for reusing existing name with other semantics is deprecating it, waiting for a while, removing and resurrecting.
Otherwise we will break backward compatibility.
It's possible sometimes but I feel now is not the case.
Between every stage we should make several releases.
It takes more than year for the whole process.

For example I prefer resp.start() to resp.prepare() but .start() was used for non-coroutine. We deprecated the method in 2015 but removed this year. Maybe we will able to switch back next year with deprecating .prepare() but honestly I not sure if benefits worth it.

@asvetlov
Copy link
Member

.can_read_body name is not used by our API and we could start switching from .has_body to .can_read_body.
And we free to make a new attribute with proper .has_body functionality but under different name.

@ku3o
Copy link
Author

ku3o commented Jun 23, 2017 via email

@asvetlov
Copy link
Member

Please do. Thank you.

@asvetlov asvetlov added this to the 2.3.0 milestone Jun 25, 2017
@asvetlov asvetlov added the good first issue Good for newcomers label Jul 2, 2017
@ghost
Copy link

ghost commented Aug 5, 2017

Any thoughts on what to call the new attribute with proper .has_body functionality?
.body_present? .body_exists? .body_received?

asvetlov pushed a commit that referenced this issue Aug 7, 2017
… (#2169)

* Deprecated BaseRequest.has_body, replaced with 2 new attributes

* Test obsolete BaseRequest.has_body attr
arthurdarcet pushed a commit to arthurdarcet/aiohttp that referenced this issue Aug 8, 2017
…libs#2005) (aio-libs#2169)

* Deprecated BaseRequest.has_body, replaced with 2 new attributes

* Test obsolete BaseRequest.has_body attr
@cecton
Copy link
Contributor

cecton commented Aug 9, 2017

Is there anything else for this issue or can we close it?

@asvetlov
Copy link
Member

asvetlov commented Aug 9, 2017

@cecton thank you for question.
Yes, the issue is done.
Fixed by #2169

@asvetlov asvetlov closed this as completed Aug 9, 2017
@lock
Copy link

lock bot commented Oct 28, 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].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants