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

Request config #2949

Merged
merged 16 commits into from
Apr 25, 2018
61 changes: 61 additions & 0 deletions aiohttp/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import time
import weakref
from collections import namedtuple
from collections.abc import Mapping
from contextlib import suppress
from math import ceil
from pathlib import Path
Expand Down Expand Up @@ -741,3 +742,63 @@ def set_result(fut, result):
def set_exception(fut, exc):
if not fut.done():
fut.set_exception(exc)


class ChainedProps(Mapping):
Copy link
Member

Choose a reason for hiding this comment

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

I think the ChainedMaps would be better name here since it's a mapping. Props suffix leaves the feeling that it's something about attr-based access. While ChainedMaps causes collision with stdlib ChainMap, still, they have a bit different naming what should cause suspicions. However, if we make this class for internal-only usage, than own ChainMap is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Naming is the hardest thing!
Maybe we can invite a name without chain word?

Copy link
Member

Choose a reason for hiding this comment

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

Hm...How about to just a Mapping? With the trick that all the maps will get merged right at the moment of it creation?

Copy link
Member

Choose a reason for hiding this comment

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

I mean, if we do merge all the maps, than this class has no reason to exists. The merge result could be returned as MappingProxy - well known immutable mapping.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's name it ChainMapProxy and keep existing implementation -- I don't like early merging.

__slots__ = ('_maps',)

def __init__(self, maps):
self._maps = tuple(maps)

def __getitem__(self, key):
for mapping in self._maps:
try:
return mapping[key]
except KeyError:
pass
raise KeyError(key)

def get(self, key, default=None):
return self[key] if key in self else default

def __len__(self):
# reuses stored hash values if possible
return len(set().union(*self._maps))

def __iter__(self):
d = {}
for mapping in reversed(self._maps):
# reuses stored hash values if possible
d.update(mapping)
return iter(d)
Copy link
Member

Choose a reason for hiding this comment

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

It feels like you can have there the same iter(set().union(*self._maps)) instead of another dict construction.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did copy collections.ChainMap without careful thinking :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep it for sake of shared implementation with ChainMap

Copy link
Member Author

Choose a reason for hiding this comment

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

Iteration is not the most common call, get/__getitem__ doesn't create a dict.
Moreover your proposal constructs a set, not sure what is better.


def __contains__(self, key):
return any(key in m for m in self._maps)

def __bool__(self):
return any(self._maps)

def __repr__(self):
content = ", ".join(map(repr, self._maps))
return 'ChainedProps({})'.format(content)
Copy link
Member

Choose a reason for hiding this comment

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

Here we should reuse class name in repr to avoid any confusions with subclasses. I know, there wouldn't any subclasses of those, but still this little bit could save few debugging hours for some hacker.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to allow subclassing. Hmm, should do it explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done



class Namespace:
__slots__ = ('_mapping')

def __init__(self, mapping):
self._mapping = mapping

def __dir__(self):
return dir(self.__class__) + list(self._mapping.keys())

def __getattr__(self, name):
try:
return self._mapping[name]
except KeyError:
raise AttributeError(name)

def __repr__(self):
content = ", ".join('{}={!r}'.format(k, v)
for k, v in self._mapping.items())
return 'Namespace({})'.format(content)
15 changes: 14 additions & 1 deletion aiohttp/web_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
from yarl import URL

from . import hdrs, multipart
from .helpers import DEBUG, HeadersMixin, reify, sentinel
from .helpers import (DEBUG, ChainedProps, HeadersMixin, Namespace, reify,
sentinel)
from .streams import EmptyStreamReader
from .web_exceptions import HTTPRequestEntityTooLarge

Expand Down Expand Up @@ -664,6 +665,18 @@ def app(self):
"""Application instance."""
return self._match_info.current_app

@property
def config_dict(self):
lst = self._match_info.apps
app = self.app
idx = lst.index(app)
sublist = list(reversed(lst[:idx + 1]))
return ChainedProps(sublist)

@property
def config(self):
return Namespace(self.config_dict)
Copy link
Member

Choose a reason for hiding this comment

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

I'm in a very doubt if we need not a mapping class here. This will cause two problems:

  1. Inconsistent access to config values. While names are valid Python attributes it's all fine, but how to access to a value with mycomponent.cache? Here, user have to fallback to getattr(request.config, 'mycomponent.cache').
  2. Linters. Linters will complain about accessing to unknown class attributes. This will be very annoying.

I would prefer to have one way to work with the config that works and right. So far, config_dict is more powerful and useful interface to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I heard complains many times that IDE doesn't autocomplete string keys but does it for custom attributes.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, they don't do keys autocomplete unless you explicitly define those keys in the same scope. However, their ability to autocomplete custom attributes is limited and based on type knowledge and similar usages.

However, for pylint you will have to put Namespace class into ignored-classes since it will think that it's your fault since you're referencing to unknown class instance attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

@socketpair I recall you wanted the feature.
Please explain your opinion

Copy link
Member Author

Choose a reason for hiding this comment

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


async def _prepare_hook(self, response):
match_info = self._match_info
if match_info is None:
Expand Down
120 changes: 120 additions & 0 deletions tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -547,3 +547,123 @@ async def test_set_exception_cancelled(loop):

with pytest.raises(asyncio.CancelledError):
await fut


# ----------- ChainedProps --------------------------

class TestChainedProps:
def test_getitem(self):
d1 = {'a': 2, 'b': 3}
d2 = {'a': 1}
cp = helpers.ChainedProps([d1, d2])
assert cp['a'] == 2
assert cp['b'] == 3

def test_getitem_not_found(self):
d = {'a': 1}
cp = helpers.ChainedProps([d])
with pytest.raises(KeyError):
cp['b']

def test_get(self):
d1 = {'a': 2, 'b': 3}
d2 = {'a': 1}
cp = helpers.ChainedProps([d1, d2])
assert cp.get('a') == 2

def test_get_default(self):
d1 = {'a': 2, 'b': 3}
d2 = {'a': 1}
cp = helpers.ChainedProps([d1, d2])
assert cp.get('c', 4) == 4

def test_get_non_default(self):
d1 = {'a': 2, 'b': 3}
d2 = {'a': 1}
cp = helpers.ChainedProps([d1, d2])
assert cp.get('a', 4) == 2

def test_len(self):
d1 = {'a': 2, 'b': 3}
d2 = {'a': 1}
cp = helpers.ChainedProps([d1, d2])
assert len(cp) == 2

def test_iter(self):
d1 = {'a': 2, 'b': 3}
d2 = {'a': 1}
cp = helpers.ChainedProps([d1, d2])
assert set(cp) == {'a', 'b'}

def test_contains(self):
d1 = {'a': 2, 'b': 3}
d2 = {'a': 1}
cp = helpers.ChainedProps([d1, d2])
assert 'a' in cp
assert 'b' in cp
assert 'c' not in cp

def test_bool(self):
assert helpers.ChainedProps([{'a': 1}])
assert not helpers.ChainedProps([{}, {}])
assert not helpers.ChainedProps([])

def test_repr(self):
d1 = {'a': 2, 'b': 3}
d2 = {'a': 1}
cp = helpers.ChainedProps([d1, d2])
assert "ChainedProps({'a': 2, 'b': 3}, {'a': 1})" == repr(cp)


# ----------- Namespace --------------------------


class TestNamespace:
def test_dir(self):
d = {'a': 1, 'b': 2}
ns = helpers.Namespace(d)
assert ['__class__',
'__delattr__',
'__dir__',
'__doc__',
'__eq__',
'__format__',
'__ge__',
'__getattr__',
'__getattribute__',
'__gt__',
'__hash__',
'__init__',
'__init_subclass__',
'__le__',
'__lt__',
'__module__',
'__ne__',
'__new__',
'__reduce__',
'__reduce_ex__',
'__repr__',
'__setattr__',
'__sizeof__',
'__slots__',
'__str__',
'__subclasshook__',
'_mapping',
'a',
'b'] == dir(ns)

