Implement dynamic prefix for subapps

[achimnol]

What do these changes do?

This allows use of variables (regular expressions) in the subapp prefixes.

Are there changes in behavior for the user?

Existing codes are not affected.

Related issue number

I am making this PR after discussion with @asvetlov in gitter.

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• I'm already in CONTRIBUTORS.txt
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[achimnol]

My implementation approach:

• Instead of calling add_prefix() to all resources in the subapp at the constructor of DynamicSubAppResource class, I strip the matched prefix from the request to resolve the subapp's routes, cloning the request with a new URL.
  • This would incur some performance overheads, but I tried to implement it "correctly" first.
  • Existing codes that use plain subapp prefixes won't be affected.
  • User-defined web handlers still receive the original request object, so I belive that user codes should not be affected in any way.
• Overlapped variable names are overrided by subapps. (e.g., a subapp with a /{name}.txt resource with a dynamic prefix /{name} will get "def" for the "name" key of match_info if resolved with "/abc/def.txt")

[codecov-io]

Codecov Report

Merging #2756 into master will decrease coverage by 0.08%.
The diff coverage is 92.45%.

@@            Coverage Diff             @@
##           master    #2756      +/-   ##
==========================================
- Coverage   97.97%   97.88%   -0.09%     
==========================================
  Files          39       39              
  Lines        7464     7528      +64     
  Branches     1310     1322      +12     
==========================================
+ Hits         7313     7369      +56     
- Misses         48       52       +4     
- Partials      103      107       +4

