launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/browser-unique-files into lp:launchpad.

Commit message:
Do not permit duplicate files to be added to a release.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #286251 in Launchpad itself: "Launchpad allows multiple download files with the same name"
  https://bugs.launchpad.net/launchpad/+bug/286251

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/browser-unique-files/+merge/135016

Users can upload different files to a release, all claiming to be the
same file. This issue overlaps with the product release finder uploading
duplicate files, where too many files cause timeouts or actual 502/504
errors.

--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * Update the UI to block the user from adding duplicate files.
    * Update the model to raise an error to ensure API scripts cannot
      do it.


QA

    * Visit https://qastaging.launchpad.net/gdp/0.5.x/0.5.9/+adddownloadfile
    * Verify you cannot add a file named gedit-developer-plugins-0.5.9.tar.gz.
    * Test the API using this script:
    {{{
    from launchpadlib.launchpad import Launchpad
    lp = Launchpad.login_with(
        'testing', service_root='https://api.qastaging.launchpad.net',
        version='devel')
    project = lp.projects['gdp']
    release = project.getRelease(version='0.5.9')
    release.addReleaseFile(
        filename='gedit-developer-plugins-0.5.9.tar.gz',
        file_content='testing', content_type='Code Release Tarball')
    }}}


LINT

    lib/lp/registry/browser/productrelease.py
    lib/lp/registry/browser/tests/test_productrelease.py
    lib/lp/registry/interfaces/productrelease.py
    lib/lp/registry/model/productrelease.py
    lib/lp/registry/tests/test_productrelease.py
    lib/lp/testing/factory.py


LoC:

    I have a 3,000 line credit this week.

TEST

    ./bin/test -vvc -t productrelease lp.registry


IMPLEMENTATION

I updated the factory to flush the product release file to fix spurious
testing errors.
    lib/lp/testing/factory.py

I added ProductRelease.hasReleaseFile() that allows callsites to check
for a release file before creating it. I updated addReleaseFile() to
raise an error if the release file already exists. Note that this rule
us subtly different from the product release finder fix. This will permit
several releases to each have a file named README, but no release can
have more than one file with that name. InvalidFilename is already
exported over the API.
    lib/lp/registry/interfaces/productrelease.py
    lib/lp/registry/model/productrelease.py
    lib/lp/registry/tests/test_productrelease.py

I updated for view to check if the file name is already used and it
adds an error if true. Note that file uploads are awkward using Zope/Lp,
The code needs to work directly with the request.form to learn
the file name.
    lib/lp/registry/browser/productrelease.py
    lib/lp/registry/browser/tests/test_productrelease.py
-- 
https://code.launchpad.net/~sinzui/launchpad/browser-unique-files/+merge/135016
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/browser-unique-files into lp:launchpad.

=== modified file 'lib/lp/registry/browser/productrelease.py'
--- lib/lp/registry/browser/productrelease.py	2012-07-06 21:25:10 +0000
+++ lib/lp/registry/browser/productrelease.py	2012-11-19 23:33:27 +0000
@@ -273,6 +273,17 @@
 
     page_title = label
 
+    def validate(self, data):
+        """See `LaunchpadFormView`."""
+        file_name = None
+        filecontent = self.request.form.get(self.widgets['filecontent'].name)
+        if filecontent:
+            file_name = filecontent.filename
+        if file_name and self.context.hasReleaseFile(file_name):
+            self.setFieldError(
+                'filecontent',
+                u"The file '%s' is already uploaded." % file_name)
+
     @action('Upload', name='add')
     def add_action(self, action, data):
         form = self.request.form

=== added file 'lib/lp/registry/browser/tests/test_productrelease.py'
--- lib/lp/registry/browser/tests/test_productrelease.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/tests/test_productrelease.py	2012-11-19 23:33:27 +0000
@@ -0,0 +1,56 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""View tests for ProductRelease pages."""
+
+__metaclass__ = type
+
+
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import LaunchpadFunctionalLayer
+from lp.testing.views import create_initialized_view
+
+
+class ProductReleaseAddDownloadFileViewTestCase(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_add_file(self):
+        release = self.factory.makeProductRelease()
+        maintainer = release.milestone.product.owner
+        upload = self.factory.makeFakeFileUpload(filename='pting.tar.gz')
+        form = {
+            'field.description': 'App 0.1 tarball',
+            'field.contenttype': 'CODETARBALL',
+            'field.filecontent': upload,
+            'field.actions.add': 'Upload',
+            }
+        with person_logged_in(maintainer):
+            view = create_initialized_view(
+                release, '+adddownloadfile', form=form)
+        self.assertEqual([], view.errors)
+        notifications = [
+            nm.message for nm in view.request.response.notifications]
+        self.assertEqual(
+            ["Your file 'pting.tar.gz' has been uploaded."], notifications)
+
+    def test_add_file_duplicate(self):
+        release = self.factory.makeProductRelease()
+        maintainer = release.milestone.product.owner
+        release_file = self.factory.makeProductReleaseFile(release=release)
+        file_name = release_file.libraryfile.filename
+        upload = self.factory.makeFakeFileUpload(filename=file_name)
+        form = {
+            'field.description': 'App 0.1 tarball',
+            'field.contenttype': 'CODETARBALL',
+            'field.filecontent': upload,
+            'field.actions.add': 'Upload',
+            }
+        with person_logged_in(maintainer):
+            view = create_initialized_view(
+                release, '+adddownloadfile', form=form)
+        self.assertEqual(
+            ["The file '%s' is already uploaded." % file_name], view.errors)

=== modified file 'lib/lp/registry/interfaces/productrelease.py'
--- lib/lp/registry/interfaces/productrelease.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/interfaces/productrelease.py	2012-11-19 23:33:27 +0000
@@ -185,7 +185,7 @@
     productrelease = exported(
         ReferenceChoice(title=_('Project release'),
                         description=_("The parent product release."),
-                        schema=Interface, # Defined later.
+                        schema=Interface,  # Defined later.
                         required=True,
                         vocabulary='ProductRelease'),
         exported_as='project_release')
@@ -254,6 +254,8 @@
         :param file_type: An `UpstreamFileType` enum value.
         :param description: Info about the file.
         :returns: `IProductReleaseFile` object.
+        :raises: InvalidFilename if the filename is invalid or a duplicate
+            of a file previously added to the release.
         """
 
     @export_write_operation()
