"Invalid attachment' displayed on every page after bookmarking attachments page

Attempting to reproduce the issue in my development environment.

It looks like I had bookmarked /attachment/ticket/1253, which is the critical step that allows the error to be reproduced. I had intended to bookmark /ticket/1253, but this looks to be a real error, nonetheless.

I've dug a little deeper into this. The problem can be reproduced when bookmarking a page in the attachment realm that is not an actual attachment. Here are two examples:

• /attachment/ticket/1/?action=new&attachfilebutton=Attach+file
• /attachment/ticket/1/

The first example is the href for the page that allows you to upload an attachment. The second example is the href for the page that loads after an attachment is added - i.e. the page that lists all the attachments for a ticket.

This error is likely to occur infrequently because looking at this practically, there is little reason to bookmark these hrefs. However, I did this by accident and it blocked me from accessing any Trac resource, which is a pretty severe failure. We can either prevent these resources from having a bookmark icon, or add special cases to handle these.

Here is one problematic case that I propose to deal with by just blacklisting the path. /attachment/ticket/n (note there is no trailing forward slash) for any ticket n, results in:

This will be a difficult resource to bookmark since it apparently doesn't have an ID when the trailing backslash is not present. It also seems pretty useless to bookmark, so blacklisting it seems like a fine solution.

(In [11672]) Refs #9785: Refactored code to use the helper functions get_resource_shortname and get_resource_summary.

Here is the patch I'm proposing. It removes the bookmark star from the ^/attachment/ticket/n$ page, but leaves the bookmark list context menu. This way, the offending page can't be bookmarked.

Before committing a change, I'd expand the blacklisted_paths variable to support a list of regexes. I'm just posting this for feedback at the moment.

• trunk/tracbookmark/__init__.py

  old	new
  28	28
  29	29	bookmark_path = re.compile(r'/bookmark')
  30	30	path_match = re.compile(r'/bookmark/(add|delete|delete_in_page)/(.*)')
  	31	blacklisted_path = re.compile(r'^/attachment/ticket/[0-9]+$')
  31	32
  32	33	image_map = { 'on'  : 'bookmark_on.png',
  33	34	'off' : 'bookmark_off.png', }
  …	…
  272	273	resource = self._get_resource_uri(req)
  273	274	bookmark = self.get_bookmark(req, resource)
  274	275
  275		if bookmark:
  276		img = tag.img(src=req.href.chrome('bookmark/' + self.image_map['on']))
  277		anchor = tag.a(img, id='bookmark_this',
  278		href=req.href.bookmark('delete', resource), title='Delete Bookmark')
  279		else:
  280		img = tag.img(src=req.href.chrome('bookmark/' + self.image_map['off']))
  281		anchor = tag.a(img, id='bookmark_this',
  282		href=req.href.bookmark('add', resource), title='Bookmark this page')
  	276	# Blacklisted paths can't be bookmarked
  	277	if not self.blacklisted_path.match(req.path_info):
  	278	if bookmark:
  	279	img = tag.img(src=req.href.chrome('bookmark/' + self.image_map['on']))
  	280	anchor = tag.a(img, id='bookmark_this',
  	281	href=req.href.bookmark('delete', resource), title='Delete Bookmark')
  	282	else:
  	283	img = tag.img(src=req.href.chrome('bookmark/' + self.image_map['off']))
  	284	anchor = tag.a(img, id='bookmark_this',
  	285	href=req.href.bookmark('add', resource), title='Bookmark this page')
  	286	elm = tag.span(anchor, id='bookmark', title='')
  	287	req.chrome.setdefault('ctxtnav', []).insert(0, elm)
  283	288
  284	289	add_script(req, 'bookmark/js/tracbookmark.js')
  285	290	add_stylesheet(req, 'bookmark/css/tracbookmark.css')
  286		elm = tag.span(anchor, id='bookmark', title='')
  287		req.chrome.setdefault('ctxtnav', []).insert(0, elm)
  288	291
  289	292	menu = tag.ul('', id='bookmark_menu', title='')

Replying to rjollos:

Here is one problematic case that I propose to deal with by just blacklisting the path. /attachment/ticket/n (note there is no trailing forward slash) for any ticket n, results in:

Yes. /attachment/wiki/WikiStart has the same problem. So the URL of attachments list page must have a trailing slash.

This will be a difficult resource to bookmark since it apparently doesn't have an ID when the trailing backslash is not present. It also seems pretty useless to bookmark, so blacklisting it seems like a fine solution.

+1. Good solution. Your patch in comment:7 basically looks good.

Replying to jun66j5:

Yes. /attachment/wiki/WikiStart has the same problem. So the URL of attachments list page must have a trailing slash.

Oh, thanks, I hadn't run across that one before. So do you think I should just blacklist ^/attachment/[^/]+/[^/]+$ ? That seems to work well for /attachment/wiki/WikiStart and /attachment/ticket/1 at least.

Replying to rjollos:

Oh, thanks, I hadn't run across that one before. So do you think I should just blacklist ^/attachment/[^/]+/[^/]+$ ?

