Launchpad itself

Code review comment for lp:~wgrant/launchpad/distroseries-source-format-selection-part1

  1. distroseries-source-format-selection-part1
  2. Merge into devel
Revision history for this message
William Grant (wgrant) wrote :

Hi Julian,

On Wed, 2009-11-11 at 21:30 +0000, William Grant wrote:
> > Hi William, thanks for making this change. There's some much-needed
> > refactoring and clearing up in nascentupload.
> >
> > The basic direction is good, I just have a few comments to make for things
> > that need fixing. Gavin will do a more thorough review as we agreed on IRC.
> >
> > General questions:
> >
> > * I can't tell from the diff but are the db permissions added for the new
> > table correctly tested in the tests? This usually just means ensuring that
> > the test runs as the correct db user.
>
> I'll have to look at that later. I'm not quite sure.

I'm not entirely sure how to check this, but I would really hope that
the upload and copy code paths were already executed as the relevant
users at some point in the test suite.

> > * The XXX about the orphaned files is interesting. I don't think we deal
> > with this right now do we? I'm trying to think if it would be bad to reject
> > the upload if we detect orphan files, and I can't think of one. What's your
> > opinion?
>
> I think there is a word or two missing there. You mean you can't think of a reason that it would be bad, or that it wouldn't be bad?
>
> I can't see any compelling reason to reject if it has extra files, apart from it seeming slightly cleaner. If you feel it needs to be done, it's probably easy enough to add.

As discussed on IRC, I've removed the XXX.