@@ -284,7 +286,7 @@
     version = exported(
         ProductReleaseVersionField(
             title=_('Version'),
-            description= u'The specific version number assigned to this '
+            description=u'The specific version number assigned to this '
             'release. Letters and numbers are acceptable, for releases like '
             '"1.2rc3".',
             constraint=sane_version)
@@ -377,6 +379,8 @@
 
         Raises a NotFoundError if no matching ProductReleaseFile exists.
         """
+    def hasReleaseFile(name):
+        """Does the release have a file that matches the name?"""
 
 
 class IProductRelease(IProductReleaseEditRestricted,

=== modified file 'lib/lp/registry/model/productrelease.py'
--- lib/lp/registry/model/productrelease.py	2012-10-06 23:40:20 +0000
+++ lib/lp/registry/model/productrelease.py	2012-11-19 23:33:27 +0000
@@ -28,6 +28,7 @@
 from zope.interface import implements
 
 from lp.app.errors import NotFoundError
+from lp.registry.errors import InvalidFilename
 from lp.registry.interfaces.person import (
     validate_person,
     validate_public_person,
@@ -168,6 +169,8 @@
                        file_type=UpstreamFileType.CODETARBALL,
                        description=None):
         """See `IProductRelease`."""
+        if self.hasReleaseFile(filename):
+            raise InvalidFilename
         # Create the alias for the file.
         filename = self.normalizeFilename(filename)
         file_obj, file_size = self._getFileObjectAndSize(file_content)
@@ -212,6 +215,14 @@
                 return file_
         raise NotFoundError(name)
 
+    def hasReleaseFile(self, name):
+        """See `IProductRelease`."""
+        try:
+            self.getProductReleaseFileByName(name)
+            return True
+        except NotFoundError:
+            return False
+
 
 class ProductReleaseFile(SQLBase):
     """A file of a product release."""

=== modified file 'lib/lp/registry/tests/test_productrelease.py'
--- lib/lp/registry/tests/test_productrelease.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/tests/test_productrelease.py	2012-11-19 23:33:27 +0000
@@ -7,10 +7,20 @@
 
 from zope.component import getUtility
 
-from lp.registry.interfaces.productrelease import IProductReleaseSet
+from lp.registry.errors import InvalidFilename
+from lp.registry.interfaces.productrelease import (
+    IProductReleaseSet,
+    UpstreamFileType,
+    )
 from lp.services.database.lpstorm import IStore
-from lp.testing import TestCaseWithFactory
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 
 
 class ProductReleaseSetTestcase(TestCaseWithFactory):
@@ -45,3 +55,37 @@
         release = self.product_release_set.getBySeriesAndVersion(
             series, '0.0.1')
         self.assertStatementCount(0, getattr, release, 'milestone')
+
+
+class ProductReleaseFileTestcase(TestCaseWithFactory):
+    """Tests for ProductReleaseFile."""
+    layer = LaunchpadFunctionalLayer
+
+    def test_hasReleaseFile(self):
+        release = self.factory.makeProductRelease()
+        release_file = self.factory.makeProductReleaseFile(release=release)
+        file_name = release_file.libraryfile.filename
+        self.assertTrue(release.hasReleaseFile(file_name))
+        self.assertFalse(release.hasReleaseFile('pting'))
+
+    def test_addReleaseFile(self):
+        release = self.factory.makeProductRelease()
+        maintainer = release.milestone.product.owner
+        with person_logged_in(maintainer):
+            release_file = release.addReleaseFile(
+                'pting.txt', 'test', 'text/plain', maintainer,
+                file_type=UpstreamFileType.README, description='desc')
+        self.assertEqual('desc', release_file.description)
+        self.assertEqual(UpstreamFileType.README, release_file.filetype)
+        self.assertEqual('pting.txt', release_file.libraryfile.filename)
+        self.assertEqual('text/plain', release_file.libraryfile.mimetype)
+
+    def test_addReleaseFile_duplicate(self):
+        release_file = self.factory.makeProductReleaseFile()
+        release = release_file.productrelease
+        library_file = release_file.libraryfile
+        maintainer = release.milestone.product.owner
+        with person_logged_in(maintainer):
+            self.assertRaises(
+                InvalidFilename, release.addReleaseFile,
+                library_file.filename, 'test', 'text/plain', maintainer)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-11-13 15:49:15 +0000
+++ lib/lp/testing/factory.py	2012-11-19 23:33:27 +0000
@@ -957,6 +957,7 @@
                 signature_filename=signature_filename,
                 signature_content=signature_content,
                 description=description)
+        IStore(release).flush()
         return release_file
 
     def makeProduct(