The mechanism of the rules for the matching of subapps by aamalev · Pull Request #2809 · aio-libs/aiohttp

aamalev · 2018-03-06T23:49:23Z

I propose to implement subdomain as part of a general approach, when subapp can be matched according to a certain rule

subapp = web.Application()
app.add_subapp(Domain('example.com'), subapp)

Are there changes in behavior for the user?

No

Related issue number

#1342
#2194

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
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."

…subapps

codecov-io · 2018-03-07T00:33:52Z

Codecov Report

Merging #2809 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2809      +/-   ##
==========================================
+ Coverage   97.99%   98.01%   +0.02%     
==========================================
  Files          39       39              
  Lines        7379     7506     +127     
  Branches     1296     1316      +20     
==========================================
+ Hits         7231     7357     +126     
  Misses         47       47              
- Partials      101      102       +1

Impacted Files	Coverage Δ
aiohttp/web_app.py	`99.13% <100%> (+0.2%)`	⬆️
aiohttp/web_urldispatcher.py	`99.27% <100%> (+0.07%)`	⬆️
aiohttp/abc.py	`100% <100%> (ø)`	⬆️
aiohttp/client_reqrep.py	`97.24% <0%> (-0.21%)`	⬇️
aiohttp/web_request.py	`99.69% <0%> (ø)`	⬆️
aiohttp/client_exceptions.py	`95.6% <0%> (+0.86%)`	⬆️

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 7f7b12a...4a57eba. Read the comment docs.

asvetlov · 2018-03-07T07:30:17Z

I think app.add_domain() is more readable.

Regarding to router changes: they are either incomplete or sense-less. Say, I see no usage of DefaultRule and many others.

aamalev · 2018-03-07T08:44:51Z

@asvetlov add_domain is more readable but highly specialized.

DefaultRule was born because of the lack of add an application without a prefix.
AbstractRuleMatching allow users to create rules themselves.

UseCases:

Merging routers of two or more subapps without prefixes
Serving different domains through a Domain rule
Serving different masks of domains and subdomains through user rules
Service of authorized and public requests in various subapps
Disabling of subdomains by billing rules

asvetlov · 2018-03-07T19:03:15Z

It still looks too complex, add_doumain() is better at least for documenting and teaching.
For example in Python classes usually are created by class A(Base): statement while technically type(...) call should be enough, and type() is potentially more flexible.

amirouche · 2018-04-17T14:34:41Z

Should this be closed?

amirouche · 2018-04-17T14:35:07Z

I am asking because I would like to continue to work on #1342

…apps

codecov-io · 2018-08-23T07:15:30Z

Codecov Report

Merging #2809 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2809      +/-   ##
=========================================
+ Coverage   97.98%     98%   +0.01%     
=========================================
  Files          44      44              
  Lines        8075    8151      +76     
  Branches     1356    1365       +9     
=========================================
+ Hits         7912    7988      +76     
  Misses         68      68              
  Partials       95      95

Impacted Files	Coverage Δ
aiohttp/web_urldispatcher.py	`99.2% <100%> (+0.08%)`	⬆️
aiohttp/web_app.py	`96.12% <100%> (+0.17%)`	⬆️

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 4b2ac43...35259d5. Read the comment docs.

aamalev · 2018-08-23T22:52:26Z

@asvetlov please review

aamalev · 2018-10-02T23:42:54Z

@asvetlov Final realization adds only one public interface Application.add_domain(str, subapp) which encapsulates the mechanism

asvetlov

Thanks for the PR.
Please address my comments

asvetlov · 2018-10-15T20:41:39Z

aiohttp/abc.py

        """Emit log to logger."""
+
+
+class AbstractRuleMatching(ABC):


I doubt if we need this abstract class: it is used for Domain class only.

Still the question

There are two and a half ways to decide his fate:

1 Throw away

2 To refine the subsystem of rules

2+1/2. Combine MatchedSubAppResource and PrefixedSubAppResource as they differ practically only by the method resolve

class PrefixedSubAppResource(PrefixResource): async def resolve(self, request): if not request.url.raw_path.startswith(self._prefix): return None, set() class MatchedSubAppResource(PrefixedSubAppResource): async def resolve(self, request): if not await self._rule.match(request): return None, set()

and implement the rule

class Prefix(AbstractRuleMatching): def __init__(self, prefix): self._prefix = prefix @property def prefix(self): return self._prefix def match(self, request): return request.url.raw_path.startswith(self._prefix)

I'd like to not introduce a new public ABC class.
Moreover, in aiohttp 4.0 I want to drop a pluggable router. By this, we'll have the only supported router implementation -- the current one. In this light adding a new ABC to API part that will never be extended outside doesn't make any sense.

If you have a feeling that ABC is valuable -- feel free to keep it but move into web_urldispatcher.py.

