Merge lp:~jcsackett/launchpad/linkifier-bugs-2 into lp:launchpad

[j.c.sackett]

[r=bac][ui=none][bug=60172,118284] Fixes linkifier so URIs of just a protocol are not linked, and parenthesis are handled better.

Summary
=======

As part of bugjam2010, a series of bugs having to do with the linkifier have been targeted. This branch deals with standard URI links that need some postprocessing after the regex capture.

URIs are identified in stringformatter via a serious regex, but to do certain things (like reject urls that consist of just a protocol with no domain) you would have to modify the regex to catch invalid urls, which would make it even harder to deal with. Instead it's easier and cleaner to just make decisions about when to linkify based on the output of the regex.

Implementation
==============

Two methods have been added to the FormattersAPI in lib/lp/app/browser/stringformatter.py

The first, _linkify_url_is_blacklisted, adds capacity to reject URIs for linkification if they are in a predefined list. The list is all of the protocols defined in the regex as standalone URIs (e.g. http://, sftp://). This is used in the linkify process to return the URL as text rather than as a link if it's only the protocol.

The second _handle_parens_in_trailers deals with closing parenthesis following the url that should be considered part of the url--e.g. the closing parenthesis is http://wikipedia.com/Fell_(comic). This is used in the linkification process to move the closing parenthesis out of the trailers and back into the URI.

Tests
=====

bin/test -t TestLinkifyingProtocols

QA
==

Create a bug comment on launchpad.dev with the following text:

"http:// isn't a link. http://example.com/something_(with_parens) should be fully linked."

The first http:// shouldn't link. The second URI should link, including the closing paren at the end.

Lint
====

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/browser/stringformatter.py
  lib/lp/app/browser/tests/test_stringformatter.py

./lib/lp/app/browser/stringformatter.py
     450: E501 line too long (90 characters)
     454: E501 line too long (88 characters)
     450: Line exceeds 78 characters.
     454: Line exceeds 78 characters.

The two lines that are too long are part of the regex, which is difficult enough to read without breaking up those sections.

[Brad Crittenden (bac)]

Hi Jon,

* _handle_parens_in_trailers needs a doc string with explanation of parameters. Just looking at it cold I cannot figure out the purpose. A trailer is something you haul cows to the sale barn in. You also agreed to add some more corner case tests -- thanks!

* In your blacklist method please use triple double-quotes for the docstring open/close and do put in closing punctuation.

* On IRC we discussed a name better than 'blacklist'. How about 'ignored' or something even better if you can think of it?

* Please create a dict and use named targets in:

83 + return '<a rel="nofollow" href="%s">%s</a>%s' % (
84 + cgi.escape(url, quote=True),
85 + add_word_breaks(cgi.escape(url)),
86 + cgi.escape(trailers))

* FormattersAPI._split_url_and_trailers is only used once but its definition is way down below it -- I think some code was re-arranged. Please put it closer to the call site. Also document in English what is being matched for the RE impaired to easily understand it.

Other than those issues the branch looks good and thorough. Thanks. Marking it approved, pending changes.

review: Approve (code)

[j.c.sackett (jcsackett)]

> Hi Jon,
>
> * _handle_parens_in_trailers needs a doc string with explanation of
> parameters. Just looking at it cold I cannot figure out the purpose. A
> trailer is something you haul cows to the sale barn in. You also agreed to
> add some more corner case tests -- thanks!

Done.

> * In your blacklist method please use triple double-quotes for the docstring
> open/close and do put in closing punctuation.

Done.

> * On IRC we discussed a name better than 'blacklist'. How about 'ignored' or
> something even better if you can think of it?

Couldn't think of better than ignored. Done.

> * Please create a dict and use named targets in:
>
> 83 + return '<a rel="nofollow" href="%s">%s</a>%s' % (
> 84 + cgi.escape(url, quote=True),
> 85 + add_word_breaks(cgi.escape(url)),
> 86 + cgi.escape(trailers))

Done.

> * FormattersAPI._split_url_and_trailers is only used once but its definition
> is way down below it -- I think some code was re-arranged. Please put it
> closer to the call site.

It's actually called in several places; I've moved the method so it's grouped with _handle_parens and the former blacklist method, that places it closer to the methods it's called in.

> Also document in English what is being matched for
> the RE impaired to easily understand it.

I'm still not sure which RE section you're asking about? Can you include the block or file number so I can find it?

>
> Other than those issues the branch looks good and thorough. Thanks. Marking
> it approved, pending changes.

[Brad Crittenden (bac)]

Thanks for the fixes. I was referring to _re_url_trailers.

[j.c.sackett (jcsackett)]

> I was referring to _re_url_trailers.

Ah, okay. I was trying to find an regex I had made or altered, therein was my confusion. I've updated comments for it. Thanks for the review.

[Robert Collins (lifeless)]

blacklisting protocols seems bizarre.

Why not just create a URL object and check that more than the scheme
is filled out?

[j.c.sackett (jcsackett)]

That actually seems bizarre to me; why build a new object when we're working with strings?

This also allows us to expand to other things we might wish to ignore, should another case crop up.

Blacklisting as a notion has been removed, if the loaded definition of that bothers you--we're just treating them as things the linkifier should ignore.

=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py	2010-12-14 17:33:05 +0000
+++ lib/lp/app/browser/stringformatter.py	2010-12-17 18:13:52 +0000
@@ -201,6 +201,34 @@
         return ' ' * len(groups[0])
 
     @staticmethod
+    def _linkify_bug_number(text, bugnum, trailers=''):
+        # Don't look up the bug or display anything about the bug, just
+        # linkify to the general bug url.
+        url = '/bugs/%s' % bugnum
+        # The text will have already been cgi escaped.
+        return '<a href="%s">%s</a>%s' % (url, text, trailers)
+
+    @staticmethod
+    def _handle_parens_in_trailers(url, trailers):
+        """Move closing parens from the trailer back into the url if needed.
+
+        If there are opening parens in the url that are matched by closing
+        parens at the start of the trailer, those closing parens should be
+        part of the url."""
+        opencount = url.count('(')
+        closedcount = url.count(')')
+        missing = opencount - closedcount
+        slice_idx = 0
+        while slice_idx < missing:
+            if trailers[slice_idx] == ')':
+                slice_idx += 1
+            else:
+                break
+        url += trailers[:slice_idx]
+        trailers = trailers[slice_idx:]
+        return url, trailers
+
+    @staticmethod
     def _split_url_and_trailers(url):
         """Given a URL return a tuple of the URL and punctuation trailers.
 
@@ -216,15 +244,31 @@
             url = url[:-len(trailers)]
         else:
             trailers = ''
-        return url, trailers
+        return FormattersAPI._handle_parens_in_trailers(url, trailers)
 
     @staticmethod
-    def _linkify_bug_number(text, bugnum, trailers=''):
-        # Don't look up the bug or display anything about the bug, just
-        # linkify to the general bug url.
-        url = '/bugs/%s' % bugnum
-        # The text will have already been cgi escaped.
-        return '<a href="%s">%s</a>%s' % (url, text, trailers)
+    def _linkify_url_should_be_ignored(url):
+        """Don't linkify URIs consisting of just the protocol."""
+
+        protocol_bases = [
+            'about',
+            'gopher',
+            'http',
+            'https',
+            'sftp',
+            'news',
+            'ftp',
+            'mailto',
+            'irc',
+            'jabber',
+            'apt',
+            'data',
+            ]
+
+        for base in protocol_bases:
+            if url in ('%s' % base, '%s:' % base, '%s://' % base):
+                return True
+        return False
 
     @staticmethod
     def _linkify_substitution(match):
@@ -235,16 +279,22 @@
             # The text will already have been cgi escaped.  We temporarily
             # unescape it so that we can strip common trailing characters
             # that aren't part of the URL.
-            url = match.group('url')
-            url, trailers = FormattersAPI._split_url_and_trailers(url)
+            full_url = match.group('url')
+            url, trailers = FormattersAPI._split_url_and_trailers(full_url)
             # We use nofollow for these links to reduce the value of
             # adding spam URLs to our comments; it's a way of moderately
             # devaluing the return on effort for spammers that consider
             # using Launchpad.
-            return '<a rel="nofollow" href="%s">%s</a>%s' % (
-                cgi.escape(url, quote=True),
-                add_word_breaks(cgi.escape(url)),
-                cgi.escape(trailers))
+            if not FormattersAPI._linkify_url_should_be_ignored(url):
+                link_string = ('<a rel="nofollow" '
+                               'href="%(url)s">%(linked_text)s</a>%(trailers)s' % {
+                                    'url': cgi.escape(url, quote=True),
+                                    'linked_text': add_word_breaks(cgi.escape(url)),
+                                    'trailers': cgi.escape(trailers)
+                                    })
+                return link_string
+            else:
+                return full_url
         elif match.group('faq') is not None:
             # This is *BAD*.  We shouldn't be doing database lookups to
             # linkify text.
@@ -423,8 +473,10 @@
     ''' % {'unreserved': "-a-zA-Z0-9._~%!$&'()*+,;="},
                              re.IGNORECASE | re.VERBOSE)
 
-    # a pattern to match common trailing punctuation for URLs that we
-    # don't want to include in the link.
+    # There is various punctuation that can occur at the end of a link that
+    # shouldn't be included. The regex below matches on the set of characters
+    # we don't generally want. See also _handle_parens_in_trailers, which
+    # re-attaches parens if we do want them to be part of the url.
     _re_url_trailers = re.compile(r'([,.?:);>]+)$')
 
     def text_to_html(self):

=== modified file 'lib/lp/app/browser/tests/test_stringformatter.py'
--- lib/lp/app/browser/tests/test_stringformatter.py	2010-12-14 19:56:40 +0000
+++ lib/lp/app/browser/tests/test_stringformatter.py	2010-12-17 18:13:52 +0000
@@ -107,8 +107,72 @@
       <tag>1234567890123456</tag>
     """
 
+
 class TestLinkifyingProtocols(TestCase):
 
+    def test_normal_set(self):
+        test_strings = [
+            "http://example.com",
+            "http://example.com/",
+            "http://example.com/path",
+            "http://example.com/path/",
+            ]
+
+        expected_strings = [
+            ('<p><a rel="nofollow" href="http://example.com">'
+             'http://<wbr></wbr>example.<wbr></wbr>com</a></p>'),
+            ('<p><a rel="nofollow" href="http://example.com/">'
+             'http://<wbr></wbr>example.<wbr></wbr>com/</a></p>'),
+            ('<p><a rel="nofollow" href="http://example.com/path">'
+             'http://<wbr></wbr>example.<wbr></wbr>com/path</a></p>'),
+            ('<p><a rel="nofollow" href="http://example.com/path/">'
+             'http://<wbr></wbr>example.<wbr></wbr>com/path/</a></p>'),
+            ]
+
+        self.assertEqual(
+            expected_strings,
+            [FormattersAPI(text).text_to_html() for text in test_strings])
+
+    def test_parens_handled_well(self):
+        test_strings = [
+            '(http://example.com)',
+            'http://example.com/path_(with_parens)',
+            '(http://example.com/path_(with_parens))',
+            '(http://example.com/path_(with_parens)and_stuff)',
+            ]
+
+        expected_html = [
+            ('<p>(<a rel="nofollow" href="http://example.com">'
+             'http://<wbr></wbr>example.<wbr></wbr>com</a>)</p>'),
+            ('<p><a rel="nofollow" '
+             'href="http://example.com/path_(with_parens)">'
+             'http://<wbr></wbr>example.<wbr></wbr>com/path_'
+             '<wbr></wbr>(with_parens)</a></p>'),
+            ('<p>(<a rel="nofollow" '
+             'href="http://example.com/path_(with_parens)">'
+             'http://<wbr></wbr>example.<wbr></wbr>com/path_'
+             '<wbr></wbr>(with_parens)</a>)</p>'),
+            ('<p>(<a rel="nofollow" '
+             'href="http://example.com/path_(with_parens)and_stuff">'
+             'http://<wbr></wbr>example.<wbr></wbr>com'
+             '/path_<wbr></wbr>(with_parens)<wbr></wbr>and_stuff</a>)</p>'),
+            ]
+
+        self.assertEqual(
+            expected_html,
+            [FormattersAPI(text).text_to_html() for text in test_strings])
+
+    def test_protocol_alone_does_not_link(self):
+        test_string = "This doesn't link: apt:"
+        html = FormattersAPI(test_string).text_to_html()
+        expected_html = "<p>This doesn't link: apt:</p>"
+        self.assertEqual(expected_html, html)
+
+        test_string = "This doesn't link: http://"
+        html = FormattersAPI(test_string).text_to_html()
+        expected_html = "<p>This doesn't link: http://</p>"
+        self.assertEqual(expected_html, html)
+
     def test_apt_is_linked(self):
         test_string = 'This becomes a link: apt:some-package'
         html = FormattersAPI(test_string).text_to_html()
@@ -124,7 +188,7 @@
         expected_html = (
             '<p>This becomes a link: '
             '<a rel="nofollow" '
-                'href="apt://some-package">apt://some-<wbr></wbr>package</a></p>')
+            'href="apt://some-package">apt://some-<wbr></wbr>package</a></p>')
         self.assertEqual(expected_html, html)
 
     def test_file_is_not_linked(self):
@@ -141,9 +205,10 @@
         expected_html = (
             "<p>This becomes a link: "
             '<a rel="nofollow" '
-            'href="data:text/plain,test">data:text/<wbr></wbr>plain,test</a></p>')
+            'href="data:text/plain,test">'
+            'data:text/<wbr></wbr>plain,test</a></p>')
         self.assertEqual(expected_html, html)
-            
+
 
 class TestDiffFormatter(TestCase):
     """Test the string formatter fmt:diff."""