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.

Sign up for GitHub

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

Jump to bottom

Implement Application.add_subdomain (#1342) #2194

Closed
wants to merge 1 commit into from
Closed

Implement Application.add_subdomain (#1342) #2194

wants to merge 1 commit into from

Conversation

amirouche
Copy link

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
Copy link
Author

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
samuelcolvin reviewed Aug 16, 2017
View reviewed changes
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

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

aiohttp/web.py
if subapp.frozen:
raise RuntimeError("Cannot add frozen application")
if subdomain == '':
raise ValueError("Subdomain cannot be empty")
Copy link
Member

Choose a reason for hiding this comment

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

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

aiohttp/web_urldispatcher.py
resource.add_prefix(prefix)

def url_for(self, *args, **kwargs):
raise RuntimeError(".url_for() is not supported "
Copy link
Member

Choose a reason for hiding this comment

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

maybe NotImplementedError would make more sense?

Copy link
Author

Choose a reason for hiding this comment

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

Ok.

aiohttp/web_urldispatcher.py

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

Choose a reason for hiding this comment

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

what if Host is missing from headers?

Copy link
Author

Choose a reason for hiding this comment

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

It will fail. Should I do something specific here?

Copy link
Member

Choose a reason for hiding this comment

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

Surely just .get() would work?

Copy link
Author

Choose a reason for hiding this comment

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

Ok.

espositofulvio
espositofulvio reviewed Aug 28, 2017
View reviewed changes
Copy link

@espositofulvio espositofulvio left a comment

Choose a reason for hiding this comment

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

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

aiohttp/web.py
@@ -197,6 +197,23 @@ def add_subapp(self, prefix, subapp):
subapp._set_loop(self._loop)
return resource

def add_subdomain(self, subdomain, subapp):
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

I agree.

aiohttp/web_urldispatcher.py

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

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

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

aiohttp/web_urldispatcher.py

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

Choose a reason for hiding this comment

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

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

@aamalev aamalev mentioned this pull request Mar 6, 2018
Merged
5 tasks
@amirouche
Copy link
Author

I think #2809 is better and more general

@amirouche amirouche closed this Oct 8, 2018
@lock
Copy link

lock bot commented Oct 28, 2019

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

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@samuelcolvin samuelcolvin samuelcolvin left review comments

@espositofulvio espositofulvio espositofulvio left review comments

Assignees
No one assigned
Labels
outdated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amirouche @samuelcolvin @espositofulvio