launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/stop-catching-integrity-error into lp:launchpad.

Commit message:
Check existence of Snap/LiveFS/Packageset before creating them, rather than catching IntegrityError.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/stop-catching-integrity-error/+merge/272546

Check existence of Snap/LiveFS/Packageset before creating them, rather than catching IntegrityError.  Packageset still requires the store.flush() after creation, since some code relies on immediately being able to fetch Packageset.id.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/stop-catching-integrity-error into lp:launchpad.

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2015-09-18 14:14:34 +0000
+++ lib/lp/snappy/model/snap.py	2015-09-28 03:38:51 +0000
@@ -7,7 +7,6 @@
     ]
 
 import pytz
-from storm.exceptions import IntegrityError
 from storm.locals import (
     Bool,
     DateTime,
@@ -340,6 +339,8 @@
 
         if branch is None and git_ref is None:
             raise NoSourceForSnap
+        if self.exists(owner, name):
+            raise DuplicateSnapName
 
         store = IMasterStore(Snap)
         snap = Snap(
@@ -348,11 +349,6 @@
             require_virtualized=require_virtualized, date_created=date_created)
         store.add(snap)
 
-        try:
-            store.flush()
-        except IntegrityError:
-            raise DuplicateSnapName
-
         if processors is None:
             processors = [
                 p for p in getUtility(IProcessorSet).getAll()

=== modified file 'lib/lp/soyuz/model/livefs.py'
--- lib/lp/soyuz/model/livefs.py	2015-09-15 20:16:47 +0000
+++ lib/lp/soyuz/model/livefs.py	2015-09-28 03:38:51 +0000
@@ -7,7 +7,6 @@
     ]
 
 import pytz
-from storm.exceptions import IntegrityError
 from storm.locals import (
     Bool,
     DateTime,
@@ -239,17 +238,15 @@
                     "%s cannot create live filesystems owned by %s." %
                     (registrant.displayname, owner.displayname))
 
+        if self.exists(owner, distro_series, name):
+            raise DuplicateLiveFSName
+
         store = IMasterStore(LiveFS)
         livefs = LiveFS(
             registrant, owner, distro_series, name, metadata,
             require_virtualized, date_created)
         store.add(livefs)
 
-        try:
-            store.flush()
-        except IntegrityError:
-            raise DuplicateLiveFSName
-
         return livefs
 
     def _getByName(self, owner, distro_series, name):

=== modified file 'lib/lp/soyuz/model/packageset.py'
--- lib/lp/soyuz/model/packageset.py	2015-07-08 16:05:11 +0000
+++ lib/lp/soyuz/model/packageset.py	2015-09-28 03:38:51 +0000
@@ -5,7 +5,6 @@
 __all__ = ['Packageset', 'PackagesetSet']
 
 import pytz
-from storm.exceptions import IntegrityError
 from storm.expr import SQL
 from storm.locals import (
     DateTime,
@@ -341,6 +340,12 @@
         """See `IPackagesetSet`."""
         store = IMasterStore(Packageset)
 
+        try:
+            self.getByName(distroseries, name)
+            raise DuplicatePackagesetName
+        except NoSuchPackageSet:
+            pass
+
         packagesetgroup = None
         if related_set is not None:
             # Use the packagesetgroup of the `related_set`.
@@ -363,12 +368,9 @@
 
         store.add(packageset)
 
-        # We need to ensure that the cached statements are flushed so that
-        # the duplicate name constraint gets triggered here.
-        try:
-            store.flush()
-        except IntegrityError:
-            raise DuplicatePackagesetName()
+        # Explicit flush since it's common to use Packageset.id immediately
+        # after creation.
+        store.flush()
 
         return packageset