launchpadlib

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

  1. avoid-beta-beta
  2. Merge into trunk
Proposed by Leonard Richardson
Status: Merged
Merged at revision: not available
Proposed branch: lp:~leonardr/launchpadlib/avoid-beta-beta
Merge into: lp:launchpadlib
Diff against target: 89 lines (+58/-0)
3 files modified
src/launchpadlib/NEWS.txt (+5/-0)
src/launchpadlib/launchpad.py (+9/-0)
src/launchpadlib/tests/test_launchpad.py (+44/-0)
To merge this branch: bzr merge lp:~leonardr/launchpadlib/avoid-beta-beta
Reviewer Review Type Date Requested Status
Paul Hummer (community) code Approve
Review via email: mp+20689@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Paul Hummer (rockstar) wrote :

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

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
8- Fixed a minor bug when using login_with() to access a version of the8- Fixed a minor bug when using login_with() to access a version of the
9 Launchpad web service other than the default.9 Launchpad web service other than the default.
1010
11- Added a check to catch old client code that would cause newer
12 versions of launchpadlib to make nonsensical requests to
13 https://api.launchpad.dev/beta/beta/, and raise a helpful exception
14 telling the developer how to fix it.
15
111.5.5161.5.5
12=====17=====
1318
1419
=== 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 @@
96 :type service_root: string96 :type service_root: string
97 """97 """
98 service_root = uris.lookup_service_root(service_root)98 service_root = uris.lookup_service_root(service_root)
99 if (service_root.endswith(version)
100 or service_root.endswith(version + '/')):
101 error = ("It looks like you're using a service root that "
102 "incorporates the name of the web service version "
103 '("%s"). Please use one of the constants from '
104 "launchpadlib.uris instead, or at least remove "
105 "the version name from the root URI." % version)
106 raise ValueError(error)
107
99 super(Launchpad, self).__init__(108 super(Launchpad, self).__init__(
100 credentials, service_root, cache, timeout, proxy_info, version)109 credentials, service_root, cache, timeout, proxy_info, version)
101110
102111
=== 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 @@
92 self.assertRaises(ValueError, uris.lookup_service_root, not_a_url)92 self.assertRaises(ValueError, uris.lookup_service_root, not_a_url)
93 self.assertRaises(ValueError, uris.lookup_web_root, not_a_url)93 self.assertRaises(ValueError, uris.lookup_web_root, not_a_url)
9494
95
96class TestServiceNameWithEmbeddedVersion(unittest.TestCase):
97 """Reject service roots that include the version at the end of the URL.
98
99 If the service root is "http://api.launchpad.net/beta/" and the
100 version is "beta", the launchpadlib constructor will raise an
101 exception.
102
103 This happens with scripts that were written against old versions
104 of launchpadlib. The alternative is to try to silently fix it (the
105 fix will eventually break as new versions of the web service are
106 released) or to go ahead and make a request to
107 http://api.launchpad.net/beta/beta/, and cause an unhelpful 404
108 error.
109 """
110
111 def test_service_name_with_embedded_version(self):
112 # Basic test. If there were no exception raised here,
113 # launchpadlib would make a request to
114 # /version-foo/version-foo.
115 version = "version-foo"
116 root = uris.service_roots['staging'] + version
117 try:
118 Launchpad(None, root, version=version)
119 except ValueError, e:
120 self.assertTrue(str(e).startswith(
121 "It looks like you're using a service root that incorporates "
122 'the name of the web service version ("version-foo")'))
123 else:
124 raise AssertionError(
125 "Expected a ValueError that was not thrown!")
126
127 # Make sure the problematic URL is caught even if it has a
128 # slash on the end.
129 root += '/'
130 self.assertRaises(ValueError, Launchpad, None, root, version=version)
131
132 # Test that the default version has the same problem
133 # when no explicit version is specified
134 default_version = NoNetworkLaunchpad.DEFAULT_VERSION
135 root = uris.service_roots['staging'] + default_version + '/'
136 self.assertRaises(ValueError, Launchpad, None, root)
137
138
95class TestLaunchpadLoginWith(unittest.TestCase):139class TestLaunchpadLoginWith(unittest.TestCase):
96 """Tests for Launchpad.login_with()."""140 """Tests for Launchpad.login_with()."""
97141

Subscribers

People subscribed via source and target branches