launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/delete-packages-api-bug-687298 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #687298 Deleting packages via the API does not delete the binaries
  https://bugs.launchpad.net/bugs/687298


Currently when source packages are deleted on the API it leaves their binaries undeleted and in limbo.  This causes the publisher to consider the packages for condemning each and every cycle it runs, which can cause huge delays (the data fixed in production caused by this bug has changed the cycle from 40 minutes to 5).

This branch fixes that by making a special requestDeletion() just for the API which calls requestDeletion() on IPublishingSet instead (which is what the UI does).

The story test is a little nasty I'm afraid, I had to also convert it to use a different distroseries that's set up for binary publications.

At some point it would be nice to convert it to a propert unit test, but not today.
-- 
https://code.launchpad.net/~julian-edwards/launchpad/delete-packages-api-bug-687298/+merge/43098
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/delete-packages-api-bug-687298 into lp:launchpad.

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2010-11-10 04:25:52 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2010-12-08 16:12:15 +0000
@@ -30,6 +30,7 @@
 from lazr.restful.declarations import (
     call_with,
     export_as_webservice_entry,
+    export_operation_as,
     export_read_operation,
     export_write_operation,
     exported,
@@ -235,16 +236,28 @@
 class IPublishingEdit(Interface):
     """Base interface for writeable Publishing classes."""
 
+    def requestDeletion(removed_by, removal_comment=None):
+        """Delete this publication.
+
+        :param removed_by: `IPerson` responsible for the removal.
+        :param removal_comment: optional text describing the removal reason.
+        """
+
     @call_with(removed_by=REQUEST_USER)
     @operation_parameters(
         removal_comment=TextLine(title=_("Removal comment"), required=False))
+    @export_operation_as("requestDeletion")
     @export_write_operation()
-    def requestDeletion(removed_by, removal_comment=None):
-        """Delete this publication.
+    def api_requestDeletion(removed_by, removal_comment=None):
+        """Delete this source and its binaries.
 
         :param removed_by: `IPerson` responsible for the removal.
         :param removal_comment: optional text describing the removal reason.
         """
+        # This is a special API method that allows a different code path
+        # to the regular requestDeletion().  In the case of sources
+        # getting deleted, it ensures source and binaries are both
+        # deleted in tandem.
 
 
 class IFilePublishing(Interface):

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2010-12-01 11:26:57 +0000
+++ lib/lp/soyuz/model/publishing.py	2010-12-08 16:12:15 +0000
@@ -843,6 +843,13 @@
                     diff.diff_content, self.archive).http_url
         return None
 
+    def api_requestDeletion(self, removed_by, removal_comment=None):
+        """See `IPublishingEdit`."""
+        # Special deletion method for the api that makes sure binaries
+        # get deleted too.
+        getUtility(IPublishingSet).requestDeletion(
+            [self], removed_by, removal_comment)
+
 
 class BinaryPackagePublishingHistory(SQLBase, ArchivePublisherBase):
     """A binary package publishing record."""
@@ -1216,6 +1223,12 @@
 
         return dict(date_to_string(result) for result in results)
 
+    def api_requestDeletion(self, removed_by, removal_comment=None):
+        """See `IPublishingEdit`."""
+        # Special deletion method for the api.  We don't do anything
+        # different here (yet).
+        self.requestDeletion(removed_by, removal_comment)
+
 
 class PublishingSet:
     """Utilities for manipulating publications in batches."""

=== modified file 'lib/lp/soyuz/stories/webservice/xx-source-package-publishing.txt'
--- lib/lp/soyuz/stories/webservice/xx-source-package-publishing.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/soyuz/stories/webservice/xx-source-package-publishing.txt	2010-12-08 16:12:15 +0000
@@ -73,6 +73,7 @@
 
     >>> distros = webservice.get("/distros").jsonBody()
     >>> ubuntu = distros['entries'][0]
+    >>> ubuntutest = distros['entries'][2]
     >>> warty = webservice.named_get(
     ...     ubuntu['self_link'], 'getSeries',
     ...     name_or_version='warty').jsonBody()
@@ -95,17 +96,21 @@
     >>> from zope.component import getUtility
     >>> from lp.registry.interfaces.distribution import IDistributionSet
     >>> from lp.registry.interfaces.person import IPersonSet
-    >>> ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
-    >>> warty_series = ubuntu.getSeries('warty')
+    >>> from lp.soyuz.enums import PackagePublishingStatus
     >>> cprov_ppa = getUtility(IPersonSet).getByName("cprov").archive
-    >>> discard = stp.getPubSource(
-    ...     archive=cprov_ppa, sourcename="testwebservice",
-    ...     distroseries=warty_series)
+    >>> source = stp.getPubSource(
+    ...     archive=cprov_ppa, sourcename="testwebservice")
+    >>> binaries = stp.getPubBinaries(
+    ...     binaryname="testwebservice-bin", pub_source=source,
+    ...     status=PackagePublishingStatus.PUBLISHED)
     >>> logout()
 
+    >>> breezy = webservice.named_get(
+    ...     ubuntutest['self_link'], 'getSeries',
+    ...     name_or_version='breezy-autotest').jsonBody()
     >>> pubs = webservice.named_get(
     ...     cprov_archive['self_link'], 'getPublishedSources',
-    ...     distro_series=warty['self_link'],
+    ...     distro_series=breezy['self_link'],
     ...     source_name="testwebservice").jsonBody()
 
     >>> from lazr.restful.testing.webservice import pprint_entry
@@ -117,8 +122,8 @@
     date_published: None
     date_removed: None
     date_superseded: None
-    display_name: u'testwebservice 666 in warty'
-    distro_series_link: u'http://.../ubuntu/warty'
+    display_name: u'testwebservice 666 in breezy-autotest'
+    distro_series_link: u'http://.../ubuntutest/breezy-autotest'
     package_creator_link: u'http://.../beta/~name16'
     package_maintainer_link: u'http://.../beta/~name16'
     package_signer_link: u'http://.../beta/~name16'
@@ -158,7 +163,7 @@
 
     >>> pubs = webservice.named_get(
     ...     cprov_archive['self_link'], 'getPublishedSources',
-    ...     distro_series=warty['self_link'],
+    ...     distro_series=breezy['self_link'],
     ...     source_name="testwebservice").jsonBody()
 
     >>> print pubs['entries'][0]['package_signer_link']
@@ -213,20 +218,29 @@
     >>> print pubs['entries'][0]['removal_comment']
     No longer needed
 
+The package's binaries are also marked for deletion:
+
+    >>> login("admin@xxxxxxxxxxxxx")
+    >>> for bin in cprov_ppa.getAllPublishedBinaries(
+    ...     name="testwebservice-bin"):
+    ...     if bin.status != PackagePublishingStatus.DELETED:
+    ...         print "%s is not deleted when it should be" % bin.displayname
+    ...     else:
+    ...         print "%s deleted OK." % bin.displayname
+    testwebservice-bin 666 in breezy-autotest i386 deleted OK.
+    testwebservice-bin 666 in breezy-autotest hppa deleted OK.
+
+
 Privacy
 =======
 
 Create a private PPA for Celso with some binaries.
 
-    >>> login("foo.bar@xxxxxxxxxxxxx")
-
     >>> from zope.component import getUtility
     >>> from lp.registry.interfaces.distribution import IDistributionSet
     >>> from lp.registry.interfaces.person import IPersonSet
     >>> from lp.soyuz.tests.test_publishing import (
     ...      SoyuzTestPublisher)
-    >>> from lp.soyuz.enums import (
-    ...     PackagePublishingStatus)
     >>> cprov_db = getUtility(IPersonSet).getByName('cprov')
     >>> ubuntu_db = getUtility(IDistributionSet).getByName('ubuntu')
     >>> cprov_private_ppa_db = factory.makeArchive(
@@ -385,7 +399,7 @@
     []
     [u'http://launchpad.dev/~cprov/+archive/ppa/+files/mozilla-firefox_0.9_i386.deb']
     []
-    []
+    [u'http://launchpad.dev/~cprov/+archive/ppa/+files/testwebservice-bin_666_all.deb']
 
 The debdiff to a particular version can also be retrieved using the
 packageDiffUrl() method.  It takes one parameter, 'to_version' which