Launchpad itself

Merge lp:~wgrant/launchpad/bug-629921-packages-empty-filter into lp:launchpad

  1. bug-629921-packages-empty-filter
  2. Merge into devel
Proposed by William Grant
Status: Merged
Approved by: Henning Eggers
Approved revision: no longer in the source branch.
Merged at revision: 11725
Proposed branch: lp:~wgrant/launchpad/bug-629921-packages-empty-filter
Merge into: lp:launchpad
Diff against target: 44 lines (+18/-1)
2 files modified
lib/lp/soyuz/browser/archive.py (+1/-1)
lib/lp/soyuz/browser/tests/test_archive_packages.py (+17/-0)
To merge this branch: bzr merge lp:~wgrant/launchpad/bug-629921-packages-empty-filter
Reviewer Review Type Date Requested Status
Henning Eggers (community) code Approve
Review via email: mp+37339@code.launchpad.net

Commit message

Treat an empty PPA package name filter as if it were absent. - Landed by henninge.

Description of the change

Passing an empty name filter string into Archive:+packages causes a query asking for packages matching '%%', and PostgreSQL appears to be insufficiently clever to optimise that away to nothing. This can result in very slow query.

The view now treats an empty name filter identically to an omitted one, and the various cases are tested.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

On Sat, Oct 2, 2010 at 8:19 PM, William Grant <email address hidden> wrote:
>
> -        if requested_name_filter is not None:
> +        if (requested_name_filter is not None and
> +            len(requested_name_filter[0]) > 0):
>             return requested_name_filter[0]

I would write this as
'if requested_name_filter and requested_name_filter[0]:'

Its more correct (if requested_name_filter[0] = None, your code will crash).

Looks good otherwise.

-Rob

Revision history for this message
Henning Eggers (henninge) wrote :

OK, as discussed with Brad on IRC, even though it violates the style guide.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/soyuz/browser/archive.py'
2--- lib/lp/soyuz/browser/archive.py 2010-09-06 20:19:45 +0000
3+++ lib/lp/soyuz/browser/archive.py 2010-10-02 09:31:08 +0000
4@@ -760,7 +760,7 @@
5 requested_name_filter = self.request.query_string_params.get(
6 'field.name_filter')
7
8- if requested_name_filter is not None:
9+ if requested_name_filter and requested_name_filter[0]:
10 return requested_name_filter[0]
11 else:
12 return None
13
14=== modified file 'lib/lp/soyuz/browser/tests/test_archive_packages.py'
15--- lib/lp/soyuz/browser/tests/test_archive_packages.py 2010-08-21 13:54:20 +0000
16+++ lib/lp/soyuz/browser/tests/test_archive_packages.py 2010-10-02 09:31:08 +0000
17@@ -89,6 +89,11 @@
18
19 layer = LaunchpadFunctionalLayer
20
21+ def getPackagesView(self, query_string=None):
22+ ppa = self.factory.makeArchive()
23+ return create_initialized_view(
24+ ppa, "+packages", query_string=query_string)
25+
26 def test_ppa_packages_menu_is_enabled(self):
27 joe = self.factory.makePerson()
28 ppa = self.factory.makeArchive()
29@@ -96,3 +101,15 @@
30 view = create_initialized_view(ppa, "+index")
31 menu = ArchiveNavigationMenu(view)
32 self.assertTrue(menu.packages().enabled)
33+
34+ def test_specified_name_filter_works(self):
35+ view = self.getPackagesView('field.name_filter=blah')
36+ self.assertEquals('blah', view.specified_name_filter)
37+
38+ def test_specified_name_filter_returns_none_on_omission(self):
39+ view = self.getPackagesView()
40+ self.assertIs(None, view.specified_name_filter)
41+
42+ def test_specified_name_filter_returns_none_on_empty_filter(self):
43+ view = self.getPackagesView('field.name_filter=')
44+ self.assertIs(None, view.specified_name_filter)