Skip to content

Commit

Permalink
Make threadSiteSubscriber install sites that are unrecognized.
Browse files Browse the repository at this point in the history 
Instead of raising LocationError. The rationale is thus:

1) We don't have unexpected 404s in production so we're not hitting this case. Therefore, it should have no production impact.
2) Raising is highly surprising.
3) We want to encourage more (correct, careful) use of scoped sites (e.g., courses can be sites, with their questions contained in their local site manager).
4) Adding more special cases devolves into a game of whack-a-mole. There's a general rule (when traversing into a site, apply it), lets use that. Because 'Special cases aren't special enough to break the rules.'
  • Loading branch information
@jamadden
jamadden committed Sep 10, 2020
1 parent 3d3298e commit 9c32023
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 71 deletions.
5 changes: 5 additions & 0 deletions CHANGES.rst
View file
Expand Up @@ -11,6 +11,11 @@

Previously, it never installed root sites.

- Make ``threadSiteSubscriber`` install sites when their configuration
is not recognized.

Previously, it would raise ``LocationError``.

- Fix tests with, and require, zope.site 4.4.0 or above. See
:issue:`34`.

Expand Down
48 changes: 31 additions & 17 deletions src/nti/site/subscribers.py
View file
Expand Up @@ -75,27 +75,40 @@ def setSiteManager(self, new_man): # pylint:disable=unused-argument
@component.adapter(ISite, IBeforeTraverseEvent)
def threadSiteSubscriber(new_site, _event):
"""
Set the current ``zope.component.hooks`` site to
the ``new_site`` object found during traversal,
being careful to maintain any previously installed host (site-name)
configurations as lower priority than the new site.
Sites encountered during traversal are expected to have the
main application site (e.g., ``nti.dataserver``) in their base chain
so we have access to its configuration and persistent utilities.
This implies that sites encountered during traversal are either
Set the current ``zope.component.hooks`` site to the ``new_site``
object found during traversal, being careful to maintain any
previously installed host (site-name) configurations as lower
priority than the new site.
Sites encountered during traversal are expected to have the main
application site (e.g., ``nti.dataserver``) in their base chain so
we have access to its configuration and persistent utilities. This
implies that sites encountered during traversal are either
synthetic (generated by a traversal adapter to use some particular
``IComponents``) or themselves persistent.
``IComponents``) or themselves persistent.
Because of this, when we encounter the root or dataserver folders
as sites, we take no action.
as sites, we take no action (unless there is no site presently
installed; in that case we install them).
We expect that something else takes care of clearing the site.
.. important::
Clearing the site is important, especially if the site was persistent.
You can use a subscriber to "end request" style events or otherwise
make it part of request lifecycle. When that's not possible, please do
so manually.
.. versionchanged:: 2.3.0
Always install the *new_site* if there is no current site.
Previously, if *new_site* provided `~.IMainApplicationFolder` or
`zope.site.interfaces.IRootFolder`, it was always ignored.
Always install the *new_site* if there
is no current site. Previously, if *new_site* provided
`~.IMainApplicationFolder` or `zope.site.interfaces.IRootFolder`,
it was always ignored.
No longer raises a ``LocationError`` if an unknown type of site
is encountered. Instead, simply installs it, replacing the current site.
"""

current_site = getSite()
Expand Down Expand Up @@ -164,9 +177,10 @@ def threadSiteSubscriber(new_site, _event):
setSite(new_fake_site)
return

# Cancel traversal using a LocationError. This typically
# will get surfaced as a 404.
raise LocationError("Unknown kind of site", new_site, current_site)
# We're out of special cases we understand. Hopefully the
# application knows what it is doing.
setSite(new_site)


@component.adapter(INewLocalSite)
def new_local_site_dispatcher(event):
Expand Down
102 changes: 54 additions & 48 deletions src/nti/site/tests/test_site.py
View file
Expand Up @@ -6,6 +6,7 @@

