Launchpad itself

Merge lp:~thumper/launchpad/fix-vocab-oops into lp:launchpad

  1. fix-vocab-oops
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~thumper/launchpad/fix-vocab-oops
Merge into: lp:launchpad
Diff against target: 90 lines
3 files modified
lib/lp/code/model/branchlookup.py (+8/-2)
lib/lp/code/model/tests/test_branchcollection.py (+36/-0)
lib/lp/code/model/tests/test_branchlookup.py (+1/-1)
To merge this branch: bzr merge lp:~thumper/launchpad/fix-vocab-oops
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Review via email: mp+14065@code.launchpad.net

Commit message

Catch the extra exceptions that getByLPPath can raise.

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

= Summary =

Don't oops if the user types something stupid into the search field.

== Proposed fix ==

Fix the branch collection search method.

== Implementation details ==

BranchLookup.getByUrl raises many different exceptions, we should catch them.

== Tests ==

test_branchcollection.TestSearch

== Demo and Q/A ==

try to link a branch by typing in an incomplete lp: url.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/model/branchcollection.py
  lib/lp/code/model/tests/test_branchcollection.py

== Pylint notices ==

lib/lp/code/model/branchcollection.py
    294: [W0703, GenericBranchCollection._getExactMatch] Catch "Exception"

Yes, I know.

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

The problem with this is that it will mask genuine problems in our code. Not voting for now, will have a think in the morning.

Revision history for this message
Tim Penhey (thumper) wrote :

On Wed, 28 Oct 2009 17:56:46 Michael Hudson wrote:
> The problem with this is that it will mask genuine problems in our code.
> Not voting for now, will have a think in the morning.

The only place this is being caught is in the place where it is being called
from search. The search method is only called from the BranchVocabulary.

The normal branch traversal stuff still uses the normal method and still
exposes the exceptions.

Tim

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

> On Wed, 28 Oct 2009 17:56:46 Michael Hudson wrote:
> > The problem with this is that it will mask genuine problems in our code.
> > Not voting for now, will have a think in the morning.
>
> The only place this is being caught is in the place where it is being called
> from search. The search method is only called from the BranchVocabulary.

As discussed on IRC, I'd be happier if you put the try:except: into getByUrl -- that would protect the other callers of this function too. Even happier if you listed the exceptions that can be triggered by dodgy input, but I won't insist.

Revision history for this message
Michael Hudson-Doyle (mwhudson) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/model/branchlookup.py'
2--- lib/lp/code/model/branchlookup.py 2009-10-20 21:52:50 +0000
3+++ lib/lp/code/model/branchlookup.py 2009-10-29 02:44:13 +0000
4@@ -26,10 +26,12 @@
5 IBranchLookup, ILinkedBranchTraversable, ILinkedBranchTraverser)
6 from lp.code.interfaces.branchnamespace import (
7 IBranchNamespaceSet, InvalidNamespace)
8-from lp.code.interfaces.linkedbranch import get_linked_branch, NoLinkedBranch
9+from lp.code.interfaces.linkedbranch import (
10+ CannotHaveLinkedBranch, get_linked_branch, NoLinkedBranch)
11 from lp.registry.interfaces.distribution import IDistribution
12 from lp.registry.interfaces.distroseries import (
13 IDistroSeries, IDistroSeriesSet, NoSuchDistroSeries)
14+from lp.registry.interfaces.person import NoSuchPerson
15 from lp.registry.interfaces.pillar import IPillarNameSet
16 from lp.registry.interfaces.product import (
17 InvalidProductName, IProduct, NoSuchProduct)
18@@ -228,7 +230,11 @@
19 return None
20 try:
21 return self.getByLPPath(uri.path.lstrip('/'))[0]
22- except NoSuchBranch:
23+ except (
24+ CannotHaveLinkedBranch, InvalidNamespace, InvalidProductName,
25+ NoSuchBranch, NoSuchPerson, NoSuchProduct,
26+ NoSuchProductSeries, NoSuchDistroSeries,
27+ NoSuchSourcePackageName, NoLinkedBranch):
28 return None
29
30 return Branch.selectOneBy(url=url)
31
32=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
33--- lib/lp/code/model/tests/test_branchcollection.py 2009-10-06 22:18:57 +0000
34+++ lib/lp/code/model/tests/test_branchcollection.py 2009-10-29 02:44:13 +0000
35@@ -787,6 +787,42 @@
36 search_results = self.collection.search(branch.codebrowse_url())
37 self.assertEqual([branch], list(search_results))
38
39+ def test_exact_match_bzr_identity(self):
40+ # If you search for the bzr identity of a branch, then you get a
41+ # single result with that branch.
42+ branch = self.factory.makeAnyBranch()
43+ not_branch = self.factory.makeAnyBranch()
44+ search_results = self.collection.search(branch.bzr_identity)
45+ self.assertEqual([branch], list(search_results))
46+
47+ def test_exact_match_bzr_identity_development_focus(self):
48+ # If you search for the development focus and it is set, you get a
49+ # single result with the development focus branch.
50+ fooix = self.factory.makeProduct(name='fooix')
51+ branch = self.factory.makeProductBranch(product=fooix)
52+ run_with_login(
53+ fooix.owner, setattr, fooix.development_focus,
54+ 'branch', branch)
55+ not_branch = self.factory.makeAnyBranch()
56+ search_results = self.collection.search('lp://dev/fooix')
57+ self.assertEqual([branch], list(search_results))
58+
59+ def test_bad_match_bzr_identity_development_focus(self):
60+ # If you search for the development focus for a project where one
61+ # isn't set, you get an empty search result.
62+ fooix = self.factory.makeProduct(name='fooix')
63+ branch = self.factory.makeProductBranch(product=fooix)
64+ not_branch = self.factory.makeAnyBranch()
65+ search_results = self.collection.search('lp://dev/fooix')
66+ self.assertEqual([], list(search_results))
67+
68+ def test_bad_match_bzr_identity_no_project(self):
69+ # If you search for the development focus for a project where one
70+ # isn't set, you get an empty search result.
71+ not_branch = self.factory.makeAnyBranch()
72+ search_results = self.collection.search('lp://dev/fooix')
73+ self.assertEqual([], list(search_results))
74+
75 def test_exact_match_url_trailing_slash(self):
76 # Sometimes, users are inconsiderately unaware of our arbitrary
77 # database restrictions and will put trailing slashes on their search
78
79=== modified file 'lib/lp/code/model/tests/test_branchlookup.py'
80--- lib/lp/code/model/tests/test_branchlookup.py 2009-10-19 06:17:25 +0000
81+++ lib/lp/code/model/tests/test_branchlookup.py 2009-10-29 02:44:13 +0000
82@@ -283,7 +283,7 @@
83 """lp: URLs for the configured prefix are supported."""
84 branch_set = getUtility(IBranchLookup)
85 url = '%s~aa/b/c' % config.codehosting.bzr_lp_prefix
86- self.assertRaises(NoSuchPerson, branch_set.getByUrl, url)
87+ self.assertIs(None, branch_set.getByUrl(url))
88 owner = self.factory.makePerson(name='aa')
89 product = self.factory.makeProduct('b')
90 branch2 = branch_set.getByUrl(url)