Merge lp:~rockstar/launchpad/sourcepackagerecipe-name-contstraint into lp:launchpad

[Paul Hummer]

This branch ensures that a person cannot own two recipes owned by the same person. There's a db patch for this as well, but Aaron and I have decided that we also need to enforce this in the model. The followup to this branch is making sure this is dealt with in the UI, but we need to get this branch landed soon to make sure edge is prevented from having this problem.

[Stuart Bishop (stub)]

UI code should ensure that invalid values are never passed through, as that will generate an OOPS.

Database constraints exist to catch things that slip through and generate the OOPS.

Guards at this level are unnecessary and wasteful, just duplicating the functionality of the db constraint.

review: Disapprove

[Robert Collins (lifeless)]

On Thu, May 27, 2010 at 6:24 PM, Stuart Bishop
<email address hidden> wrote:
> Review: Disapprove
> UI code should ensure that invalid values are never passed through, as that will generate an OOPS.
>
> Database constraints exist to catch things that slip through and generate the OOPS.
>
> Guards at this level are unnecessary and wasteful, just duplicating the functionality of the db constraint.

What level do LP APIs operate at? I thought they spoke directly to the
model code, and we certainly don't want mistaken LP API calls to
generate a server side OOPS due to DB constraint violation.

[Stuart Bishop (stub)]

On Thu, May 27, 2010 at 2:54 PM, Robert Collins
<email address hidden> wrote:
> On Thu, May 27, 2010 at 6:24 PM, Stuart Bishop
> <email address hidden> wrote:
>> Review: Disapprove
>> UI code should ensure that invalid values are never passed through, as that will generate an OOPS.
>>
>> Database constraints exist to catch things that slip through and generate the OOPS.
>>
>> Guards at this level are unnecessary and wasteful, just duplicating the functionality of the db constraint.
>
> What level do LP APIs operate at? I thought they spoke directly to the
> model code, and we certainly don't want mistaken LP API calls to
> generate a server side OOPS due to DB constraint violation.

That might be a point. I'm not sure. How do we currently handle all
the other unique fields?

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

Unmerged revisions

10926. By Paul Hummer on 2010-05-27

The eagle. It has landed.

10925. By Paul Hummer on 2010-05-27

Made the tests fail properly

10924. By Paul Hummer on 2010-05-27

Fixed the creation story

10923. By Paul Hummer on 2010-05-27

Added tests

10922. By Paul Hummer on 2010-05-26

Added abstraction for SourcePackageRecipe.owner and SourcePackageRecipe.name

=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2010-04-06 20:38:34 +0000
+++ lib/lp/code/errors.py	2010-05-27 05:25:46 +0000
@@ -14,6 +14,7 @@
     'ClaimReviewFailed',
     'InvalidBranchMergeProposal',
     'ReviewNotPending',
+    'SourcePackageRecipeExists',
     'UnknownBranchTypeError',
     'UserHasExistingReview',
     'UserNotBranchReviewer',
@@ -91,3 +92,7 @@
     """Raised when the user requests an import that is already running."""
 
     webservice_error(400)
+
+
+class SourcePackageRecipeExists(Exception):
+    """Raised when a source package recipe is isn't unique on owner/name."""

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2010-05-26 19:23:20 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2010-05-27 05:25:46 +0000
@@ -22,6 +22,7 @@
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.launchpad.interfaces.lpstorm import IMasterStore, IStore
 
+from lp.code.errors import SourcePackageRecipeExists
 from lp.code.interfaces.sourcepackagerecipe import (
     ISourcePackageRecipe, ISourcePackageRecipeSource,
     ISourcePackageRecipeData)
@@ -68,7 +69,13 @@
     date_last_modified = UtcDateTimeCol(notNull=True)
 
     owner_id = Int(name='owner', allow_none=True)
-    owner = Reference(owner_id, 'Person.id')
+    _owner = Reference(owner_id, 'Person.id')
+    def set_owner(self, value):
+        self.ensureUnique(owner=value)
+        self._owner = value
+    def get_owner(self):
+        return self._owner
+    owner = property(get_owner, set_owner)
 
     registrant_id = Int(name='registrant', allow_none=True)
     registrant = Reference(registrant_id, 'Person.id')
@@ -87,7 +94,14 @@
     def _sourcepackagename_text(self):
         return self.sourcepackagename.name
 
-    name = Unicode(allow_none=True)
+    _name = Unicode('name', allow_none=True)
+    def set_name(self, value):
+        self.ensureUnique(name=value)
+        self._name = value
+    def get_name(self):
+        return self._name
+    name = property(get_name, set_name)
+
     description = Unicode(allow_none=False)
 
     @property
@@ -122,12 +136,15 @@
             builder_recipe, description):
         """See `ISourcePackageRecipeSource.new`."""
         store = IMasterStore(SourcePackageRecipe)
+
         sprecipe = SourcePackageRecipe()
+        sprecipe.ensureUnique(owner=owner, name=name)
+
         SourcePackageRecipeData(builder_recipe, sprecipe)
         sprecipe.registrant = registrant
-        sprecipe.owner = owner
+        sprecipe._owner = owner
         sprecipe.sourcepackagename = sourcepackagename
-        sprecipe.name = name
+        sprecipe._name = name
         for distroseries_item in distroseries:
             sprecipe.distroseries.add(distroseries_item)
         sprecipe.description = description
@@ -197,3 +214,18 @@
         if count == 0:
             return None
         return result[count/2]
+
+    def ensureUnique(self, owner=None, name=None):
+        """Raise an exception if the recipe will no longer be unique."""
+        if not owner:
+            owner = self._owner
+        if not name:
+            name = self._name
+
+        store = IMasterStore(SourcePackageRecipe)
+        recipe = store.find(
+            SourcePackageRecipe,
+            SourcePackageRecipe._owner == owner,
+            SourcePackageRecipe._name == name).one()
+        if recipe:
+            raise SourcePackageRecipeExists

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2010-05-18 19:14:16 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2010-05-27 05:25:46 +0000
@@ -29,6 +29,7 @@
     InvalidPocketForPPA)
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
 from lp.buildmaster.model.buildqueue import BuildQueue
+from lp.code.errors import SourcePackageRecipeExists
 from lp.code.interfaces.sourcepackagerecipe import (
     ForbiddenInstruction, ISourcePackageRecipe, ISourcePackageRecipeSource,
     TooNewRecipeFormat, MINIMAL_RECIPE_TEXT)
@@ -85,6 +86,40 @@
             (recipe.registrant, recipe.owner, set(recipe.distroseries),
              recipe.sourcepackagename, recipe.name))
 
+    def test_creation_dupe(self):
+        # The metadata supplied when a SourcePackageRecipe is created is
+        # present on the new object.
+        recipe = self.factory.makeSourcePackageRecipe()
+
+        self.assertRaises(SourcePackageRecipeExists,
+            getUtility(ISourcePackageRecipeSource).new,
+            recipe.registrant, recipe.owner, [], recipe.sourcepackagename,
+            recipe.name, self.factory.makeRecipe(), recipe.description)
+
+    def test_edit_name_dupe(self):
+        recipe = self.factory.makeSourcePackageRecipe()
+        recipe_dupe = self.factory.makeSourcePackageRecipe(owner=recipe.owner)
+
+        try:
+            with person_logged_in(recipe_dupe.owner):
+                recipe_dupe.name = recipe.name
+                self.fail(
+                    'Duplicate name does not raise SourcePackageRecipeExists')
+        except SourcePackageRecipeExists:
+            pass
+
+    def test_edit_owner_dupe(self):
+        recipe = self.factory.makeSourcePackageRecipe()
+        recipe_dupe = self.factory.makeSourcePackageRecipe(name=recipe.name)
+
+        try:
+            with person_logged_in(recipe_dupe.owner):
+                recipe_dupe.owner = recipe.owner
+                self.fail(
+                    'Duplicate owner does not raise SourcePackageRecipeExists')
+        except SourcePackageRecipeExists:
+            pass
+
     def test_source_implements_interface(self):
         # The SourcePackageRecipe class implements ISourcePackageRecipeSource.
         self.assertProvides(

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-05-10 13:52:14 +0000
+++ lib/lp/registry/model/person.py	2010-05-27 05:25:46 +0000
@@ -2276,8 +2276,8 @@
     def getRecipe(self, name):
         from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
         return Store.of(self).find(
-            SourcePackageRecipe, SourcePackageRecipe.owner == self,
-            SourcePackageRecipe.name == name).one()
+            SourcePackageRecipe, SourcePackageRecipe._owner == self,
+            SourcePackageRecipe._name == name).one()
 
     def isUploader(self, distribution):
         """See `IPerson`."""
@@ -2395,7 +2395,7 @@
         store = Store.of(self)
         return store.find(
             SourcePackageRecipe,
-            SourcePackageRecipe.owner == self)
+            SourcePackageRecipe._owner == self)
 
 
 class PersonSet: