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

Proposed by Paul Hummer on 2010-05-27
Status: Rejected
Rejected by: Paul Hummer on 2010-05-28
Proposed branch: lp:~rockstar/launchpad/sourcepackagerecipe-name-contstraint
Merge into: lp:launchpad
Diff against target: 176 lines (+79/-7)
4 files modified
lib/lp/code/errors.py (+5/-0)
lib/lp/code/model/sourcepackagerecipe.py (+36/-4)
lib/lp/code/model/tests/test_sourcepackagerecipe.py (+35/-0)
lib/lp/registry/model/person.py (+3/-3)
To merge this branch: bzr merge lp:~rockstar/launchpad/sourcepackagerecipe-name-contstraint
Reviewer Review Type Date Requested Status
Stuart Bishop 2010-05-27 Disapprove on 2010-05-27
Review via email: mp+26122@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Stuart Bishop (stub) wrote :

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) 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.

Stuart Bishop (stub) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/errors.py'
2--- lib/lp/code/errors.py 2010-04-06 20:38:34 +0000
3+++ lib/lp/code/errors.py 2010-05-27 05:25:46 +0000
4@@ -14,6 +14,7 @@
5 'ClaimReviewFailed',
6 'InvalidBranchMergeProposal',
7 'ReviewNotPending',
8+ 'SourcePackageRecipeExists',
9 'UnknownBranchTypeError',
10 'UserHasExistingReview',
11 'UserNotBranchReviewer',
12@@ -91,3 +92,7 @@
13 """Raised when the user requests an import that is already running."""
14
15 webservice_error(400)
16+
17+
18+class SourcePackageRecipeExists(Exception):
19+ """Raised when a source package recipe is isn't unique on owner/name."""
20
21=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
22--- lib/lp/code/model/sourcepackagerecipe.py 2010-05-26 19:23:20 +0000
23+++ lib/lp/code/model/sourcepackagerecipe.py 2010-05-27 05:25:46 +0000
24@@ -22,6 +22,7 @@
25 from canonical.database.datetimecol import UtcDateTimeCol
26 from canonical.launchpad.interfaces.lpstorm import IMasterStore, IStore
27
28+from lp.code.errors import SourcePackageRecipeExists
29 from lp.code.interfaces.sourcepackagerecipe import (
30 ISourcePackageRecipe, ISourcePackageRecipeSource,
31 ISourcePackageRecipeData)
32@@ -68,7 +69,13 @@
33 date_last_modified = UtcDateTimeCol(notNull=True)
34
35 owner_id = Int(name='owner', allow_none=True)
36- owner = Reference(owner_id, 'Person.id')
37+ _owner = Reference(owner_id, 'Person.id')
38+ def set_owner(self, value):
39+ self.ensureUnique(owner=value)
40+ self._owner = value
41+ def get_owner(self):
42+ return self._owner
43+ owner = property(get_owner, set_owner)
44
45 registrant_id = Int(name='registrant', allow_none=True)
46 registrant = Reference(registrant_id, 'Person.id')
47@@ -87,7 +94,14 @@
48 def _sourcepackagename_text(self):
49 return self.sourcepackagename.name
50
51- name = Unicode(allow_none=True)
52+ _name = Unicode('name', allow_none=True)
53+ def set_name(self, value):
54+ self.ensureUnique(name=value)
55+ self._name = value
56+ def get_name(self):
57+ return self._name
58+ name = property(get_name, set_name)
59+
60 description = Unicode(allow_none=False)
61
62 @property
63@@ -122,12 +136,15 @@
64 builder_recipe, description):
65 """See `ISourcePackageRecipeSource.new`."""
66 store = IMasterStore(SourcePackageRecipe)
67+
68 sprecipe = SourcePackageRecipe()
69+ sprecipe.ensureUnique(owner=owner, name=name)
70+
71 SourcePackageRecipeData(builder_recipe, sprecipe)
72 sprecipe.registrant = registrant
73- sprecipe.owner = owner
74+ sprecipe._owner = owner
75 sprecipe.sourcepackagename = sourcepackagename
76- sprecipe.name = name
77+ sprecipe._name = name
78 for distroseries_item in distroseries:
79 sprecipe.distroseries.add(distroseries_item)
80 sprecipe.description = description
81@@ -197,3 +214,18 @@
82 if count == 0:
83 return None
84 return result[count/2]
85+
86+ def ensureUnique(self, owner=None, name=None):
87+ """Raise an exception if the recipe will no longer be unique."""
88+ if not owner:
89+ owner = self._owner
90+ if not name:
91+ name = self._name
92+
93+ store = IMasterStore(SourcePackageRecipe)
94+ recipe = store.find(
95+ SourcePackageRecipe,
96+ SourcePackageRecipe._owner == owner,
97+ SourcePackageRecipe._name == name).one()
98+ if recipe:
99+ raise SourcePackageRecipeExists
100
101=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
102--- lib/lp/code/model/tests/test_sourcepackagerecipe.py 2010-05-18 19:14:16 +0000
103+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py 2010-05-27 05:25:46 +0000
104@@ -29,6 +29,7 @@
105 InvalidPocketForPPA)
106 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
107 from lp.buildmaster.model.buildqueue import BuildQueue
108+from lp.code.errors import SourcePackageRecipeExists
109 from lp.code.interfaces.sourcepackagerecipe import (
110 ForbiddenInstruction, ISourcePackageRecipe, ISourcePackageRecipeSource,
111 TooNewRecipeFormat, MINIMAL_RECIPE_TEXT)
112@@ -85,6 +86,40 @@
113 (recipe.registrant, recipe.owner, set(recipe.distroseries),
114 recipe.sourcepackagename, recipe.name))
115
116+ def test_creation_dupe(self):
117+ # The metadata supplied when a SourcePackageRecipe is created is
118+ # present on the new object.
119+ recipe = self.factory.makeSourcePackageRecipe()
120+
121+ self.assertRaises(SourcePackageRecipeExists,
122+ getUtility(ISourcePackageRecipeSource).new,
123+ recipe.registrant, recipe.owner, [], recipe.sourcepackagename,
124+ recipe.name, self.factory.makeRecipe(), recipe.description)
125+
126+ def test_edit_name_dupe(self):
127+ recipe = self.factory.makeSourcePackageRecipe()
128+ recipe_dupe = self.factory.makeSourcePackageRecipe(owner=recipe.owner)
129+
130+ try:
131+ with person_logged_in(recipe_dupe.owner):
132+ recipe_dupe.name = recipe.name
133+ self.fail(
134+ 'Duplicate name does not raise SourcePackageRecipeExists')
135+ except SourcePackageRecipeExists:
136+ pass
137+
138+ def test_edit_owner_dupe(self):
139+ recipe = self.factory.makeSourcePackageRecipe()
140+ recipe_dupe = self.factory.makeSourcePackageRecipe(name=recipe.name)
141+
142+ try:
143+ with person_logged_in(recipe_dupe.owner):
144+ recipe_dupe.owner = recipe.owner
145+ self.fail(
146+ 'Duplicate owner does not raise SourcePackageRecipeExists')
147+ except SourcePackageRecipeExists:
148+ pass
149+
150 def test_source_implements_interface(self):
151 # The SourcePackageRecipe class implements ISourcePackageRecipeSource.
152 self.assertProvides(
153
154=== modified file 'lib/lp/registry/model/person.py'
155--- lib/lp/registry/model/person.py 2010-05-10 13:52:14 +0000
156+++ lib/lp/registry/model/person.py 2010-05-27 05:25:46 +0000
157@@ -2276,8 +2276,8 @@
158 def getRecipe(self, name):
159 from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
160 return Store.of(self).find(
161- SourcePackageRecipe, SourcePackageRecipe.owner == self,
162- SourcePackageRecipe.name == name).one()
163+ SourcePackageRecipe, SourcePackageRecipe._owner == self,
164+ SourcePackageRecipe._name == name).one()
165
166 def isUploader(self, distribution):
167 """See `IPerson`."""
168@@ -2395,7 +2395,7 @@
169 store = Store.of(self)
170 return store.find(
171 SourcePackageRecipe,
172- SourcePackageRecipe.owner == self)
173+ SourcePackageRecipe._owner == self)
174
175
176 class PersonSet: