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
=== modified file 'lib/lp/code/model/branchlookup.py'
--- lib/lp/code/model/branchlookup.py 2009-10-20 21:52:50 +0000
+++ lib/lp/code/model/branchlookup.py 2009-10-29 02:44:13 +0000
@@ -26,10 +26,12 @@
26 IBranchLookup, ILinkedBranchTraversable, ILinkedBranchTraverser)26 IBranchLookup, ILinkedBranchTraversable, ILinkedBranchTraverser)
27from lp.code.interfaces.branchnamespace import (27from lp.code.interfaces.branchnamespace import (
28 IBranchNamespaceSet, InvalidNamespace)28 IBranchNamespaceSet, InvalidNamespace)
29from lp.code.interfaces.linkedbranch import get_linked_branch, NoLinkedBranch29from lp.code.interfaces.linkedbranch import (
30 CannotHaveLinkedBranch, get_linked_branch, NoLinkedBranch)
30from lp.registry.interfaces.distribution import IDistribution31from lp.registry.interfaces.distribution import IDistribution
31from lp.registry.interfaces.distroseries import (32from lp.registry.interfaces.distroseries import (
32 IDistroSeries, IDistroSeriesSet, NoSuchDistroSeries)33 IDistroSeries, IDistroSeriesSet, NoSuchDistroSeries)
34from lp.registry.interfaces.person import NoSuchPerson
33from lp.registry.interfaces.pillar import IPillarNameSet35from lp.registry.interfaces.pillar import IPillarNameSet
34from lp.registry.interfaces.product import (36from lp.registry.interfaces.product import (
35 InvalidProductName, IProduct, NoSuchProduct)37 InvalidProductName, IProduct, NoSuchProduct)
@@ -228,7 +230,11 @@
228 return None230 return None
229 try:231 try:
230 return self.getByLPPath(uri.path.lstrip('/'))[0]232 return self.getByLPPath(uri.path.lstrip('/'))[0]
231 except NoSuchBranch:233 except (
234 CannotHaveLinkedBranch, InvalidNamespace, InvalidProductName,
235 NoSuchBranch, NoSuchPerson, NoSuchProduct,
236 NoSuchProductSeries, NoSuchDistroSeries,
237 NoSuchSourcePackageName, NoLinkedBranch):
232 return None238 return None
233239
234 return Branch.selectOneBy(url=url)240 return Branch.selectOneBy(url=url)
235241
=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py 2009-10-06 22:18:57 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py 2009-10-29 02:44:13 +0000
@@ -787,6 +787,42 @@
787 search_results = self.collection.search(branch.codebrowse_url())787 search_results = self.collection.search(branch.codebrowse_url())
788 self.assertEqual([branch], list(search_results))788 self.assertEqual([branch], list(search_results))
789789
790 def test_exact_match_bzr_identity(self):
791 # If you search for the bzr identity of a branch, then you get a
792 # single result with that branch.
793 branch = self.factory.makeAnyBranch()
794 not_branch = self.factory.makeAnyBranch()
795 search_results = self.collection.search(branch.bzr_identity)
796 self.assertEqual([branch], list(search_results))
797
798 def test_exact_match_bzr_identity_development_focus(self):
799 # If you search for the development focus and it is set, you get a
800 # single result with the development focus branch.
801 fooix = self.factory.makeProduct(name='fooix')
802 branch = self.factory.makeProductBranch(product=fooix)
803 run_with_login(
804 fooix.owner, setattr, fooix.development_focus,
805 'branch', branch)
806 not_branch = self.factory.makeAnyBranch()
807 search_results = self.collection.search('lp://dev/fooix')
808 self.assertEqual([branch], list(search_results))
809
810 def test_bad_match_bzr_identity_development_focus(self):
811 # If you search for the development focus for a project where one
812 # isn't set, you get an empty search result.
813 fooix = self.factory.makeProduct(name='fooix')
814 branch = self.factory.makeProductBranch(product=fooix)
815 not_branch = self.factory.makeAnyBranch()
816 search_results = self.collection.search('lp://dev/fooix')
817 self.assertEqual([], list(search_results))
818
819 def test_bad_match_bzr_identity_no_project(self):
820 # If you search for the development focus for a project where one
821 # isn't set, you get an empty search result.
822 not_branch = self.factory.makeAnyBranch()
823 search_results = self.collection.search('lp://dev/fooix')
824 self.assertEqual([], list(search_results))
825
790 def test_exact_match_url_trailing_slash(self):826 def test_exact_match_url_trailing_slash(self):
791 # Sometimes, users are inconsiderately unaware of our arbitrary827 # Sometimes, users are inconsiderately unaware of our arbitrary
792 # database restrictions and will put trailing slashes on their search828 # database restrictions and will put trailing slashes on their search
793829
=== modified file 'lib/lp/code/model/tests/test_branchlookup.py'
--- lib/lp/code/model/tests/test_branchlookup.py 2009-10-19 06:17:25 +0000
+++ lib/lp/code/model/tests/test_branchlookup.py 2009-10-29 02:44:13 +0000
@@ -283,7 +283,7 @@
283 """lp: URLs for the configured prefix are supported."""283 """lp: URLs for the configured prefix are supported."""
284 branch_set = getUtility(IBranchLookup)284 branch_set = getUtility(IBranchLookup)
285 url = '%s~aa/b/c' % config.codehosting.bzr_lp_prefix285 url = '%s~aa/b/c' % config.codehosting.bzr_lp_prefix
286 self.assertRaises(NoSuchPerson, branch_set.getByUrl, url)286 self.assertIs(None, branch_set.getByUrl(url))
287 owner = self.factory.makePerson(name='aa')287 owner = self.factory.makePerson(name='aa')
288 product = self.factory.makeProduct('b')288 product = self.factory.makeProduct('b')
289 branch2 = branch_set.getByUrl(url)289 branch2 = branch_set.getByUrl(url)