def test_getattr_found(self):
d = {'a': 1, 'b': 2}
ns = helpers.Namespace(d)
assert 1 == ns.a

def test_getattr_not_found(self):
d = {'a': 1, 'b': 2}
ns = helpers.Namespace(d)
with pytest.raises(AttributeError):
ns.c

def test_repr(self):
d = {'a': 1, 'b': 2}
ns = helpers.Namespace(d)
assert 'Namespace(a=1, b=2)' == repr(ns)
113 changes: 113 additions & 0 deletions tests/test_web_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -398,3 +398,116 @@ async def inner(app):
await app.cleanup()
assert "has more than one 'yield'" in str(ctx.value)
assert out == ['pre_1', 'post_1']


async def test_subapp_chained_config_dict_visibility(aiohttp_client):

async def main_handler(request):
assert request.config_dict['key1'] == 'val1'
assert 'key2' not in request.config_dict
return web.Response(status=200)

root = web.Application()
root['key1'] = 'val1'
root.add_routes([web.get('/', main_handler)])

async def sub_handler(request):
assert request.config_dict['key1'] == 'val1'
assert request.config_dict['key2'] == 'val2'
return web.Response(status=201)

sub = web.Application()
sub['key2'] = 'val2'
sub.add_routes([web.get('/', sub_handler)])
root.add_subapp('/sub', sub)

client = await aiohttp_client(root)

resp = await client.get('/')
assert resp.status == 200
resp = await client.get('/sub/')
assert resp.status == 201


async def test_subapp_chained_config_dict_overriding(aiohttp_client):

async def main_handler(request):
assert request.config_dict['key'] == 'val1'
return web.Response(status=200)

root = web.Application()
root['key'] = 'val1'
root.add_routes([web.get('/', main_handler)])

async def sub_handler(request):
assert request.config_dict['key'] == 'val2'
return web.Response(status=201)

sub = web.Application()
sub['key'] = 'val2'
sub.add_routes([web.get('/', sub_handler)])
root.add_subapp('/sub', sub)

client = await aiohttp_client(root)

resp = await client.get('/')
assert resp.status == 200
resp = await client.get('/sub/')
assert resp.status == 201


async def test_subapp_chained_config_visibility(aiohttp_client):

async def main_handler(request):
assert request.config.key1 == 'val1'
with pytest.raises(AttributeError):
request.config.key2
return web.Response(status=200)

root = web.Application()
root['key1'] = 'val1'
root.add_routes([web.get('/', main_handler)])

async def sub_handler(request):
assert request.config.key1 == 'val1'
assert request.config.key2 == 'val2'
return web.Response(status=201)

sub = web.Application()
sub['key2'] = 'val2'
sub.add_routes([web.get('/', sub_handler)])
root.add_subapp('/sub', sub)

client = await aiohttp_client(root)

resp = await client.get('/')
assert resp.status == 200
resp = await client.get('/sub/')
assert resp.status == 201


async def test_subapp_chained_config_overriding(aiohttp_client):

async def main_handler(request):
assert request.config.key == 'val1'
return web.Response(status=200)

root = web.Application()
root['key'] = 'val1'
root.add_routes([web.get('/', main_handler)])

async def sub_handler(request):
assert request.config.key == 'val2'
return web.Response(status=201)

sub = web.Application()
sub['key'] = 'val2'
sub.add_routes([web.get('/', sub_handler)])
root.add_subapp('/sub', sub)

client = await aiohttp_client(root)

resp = await client.get('/')
assert resp.status == 200
resp = await client.get('/sub/')
assert resp.status == 201