Implement Application.add_subdomain (#1342) by amirouche · Pull Request #2194 · aio-libs/aiohttp

amirouche · 2017-08-12T21:31:01Z

What do these changes do?

Implement basic subdomain support

Are there changes in behavior for the user?

Yes, one can add a subapp for a given domain as follow:

    subdomain = web.Application()
    subdomain.router.add_get('/to', handler)

    # setup root app
    app = web.Application()
    app.add_subdomain('example.aiohttp.test', subdomain)

Related issue number

#1342

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.bug)
- 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."

amirouche · 2017-08-12T21:37:44Z

It's not exactly what is explained in #1342 but I think it's what was intended.

Mind the fact, that I don't think this can be merged given the checklist is not complete and that IMO the user experience is not good enough. With the current code, the domain is hardcoded which, I think, will be a pain during dev because the dev has to override DNS resolution in her /etc/hosts making it impossible to access the production website via the normal URL. What would be best, is somehow to add somewhere the information about the domain to the root app. So that, it can be configured in production and dev.

WDYT?

samuelcolvin

Apparent from comments I think this is looking good. Would be great if if could be included in the next release?

samuelcolvin · 2017-08-16T09:24:40Z

aiohttp/web.py

+        if subapp.frozen:
+            raise RuntimeError("Cannot add frozen application")
+        if subdomain == '':
+            raise ValueError("Subdomain cannot be empty")


Can we do any more validation of the subdomain? Eg. that it doesn't contain spaces or /, does yarl or urllib allow this?

samuelcolvin · 2017-08-16T09:25:02Z

aiohttp/web_urldispatcher.py

+            resource.add_prefix(prefix)
+
+    def url_for(self, *args, **kwargs):
+        raise RuntimeError(".url_for() is not supported "


maybe NotImplementedError would make more sense?

samuelcolvin · 2017-08-16T09:26:55Z

aiohttp/web_urldispatcher.py

+
+    @asyncio.coroutine
+    def resolve(self, request):
+        if not request.headers['Host'] == self._subdomain:


what if Host is missing from headers?

It will fail. Should I do something specific here?

Surely just .get() would work?

espositofulvio

I guess overall is looking very good, I just made a few comments on the possibility to replace subdomain with domain inline with code

espositofulvio · 2017-08-28T15:54:48Z

aiohttp/web.py

            subapp._set_loop(self._loop)
        return resource

+    def add_subdomain(self, subdomain, subapp):


Can we rename this to add_domain(self, domain, subapp)? The way you did it actually enables multi-site apps on different domains, not only subdomains I think, so why don't just change the name?

espositofulvio · 2017-08-28T15:56:22Z

aiohttp/web_urldispatcher.py

+
+    def __init__(self, subdomain, app):
+        super().__init__()
+        self._subdomain = subdomain


If we go for changing subdomain to domain, I guess it might be useful to use Yarl to parse the subdomain here so we get for free the port number and can actually dispatch on the actual host (domain + port number)

Do you mean that we should patch yarl to parse the domain wiht subdomain?

espositofulvio · 2017-08-28T15:57:16Z

aiohttp/web_urldispatcher.py

+
+    @asyncio.coroutine
+    def resolve(self, request):
+        if not request.headers['Host'] == self._subdomain:


Again, pending the decision to have domain in place of subdomain, I guess using Yarl to parse the Host header here is also safer.

amirouche · 2018-10-08T19:38:17Z

I think #2809 is better and more general

lock · 2019-10-28T00:04:49Z

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].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

Implement Application.add_subdomain (#1342)

4b0c9ff

samuelcolvin reviewed Aug 16, 2017

View reviewed changes

espositofulvio reviewed Aug 28, 2017

View reviewed changes

aamalev mentioned this pull request Mar 6, 2018

The mechanism of the rules for the matching of subapps #2809

Merged

5 tasks

amirouche closed this Oct 8, 2018

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

Uh oh!

Conversation

amirouche commented Aug 12, 2017

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

amirouche commented Aug 12, 2017

Uh oh!

samuelcolvin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

espositofulvio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirouche commented Oct 8, 2018

Uh oh!

lock bot commented Oct 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants