Make OAuth2Mixin work even when OAUTH_AUTHORIZE_URL doesn't end with "?"

Based on https://github.com/facebook/tornado/pull/269

tornado/auth.py

@@ -58,6 +58,7 @@ import uuid
 
 from tornado import httpclient
 from tornado import escape
+from tornado.httputil import url_concat
 from tornado.ioloop import IOLoop
 from tornado.util import bytes_type
 
@@ -388,8 +389,8 @@ class OAuth2Mixin(object):
           "client_id": client_id
         }
         if extra_params: args.update(extra_params)
-        self.redirect(self._OAUTH_AUTHORIZE_URL +
-              urllib.urlencode(args))
+        self.redirect(
+                url_concat(self._OAUTH_AUTHORIZE_URL, args))
 
     def _oauth_request_token_url(self, redirect_uri= None, client_id = None,
                                  client_secret=None, code=None,
@@ -402,7 +403,7 @@ class OAuth2Mixin(object):
             client_secret=client_secret,
             )
         if extra_params: args.update(extra_params)
-        return url + urllib.urlencode(args)
+        return url_concat(url, args)
 
 class TwitterMixin(OAuthMixin):
     """Twitter OAuth authentication.

tornado/httputil.py

@@ -16,6 +16,7 @@
 
 """HTTP utility code shared by clients and servers."""
 
+import urllib
 import re
 
 class HTTPHeaders(dict):
@@ -148,6 +149,18 @@ class HTTPHeaders(dict):
             return normalized
 
 
+def url_concat(url, args):
+    """Concatenate url and argument dictionary regardless of whether
+    url has existing query parameters.
+
+    >>> url_concat("http://example.com/foo?a=b", dict(c="d"))
+    'http://example.com/foo?a=b&c=d'
+    """
+    if url[-1] not in ('?', '&'):
+        url += '&' if ('?' in url) else '?'
+    return url + urllib.urlencode(args)
+
+
 def doctests():
     import doctest
     return doctest.DocTestSuite()

tornado/test/httputil_test.py

@@ -0,0 +1,49 @@
+#!/usr/bin/env python
+
+from tornado.httputil import url_concat
+import unittest
+
+
+class TestUrlConcat(unittest.TestCase):
+
+    def test_url_concat_no_query_params(self):
+        url = url_concat(
+                "https://localhost/path",
+                {'y':'y', 'z':'z'},
+                )
+        self.assertEqual(url, "https://localhost/path?y=y&z=z")
+
+    def test_url_concat_encode_args(self):
+        url = url_concat(
+                "https://localhost/path",
+                {'y':'/y', 'z':'z'},
+                )
+        self.assertEqual(url, "https://localhost/path?y=%2Fy&z=z")
+
+    def test_url_concat_trailing_q(self):
+        url = url_concat(
+                "https://localhost/path?",
+                {'y':'y', 'z':'z'},
+                )
+        self.assertEqual(url, "https://localhost/path?y=y&z=z")
+
+    def test_url_concat_q_with_no_trailing_amp(self):
+        url = url_concat(
+                "https://localhost/path?x",
+                {'y':'y', 'z':'z'},
+                )
+        self.assertEqual(url, "https://localhost/path?x&y=y&z=z")
+
+    def test_url_concat_trailing_amp(self):
+        url = url_concat(
+                "https://localhost/path?x&",
+                {'y':'y', 'z':'z'},
+                )
+        self.assertEqual(url, "https://localhost/path?x&y=y&z=z")
+
+    def test_url_concat_mult_params(self):
+        url = url_concat(
+                "https://localhost/path?a=1&b=2",
+                {'y':'y', 'z':'z'},
+                )
+        self.assertEqual(url, "https://localhost/path?a=1&b=2&y=y&z=z")

tornado/test/runtests.py

@@ -7,6 +7,7 @@ TEST_MODULES = [
     'tornado.util.doctests',
     'tornado.test.escape_test',
     'tornado.test.httpserver_test',
+    'tornado.test.httputil_test',
     'tornado.test.ioloop_test',
     'tornado.test.iostream_test',
     'tornado.test.simple_httpclient_test',