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

[Ryan J Ollos]

After uploading an attachment named to ticket 1253, I see "Invalid attachment" displayed when trying to navigate to any realm. The following was captured when navigating to the /report page:

The attachment name is hardware mode error.png. The ticket summary is BLS Program Execution Hardware Fault. Disabling the BookmarkPlugin eliminates the error.

Here is the traceback from the log file.

2012-02-10 15:12:40,675 Trac[main] WARNING: HTTPNotFound: 404 Invalid Attachment (Attachment 'ticket:: 1253' does not exist.)
2012-02-10 15:12:41,984 Trac[main] ERROR: Exception caught while post-processing request:
Traceback (most recent call last):
  File "/usr/local/python26_trac12/lib/python2.6/site-packages/Trac-0.12.3-py2.6.egg/trac/web/main.py", line 276, in dispatch
    self._post_process_request(req)
  File "/usr/local/python26_trac12/lib/python2.6/site-packages/Trac-0.12.3-py2.6.egg/trac/web/main.py", line 365, in _post_process_request
    f.post_process_request(req, *(None,)*extra_arg_count)
  File "build/bdist.linux-x86_64/egg/tracbookmark/__init__.py", line 173, in post_process_request
    self.render_bookmarker(req)
  File "build/bdist.linux-x86_64/egg/tracbookmark/__init__.py", line 291, in render_bookmarker
    params = self.__format_name(req, url)
  File "build/bdist.linux-x86_64/egg/tracbookmark/__init__.py", line 238, in __format_name
    name = get_resource_description(self.env, resource, 'summary')
  File "/usr/local/python26_trac12/lib/python2.6/site-packages/Trac-0.12.3-py2.6.egg/trac/resource.py", line 333, in get_resource_description
    return manager.get_resource_description(resource, format, **kwargs)
  File "/usr/local/python26_trac12/lib/python2.6/site-packages/Trac-0.12.3-py2.6.egg/trac/attachment.py", line 610, in get_resource_description
    return Attachment(self.env, resource).description
  File "/usr/local/python26_trac12/lib/python2.6/site-packages/Trac-0.12.3-py2.6.egg/trac/attachment.py", line 126, in __init__
    self._fetch(self.resource.id, db)
  File "/usr/local/python26_trac12/lib/python2.6/site-packages/Trac-0.12.3-py2.6.egg/trac/attachment.py", line 155, in _fetch
    _('Invalid Attachment'))
ResourceNotFound: Attachment 'ticket:: 1253' does not exist.

[Ryan J Ollos]

Attempting to reproduce the issue in my development environment.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

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

@@ -28 +28 @@
 
     bookmark_path = re.compile(r'/bookmark')
     path_match = re.compile(r'/bookmark/(add|delete|delete_in_page)/(.*)')
+    blacklisted_path = re.compile(r'^/attachment/ticket/[0-9]+$')
 
     image_map = { 'on'  : 'bookmark_on.png',
                   'off' : 'bookmark_off.png', }
@@ -272 +273 @@
         resource = self._get_resource_uri(req)
         bookmark = self.get_bookmark(req, resource)
 
-        if bookmark:
-            img = tag.img(src=req.href.chrome('bookmark/' + self.image_map['on']))
-            anchor = tag.a(img, id='bookmark_this',
-                    href=req.href.bookmark('delete', resource), title='Delete Bookmark')
-        else:
-            img = tag.img(src=req.href.chrome('bookmark/' + self.image_map['off']))
-            anchor = tag.a(img, id='bookmark_this',
-                    href=req.href.bookmark('add', resource), title='Bookmark this page')
+        # Blacklisted paths can't be bookmarked
+        if not self.blacklisted_path.match(req.path_info):
+            if bookmark:
+                img = tag.img(src=req.href.chrome('bookmark/' + self.image_map['on']))
+                anchor = tag.a(img, id='bookmark_this',
+                               href=req.href.bookmark('delete', resource), title='Delete Bookmark')
+            else:
+                img = tag.img(src=req.href.chrome('bookmark/' + self.image_map['off']))
+                anchor = tag.a(img, id='bookmark_this',
+                               href=req.href.bookmark('add', resource), title='Bookmark this page')
+            elm = tag.span(anchor, id='bookmark', title='')
+            req.chrome.setdefault('ctxtnav', []).insert(0, elm)
 
         add_script(req, 'bookmark/js/tracbookmark.js')
         add_stylesheet(req, 'bookmark/css/tracbookmark.css')
-        elm = tag.span(anchor, id='bookmark', title='')
-        req.chrome.setdefault('ctxtnav', []).insert(0, elm)
 
         menu = tag.ul('', id='bookmark_menu', title='')

[Jun Omae]

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.

[Ryan J Ollos]

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.

[Jun Omae]

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('/')):

[Ryan J Ollos]

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.

[Ryan J Ollos]

Related to ​t:#10280.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

(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

[Ryan J Ollos]

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