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

1letter/fix#671 #675

Merged
merged 11 commits into from
Oct 23, 2023
Merged
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/671.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix link_redirect_view, respect vhm vs none-vhm url schemes @1letter
52 changes: 47 additions & 5 deletions plone/app/contenttypes/browser/link_redirect_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,44 @@
]


def normalize_uid_from_path(url=None):
"""
Args:
url (string): a path or orl

Returns:
tuple: tuple of (uid, fragment) a fragment is an anchor id e.g. #head1
"""
uid = None
fragment = None

if not url:
return uid, fragment

# resolve uid
paths = url.split("/")
paths_lower = [_item.lower() for _item in paths]

if "resolveuid" in paths_lower:
ri = paths_lower.index("resolveuid")
if ri + 1 != len(paths):
uid = paths[ri + 1]
if uid == "":
uid = None

if not uid:
return uid, fragment

# resolve fragment
parts = urlparse(uid)

uid = parts.path

fragment = f"#{parts.fragment}" if parts.fragment else None

return uid, fragment


class LinkRedirectView(BrowserView):
index = ViewPageTemplateFile("templates/link.pt")

Expand Down Expand Up @@ -109,12 +147,16 @@ def absolute_target_url(self):
url = "/".join([context_state.canonical_object_url(), url])
else:
if "resolveuid" in url:
uid = url.split("/")[-1]
uid, fragment = normalize_uid_from_path(url)
obj = uuidToObject(uid)
if obj:
url = "/".join(obj.getPhysicalPath()[2:])
if not url.startswith("/"):
url = "/" + url
if obj is None:
# uid can't resolve, return the url
return url

url = obj.absolute_url()
Copy link
Member

Choose a reason for hiding this comment

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

I'd check for obj first (keep the if from line 114) ... it might be that the uuid is missing.

Copy link
Contributor

@yurj yurj Oct 19, 2023

Choose a reason for hiding this comment

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

also uid = url.split("/")[-1] above is fragile ("/resolveuid/" vs "resolveuid/" vs "/a/relativepath/resolveuid"). Generally, you should find the index of 'resolveuid' in the path, and get the next one if it exists. It should be something like this (uid = getuid(path)):

def get_uid(path):
    paths = path.split('/')
    uid = None
    if 'resolveuid' in paths:
        ri = paths.index('resolveuid')
        if ri + 1 != len(paths):
            uid = paths[ri + 1]
            if uid == '':
               uid = None
    return uid 
>>> get_uid('gfdgdf')
>>> get_uid('')
>>> get_uid('/resolveuid/')
>>> get_uid('/resolveuidgfdgd/')
>>> get_uid('/resolveuid/4342432')
'4342432'
>>> get_uid('/resolveuid/4342432/')
'4342432'
>>> get_uid('fdsfd/fdf/resolveuid/4342432/')
'4342432'
>>> get_uid('/fdsfd/fdf/resolveuid/4342432/')
'4342432'
>>> 

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

is resolve[uU]id still a thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@petschki maybe, but i'm not sure

Copy link
Member

@petschki petschki Oct 20, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

def get_uid(path):
    paths = path.split('/')
    uid = None
    if 'resolveuid' in paths:
        ri = paths.index('resolveuid')
        if ri + 1 != len(paths):
            uid = paths[ri + 1]
            if '#' in uid:
                uid = uid.split('#')[0]
            if uid == '':
               uid = None
    return uid 
>>> get_uid('gfdgdf')
>>> get_uid('')
>>> get_uid('/resolveuid/')
>>> get_uid('/resolveuidgfdgd/')
>>> get_uid('/resolveuid/4342432')
'4342432'
>>> get_uid('/resolveuid/4342432/')
'4342432'
>>> get_uid('fdsfd/fdf/resolveuid/4342432/')
'4342432'
>>> get_uid('/fdsfd/fdf/resolveuid/4342432/')
'4342432'
>>> get_uid('/resolveuid/fb91bddde7eb46efbed20e9c10fb4929#autotoc-item-autotoc-1')
'fb91bddde7eb46efbed20e9c10fb4929'
>>> 

Copy link
Contributor

Choose a reason for hiding this comment

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

@1letter solution to use urlparse is very good! Question: uid's are all lowercase? What if an UID has uppercase letters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

At least plone.outputfilters still takes it into account: https://github.com/plone/plone.outputfilters/blob/master/plone/outputfilters/filters/resolveuid_and_caption.py#L31

https://github.com/plone/plone.outputfilters/blob/278e2752c4790b5af2470bfc3c91a8aa26a9a80f/plone/outputfilters/filters/resolveuid_and_caption.py#L130

here is what plone.output filter does. It uses the re above to get all parts of the url. If there's an uid, find the object, translate it to the absolute_url, add the other parts.

