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
=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py 2010-09-06 20:19:45 +0000
+++ lib/lp/soyuz/browser/archive.py 2010-10-02 09:31:08 +0000
@@ -760,7 +760,7 @@
760 requested_name_filter = self.request.query_string_params.get(760 requested_name_filter = self.request.query_string_params.get(
761 'field.name_filter')761 'field.name_filter')
762762
763 if requested_name_filter is not None:763 if requested_name_filter and requested_name_filter[0]:
764 return requested_name_filter[0]764 return requested_name_filter[0]
765 else:765 else:
766 return None766 return None
767767
=== modified file 'lib/lp/soyuz/browser/tests/test_archive_packages.py'
--- lib/lp/soyuz/browser/tests/test_archive_packages.py 2010-08-21 13:54:20 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_packages.py 2010-10-02 09:31:08 +0000
@@ -89,6 +89,11 @@
8989
90 layer = LaunchpadFunctionalLayer90 layer = LaunchpadFunctionalLayer
9191
92 def getPackagesView(self, query_string=None):
93 ppa = self.factory.makeArchive()
94 return create_initialized_view(
95 ppa, "+packages", query_string=query_string)
96
92 def test_ppa_packages_menu_is_enabled(self):97 def test_ppa_packages_menu_is_enabled(self):
93 joe = self.factory.makePerson()98 joe = self.factory.makePerson()
94 ppa = self.factory.makeArchive()99 ppa = self.factory.makeArchive()
@@ -96,3 +101,15 @@
96 view = create_initialized_view(ppa, "+index")101 view = create_initialized_view(ppa, "+index")
97 menu = ArchiveNavigationMenu(view)102 menu = ArchiveNavigationMenu(view)
98 self.assertTrue(menu.packages().enabled)103 self.assertTrue(menu.packages().enabled)
104
105 def test_specified_name_filter_works(self):
106 view = self.getPackagesView('field.name_filter=blah')
107 self.assertEquals('blah', view.specified_name_filter)
108
109 def test_specified_name_filter_returns_none_on_omission(self):
110 view = self.getPackagesView()
111 self.assertIs(None, view.specified_name_filter)
112
113 def test_specified_name_filter_returns_none_on_empty_filter(self):
114 view = self.getPackagesView('field.name_filter=')
115 self.assertIs(None, view.specified_name_filter)