# disable: accessing protected members, too many methods
# pylint: disable=W0212,R0904
# pylint:disable=too-many-ancestors

from hamcrest import is_
from hamcrest import is_not
Expand Down Expand Up @@ -33,9 +34,8 @@
from zope.component.hooks import setSite
from zope.component.hooks import site as currentSite

from zope.component.interfaces import IComponents
from zope.interface.interfaces import IComponents

from zope.location.interfaces import LocationError

from zope.component import globalSiteManager as BASE

Expand All @@ -62,7 +62,7 @@

import fudge

class IMock(Interface):
class IMock(Interface): # pylint:disable=inherit-non-class
pass

@interface.implementer(IMock)
Expand All @@ -77,7 +77,7 @@ def __init__(self, site_man=None):
def getSiteManager(self):
return self.site_man

class IFoo(Interface):
class IFoo(Interface): # pylint:disable=inherit-non-class
pass

@interface.implementer(IFoo)
Expand Down Expand Up @@ -112,17 +112,17 @@ def testProxyHostComps(self):
# It should have the marker property
assert_that(cur_site.getSiteManager(),
has_property('host_components',
host_comps))
host_comps))

assert_that(ro.ro(cur_site.getSiteManager()),
contains(
# The first entry is synthesized
has_property('__name__', new_comps.__name__),
pers_comps,
# The host comps appear after all the bases
# in the ro of the new site
host_comps,
BASE))
# The first entry is synthesized
has_property('__name__', new_comps.__name__),
pers_comps,
# The host comps appear after all the bases
# in the ro of the new site
host_comps,
BASE))

def testTraverseFailsIntoSiblingSiteExceptHostPolicyFolders(self):
new_comps = BaseComponents(BASE, 'sub_site', ())
Expand All @@ -137,16 +137,20 @@ def testTraverseFailsIntoSiblingSiteExceptHostPolicyFolders(self):
new_site2 = MockSite(new_comps2)
new_site2.__name__ = new_comps2.__name__

# ... we fail...
assert_that(calling(threadSiteSubscriber).with_args(new_site2, None),
raises(LocationError))
# ... and that's allowed, completely replacing the current site...
threadSiteSubscriber(new_site2, None)
assert_that(getSite(), is_(same_instance(new_site2)))

# ... reset ...
threadSiteSubscriber(new_site, None)
assert_that(getSite(), is_(same_instance(new_site)))

# ...unless they are both HostPolicyFolders...
# ... if they are both HostPolicyFolders...
interface.alsoProvides(new_site, IHostPolicyFolder)
interface.alsoProvides(new_site2, IHostPolicyFolder)
threadSiteSubscriber(new_site2, None)

# ... which does not change the site
# ... traversal does not change the site
assert_that(getSite(), is_(same_instance(new_site)))

def test_components_unregister_on_site_removal(self):
Expand Down Expand Up @@ -189,7 +193,7 @@ class PersistentTrivialSite(Persistent, TrivialSite):
try:
x = get_site_for_site_names(('',), trivial_site)
finally:
C3.STRICT_IRO = C3.ORIG_STRICT_IRO
C3.STRICT_IRO = C3.ORIG_STRICT_IRO # pylint:disable=no-member

assert_that(x, is_not(Persistent))
assert_that(x, is_(TrivialSite))
Expand All @@ -208,7 +212,7 @@ def test_get_component_hierarchy(self, fake_find):
# correct behaviour
pers_comps = BaseComponents(BASE, 'persistent', (BASE,))

class MockSite(object):
class LocalMockSite(object):
__parent__ = None
__name__ = None

Expand All @@ -218,7 +222,7 @@ def __init__(self):
site_comps_1 = BaseComponents(pers_comps, '1', (pers_comps,))
site_comps_2 = BaseComponents(site_comps_1, '2', (site_comps_1,))