> > Things that need fixing:
> >
> > * The upload format check is not tested. (The "%s: format '%s' is not
> > permitted in %s." one)
>
> Ah, yes, forgot about that. The other half of the branch has 3.0 format test source packages, and tests that. I suppose I should bring one of those in.

I've created a 3.0 (quilt) source package and brought across the
relevant test from the second branch.

> > * IDistroSeries methods permitSourcePackageFormat() and
> > isSourcePackageFormatPermitted() should be in a utility, e.g.
> > SourcePackageFormatSet. This ensures that in a zopeful environment they are
> > security wrapped. Although the upload processor is zopeless, we might need to
> > do that one day. Something like
> > * SourcePackageFormatSet.add()
> > * SourcePackageFormatSet.getByFormat()
> > would fit with our current style. You can keep the
> > isSourcePackageFormatPermitted() and make it call the utility method as a
> > convenience, but the permitSourcePackageFormat() should go.
> >
> > * Once that's done, you should remove the __init__ on the SourcePackageFormat
> > model class and make the new utility's add() method initialise the correct
> > fields.
>
> OK, sure. Will fix.

Fixed.

The intermediate diff is attached.

9850-9857.diff
1=== modified file 'database/schema/security.cfg'
2--- database/schema/security.cfg 2009-11-10 13:09:26 +0000
3+++ database/schema/security.cfg 2009-11-11 21:34:02 +0000
4@@ -987,8 +987,8 @@
5 public.section = SELECT, INSERT, UPDATE
6 public.sectionselection = SELECT, INSERT, UPDATE
7 public.signedcodeofconduct = SELECT, INSERT, UPDATE
8+public.sourcepackagefilepublishing = SELECT, INSERT, UPDATE
9 public.sourcepackageformatselection = SELECT, INSERT
10-public.sourcepackagefilepublishing = SELECT, INSERT, UPDATE
11 public.sourcepackagename = SELECT, INSERT, UPDATE
12 public.sourcepackagepublishinghistory = SELECT
13 public.securesourcepackagepublishinghistory = SELECT, INSERT, UPDATE
14
15=== modified file 'lib/lp/archiveuploader/nascentupload.py'
16--- lib/lp/archiveuploader/nascentupload.py 2009-11-10 13:09:26 +0000
17+++ lib/lp/archiveuploader/nascentupload.py 2009-11-12 11:44:36 +0000
18@@ -325,9 +325,6 @@
19
20
21 # It is never sane to upload more than one source at a time.
22- # XXX: What about orphaned files? How will that work?
23- # I think we might need to verify that all source files are
24- # claimed by a dsc.
25 if dsc > 1:
26 self.reject("Changes file lists more than one .dsc")
27
28
29=== added directory 'lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt'
30=== added file 'lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt/bar_1.0-1.debian.tar.gz'
31Binary files lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt/bar_1.0-1.debian.tar.gz 1970-01-01 00:00:00 +0000 and lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt/bar_1.0-1.debian.tar.gz 2009-11-12 11:51:53 +0000 differ
32=== added file 'lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt/bar_1.0-1.dsc'
33--- lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt/bar_1.0-1.dsc 1970-01-01 00:00:00 +0000
34+++ lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt/bar_1.0-1.dsc 2009-11-12 11:51:53 +0000
35@@ -0,0 +1,16 @@
36+Format: 3.0 (quilt)
37+Source: bar
38+Binary: bar
39+Architecture: any
40+Version: 1.0-1
41+Maintainer: Launchpad team <launchpad@lists.canonical.com>
42+Standards-Version: 3.6.2
43+Checksums-Sha1:
44+ 73a04163fee97fd2257ab266bd48f1d3d528e012 164 bar_1.0.orig.tar.gz
45+ abce262314a7c0ca00e43598f21b41a3e6ff6b21 688 bar_1.0-1.debian.tar.gz
46+Checksums-Sha256:
47+ f1ecff929899b567f45d6734b69d59a4f3c04dabce3cc8e6ed6d64073eda360e 164 bar_1.0.orig.tar.gz
48+ ffdcce60fca14618f68483ca77a206f332a3773dc7ece1c3e6de55c0118c69c6 688 bar_1.0-1.debian.tar.gz
49+Files:
50+ fc1464e5985b962a042d5354452f361d 164 bar_1.0.orig.tar.gz
51+ 056db4dfe7de8322296b6d417592ee01 688 bar_1.0-1.debian.tar.gz
52
53=== added file 'lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt/bar_1.0-1_source.changes'
54--- lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt/bar_1.0-1_source.changes 1970-01-01 00:00:00 +0000
55+++ lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt/bar_1.0-1_source.changes 2009-11-12 11:51:54 +0000
56@@ -0,0 +1,28 @@
57+Format: 1.8
58+Date: Thu, 16 Feb 2006 15:34:09 +0000
59+Source: bar
60+Binary: bar
61+Architecture: source
62+Version: 1.0-1
63+Distribution: breezy
64+Urgency: low
65+Maintainer: Launchpad team <launchpad@lists.canonical.com>
66+Changed-By: Daniel Silverstone <daniel.silverstone@canonical.com>
67+Description:
68+ bar - Stuff for testing
69+Changes:
70+ bar (1.0-1) breezy; urgency=low
71+ .
72+ * Initial version
73+Checksums-Sha1:
74+ bc97e185cf31af33bf8d109044ce51f32d09c229 645 bar_1.0-1.dsc
75+ 73a04163fee97fd2257ab266bd48f1d3d528e012 164 bar_1.0.orig.tar.gz
76+ abce262314a7c0ca00e43598f21b41a3e6ff6b21 688 bar_1.0-1.debian.tar.gz
77+Checksums-Sha256:
78+ ae0fb16941a95518332a8ee962d00d55963b491c2df94b3f230a65d2bdbeedf8 645 bar_1.0-1.dsc
79+ f1ecff929899b567f45d6734b69d59a4f3c04dabce3cc8e6ed6d64073eda360e 164 bar_1.0.orig.tar.gz
80+ ffdcce60fca14618f68483ca77a206f332a3773dc7ece1c3e6de55c0118c69c6 688 bar_1.0-1.debian.tar.gz
81+Files:
82+ c320d2827f08f09ec2e1bbbac635225c 645 devel optional bar_1.0-1.dsc
83+ fc1464e5985b962a042d5354452f361d 164 devel optional bar_1.0.orig.tar.gz
84+ 056db4dfe7de8322296b6d417592ee01 688 devel optional bar_1.0-1.debian.tar.gz
85
86=== added file 'lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt/bar_1.0.orig.tar.gz'
87Binary files lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt/bar_1.0.orig.tar.gz 1970-01-01 00:00:00 +0000 and lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_3.0-quilt/bar_1.0.orig.tar.gz 2009-11-12 11:47:52 +0000 differ
88=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
89--- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2009-11-10 13:09:26 +0000
90+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2009-11-12 11:49:00 +0000
91@@ -49,7 +49,8 @@
92 from lp.soyuz.interfaces.archivepermission import (
93 ArchivePermissionType, IArchivePermissionSet)
94 from lp.soyuz.interfaces.component import IComponentSet
95-from lp.soyuz.interfaces.sourcepackageformat import SourcePackageFormat
96+from lp.soyuz.interfaces.sourcepackageformat import (
97+ ISourcePackageFormatSelectionSet, SourcePackageFormat)
98 from lp.registry.interfaces.person import IPersonSet
99 from lp.registry.interfaces.sourcepackagename import (
100 ISourcePackageNameSet)
101@@ -192,7 +193,9 @@
102 permitted_formats = [SourcePackageFormat.FORMAT_1_0]
103
104 for format in permitted_formats:
105- self.breezy.permitSourcePackageFormat(format)
106+ if not self.breezy.isSourcePackageFormatPermitted(format):
107+ getUtility(ISourcePackageFormatSelectionSet).add(
108+ self.breezy, format)
109
110 def addMockFile(self, filename, content="anything"):
111 """Return a librarian file."""
112@@ -1404,6 +1407,28 @@
113 ]
114 self.assertEmail(contents, recipients=recipients)
115
116+ def test30QuiltUploadToUnsupportingSeriesIsRejected(self):
117+ """Ensure that uploads to series without format support are rejected.
118+
119+ Series can restrict the source formats that they accept. Uploads
120+ should be rejected if an unsupported format is uploaded.
121+ """
122+ self.setupBreezy()
123+ self.layer.txn.commit()
124+ self.options.context = 'absolutely-anything'
125+ uploadprocessor = UploadProcessor(
126+ self.options, self.layer.txn, self.log)
127+
128+ # Upload the source.
129+ upload_dir = self.queueUpload("bar_1.0-1_3.0-quilt")
130+ self.processUpload(uploadprocessor, upload_dir)
131+ # Make sure it was rejected.
132+ from_addr, to_addrs, raw_msg = stub.test_emails.pop()
133+ self.assertTrue(
134+ "bar_1.0-1.dsc: format '3.0 (quilt)' is not permitted in "
135+ "breezy." in raw_msg,
136+ "Source was not rejected properly:\n%s" % raw_msg)
137+
138
139 def test_suite():
140 return unittest.TestLoader().loadTestsFromName(__name__)
141
142=== modified file 'lib/lp/archiveuploader/tests/test_utils.py'
143--- lib/lp/archiveuploader/tests/test_utils.py 2009-11-10 13:09:26 +0000
144+++ lib/lp/archiveuploader/tests/test_utils.py 2009-11-11 21:49:51 +0000
145@@ -24,17 +24,19 @@
146 from lp.archiveuploader.utils import determine_source_file_type
147
148 self.assertEquals(
149- determine_source_file_type('foo_1.0-1.dsc'),
150- SourcePackageFileType.DSC)
151- self.assertEquals(
152- determine_source_file_type('foo_1.0-1.diff.gz'),
153- SourcePackageFileType.DIFF)
154- self.assertEquals(
155- determine_source_file_type('foo_1.0.orig.tar.gz'),
156- SourcePackageFileType.ORIG_TARBALL)
157- self.assertEquals(
158- determine_source_file_type('foo_1.0.tar.gz'),
159- SourcePackageFileType.NATIVE_TARBALL)
160+ SourcePackageFileType.DSC,
161+ determine_source_file_type('foo_1.0-1.dsc'))
162+ self.assertEquals(
163+ SourcePackageFileType.DIFF,
164+ determine_source_file_type('foo_1.0-1.diff.gz'))
165+ self.assertEquals(
166+ SourcePackageFileType.ORIG_TARBALL,
167+ determine_source_file_type('foo_1.0.orig.tar.gz'))
168+ self.assertEquals(
169+ SourcePackageFileType.NATIVE_TARBALL,
170+ determine_source_file_type('foo_1.0.tar.gz'))
171+ self.assertEquals(None, determine_source_file_type('foo_1.0'))
172+ self.assertEquals(None, determine_source_file_type('foo_1.0.blah.gz'))
173
174 def testPrefixMultilineString(self):
175 """lp.archiveuploader.utils.prefix_multi_line_string should work"""
176
177=== modified file 'lib/lp/archiveuploader/utils.py'
178--- lib/lp/archiveuploader/utils.py 2009-11-10 13:09:26 +0000
179+++ lib/lp/archiveuploader/utils.py 2009-11-11 21:49:20 +0000
180@@ -34,12 +34,11 @@
181
182 re_isadeb = re.compile(r"(.+?)_(.+?)_(.+)\.(u?d?deb)$")
183
184+source_file_exts = ['orig.tar.gz', 'diff.gz', 'tar.gz', 'dsc']
185 re_issource = re.compile(
186- r"(.+)_(.+?)\."
187- "(orig\.tar\.gz"
188- "|diff\.gz"
189- "|tar\.gz"
190- "|dsc)$")
191+ r"(.+)_(.+?)\.(%s)" % "|".join(
192+ re.escape(ext) for ext in source_file_exts))
193+
194 re_is_orig_tar_ext = re.compile(r"^orig.tar.gz$")
195 re_is_native_tar_ext = re.compile(r"^tar.gz$")
196
197@@ -78,6 +77,8 @@
198 return SourcePackageFileType.ORIG_TARBALL
199 elif re_is_native_tar_ext.match(extension):
200 return SourcePackageFileType.NATIVE_TARBALL
201+ else:
202+ return None
203
204
205 def prefix_multi_line_string(str, prefix, include_blank_lines=0):
206
207=== modified file 'lib/lp/registry/interfaces/distroseries.py'
208--- lib/lp/registry/interfaces/distroseries.py 2009-11-10 13:09:26 +0000
209+++ lib/lp/registry/interfaces/distroseries.py 2009-11-12 11:00:33 +0000
210@@ -205,12 +205,6 @@
211 def newMilestone(name, dateexpected=None, summary=None, code_name=None):
212 """Create a new milestone for this DistroSeries."""
213
214- def permitSourcePackageFormat(format):
215- """Permit a source format to be uploaded to this series.
216-
217- :param format: The SourcePackageFormat to permit.
218- """
219-
220
221 class ISeriesMixin(Interface):
222 """Methods & properties shared between distro & product series."""
223
224=== modified file 'lib/lp/registry/model/distroseries.py'
225--- lib/lp/registry/model/distroseries.py 2009-11-10 13:09:26 +0000
226+++ lib/lp/registry/model/distroseries.py 2009-11-12 11:34:49 +0000
227@@ -82,7 +82,6 @@
228 from lp.registry.model.sourcepackagename import SourcePackageName
229 from lp.soyuz.model.sourcepackagerelease import (
230 SourcePackageRelease)
231-from lp.soyuz.model.sourcepackageformat import SourcePackageFormatSelection
232 from lp.blueprints.model.specification import (
233 HasSpecificationsMixin, Specification)
234 from lp.translations.model.translationimportqueue import (
235@@ -119,6 +118,8 @@
236 from canonical.launchpad.webapp.interfaces import (
237 IStoreSelector, MAIN_STORE, NotFoundError, SLAVE_FLAVOR,
238 TranslationUnavailable)
239+from lp.soyuz.interfaces.sourcepackageformat import (
240+ ISourcePackageFormatSelectionSet)
241
242
243 class SeriesMixin:
244@@ -1744,14 +1745,8 @@
245 return '%s%s' % (self.name, pocketsuffix[pocket])
246
247 def isSourcePackageFormatPermitted(self, format):
248- return Store.of(self).find(
249- SourcePackageFormatSelection, distroseries=self,
250- format=format).count() == 1
251-
252- def permitSourcePackageFormat(self, format):
253- if not self.isSourcePackageFormatPermitted(format):
254- return Store.of(self).add(
255- SourcePackageFormatSelection(self, format))
256+ return getUtility(ISourcePackageFormatSelectionSet
257+ ).getBySeriesAndFormat(self, format) is not None
258
259
260 class DistroSeriesSet:
261
262=== modified file 'lib/lp/soyuz/configure.zcml'
263--- lib/lp/soyuz/configure.zcml 2009-10-30 06:28:19 +0000
264+++ lib/lp/soyuz/configure.zcml 2009-11-12 10:49:55 +0000
265@@ -791,6 +791,28 @@
266 interface="lp.soyuz.interfaces.section.ISectionSet"/>
267 </securedutility>
268
269+ <!-- SourcePackageFormatSelection -->
270+
271+ <class
272+ class="lp.soyuz.model.sourcepackageformat.SourcePackageFormatSelection">
273+ <allow
274+ interface="lp.soyuz.interfaces.sourcepackageformat.ISourcePackageFormatSelection"/>
275+ </class>
276+
277+ <!-- SourcePackageFormatSelectionSet -->
278+
279+ <class
280+ class="lp.soyuz.model.sourcepackageformat.SourcePackageFormatSelectionSet">
281+ <allow
282+ interface="lp.soyuz.interfaces.sourcepackageformat.ISourcePackageFormatSelectionSet"/>
283+ </class>
284+ <securedutility
285+ class="lp.soyuz.model.sourcepackageformat.SourcePackageFormatSelectionSet"
286+ provides="lp.soyuz.interfaces.sourcepackageformat.ISourcePackageFormatSelectionSet">
287+ <allow
288+ interface="lp.soyuz.interfaces.sourcepackageformat.ISourcePackageFormatSelectionSet"/>
289+ </securedutility>
290+
291 <!-- SourcePackageReleaseFile -->
292
293 <class
294
295=== modified file 'lib/lp/soyuz/interfaces/sourcepackageformat.py'
296--- lib/lp/soyuz/interfaces/sourcepackageformat.py 2009-11-10 13:09:26 +0000
297+++ lib/lp/soyuz/interfaces/sourcepackageformat.py 2009-11-12 10:46:22 +0000
298@@ -8,6 +8,7 @@
299 __all__ = [
300 'SourcePackageFormat',
301 'ISourcePackageFormatSelection',
302+ 'ISourcePackageFormatSelectionSet',
303 ]
304
305 from zope.interface import Attribute, Interface
306@@ -50,3 +51,14 @@
307 id = Attribute("ID")
308 distroseries = Attribute("Target series")
309 format = Attribute("Permitted source package format")
310+
311+
312+class ISourcePackageFormatSelectionSet(Interface):
313+ """Set manipulation tools for the SourcePackageFormatSelection table."""
314+
315+ def getBySeriesAndFormat(distroseries, format):
316+ """Return the ISourcePackageFormatSelection for the given series and
317+ format."""
318+
319+ def add(distroseries, format):
320+ """Allow the given source package format in the given series."""
321
322=== modified file 'lib/lp/soyuz/model/sourcepackageformat.py'
323--- lib/lp/soyuz/model/sourcepackageformat.py 2009-11-10 13:09:26 +0000
324+++ lib/lp/soyuz/model/sourcepackageformat.py 2009-11-12 11:02:48 +0000
325@@ -5,25 +5,26 @@
326
327 __all__ = [
328 'SourcePackageFormatSelection',
329+ 'SourcePackageFormatSelectionSet',
330 ]
331
332 from storm.locals import Storm, Int, Reference
333+from zope.component import getUtility
334 from zope.interface import implements
335
336+from canonical.launchpad.webapp.interfaces import (
337+ IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR, MASTER_FLAVOR)
338 from canonical.database.enumcol import DBEnum
339 from lp.soyuz.interfaces.sourcepackageformat import (
340- ISourcePackageFormatSelection, SourcePackageFormat)
341+ ISourcePackageFormatSelection, ISourcePackageFormatSelectionSet,
342+ SourcePackageFormat)
343+
344
345 class SourcePackageFormatSelection(Storm):
346 """See ISourcePackageFormatSelection."""
347
348 implements(ISourcePackageFormatSelection)
349
350- def __init__(self, distroseries, format):
351- super(SourcePackageFormatSelection, self).__init__()
352- self.distroseries = distroseries
353- self.format = format
354-
355 __storm_table__ = 'sourcepackageformatselection'
356
357 id = Int(primary=True)
358@@ -33,3 +34,23 @@
359
360 format = DBEnum(enum=SourcePackageFormat)
361
362+
363+class SourcePackageFormatSelectionSet:
364+ """See ISourcePackageFormatSelectionSet."""
365+
366+ implements(ISourcePackageFormatSelectionSet)
367+
368+ def getBySeriesAndFormat(self, distroseries, format):
369+ """See `ISourcePackageFormatSelection`."""
370+ return getUtility(IStoreSelector).get(
371+ MAIN_STORE, DEFAULT_FLAVOR).find(
372+ SourcePackageFormatSelection, distroseries=distroseries,
373+ format=format).one()
374+
375+ def add(self, distroseries, format):
376+ """See `ISourcePackageFormatSelection`."""
377+ spfs = SourcePackageFormatSelection()
378+ spfs.distroseries = distroseries
379+ spfs.format = format
380+ return getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR).add(
381+ spfs)
382
383=== modified file 'lib/lp/soyuz/scripts/tests/test_copypackage.py'
384--- lib/lp/soyuz/scripts/tests/test_copypackage.py 2009-11-10 13:09:26 +0000
385+++ lib/lp/soyuz/scripts/tests/test_copypackage.py 2009-11-12 11:18:42 +0000
386@@ -37,7 +37,8 @@
387 PackagePublishingStatus, active_publishing_status)
388 from lp.soyuz.interfaces.queue import (
389 PackageUploadCustomFormat, PackageUploadStatus)
390-from lp.soyuz.interfaces.sourcepackageformat import SourcePackageFormat
391+from lp.soyuz.interfaces.sourcepackageformat import (
392+ ISourcePackageFormatSelectionSet, SourcePackageFormat)
393 from lp.soyuz.model.publishing import (
394 SecureSourcePackagePublishingHistory,
395 SecureBinaryPackagePublishingHistory)
396@@ -728,7 +729,8 @@
397 # Get hoary, and configure it to accept 3.0 (quilt) uploads.
398 ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
399 hoary = ubuntu.getSeries('hoary')
400- hoary.permitSourcePackageFormat(SourcePackageFormat.FORMAT_3_0_QUILT)
401+ getUtility(ISourcePackageFormatSelectionSet).add(
402+ hoary, SourcePackageFormat.FORMAT_3_0_QUILT)
403
404 # Create a 3.0 (quilt) source.
405 source = self.test_publisher.getPubSource(

« Back to merge proposal