Impacted Files	Coverage Δ
aiohttp/web_app.py	99.09% <100%> (ø)	⬆️
aiohttp/abc.py	100% <100%> (ø)	⬆️
aiohttp/web_urldispatcher.py	98.14% <92%> (-1.08%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39831ca...245be63. Read the comment docs.

[achimnol]

To allow access to all overriden variables, we could use ChainMap / MultiDict instead of dict for building UrlMappingMatchInfo class, but I'm not sure if just enumerating multi-values in the nesting order (like its _apps property) would be sufficient or not.
@asvetlov How about your ideas?

[achimnol]

Okay, I implemented using ChainMap. 😉

[asvetlov]

Good catch!

[asvetlov]

@achimnol thanks for PR.
It covers 99% but I see the only one problem: what if {name} is present in both dynamic rules: once in subapp prefix itself and other one in routed sub application?

I see the solution:

1. Add .parent property to UrlMappingMatchInfo. For nested apps it will point on subapp prefix route.
2. Don't derive UrlMappingMatchInfo from dict but implement collections.abc.Mapping interface and __getitem__ method. The method can look into .parent if the key is absent.

Right now I don't care about performance, clean API is the first target. Later we can add Cythonized version of UrlMappingMatchInfo if needed.

What do you think about?

[achimnol]

1. My PR handles such cases using ChainMap (see the test case test_nested_dynamic_subapp_resolution) -- .maps can serve the purpose of .parent you suggest, like the _apps internal property. I think .parent may not be very useful because from the subapp's perspective it is anyway "unknown" how many times it should repeat traversal recursively to get the "right" parent. I thought it is better to expose all maps as-is, like ChainMap already does. However, if you prefer .parent, I can rewrite it along with 2.

2. The original code was deriving UrlMappingMatchInfo from dict so I followed that style. If you'd like to change this, I can rewrite it.

[achimnol]

To change UrlMappingMatchInfo to be a self-nested data structure (e.g., to use match_info.parent.parent), I think we need to rewrite its apps property handling as well because it also treats the list of nested apps as its own property rather than having a single reference to the current app -- so that the user code becomes match_info.current_app / match_info.parent.current_app.

Also I still don't get the benefit of having multiple per-app-level match_info objects for a single resolution operation with duplicate references to routes for each of them since sub-app resources are not treated as final destinations.

I think it is sufficient to have nested maps via ChainMap just like apps property. 😕

[asvetlov]

I've convinced. Let's use chained map approach.
But please wait for my careful review.

[asvetlov]

Sorry for delay and thank you for patience.

I have some comments.
Let's discuss my proposals.

[asvetlov]

I really like the fact that UrlMappingMatchInfo is immutable now.

[asvetlov]

super() call is not needed, Python machinery will work fine without it.

[asvetlov]

I suspect you should insert a new dict in front of maps -- like we do it for add_app().
Moreover maybe merging add_app() and add_variables() functionality makes sense.
What do you think about _add_app(app, match_dict) method as replacement for add_app.
Adding underscore emphasizes that the method is private. add_app() was never a part of documented public API, I think the change is safe in backward compatibility perspective.

[achimnol]

ChainMap resolves from the beginning of its maps. The point here is that add_variables() is called after resolving the subapp's routes, which means that subapp's match variables are added to the chain map first. Since we want to give priority to the innermost subapp's variables in the merged view of this chain map, this append is natural.

I think merging this method into add_app(app, match_dict) is a good idea. 👍

[achimnol]

Hm.... merging into a single method causes a lot of problem:

• When instantiating UrlMappingMatchInfo we already have match_dict (so the chain map begins with a single layer) but _app is None and _apps is an empty tuple. This makes the number of nested maps and apps inconsistent.
• Changing UrlMappingMatchInfo's constructor to accept an explicit app argument requires Route and Resource classes to keep track of their owner app because resolve() method should be able to indicate the exact app instance to create the match info object. request.app is a self null reference there.
• add_app() is called at many other places such as test_utils.py and web_app.py where match_dict is out-of-scope.

The root cause of this mess is that app and match_dict are added to UrlMappingMatchInfo at different places and timings. 😕

[asvetlov]

repr(self._variables) instead of repr(dict(self)) is better I believe. There is no reason to hide chained variables.

[achimnol]

This was to make existing test cases that compares the repr result with a constant string to pass. Then I will change those test cases to match with ChainMap's repr result.

[asvetlov]

Maybe let's use just maps name?
maps is shorted, in documentation we could refer to ChainMap.maps. maps is mentally more familiar to people if they know about ChainMap already.

[achimnol]

This is also related to the fact that resolution order of the chain map and its build order are reversed. The use of reversed() here is to be consistent with the order of the return value of apps property.
Also I wanted to make it read-only since ChainMap's maps property is mutable -- so I've used a different name because they are not the same. Still, I'd like to change the name to maps if you still want it. :) If so, I will update the docs as well.

[asvetlov]

The doc is not relevant anymore, Mapping should replace ChainMap.
Also .variable_maps or .maps attribute should be documented.

[asvetlov]

Let's consider the following:

async def handler(request):
    return web.Response(text=request.math_info['name'])

subapp = web.Application()
subapp.add_routes([web.get('/{name}', handler)])

app = web.Application()
app.add_subapp('/{name}', subapp)

What name should be returned by the handler?

[achimnol]

In this case, request.match_info['name'] in the handler will return the matched value of the innermost {name} definition: the route one.

[asvetlov]

Perfect!

[asvetlov]

I see more complex problem.
.url_for() for all resources from sub-application are broken for dynamic prefixed resource.

Sorry, but the problem is blocker for merging.

[achimnol]

Yes, that's what I mentioned in the gitter channel.
I think rewriting url_for() to have a recursive architecture so that route resources of different types in different subapp levels could be chained requires a lot of work, so I'd like to suggest it as a separate PR.
Maybe we could mark this PR as "provisional" if merged before it.

[achimnol]

I'm currently very busy due to my business work -- once I get some time after a few weeks, I'll give a look on the extension plan for url_for().

[asvetlov]

The problem is that without url_for fix not only url_for() for subapp itself doesn't work but all url_for() calls for subapp resources are broken.
That's why I think everything should be done in single PR.

[achimnol]

Yeah, I agree with you if possible it's better to make the changes in a single PR.
While I have to be a little bit away from this PR right now, other people's contributions to this PR are welcome!

[achimnol]

Implemented support for url_for().
There is an issue to decide on: unlike the chained match_info, the **kwargs passed to url_for() does not distinguish which key is for which subapp prefix.

• Let it just do what it does now. Existing code should work as-is.
• Add a mandatory property name to PrefixedSubAppResource and DynamicSubAppResource.
  Let users prefix specific keys with those names, e.g., "myprefix.version" and "version" will treated differently. In this case, the chained match_dict may be also replaced with a flat dict with prefixed keys. But this requires a little change to existing codes that use subapps.

@asvetlov What do you think?

[achimnol]

Ah, since dynamic prefix is not a released API yet, I think it is safe to make the proposed name-and-predixed-keys scheme optional.
If the user wants to avoid key collision then they can use this optional name prefixes, but when not, they can just use as-is. No breaking for existing codes. Yay.

[asvetlov]

Why not raw_path?

[achimnol]

What if the prefix includes arbitrary unicode characters?

[asvetlov]

Mixing PCT encoded and decoded data can make a mess easy.
I believe we should follow the strategy:

1. Convert all user provided path, prefix etc into normalized (PCT-encoded actually) form by using yarl. Already normalized paths passed as-is by yarl.
2. Normalize given HTTP path (we do it).
3. Match normalized path to already pre-normalized patterns.
4. Return request.match_info['name'] in PCT-decoded form because users most likely want getting 이름 instead of %EC%9D%B4%EB%A6%84

If aiohttp code violates the rule somewhere -- it should be fixed.
Do you agree?

[achimnol]

Okay, I agree to keep the rule consistent. Let me inspect and fix up this PR.

[achimnol]

I think then PlainResource.__init__() also need this, like PrefixResource:

self._path = URL.build(path=path).raw_path

[achimnol]

Now if we have a nested dynamic subapp chain:

root_app = web.Application()
outer_app = web.Application()
inner_app = web.Application()
resource = inner_app.router.add_resource('/{var}')
route = resource.add_route('GET', ...)
outer_app.add_subapp('/{var}', inner_app, name='b')
root_app.add_subapp('/{var}', outer_app, name='a')

Then match_dict after resolving "/abc/def/ghi" has the content:

{
  'a.var': 'abc',
  'b.var': 'def',
  'var': 'ghi',
}

route.url_for(**{'a.var': '123', 'b.var': '456', 'var': '789'}) gives:

'/123/456/789'

When inner_app and outer_app has beend added without names,
the match result contains only the innermost variable:

{
  'var': 'ghi'
}

and route.url_for(var='xxx') gives:

'/xxx/xxx/xxx'

No more ChainMap!

[achimnol]

Rebased against latest master.

[achimnol]

I think it's ready to get reviewed. :)

[asvetlov]

Let me meditate for a while.
"name." looks... strange. How to manage a situation if name argument for app.add_subapp was not passed?
Another thing worries me is: suppose I have a common-usable application like admin or debug toolbar.
I'm adding it with variable prefix and the subapp becomes broken: it has a knowledge about its required parents for generating urls.
The worse thing is that breakage appears on subapp usage (url generations for filing HTTP templates), not on subapp adding.

If you have ideas how to make it steady -- please share your vision.
We a free to invite a new API or even restrict subapp type to VariablePrefixAwareSubapp inherited from web.Application but the game rules should be discussed and fixed in stone before the PR merging.

I understand your striving to make subapp prefix flexible but my guts feel that we should be very careful with chosen design. Otherwise fitting a opened can of worms will be painful.

I suggest excluding the PR from aiohttp 3.1 but working hard to publish it as part of aiohttp 3.2
Your idea for chaining match_info is brilliant.
There is a very visible request for providing chained info for nested applications:

• Sharing db connection to nested nested app #2689 for accessing parent configuration like request.app['db']
• access subapp routes via <app-name>:<route-name> #2314 for building URLs from sibling sub-apps
  We should make a uniform approach for all of these.

P.S.
I'll go to Ho Chi Minh tomorrow evening for a couple weeks. Not sure if I will have enough time to work on aiohttp these dates.
The city is much closer to Seoul than Kiev but unfortunately still so far for meeting with you offline :(

[achimnol]

I see. You want more isolation between apps -- and I agree with that.
I thought having explicit names was better than nested scopes because it is clear to read.

I'm fine to go back to the first ChainMap design for match_info, keeping my "recursive" implementation.
The problem is url_for().

To allow use url_for() in the innermost ("leaf") app without knowing whatever variables are included in its outer subapp prefixes, then I think we need a strong assumption that url_for() is only used inside request handlers where match_info is available in the current task-local context.
Once url_for() has access to the nested match_info's ChainMap then it can fill the prefix variables using the matched values of the current request's path.

Still we have two issues:

1. Python asyncio currently lacks task-local context support which automatically propagates to subtasks. To workaround this, it may be necessary to change the url_for() API to accept the request object (since it serves the role of handler-local context now) or its match_info attribute. But this would break use of url_for() in template rendering. access subapp routes via <app-name>:<route-name> #2314's request.url_for() may be an option here.
2. url_for() may be used to refer URLs with prefixes other than the current request's path has. In this case users need to provide the prefix variables anyway, like my "name." design (of course the concrete method may be different).

@asvetlov What do you think?

[achimnol]

More thoughts:
Maybe could we ingore the 2nd case if we stick with "isolated" apps?
If each app never calls url_for() against routes in other apps, then filling the prefix variables with the current request's path should be ok. But in the reality, like in one of my Django projects, cross-reference to URLs in other apps in one app's template rendering often happens because a single rendered view of webpage usually involves multiple apps (e.g., dashboard, user authentication, posts, comments, etc.). Nonetheless it is also possible to argue that such cases must be implemented as a single monolithic app — there is no reason to match the semantic of aiohttp's app with Django's app.
See below!

[achimnol]

More more thoughts:
When composing multiple apps, cross-reference to URLs in other apps usually happen in a specific direction only: from outer apps to inner apps, not to parent/sibling apps.

From the perspective of inner apps, it's safe to assume that the aiohttp's web framework will take care of filling outer prefix variables with the handler-local match_info context.

From the perspective of outer apps, they need to explicitly give the value of prefix variables when calling url_for() of routes of inner apps. In this specific case, we need a way to distinguish the duplicate variable names in the prefix names.

[asvetlov]

Maybe name prefixes are not bad but I need a time to experiment with.
Regarding to mandatory params from outer apps etc:

1. We can invite a new API for it, something like request.build_url() (the name is a subject for changing)
2. Context local variables (implicit context) are not required as we always have explicit context as request.
3. Inner/outer applications can be useful terminology.