site = MockSite()
site = LocalMockSite()
site.__parent__[site_comps_1.__name__] = site_comps_1
site.__parent__[site_comps_2.__name__] = site_comps_2

Expand Down Expand Up @@ -246,7 +250,9 @@ def __init__(self):
import zodbpickle.fastpickle as zpickle
except ImportError:
import zodbpickle.pickle as zpickle # pypy
import BTrees.OOBTree
from BTrees import family64
OOBTree = family64.OO.BTree
OIBTree = family64.OI.BTree

class TestBTreeSiteMan(AbstractTestBase):

Expand Down Expand Up @@ -374,13 +380,13 @@ def test_pickle_zodb_lookup_adapter(self):
required=(IFoo,),
provided=IMock)

assert_that(new_base._adapter_registrations, is_(BTrees.OOBTree.OOBTree))
assert_that(new_base._adapter_registrations, is_(OOBTree))
assert_that(new_base._adapter_registrations.keys(),
contains(
((IFoo,), IMock, u''),
((implementedBy(object),), IFoo, u'' ),
((implementedBy(object),), IFoo, u''),
))
assert_that(new_base.adapters._provided, is_(BTrees.family64.OI.BTree))
assert_that(new_base.adapters._provided, is_(OIBTree))
assert_that(new_base.adapters._adapters[0], is_({}))

assert_that(new_base.adapters._adapters[1][IFoo], is_(dict))
Expand All @@ -390,7 +396,7 @@ def test_pickle_zodb_lookup_adapter(self):
required=(IFoo,),
provided=IFoo)

assert_that(new_base.adapters._adapters[1][IFoo], is_(BTrees.family64.OO.BTree))
assert_that(new_base.adapters._adapters[1][IFoo], is_(OOBTree))

transaction.commit()
conn.close()
Expand Down Expand Up @@ -428,17 +434,17 @@ def test_register_implemented_by_lookup_utility(self):
provided2 = new_base.adapters._provided
# Make sure that it only converted once
assert_that(provided1, is_(same_instance(provided2)))
assert_that(new_base._utility_registrations, is_(BTrees.OOBTree.OOBTree))
assert_that(new_base._utility_registrations, is_(OOBTree))

assert_that(new_base._utility_registrations.keys(),
contains(
(IFoo, u''),
((implementedBy(MockSite), u'foo')),
))
assert_that(new_base.utilities._provided, is_(BTrees.family64.OI.BTree))
assert_that(new_base.utilities._adapters[0], is_(BTrees.family64.OO.BTree))
assert_that(new_base.utilities._provided, is_(OIBTree))
assert_that(new_base.utilities._adapters[0], is_(OOBTree))

assert_that(new_base.utilities._adapters[0][IFoo], is_(BTrees.family64.OO.BTree))
assert_that(new_base.utilities._adapters[0][IFoo], is_(OOBTree))


transaction.commit()
Expand Down Expand Up @@ -488,18 +494,18 @@ def test_pickle_zodb_lookup_utility(self):
provided2 = new_base.adapters._provided
# Make sure that it only converted once
assert_that(provided1, is_(same_instance(provided2)))
assert_that(new_base._utility_registrations, is_(BTrees.OOBTree.OOBTree))
assert_that(new_base._utility_registrations, is_(OOBTree))

assert_that(new_base._utility_registrations.keys(),
contains(
(IFoo, u''),
(IMock, u'foo'),
(implementedBy(object), u'foo'),
))
assert_that(new_base.utilities._provided, is_(BTrees.family64.OI.BTree))
assert_that(new_base.utilities._adapters[0], is_(BTrees.family64.OO.BTree))
assert_that(new_base.utilities._provided, is_(OIBTree))
assert_that(new_base.utilities._adapters[0], is_(OOBTree))

assert_that(new_base.utilities._adapters[0][IFoo], is_(BTrees.family64.OO.BTree))
assert_that(new_base.utilities._adapters[0][IFoo], is_(OOBTree))