tests/test_urldispatch.py

…e-matching-subapps

aamalev · 2018-10-17T08:09:25Z

@asvetlov I moved AbstractRuleMatching to web_urldispatcher
but mypy shot in an unexpected place

aiohttp/helpers.py:222: error: Too few arguments for "BasicAuth"
aiohttp/http_websocket.py:547: error: The return type of "__init__" must be None

and local mypy-0.641

aiohttp/helpers.py:221: error: Argument 1 to "BasicAuth" has incompatible type "Optional[str]"; expected "str"
aiohttp/helpers.py:221: error: Argument 2 to "BasicAuth" has incompatible type "Optional[str]"; expected "str"

asvetlov · 2018-10-17T08:18:19Z

Please merge master, mypy is updated and helpers.py is fixed

…e-matching-subapps

asvetlov · 2018-10-17T16:03:58Z

Thanks!

amirouche · 2018-12-07T23:22:48Z

docs/web_reference.rst


      :returns: a :class:`PrefixedSubAppResource` instance.

+   .. method:: add_domain(domain, subapp)


@aamalev Maybe domain might have been better named pattern. Also there is no documentation about how domain is matched against the host ie. the fact that foo* match foobar.

The pull request is welcome!

lock · 2019-12-08T01:11:15Z

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

The implementation of the mechanism of the rules for the matching of …

1fb9b83

…subapps

aamalev force-pushed the rule-matching-subapps branch from b85b227 to 1fb9b83 Compare March 7, 2018 00:05

Merge branch 'master' into rule-matching-subapps

0d3e295

aamalev force-pushed the rule-matching-subapps branch from fb0e4b9 to bbe37ed Compare March 8, 2018 21:34

add_domain impl

8241398

aamalev force-pushed the rule-matching-subapps branch from bbe37ed to 8241398 Compare March 8, 2018 22:20

add_domain doc

4a57eba

aamalev added 3 commits August 13, 2018 02:53

Merge remote-tracking branch 'aio-libs/master' into rule-matching-sub…

f2e4f3a

…apps

refact add_domain

769e8bc

fix isort

7097208

aamalev force-pushed the rule-matching-subapps branch from 66d1513 to 7097208 Compare August 23, 2018 06:34

aamalev added 3 commits August 23, 2018 22:33

test_add_domain_not_str

d3cc9d2

news fragment for add_domain

a542d82

fix doc add_domain

4007514

amirouche mentioned this pull request Oct 8, 2018

Implement Application.add_subdomain (#1342) #2194

Closed

5 tasks

asvetlov requested changes Oct 15, 2018

View reviewed changes

aamalev added 3 commits October 17, 2018 00:08

Merge branch 'master' of https://github.com/aio-libs/aiohttp into rul…

2aa9c99

…e-matching-subapps

fix SyntaxError on test_domain_valid

2d97c3f

move AbstractRuleMatching from abc to web_urldispatcher

21b6697

Merge branch 'master' of https://github.com/aio-libs/aiohttp into rul…

35259d5

…e-matching-subapps

asvetlov approved these changes Oct 17, 2018

View reviewed changes

asvetlov merged commit 11d5f99 into aio-libs:master Oct 17, 2018

amirouche reviewed Dec 7, 2018

View reviewed changes

samuelcolvin mentioned this pull request Jan 4, 2019

Route for subdomains #1342

Closed

lock bot added the outdated label Dec 8, 2019

lock bot locked as resolved and limited conversation to collaborators Dec 8, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Dec 8, 2019

aamalev deleted the rule-matching-subapps branch December 9, 2019 00:13


		:returns: a :class:`PrefixedSubAppResource` instance.

		.. method:: add_domain(domain, subapp)

Uh oh!

Conversation

aamalev commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

codecov-io commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

asvetlov commented Mar 7, 2018

Uh oh!

aamalev commented Mar 7, 2018

Uh oh!

asvetlov commented Mar 7, 2018

Uh oh!

amirouche commented Apr 17, 2018

Uh oh!

amirouche commented Apr 17, 2018

Uh oh!

codecov-io commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aamalev commented Aug 23, 2018

Uh oh!

aamalev commented Oct 2, 2018

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

asvetlov Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

asvetlov Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

aamalev Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

asvetlov Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aamalev commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asvetlov commented Oct 17, 2018

Uh oh!

asvetlov commented Oct 17, 2018

Uh oh!

amirouche Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov Dec 8, 2018

Choose a reason for hiding this comment

Uh oh!

lock bot commented Dec 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aamalev commented Mar 6, 2018 •

edited

Loading

codecov-io commented Mar 7, 2018 •

edited

Loading

codecov-io commented Aug 23, 2018 •

edited

Loading

aamalev commented Oct 17, 2018 •

edited

Loading

amirouche Dec 7, 2018 •

edited

Loading