launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:py3-distribution-constructor into launchpad:master.

Commit message:
Avoid construction order problems with Distribution

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/398962

On Python 3.5, dict ordering is randomized and the order of keyword arguments is not preserved (the latter is fixed in Python 3.6), so SQLBase.__init__ may set arguments in any order, which can cause problems if e.g. owner validation fails before display_name is set and thus __repr__ fails.  We'll want an explicit constructor when we convert Distribution to StormBase anyway, so do that now to fix the order in which attributes are set and handle errors properly.

This is made a little easier because Distribution.__init__ is only called from DistributionSet.new.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:py3-distribution-constructor into launchpad:master.

diff --git a/lib/lp/registry/model/distribution.py b/lib/lp/registry/model/distribution.py
index 5296718..1000bea 100644
--- a/lib/lp/registry/model/distribution.py
+++ b/lib/lp/registry/model/distribution.py
@@ -278,6 +278,28 @@ class Distribution(SQLBase, BugTargetBase, MakesAnnouncements,
     oci_registry_credentials = Reference(
         oci_registry_credentials_id, "OCIRegistryCredentials.id")
 
+    def __init__(self, name, display_name, title, description, summary,
+                 domainname, members, owner, registrant, mugshot=None,
+                 logo=None, icon=None, vcs=None):
+        try:
+            self.name = name
+            self.display_name = display_name
+            self._title = title
+            self.description = description
+            self.summary = summary
+            self.domainname = domainname
+            self.members = members
+            self.mirror_admin = owner
+            self.owner = owner
+            self.registrant = registrant
+            self.mugshot = mugshot
+            self.logo = logo
+            self.icon = icon
+            self.vcs = vcs
+        except Exception:
+            IStore(self).remove(self)
+            raise
+
     def __repr__(self):
         display_name = backslashreplace(self.display_name)
         return "<%s '%s' (%s)>" % (
@@ -1555,18 +1577,18 @@ class DistributionSet:
         distro = Distribution(
             name=name,
             display_name=display_name,
-            _title=title,
+            title=title,
             description=description,
             summary=summary,
             domainname=domainname,
             members=members,
-            mirror_admin=owner,
             owner=owner,
             registrant=registrant,
             mugshot=mugshot,
             logo=logo,
             icon=icon,
             vcs=vcs)
+        IStore(distro).add(distro)
         getUtility(IArchiveSet).new(distribution=distro,
             owner=owner, purpose=ArchivePurpose.PRIMARY)
         policies = itertools.product(