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

Unify behavior between asgi and wsgi #1679

Closed
CaselIT opened this issue Feb 19, 2020 · 10 comments · Fixed by #1695
Closed

Unify behavior between asgi and wsgi #1679

CaselIT opened this issue Feb 19, 2020 · 10 comments · Fixed by #1695

Comments

@CaselIT
Copy link
Member

CaselIT commented Feb 19, 2020

Currently the implementation of the response class for the wsgi and the asgi differ when handling the data property. Namely the asgi version does not override data with the media value, while the wsgi version overrides the data with the media serialization data.

In the code on master there are currently these todos

falcon/falcon/response.py

Lines 192 to 195 in c3e9187

# TODO(kgriffs): Remove the side-effect that accessing this
# property causes (do something similar to what we did on the
# ASGI side). This will be a breaking change, so caution is
# advised.

falcon/falcon/response.py

Lines 192 to 195 in c3e9187

# TODO(kgriffs): Remove the side-effect that accessing this
# property causes (do something similar to what we did on the
# ASGI side). This will be a breaking change, so caution is
# advised.

Should the wsgi response class behave like the asgi, and use the method render_body to get the body in the response?

@open-collective-bot
Copy link

Hi 👋,

Thanks for using Falcon. The large amount of time and effort needed to
maintain the project and develop new features is not sustainable without
the generous financial support of community members like you.

Please consider helping us secure the future of the Falcon framework with a
one-time or recurring donation.

Thank you for your support!

@CaselIT
Copy link
Member Author

CaselIT commented Feb 20, 2020

@vytas7 on gitter pointed out that there are also other differences between wsgi and asgi. Like the Request.get_media and the current draft of the async multipart media handler

This is mainly because an async property is odd, and may trick people that are not used to this.

To try and have an api as uniform as possible between asgi and wsgi, maybe an heuristic could be:

  • takes constant time to execute regardless of the request? a property is probably fine in both cases (not async in asgi)
  • depends on the request/resposne? a function is probably better, and the asgi would be async

@vytas7
Copy link
Member

vytas7 commented Feb 20, 2020

Anything doing I/O is probably better off as a method.
Async makes it crystal clear by requiring you to await things.

However, even if we adopt this thinking, we'll have to provide WSGI property compatibility aliases for a long time. Serious interface stability is among the main selling points of Falcon, after all.

@CaselIT
Copy link
Member Author

CaselIT commented Feb 20, 2020

If they are just getters, the alias would be just media = property(get_media)

Maybe the path could be. Alias in v3 (maybe deprecated just in the comments), deprecate in v4, remove v5?

For asgi and multipart, since they are new, the alias would not be there?

@CaselIT CaselIT changed the title Unify behavior of response class data field Unify behavior between asgi and wsgi Feb 22, 2020
@kgriffs
Copy link
Member

kgriffs commented Feb 25, 2020

Maybe the path could be. Alias in v3 (maybe deprecated just in the comments), deprecate in v4, remove v5?

I'd say just alias in v3 but do not deprecated until v4.

For asgi and multipart, since they are new, the alias would not be there?

For ASGI we are trying to make it as similar as possible to WSGI (minus stuff that is deprecated in WSGI for v3), so I think it would still be a good idea to, for example, add a req.media property alias.

@CaselIT
Copy link
Member Author

CaselIT commented Feb 28, 2020

I can try working on it. I'll start by unifying the request and response classes

@vytas7
Copy link
Member

vytas7 commented Feb 28, 2020

@CaselIT I think you should coordinate this with @kgriffs as we've just merged the PR for ASGI reintroducing the async asgi.Request.media property.
It might be worth getting some feedback from the community first, i.e., whether people want to move away from properties at all wrt WSGI Falcon. On WSGI Falcon it's convenient to do, for instance, just req.media.

One thing that is IMHO clearly worth unifying, is render_body(). I'm not very opinionated on property vs method otherwise.

@CaselIT
Copy link
Member Author

CaselIT commented Feb 28, 2020

Yes, that was the plan for now I think. Have both, at least in the wsgi version.
Then for v4 it can be decided if only one solution should be kept or not, and apply the appropriate deprecations.

@kgriffs
Copy link
Member

kgriffs commented Mar 5, 2020

Let's start with a PR and breaking change notes in docstrings and towncrier news for updating WSGI per render_body().

Can you enumerate/add a checklist for any other changes you were considering?

@CaselIT
Copy link
Member Author

CaselIT commented Mar 5, 2020

Here is what I've found:

  • Request: bounded_stream vs stream. Not sure any change can be made here since in wsgi are two different values, while asgi are aliased
  • BodyPart: I guess all properties should be alias of methods get_X

It's probably not exhaustive

kgriffs added a commit that referenced this issue Apr 8, 2020
* refactor(requrest): avoid duplication of prefix, add get_media to wsgi method

* refactor(response): add render_body to wsgi response.

Also unify data and media behavior between asgi and wsgi

* test(response): additional tests to render_body

* docs: add newsfragment

* docs: fix typo in changelog

* doc(falcon.Response): Expand news fragment for render_body()

* doc(Response): Update docstrings for render_body()

* doc(Response): Update docstrings for media and get_body()

* doc(Request): Add news fragment re get_media()

Fixes #1679
Partially-Implements #1358

Co-authored-by: Vytautas Liuolia <vytautas.liuolia@gmail.com>
Co-authored-by: Kurt Griffiths <mail@kgriffs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants