launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/packageset-score-rename-support into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1000570 in Launchpad itself: ""Packageset.score" is badly named"
  https://bugs.launchpad.net/launchpad/+bug/1000570

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/packageset-score-rename-support/+merge/107981

== Summary ==

Bug 1000570 reports that Packageset.score is misnamed.  I renamed the Python attribute a while back, but this branch prepares for renaming the database column as well.

== Proposed fix ==

Unashamedly plagiarise https://code.launchpad.net/~jml/launchpad/archive-commercial-rename-support/+merge/107819, with the same rationale.

== Implementation details ==

Unlike Archive.commercial, there was one instance of Packageset.score being used a query (indirectly via a Storm aggregate of Packageset.relative_build_score).  The tests caught this and I temporarily switched from package_sets.max(Packageset.relative_build_score) to max(ps.relative_build_score for ps in package_sets).  This will be slightly less efficient, but it's very unlikely to be a performance hot-spot, so it won't be a problem to have this in place for a few days.

== LOC Rationale ==

+47, but I'll revert all of that after the corresponding database patch has been deployed.

== Tests ==

bin/test -vvct TestBuildPackageJobScore -t TestBuildQueueManual

== Lint ==

./lib/lp/soyuz/model/packageset.py
     114: redefinition of function 'relative_build_score' from line 105

False positive due to pocketlint not understanding setter properties.
-- 
https://code.launchpad.net/~cjwatson/launchpad/packageset-score-rename-support/+merge/107981
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/packageset-score-rename-support into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/buildpackagejob.py'
--- lib/lp/soyuz/model/buildpackagejob.py	2012-05-17 16:24:42 +0000
+++ lib/lp/soyuz/model/buildpackagejob.py	2012-05-30 14:08:24 +0000
@@ -40,7 +40,6 @@
     )
 from lp.soyuz.interfaces.packageset import IPackagesetSet
 from lp.soyuz.model.buildfarmbuildjob import BuildFarmBuildJob
-from lp.soyuz.model.packageset import Packageset
 
 
 class BuildPackageJob(BuildFarmJobOldDerived, Storm):
@@ -111,7 +110,7 @@
             self.build.source_package_release.name,
             distroseries=self.build.distro_series)
         if not package_sets.is_empty():
-            score += package_sets.max(Packageset.relative_build_score)
+            score += max(ps.relative_build_score for ps in package_sets)
 
         # Calculates the build queue time component of the score.
         right_now = datetime.now(pytz.timezone('UTC'))

=== modified file 'lib/lp/soyuz/model/packageset.py'
--- lib/lp/soyuz/model/packageset.py	2012-05-17 16:24:42 +0000
+++ lib/lp/soyuz/model/packageset.py	2012-05-30 14:08:24 +0000
@@ -14,6 +14,7 @@
     Storm,
     Unicode,
     )
+from storm.store import Store
 from zope.component import getUtility
 from zope.interface import implements
 
@@ -28,6 +29,8 @@
     IMasterStore,
     IStore,
     )
+from lp.services.database.postgresql import table_has_column
+from lp.services.database.sqlbase import cursor
 from lp.services.helpers import ensure_unicode
 from lp.soyuz.interfaces.packageset import (
     DuplicatePackagesetName,
@@ -70,7 +73,52 @@
     packagesetgroup_id = Int(name='packagesetgroup', allow_none=False)
     packagesetgroup = Reference(packagesetgroup_id, 'PackagesetGroup.id')
 
-    relative_build_score = Int(name="score", allow_none=False)
+    # Provide a manual property instead of declaring the column in Storm.
+    # This is to handle a transition period where we rename the 'score'
+    # column to 'relative_build_score'.  During the transition period, the
+    # code needs to work with both column names.
+    #
+    # The approach taken here only works because we never use 'score' in a
+    # WHERE clause or anything like that.
+    #
+    # Once the database change has taken place, this property should be
+    # deleted, and replaced with a class variable declaration as follows:
+    #
+    #   relative_build_score = Int(allow_none=False)
+
+    def _get_relative_build_score_column_name(self):
+        """Get the name of the column containing a relative build score.
+
+        Older versions of the database call it 'score'; newer ones call it
+        'relative_build_score'.
+
+        Works by interrogating PostgreSQL's own records.
+        """
+        # Chose this look-before-you-leap implementation so as to avoid
+        # invalidating the query by forcing a ProgrammingError.
+        cur = cursor()
+        if table_has_column(cur, "packageset", "score"):
+            return "score"
+        else:
+            return "relative_build_score"
+
+    @property
+    def relative_build_score(self):
+        """See `IPackageset`."""
+        store = Store.of(self)
+        store.flush()
+        column = self._get_relative_build_score_column_name()
+        query = "SELECT %s FROM packageset WHERE id=%%s" % column
+        return store.execute(query, (self.id,)).get_one()[0]
+
+    @relative_build_score.setter
+    def relative_build_score(self, score):
+        """See `IPackageset`."""
+        store = Store.of(self)
+        store.flush()
+        column = self._get_relative_build_score_column_name()
+        query = "UPDATE packageset SET %s=%%s WHERE id=%%s" % column
+        return store.execute(query, (int(score), self.id))
 
     def add(self, data):
         """See `IPackageset`."""