launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-get-by-unique-name-oops into lp:launchpad.

Commit message:
Fix OOPS when trying to look up ~user/project:branch as a unique Git repository name.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1771118 in Launchpad itself: "Wrong git MP prerequisite repo causes OOPS"
  https://bugs.launchpad.net/launchpad/+bug/1771118

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-get-by-unique-name-oops/+merge/350447
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-get-by-unique-name-oops into lp:launchpad.

=== modified file 'lib/lp/code/model/gitlookup.py'
--- lib/lp/code/model/gitlookup.py	2015-07-09 12:18:51 +0000
+++ lib/lp/code/model/gitlookup.py	2018-07-23 11:05:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Database implementation of the Git repository lookup utility."""
@@ -347,7 +347,7 @@
                 if repository is None or trailing or list(segments):
                     raise InvalidNamespace(unique_name)
                 return repository
-        except (InvalidNamespace, NameLookupFailed):
+        except (InvalidNamespace, InvalidProductName, NameLookupFailed):
             pass
         return None
 

=== modified file 'lib/lp/code/model/tests/test_gitlookup.py'
--- lib/lp/code/model/tests/test_gitlookup.py	2017-10-04 01:29:35 +0000
+++ lib/lp/code/model/tests/test_gitlookup.py	2018-07-23 11:05:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the IGitLookup implementation."""
@@ -65,22 +65,47 @@
         unused_name = self.factory.getUniqueString()
         self.assertIsNone(self.lookup.getByUniqueName(unused_name))
 
+    def test_invalid_name(self):
+        # repository:branch forms are not valid as repository unique names.
+        # Provoke various failure modes depending on where the invalid
+        # component occurs in the traversal.
+        repository = self.factory.makeGitRepository()
+        self.assertIsNone(self.lookup.getByUniqueName(
+            repository.unique_name + ":branch-name"))
+        with person_logged_in(repository.owner):
+            getUtility(IGitRepositorySet).setDefaultRepositoryForOwner(
+                repository.owner, repository.target, repository,
+                repository.owner)
+        self.assertIsNone(self.lookup.getByUniqueName(
+            repository.shortened_path + ":branch-name"))
+        with person_logged_in(repository.target.owner):
+            getUtility(IGitRepositorySet).setDefaultRepository(
+                repository.target, repository)
+        self.assertIsNone(self.lookup.getByUniqueName(
+            repository.shortened_path + ":branch-name"))
+
     def test_project(self):
         repository = self.factory.makeGitRepository()
         self.assertEqual(
             repository, self.lookup.getByUniqueName(repository.unique_name))
+        self.assertIsNone(self.lookup.getByUniqueName(
+            repository.unique_name + "-nonexistent"))
 
     def test_package(self):
         dsp = self.factory.makeDistributionSourcePackage()
         repository = self.factory.makeGitRepository(target=dsp)
         self.assertEqual(
             repository, self.lookup.getByUniqueName(repository.unique_name))
+        self.assertIsNone(self.lookup.getByUniqueName(
+            repository.unique_name + "-nonexistent"))
 
     def test_personal(self):
         owner = self.factory.makePerson()
         repository = self.factory.makeGitRepository(owner=owner, target=owner)
         self.assertEqual(
             repository, self.lookup.getByUniqueName(repository.unique_name))
+        self.assertIsNone(self.lookup.getByUniqueName(
+            repository.unique_name + "-nonexistent"))
 
 
 class TestGetByPath(TestCaseWithFactory):