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.

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

Simplify traversal #105

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/105.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Simplify traversal, fixing e.g. plone.app.theming #165 [petri, petschki]
2 changes: 0 additions & 2 deletions plone-5.2.x.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ virtualenv = 20.0.35
# Error: The requirement ('pep517>=0.9') is not allowed by your [versions] constraint (0.8.2)
pep517 = 0.9.1

# Error: The requirement ('importlib-metadata>=1') is not allowed by your [versions] constraint (0.23)
importlib-metadata = 2.0.0

# cryptography 3.4 requires a rust compiler installed on the system:
# https://github.com/pyca/cryptography/blob/master/CHANGELOG.rst#34---2021-02-07
Expand Down
38 changes: 27 additions & 11 deletions src/plone/rest/tests/test_traversal.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,17 +106,6 @@ def test_html_request_on_existing_view_returns_view(self):
obj = self.traverse(path="/plone/folder1/search", accept="text/html")
self.assertFalse(isinstance(obj, Service), "Got a service")

def test_virtual_hosting(self):
app = self.layer["app"]
vhm = VirtualHostMonster()
vhm.id = "virtual_hosting"
vhm.addToContainer(app)
obj = self.traverse(
path="/VirtualHostBase/http/localhost:8080/plone/VirtualHostRoot/"
) # noqa
self.assertTrue(isinstance(obj, Service), "Not a service")
del app["virtual_hosting"]

def test_json_request_to_regular_view_returns_view(self):
obj = self.traverse("/plone/folder_contents")
self.assertTrue(IBrowserView.providedBy(obj), "IBrowserView expected")
Expand All @@ -132,3 +121,30 @@ def test_json_request_to_view_namespace_returns_view(self):
self.portal[self.portal.invokeFactory("Folder", id="folder1")]
obj = self.traverse("/plone/folder1/@@folder_contents")
self.assertTrue(IBrowserView.providedBy(obj), "IBrowserView expected")

def setup_vhm(self):
app = self.layer["app"]
vhm = VirtualHostMonster()
vhm.id = "virtual_hosting"
vhm.addToContainer(app)

def teardown_vhm(self):
del self.layer["app"]["virtual_hosting"]

def test_virtual_hosting(self):
self.setup_vhm()
obj = self.traverse(
path="/VirtualHostBase/http/localhost:8080/plone/VirtualHostRoot/"
) # noqa
self.assertTrue(isinstance(obj, Service), "Not a service")
self.teardown_vhm()

def test_virtual_hosting_on_navroot_folder(self):
self.setup_vhm()
folder = self.portal[self.portal.invokeFactory("Folder", id="folder1")]
alsoProvides(folder, INavigationRoot)
obj = self.traverse(
path="/VirtualHostBase/http/localhost:8080/plone/folder1/VirtualHostRoot/"
) # noqa
self.assertTrue(isinstance(obj, Service), "Not a service")
self.teardown_vhm()
83 changes: 20 additions & 63 deletions src/plone/rest/traverse.py
Original file line number Diff line number Diff line change
@@ -1,57 +1,45 @@
# -*- coding: utf-8 -*-
from Products.CMFCore.interfaces import ISiteRoot
from Products.SiteAccess.VirtualHostMonster import VirtualHostMonster
from ZPublisher.BaseRequest import DefaultPublishTraverse
from plone.rest.interfaces import IAPIRequest
from plone.rest.interfaces import IService
from plone.rest.events import mark_as_api_request
from zope.component import adapter
from zope.component import queryMultiAdapter
from zope.interface import implementer
from zope.publisher.interfaces.browser import IBrowserPublisher
from ZPublisher.BaseRequest import DefaultPublishTraverse
from zope.traversing.interfaces import ITraversable
from Products.CMFCore.interfaces import IContentish
from Products.CMFCore.interfaces import ISiteRoot


