Merge lp:~leonardr/launchpadlib/avoid-beta-beta into lp:launchpadlib

[Leonard Richardson]

Old versions of launchpadlib took a service root URI argument, and the service URI included "/beta/" at the end. Then we introduced multi-version code, and there was a separate argument for the version, which gets stuck on to the end of the service URI.

Third-party code that used the constants in launchpadlib.uris are safe. But some code didn't use those constants: it hard-coded the URI service root in a string. That code is now going to be making HTTP requests to http://api.launchpad.net/beta/beta/, and getting an unhelpful 404 error.

This branch adds a check to the Launchpad constructor that will catch code like this and complain with a helpful exception. I originally thought I could silently fix this, but the fix would only work so long as the default web service version was 'beta', which won't be much longer. Better to get the problem fixed now.

I'm uncertain about the best way to write a unit test for the message contained in an exception. I think the way I did it is a little hacky. Let me know if there's a better way.

[Paul Hummer (rockstar)]

Ah! Looks like I've got some code to change... :)

review: Approve (code)

=== modified file 'src/launchpadlib/NEWS.txt'
--- src/launchpadlib/NEWS.txt	2010-03-04 15:39:21 +0000
+++ src/launchpadlib/NEWS.txt	2010-03-04 19:37:14 +0000
@@ -8,6 +8,11 @@
 - Fixed a minor bug when using login_with() to access a version of the
   Launchpad web service other than the default.
 
+- Added a check to catch old client code that would cause newer
+  versions of launchpadlib to make nonsensical requests to
+  https://api.launchpad.dev/beta/beta/, and raise a helpful exception
+  telling the developer how to fix it.
+
 1.5.5
 =====
 

=== modified file 'src/launchpadlib/launchpad.py'
--- src/launchpadlib/launchpad.py	2010-03-04 15:08:07 +0000
+++ src/launchpadlib/launchpad.py	2010-03-04 19:37:14 +0000
@@ -96,6 +96,15 @@
         :type service_root: string
         """
         service_root = uris.lookup_service_root(service_root)
+        if (service_root.endswith(version)
+            or service_root.endswith(version + '/')):
+            error = ("It looks like you're using a service root that "
+                     "incorporates the name of the web service version "
+                     '("%s"). Please use one of the constants from '
+                     "launchpadlib.uris instead, or at least remove "
+                     "the version name from the root URI." % version)
+            raise ValueError(error)
+
         super(Launchpad, self).__init__(
             credentials, service_root, cache, timeout, proxy_info, version)
 

=== modified file 'src/launchpadlib/tests/test_launchpad.py'
--- src/launchpadlib/tests/test_launchpad.py	2010-03-04 15:39:21 +0000
+++ src/launchpadlib/tests/test_launchpad.py	2010-03-04 19:37:14 +0000
@@ -92,6 +92,50 @@
         self.assertRaises(ValueError, uris.lookup_service_root, not_a_url)
         self.assertRaises(ValueError, uris.lookup_web_root, not_a_url)
 
+
+class TestServiceNameWithEmbeddedVersion(unittest.TestCase):
+    """Reject service roots that include the version at the end of the URL.
+
+    If the service root is "http://api.launchpad.net/beta/" and the
+    version is "beta", the launchpadlib constructor will raise an
+    exception.
+
+    This happens with scripts that were written against old versions
+    of launchpadlib. The alternative is to try to silently fix it (the
+    fix will eventually break as new versions of the web service are
+    released) or to go ahead and make a request to
+    http://api.launchpad.net/beta/beta/, and cause an unhelpful 404
+    error.
+    """
+
+    def test_service_name_with_embedded_version(self):
+        # Basic test. If there were no exception raised here,
+        # launchpadlib would make a request to
+        # /version-foo/version-foo.
+        version = "version-foo"
+        root = uris.service_roots['staging'] + version
+        try:
+            Launchpad(None, root, version=version)
+        except ValueError, e:
+            self.assertTrue(str(e).startswith(
+                "It looks like you're using a service root that incorporates "
+                'the name of the web service version ("version-foo")'))
+        else:
+            raise AssertionError(
+                "Expected a ValueError that was not thrown!")
+
+        # Make sure the problematic URL is caught even if it has a
+        # slash on the end.
+        root += '/'
+        self.assertRaises(ValueError, Launchpad, None, root, version=version)
+
+        # Test that the default version has the same problem
+        # when no explicit version is specified
+        default_version = NoNetworkLaunchpad.DEFAULT_VERSION
+        root = uris.service_roots['staging'] + default_version + '/'
+        self.assertRaises(ValueError, Launchpad, None, root)
+
+
 class TestLaunchpadLoginWith(unittest.TestCase):
     """Tests for Launchpad.login_with()."""