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

1letter/fix#671 #675

Merged
merged 11 commits into from
Oct 23, 2023
Merged

1letter/fix#671 #675

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a251928
Fix link_redirect_view
1letter Oct 19, 2023
08635f7
Add News
1letter Oct 19, 2023
f64ecd9
Fix Test
1letter Oct 19, 2023
4fae87e
Fix path construction
1letter Oct 19, 2023
9a2e653
Simplify url constrction in link_redirect_view
1letter Oct 19, 2023
7693b1a
remove unused imports
1letter Oct 19, 2023
6f805c8
Make resolution of uid more robust
1letter Oct 20, 2023
e4c5d65
Update Tests
1letter Oct 20, 2023
68f1f42
Calculate a fragment of path
1letter Oct 20, 2023
798963f
Update tests
1letter Oct 20, 2023
614d5a4
Update 671.bugfix
1letter Oct 20, 2023
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
View file
Open in desktop
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
View file
Open in desktop
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 •
edited
Loading

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.

Also /resolveuid/fb91bddde7eb46efbed20e9c10fb4929#autotoc-item-autotoc-1 should still work.

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 •
edited
Loading

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

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.

@yurj i use a copy of the list with lowercase values https://github.com/plone/plone.app.contenttypes/pull/675/files#diff-c9937c907b85ad438b19cb4ce7abe10606d94a443dc66bef1661a216690f3547R44

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
View file
Open in desktop
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")