The regular expression doesn't match /attachment/wiki/WikiName/SubWiki.

I think it would be simple as the following.

        # Ignore the page which begins with /attachment/ and doesn't have a trailing slash
        if not (req.path_info.startswith('/attachment/') and not req.path_info.endswith('/')):

Replying to jun66j5:

The regular expression doesn't match /attachment/wiki/WikiName/SubWiki.

Thanks for catching that. I'll implement the change that you propose.

Related to t:#10280.

This is a bit more complex than I thought initially. First, we need to deal with ambiguity of whether the page that is bookmarked has a trailing slash or not. That is dealt with by:

• Right-strip slashes in _get_resource_uri, so that /attachment/parent_realm/parent_id/attachment_id and /attachment/parent_realm/parent_id/attachment_id/ are both treated as the same bookmark.
• Append a trailing slash to the href when the page is /attachment/parent_realm/parent_id.

So now, it doesn't matter whether /attachment/parent_realm/parent_id/attachment_id or /attachment/parent_realm/parent_id/attachment_id/ is bookmarked, the bookmark will point to /attachment/parent_realm/parent_id/.

Second, when getting the resource name in _format_name, we need to handle the case that that path could be to either an attachment list page or an attachment resource. To handle this, we assume first that the path points to an attachment resource. If that resource is not found, assume it points to an attachment list page.

parent = Resource(path[2], '/'.join(path[3:-1]))
resource = Resource(realm, path[-1], False, parent)
if resource_exists(self.env, resource):
    linkname = get_resource_name(self.env, resource)
else:
    parent = Resource(path[2], '/'.join(path[3:]))
    resource = Resource(realm, parent=parent)
    linkname = get_resource_name(self.env, resource)

I'll check-in this fix after I do a bit more testing.

Replying to rjollos:

... To handle this, we assume first that the path points to an attachment resource. If that resource is not found, assume it points to an attachment list page.

This didn't quite cover all of the cases since we want to add the missing class to attachments that don't exist. Here is what I finally came up with:

# Assume it's a file and check existence
parent = Resource(path[2], '/'.join(path[3:-1]))
resource = Resource(realm, path[-1], False, parent)
linkname = get_resource_name(self.env, resource)
try:
    Attachment(self.env, resource)
except ResourceNotFound:
    # Assume it's an attachment list page
    # and check existence
    parent = Resource(path[2], '/'.join(path[3:]))
    resource = Resource(realm, parent=parent)
    if resource_exists(self.env, resource):
        # Appending slash required for Trac < 1.0, t:#10280
        if not query_string:
            href += '/'
        linkname = get_resource_name(self.env, resource)
    else:
        # Assume it's a missing attachment
        class_ = 'missing ' + realm

The need for such complicated code seems to be a product of how we store the resource information in the database, and we may want to consider changing the schema, as hasienda has already suggested for the similar VotePlugin in #10942.

I've iterated further, and arrived at:

# Assume a file and check existence
parent = Resource(path[1], '/'.join(path[2:-1]))
resource = Resource(realm, path[-1], parent=parent)
linkname = get_resource_name(self.env, resource)
if not resource_exists(self.env, resource):
    # Assume an attachment list page and check existence
    parent = Resource(path[1], '/'.join(path[2:]))
    if resource_exists(self.env, parent):
        resource = Resource(realm, parent=parent)
        linkname = get_resource_name(self.env, resource)
        if 'action=new' in query_string:
            linkname = _("Add Attachment to %(id)s"
                         % {'id': parent.id})
        elif not query_string:
            # Trailing slash needed for Trac < 1.0, t:#10280
            href += '/'
    else:
        # Assume it's a missing attachment
        class_ = 'missing ' + realm

The code in comment:16 didn't quite work because resource_exists checks whether the attachment path exists in the file system, so if a page such as /attachment/wiki/TracEnvironment is bookmarked, but attachments have never been added to the resource, then resource_exists for the code in comment:16 would return false.

The logic of the above code is:

1. Assume the path points to an attachment and check for its existence.
2. If resource_exists returns false for (2), assume the path points to the attachment page for a resource and check for its existence.
3. If resource_exists returns false for (2), assume the attachment is missing.

#11043 is a follow-on ticket in which edit and delete pages will be blacklisted from having a bookmark icon, at least by default.

(In [13029]) Fixes #9785:

• Allow attachments, attachment list pages and attachment add pages to be bookmarked. Each has a unique description.
• Prefix the version to wiki page description with an @ followed by the version number, if the version is contained in the URL
• Prefix milestones with "Milestone " so that they can be readily differentiated from wiki pages.

Tested with Trac 0.11.7 and 1.0.2dev

Replying to rjollos:

Tested with Trac 0.11.7 and 1.0.2dev

This was not quite true. I tested with Trac 0.11-stable (i.e. 0.11.8dev), and while reviewing a patch from hasienda recently I noticed his comment that resource_exists isn't in Trac <= 0.11.7. I'll have to backport the function to make BookmarkPlugin compatible back to Trac 0.11.0.

• resource.py in 0.11-stable
• resource.py in 0.11.7