If we want to do this the same way outputfilter does, we should use this code. This code also check IResolveUidsEnabler. Maybe this last part is not important (if a site disable uids and use resolveuid as a folder name...).

Generally, resoulveuid and catalog path resolution should be done in one place, I think in plone.api. I don't know about circular dipendencies...

Anyway, the actual code is ok, should works as expected and it is a real improvement.

if fragment is not None:
url = f"{url}{fragment}"

if not url.startswith(("http://", "https://")):
url = self.request["SERVER_URL"] + url

Expand Down
131 changes: 130 additions & 1 deletion plone/app/contenttypes/tests/test_link.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from datetime import datetime
from plone.app.contenttypes.browser.link_redirect_view import normalize_uid_from_path
from plone.app.contenttypes.interfaces import ILink
from plone.app.contenttypes.testing import ( # noqa
PLONE_APP_CONTENTTYPES_FUNCTIONAL_TESTING,
Expand Down Expand Up @@ -230,6 +231,117 @@ def test_ftp_type(self):
self.assertTrue(view())
self._assert_redirect(self.link.remoteUrl)

def test_normalize_uid_from_path(self):
url = None
uid, fragment = normalize_uid_from_path(url)
self.assertIsNone(uid)
self.assertIsNone(fragment)

url = "http://nohost"
uid, fragment = normalize_uid_from_path(url)
self.assertIsNone(uid)
self.assertIsNone(fragment)

url = "http://nohost/test1"
uid, fragment = normalize_uid_from_path(url)
self.assertIsNone(uid)
self.assertIsNone(fragment)

url = "http://nohost/test1/resolveuid"
uid, fragment = normalize_uid_from_path(url)
self.assertIsNone(uid)
self.assertIsNone(fragment)

url = "http://nohost/test1/resolveuid/"
uid, fragment = normalize_uid_from_path(url)
self.assertIsNone(uid)
self.assertIsNone(fragment)

url = "http://nohost/test1/resolveuid123/"
uid, fragment = normalize_uid_from_path(url)
self.assertIsNone(uid)
self.assertIsNone(fragment)

url = "http://nohost/test1/resolveuid/123"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertIsNone(fragment)

url = "http://nohost/test1/resolveuid/123#my-id"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertEqual(fragment, "#my-id")

url = "http://nohost/test1/resolveuid/123/"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertIsNone(fragment)

url = "http://nohost/test1/resolveuid/123#my-id/"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertEqual(fragment, "#my-id")

url = "http://nohost/test1/resolveuid/123/test"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertIsNone(fragment)

url = "resolveuid/123"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertIsNone(fragment)

url = "resolveuid/123#my-id"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertEqual(fragment, "#my-id")

url = "resolveuid/123/"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertIsNone(fragment)

url = "resolveuid/123#my-id/"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertEqual(fragment, "#my-id")

url = "resolveuid/123/abc"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertIsNone(fragment)

url = "/resolveuid/123"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertIsNone(fragment)

url = "/resolveuid/123#my-id"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertEqual(fragment, "#my-id")

url = "/resolveuid/123/"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertIsNone(fragment)

url = "/resolveuid/123#my-id/"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertEqual(fragment, "#my-id")

url = "/resolveuid/123/abc"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertIsNone(fragment)

url = "/resolveUid/123#my-id"
uid, fragment = normalize_uid_from_path(url)
self.assertEqual(uid, "123")
self.assertEqual(fragment, "#my-id")

def _publish(self, obj):
portal_workflow = getToolByName(self.portal, "portal_workflow")
portal_workflow.doActionFor(obj, "publish")
Expand Down Expand Up @@ -374,5 +486,22 @@ def test_resolve_uid_to_absolute_target(self):
)
doc1 = self.portal["doc1"]
uid = IUUID(doc1)

portal_state = getMultiAdapter(
(self.link, self.request), name="plone_portal_state"
)
portal_url = portal_state.portal_url()

# check an internal link
self.link.remoteUrl = f"${{portal_url}}/resolveuid/{uid}"
self.assertEqual(view.absolute_target_url(), "http://nohost/doc1")
self.assertEqual(view.absolute_target_url(), f"{portal_url}/doc1")

# check an internal link with fragment
self.link.remoteUrl = f"${{portal_url}}/resolveuid/{uid}#autotoc-item-autotoc-1"
self.assertEqual(
view.absolute_target_url(), f"{portal_url}/doc1#autotoc-item-autotoc-1"
)

# check not resolvable uid
self.link.remoteUrl = "/resolveuid/abc123"
self.assertEqual(view.absolute_target_url(), "/resolveuid/abc123")