-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Implement dict-like API on request objects for custom data. #1666
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
Changes from all commits
d40a8b0
4f7f3b0
0a8ff24
6bd8378
ef72028
3e59ed9
041555a
628f71d
5c0cbd9
9966a13
a104067
150dbb5
fc6b5e2
ca238d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
|
|
||
| from collections import defaultdict, namedtuple | ||
| from http.cookies import SimpleCookie | ||
| from types import SimpleNamespace | ||
| from urllib.parse import parse_qs, parse_qsl, unquote, urlunparse | ||
|
|
||
| from httptools import parse_url | ||
|
|
@@ -71,7 +72,7 @@ def is_full(self): | |
| return self._queue.full() | ||
|
|
||
|
|
||
| class Request(dict): | ||
| class Request: | ||
| """Properties of an HTTP request such as URL, headers, etc.""" | ||
|
|
||
| __slots__ = ( | ||
|
|
@@ -84,6 +85,7 @@ class Request(dict): | |
| "_socket", | ||
| "app", | ||
| "body", | ||
| "ctx", | ||
| "endpoint", | ||
| "headers", | ||
| "method", | ||
|
|
@@ -113,6 +115,7 @@ def __init__(self, url_bytes, headers, version, method, transport, app): | |
|
|
||
| # Init but do not inhale | ||
| self.body_init() | ||
| self.ctx = SimpleNamespace() | ||
| self.parsed_forwarded = None | ||
| self.parsed_json = None | ||
| self.parsed_form = None | ||
|
|
@@ -129,10 +132,30 @@ def __repr__(self): | |
| self.__class__.__name__, self.method, self.path | ||
| ) | ||
|
|
||
| def __bool__(self): | ||
| if self.transport: | ||
| return True | ||
| return False | ||
| def get(self, key, default=None): | ||
| """.. deprecated:: 19.9 | ||
| Custom context is now stored in `request.custom_context.yourkey`""" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed there is a commit doing |
||
| return self.ctx.__dict__.get(key, default) | ||
|
|
||
| def __contains__(self, key): | ||
| """.. deprecated:: 19.9 | ||
| Custom context is now stored in `request.custom_context.yourkey`""" | ||
| return key in self.ctx.__dict__ | ||
|
|
||
| def __getitem__(self, key): | ||
| """.. deprecated:: 19.9 | ||
| Custom context is now stored in `request.custom_context.yourkey`""" | ||
| return self.ctx.__dict__[key] | ||
|
|
||
| def __delitem__(self, key): | ||
| """.. deprecated:: 19.9 | ||
| Custom context is now stored in `request.custom_context.yourkey`""" | ||
| del self.ctx.__dict__[key] | ||
|
|
||
| def __setitem__(self, key, value): | ||
| """.. deprecated:: 19.9 | ||
| Custom context is now stored in `request.custom_context.yourkey`""" | ||
| setattr(self.ctx, key, value) | ||
|
|
||
| def body_init(self): | ||
| self.body = [] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,22 +8,72 @@ | |
| except ImportError: | ||
| from json import loads | ||
|
|
||
| def test_custom_context(app): | ||
| @app.middleware("request") | ||
| def store(request): | ||
| request.ctx.user = "sanic" | ||
| request.ctx.session = None | ||
|
|
||
| @app.route("/") | ||
| def handler(request): | ||
| # Accessing non-existant key should fail with AttributeError | ||
| try: | ||
| invalid = request.ctx.missing | ||
| except AttributeError as e: | ||
| invalid = str(e) | ||
| return json({ | ||
| "user": request.ctx.user, | ||
| "session": request.ctx.session, | ||
| "has_user": hasattr(request.ctx, "user"), | ||
| "has_session": hasattr(request.ctx, "session"), | ||
| "has_missing": hasattr(request.ctx, "missing"), | ||
| "invalid": invalid | ||
| }) | ||
|
|
||
| request, response = app.test_client.get("/") | ||
| assert response.json == { | ||
| "user": "sanic", | ||
| "session": None, | ||
| "has_user": True, | ||
| "has_session": True, | ||
| "has_missing": False, | ||
| "invalid": "'types.SimpleNamespace' object has no attribute 'missing'", | ||
| } | ||
|
|
||
|
|
||
| def test_storage(app): | ||
| # Remove this once the deprecated API is abolished. | ||
| def test_custom_context_old(app): | ||
| @app.middleware("request") | ||
| def store(request): | ||
| try: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should copy this test to make another that uses |
||
| request["foo"] | ||
| except KeyError: | ||
| pass | ||
| request["user"] = "sanic" | ||
| request["sidekick"] = "tails" | ||
| sidekick = request.get("sidekick", "tails") # Item missing -> default | ||
| request["sidekick"] = sidekick | ||
| request["bar"] = request["sidekick"] | ||
| del request["sidekick"] | ||
|
|
||
| @app.route("/") | ||
| def handler(request): | ||
| return json( | ||
| {"user": request.get("user"), "sidekick": request.get("sidekick")} | ||
| { | ||
| "user": request.get("user"), | ||
| "sidekick": request.get("sidekick"), | ||
| "has_bar": "bar" in request, | ||
| "has_sidekick": "sidekick" in request, | ||
| } | ||
| ) | ||
|
|
||
| request, response = app.test_client.get("/") | ||
|
|
||
| assert response.json == { | ||
| "user": "sanic", | ||
| "sidekick": None, | ||
| "has_bar": True, | ||
| "has_sidekick": False, | ||
| } | ||
| response_json = loads(response.text) | ||
| assert response_json["user"] == "sanic" | ||
| assert response_json.get("sidekick") is None | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change will break any and all current usage where someone is iterating over the request object like
for _key in request. Is there a reason why these changes is required?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harshanarayana Is this actually being used? Iteration, or the entire storage feature? Request is not a dictionary, so it shouldn't inherit just to gain those features. This has led to problems as pointed out in #1309.
The iteration APIs of dict could be added, but frankly I'd rather do what @abuckenheimer suggested and make it a public property instead. If anything, I'd expect request[...] to fetch me HTTP headers (but keeping that at request.headers[...] is better, just like request.storage[...]).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't tell you if it is being used or not. I mean the iterable nature of it. This was how it was working. So before we break this, it might be a good idea to give out a deprecation in the next release and then remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea iteration is weird but it actually worked properly in testing (only iterating over custom key/values) so I think we should just add a deprecated:
to keep backwards compatibility