transaction.commit()
Expand Down Expand Up @@ -564,15 +570,15 @@ class AUtility(object):
autility = AUtility()
interface.alsoProvides(autility, IFoo)
comps.registerUtility(autility)
assert_that(comps._utility_registrations, is_(BTrees.OOBTree.OOBTree))
assert_that(comps._utility_registrations, is_(OOBTree))
assert_that(comps._utility_registrations.keys(),
contains(
((IFoo, u'')),
))
assert_that(comps.utilities._provided, is_(BTrees.family64.OI.BTree))
assert_that(comps.utilities._adapters[0], is_(BTrees.family64.OO.BTree))
assert_that(comps.utilities._provided, is_(OIBTree))
assert_that(comps.utilities._adapters[0], is_(OOBTree))

assert_that(comps.utilities._adapters[0][IFoo], is_(BTrees.family64.OO.BTree))
assert_that(comps.utilities._adapters[0][IFoo], is_(OOBTree))

def test_convert_with_adapter_registered_on_class(self):
comps = BLSM(None)
Expand All @@ -582,9 +588,9 @@ def test_convert_with_adapter_registered_on_class(self):
comps.utilities.btree_map_threshold = 0

comps.registerAdapter(
_foo_factory,
required=(object, type('str')),
provided=IFoo)
_foo_factory,
required=(object, type('str')),
provided=IFoo)

x = comps.getMultiAdapter((object(), 'str'), IFoo)
assert_that(x, is_(1))
Expand All @@ -598,22 +604,22 @@ def test_convert_mark_self_changed(self):
comps.utilities.btree_map_threshold = 0

comps.registerAdapter(
_foo_factory,
required=(object, type('str')),
provided=IFoo)
_foo_factory,
required=(object, type('str')),
provided=IFoo)

comps.registerAdapter(
_foo_factory2,
required=(object, type(0)),
provided=IFoo)
_foo_factory2,
required=(object, type(0)),
provided=IFoo)

x = comps.getMultiAdapter((object(), 'str'), IFoo)
assert_that(x, is_(1))


def _foo_factory(*args):
def _foo_factory(*_args):
return 1
def _foo_factory2(*args):
def _foo_factory2(*_args):
return 2

class TestPermissiveOOBTree(unittest.TestCase):
Expand Down
15 changes: 9 additions & 6 deletions src/nti/site/tests/test_sync.py
View file
Expand Up @@ -34,7 +34,6 @@
from zope.component.hooks import setSite
from zope.component.hooks import site as currentSite

from zope.location.interfaces import LocationError

from nti.site.interfaces import ISiteMapping
from nti.site.interfaces import SiteNotFoundError
Expand Down Expand Up @@ -125,18 +124,22 @@ def testTraverseFailsIntoSiblingSiteExceptHostPolicyFolders(self):
new_site2 = MockSite(new_comps2)
new_site2.__name__ = new_comps2.__name__

# ... we fail...
assert_that(calling(threadSiteSubscriber).with_args(new_site2, None),
raises(LocationError))
# ... and that's allowed, completely replacing the current site...
threadSiteSubscriber(new_site2, None)
assert_that(getSite(), is_(same_instance(new_site2)))

# ... reset ...
threadSiteSubscriber(new_site, None)
assert_that(getSite(), is_(same_instance(new_site)))

# ...unless they are both HostPolicyFolders...
# ...If they are both HostPolicyFolders...
interface.alsoProvides(new_site, IHostPolicyFolder)
interface.alsoProvides(new_site2, IHostPolicyFolder)
repr(new_site) # coverage
str(new_site)
threadSiteSubscriber(new_site2, None)

# ... which does not change the site
# ... traversal does not change the site
assert_that(getSite(), is_(same_instance(new_site)))

# Match a hierarchy we have in nti.app.sites.demo:
Expand Down

0 comments on commit 9c32023

Please sign in to comment.