From 9c32023a61cd3a41f4c4139d3f1f92d66a335734 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Thu, 10 Sep 2020 15:24:53 -0500 Subject: [PATCH] Make threadSiteSubscriber install sites that are unrecognized. 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.' --- CHANGES.rst | 5 ++ src/nti/site/subscribers.py | 48 +++++++++------ src/nti/site/tests/test_site.py | 102 +++++++++++++++++--------------- src/nti/site/tests/test_sync.py | 15 +++-- 4 files changed, 99 insertions(+), 71 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index a4c22e2..4bf3467 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -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`. diff --git a/src/nti/site/subscribers.py b/src/nti/site/subscribers.py index 8b7a61a..a2a4f60 100644 --- a/src/nti/site/subscribers.py +++ b/src/nti/site/subscribers.py @@ -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() @@ -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): diff --git a/src/nti/site/tests/test_site.py b/src/nti/site/tests/test_site.py index 9a214ca..2c11c8e 100644 --- a/src/nti/site/tests/test_site.py +++ b/src/nti/site/tests/test_site.py @@ -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 @@ -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 @@ -62,7 +62,7 @@ import fudge -class IMock(Interface): +class IMock(Interface): # pylint:disable=inherit-non-class pass @interface.implementer(IMock) @@ -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) @@ -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', ()) @@ -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): @@ -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)) @@ -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 @@ -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 @@ -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): @@ -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)) @@ -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() @@ -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() @@ -488,7 +494,7 @@ 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( @@ -496,10 +502,10 @@ def test_pickle_zodb_lookup_utility(self): (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() @@ -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) @@ -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)) @@ -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): diff --git a/src/nti/site/tests/test_sync.py b/src/nti/site/tests/test_sync.py index ed7e9f1..5176cfa 100644 --- a/src/nti/site/tests/test_sync.py +++ b/src/nti/site/tests/test_sync.py @@ -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 @@ -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: