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

SansIO Wrappers #2005

Merged
merged 3 commits into from
Jan 21, 2021
Merged

SansIO Wrappers #2005

merged 3 commits into from
Jan 21, 2021

Conversation

pgjones
Copy link
Member

@pgjones pgjones commented Jan 16, 2021

This might be clearer if you consider the names as BaseRequest/BaseResponse and WSGIRequest/WSGIResponse (although these names aren't possible for compatibility reasons). The SansIORequest/SansIOResponse contains all the information that is not WSGI or IO dependent thereby allowing it to be used in ASGI projects, notably Quart.

Does this need specific docs? (At this stage?)

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@property
def is_secure(self) -> bool:
"`True` if the request is secure."
return self.scheme in {"https", "wss"}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change from the previous code, but I think it makes sense.

@pgjones pgjones force-pushed the wrappers branch 2 times, most recently from a06c412 to 2bf997c Compare January 19, 2021 16:35
@davidism davidism added this to the 2.0.0 milestone Jan 19, 2021
@davidism
Copy link
Member

Why were only some of the WSGI values extracted? https://asgi.readthedocs.io/en/latest/specs/www.html#wsgi-compatibility It seems like you'd also want at least SCRIPT_NAME (root_path), REMOTE_ADDR (client[0]), and SERVER_NAME, SERVER_PORT (server). This would let you copy over the remaining properties from Request to sansio.Request.

@davidism davidism added the ASGI label Jan 20, 2021
@pgjones
Copy link
Member Author

pgjones commented Jan 20, 2021

I'm not sure SCRIPT_NAME is equivalent to root_path, I may have a bug in Quart I'm investigating at the moment regarding this. I'm also not sure a script_name property is the best name for the sansio version (seems WSGI specific). I've left the server name parts till I understand how to hook up the host parts (in Quart).

I've updated with REMOTE_ADDR usage, thanks.

@davidism
Copy link
Member

The link I posted suggests SCRIPT_NAME is root_path, if it's not then there's no way to know where the app is mounted. I'm fine with root_path as the variable name in the sans-io part.

Could you give a brief outline on the issues with root_path and server? Or a link to the Quart issues? Just so I know what's going on.

@pgjones
Copy link
Member Author

pgjones commented Jan 20, 2021

Updated the script_info usage, to use root_path in the sansio layer. Will leave any host (SERVER_NAME etc) changes to a future PR so as to allow me to understand the iri and related code.

See also django/asgiref#229 for a potential issue with ASGI and root_path.

This might be clearer if you consider the names as `BaseRequest` and
`WSGIRequest` (although these names aren't possible for compatibility
reasons). The `SansIORequest` contains all the information that is not
WSGI or IO dependent thereby allowing it to be used in ASGI projects,
notably Quart.

This has a very slight performance impact as the path, scheme,
headers, and method are no longer lazily loaded. However, this impact
is likely just a dictionary lookup.

The changes in other files are minor typing improvements related to
the typing changed in the request wrappers.
This might be clearer if you consider the names as `BaseResponse` and
`WSGIResponse` (although these names aren't possible for compatibility
reasons). The `SansIOResponse` contains all the information that is not
WSGI or IO dependent thereby allowing it to be used in ASGI projects,
notably Quart.
This makes the naming a little easier (called request and response),
and allows the sansio module to be considered private/development. The
latter is desired as it isn't clear (yet) how to specify the IO
interface - in an abstract manner so that both sync and async
implementations exist.

This also allows further sansio, rather than WSGI based code, to be
added in a clear location - clear to future authors that the code must
not be WSGI or ASGI or IO specific.
@davidism davidism merged commit 3b8248d into pallets:master Jan 21, 2021
@pgjones
Copy link
Member Author

pgjones commented Jan 21, 2021

Thanks!

@pgjones pgjones deleted the wrappers branch January 21, 2021 09:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2021
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