@adapter(ISiteRoot, IAPIRequest)
class RESTTraverse(DefaultPublishTraverse):
class RESTPublishTraverse(object):
def publishTraverse(self, request, name):
try:
obj = super(RESTTraverse, self).publishTraverse(request, name)
if not IContentish.providedBy(obj) and not IService.providedBy(obj):
if isinstance(obj, VirtualHostMonster):
return obj
else:
raise KeyError
except KeyError:
# No object, maybe a named rest service
service = queryMultiAdapter(
(self.context, request), name=request._rest_service_id + name
)
if service is None:
# No service, fallback to regular view
view = queryMultiAdapter((self.context, request), name=name)
if view is not None:
return view
raise
service = queryMultiAdapter(
(self.context, request), name=request._rest_service_id + name
)
if service is not None:
return service

if name.startswith(request._rest_service_id):
return obj
adapter = DefaultPublishTraverse(self.context, request)
obj = adapter.publishTraverse(request, name)

# Do not handle view namespace
if "@@" in request["PATH_INFO"] or "++view++" in request["PATH_INFO"]:
return obj
if IContentish.providedBy(obj) and not (
Copy link
Member

@petri petri Jun 7, 2020

Choose a reason for hiding this comment

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

Should we check for ITraversable rather than or in addition to IContentish? Doing that seems to still solve plone/plone.restapi#680 and plone/plone.app.theming#165 (even when converted to IService!) whilst still passing at least plone.rest tests.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I'd guess that IContentish is always ITraversable. IContentish excludes the SiteRoot, not sure if that is traversable. Things are complex and different people worked on different parts that depend on plone.rest, so I'd like them to weight in as well. cc @buchi @jaroel @sneridagh

Copy link
Member

Choose a reason for hiding this comment

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

I actively remove IContentish from Plone Site in https://github.com/plone/Products.CMFPlone/pull/2612/files#diff-860754a57fed2533e4aae6aab34a27caR238-R242 . I did not check how that will affect plone.rest stuff, but so far it seemed to work.
My gut reaction is that ITraversable is correct here.

Copy link
Member

Choose a reason for hiding this comment

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

We're adding IContentish back to PloneSite.
I don't think this changes my position on ITraversable, but it probably will affect other stuff.

Is this whole branch/approach still relevant actually?

"@@" in request["PATH_INFO"] or "++view++" in request["PATH_INFO"]
):
return RESTWrapper(obj, request)

# Wrap object to ensure we handle further traversal
return RESTWrapper(obj, request)
return obj

def browserDefault(self, request):
# Called when we have reached the end of the path
# In our case this means an unamed service
return self.context, (request._rest_service_id,)


@adapter(ISiteRoot, IAPIRequest)
class RESTTraverse(RESTPublishTraverse, DefaultPublishTraverse):
"""traversal object during REST requests."""


@implementer(ITraversable)
class MarkAsRESTTraverser(object):
"""
Expand All @@ -69,7 +57,7 @@ def traverse(self, name_ignored, subpath_ignored):


@implementer(IBrowserPublisher)
class RESTWrapper(object):
class RESTWrapper(RESTPublishTraverse):
"""A wrapper for objects traversed during a REST request."""

def __init__(self, context, request):
Expand All @@ -93,34 +81,3 @@ def __before_publishing_traverse__(self, arg1, arg2=None):
if not self._bpth_called:
self._bpth_called = True
bpth(arg1, arg2)

def publishTraverse(self, request, name):
# Try to get an object using default traversal
adapter = DefaultPublishTraverse(self.context, request)
try:
obj = adapter.publishTraverse(request, name)
if not IContentish.providedBy(obj) and not IService.providedBy(obj):
raise KeyError

# If there's no object with the given name, we get a KeyError.
# In a non-folderish context a key lookup results in an AttributeError.
except (KeyError, AttributeError):
# No object, maybe a named rest service
service = queryMultiAdapter(
(self.context, request), name=request._rest_service_id + name
)
if service is None:
# No service, fallback to regular view
view = queryMultiAdapter((self.context, request), name=name)
if view is not None:
return view
raise
return service
else:
# Wrap object to ensure we handle further traversal
return RESTWrapper(obj, request)

def browserDefault(self, request):
# Called when we have reached the end of the path
# In our case this means an unamed service
return self.context, (request._rest_service_id,)