Launchpad itself

Merge lp:~jcsackett/launchpad/convert-sql-627631-storm into lp:launchpad/db-devel

  1. convert-sql-627631-storm
  2. Merge into db-devel
Proposed by j.c.sackett
Status: Merged
Approved by: j.c.sackett
Approved revision: no longer in the source branch.
Merged at revision: 10000
Proposed branch: lp:~jcsackett/launchpad/convert-sql-627631-storm
Merge into: lp:launchpad/db-devel
Prerequisite: lp:~jcsackett/launchpad/convert-sql-627631
Diff against target: 233 lines (+62/-25)
7 files modified
lib/canonical/launchpad/database/launchpadstatistic.py (+7/-5)
lib/lp/registry/model/product.py (+5/-1)
lib/lp/registry/model/projectgroup.py (+18/-13)
lib/lp/translations/model/translationsperson.py (+7/-2)
lib/lp/translations/scripts/tests/test_translations_to_branch.py (+2/-1)
lib/lp/translations/scripts/translations_to_branch.py (+6/-3)
lib/lp/translations/stories/standalone/xx-products-with-translations.txt (+17/-0)
To merge this branch: bzr merge lp:~jcsackett/launchpad/convert-sql-627631-storm
Reviewer Review Type Date Requested Status
Edwin Grubbs (community) code Approve
Benji York (community) code Approve
Review via email: mp+41011@code.launchpad.net

Commit message

[r=benji,edwin-grubbs][ui=none][bug=627631] Update storm queries to use usage enums instead of official booleans.

Description of the change

Summary
=======

There's an ongoing effort to phase out the official_* booleans which indicate how a product/distribution uses the various service on Launchpad. While previous branches cleaned up a lot of model code, they didn't address queries made to the db that used the Boolean columns.

This branch rectifies that by cleaning up those sections of code to reference the EnumCol, so that we can (if we choose to) drop the official_* code completely.

This branch was split out separately from the one it depends on b/c it seemed that storm queries needed special handling, and were related to some other issues in the code base.

Proposed fix
============

Replace locations making Storm queries and update them to use the EnumCol and ServiceUsage DBItem.

Preimplementation talk
======================

Spoke with Curtis Hovey.

Implementation details
======================

As in proposed. Additionally, as low hanging fruit a query was converted to storm to fix a related issue in projectgroups.

Tests
=====

Because of the number of things that end up effected, most modules need testing. Testing registry alone is a reasonable proxy, but not ideal.

bin/test -m lp.registry.tests

OR

bin/test -m lp.registry
bin/test -m lp.translations
bin/test -m lp.answers

Demo & QA
=========

Operation of the various main pages (e.g. +translations, +questions) for a product or distribution shouldn't fail in anyway or appear different; b/c of prior work this change should be invisible.

Lint
====

make lint output:

= Launchpad lint =
  2
  3 Checking for conflicts and issues in changed files.
  4
  5 Linting changed files:
  6 lib/canonical/launchpad/database/launchpadstatistic.py
  7 lib/lp/answers/model/question.py
  8 lib/lp/registry/doc/product.txt
  9 lib/lp/registry/model/product.py
 10 lib/lp/registry/model/projectgroup.py
 11 lib/lp/translations/doc/translationsoverview.txt
 12 lib/lp/translations/model/translationsoverview.py
 13 lib/lp/translations/model/translationsperson.py
 14 lib/lp/translations/scripts/translations_to_branch.py
 15

To post a comment you must log in.
Revision history for this message
Benji York (benji) wrote :

This branch looks good.

Replacing hard-coded SQL with Storm expressions is a nice bonus.

I'm curious why _translations_usage has a leading underscore, but that
wasn't introduced in this branch, so not really relevant.

Most people include the lint output as you do, but personally I prefer
to include the output of lint /before/ I fix the things it notes. That
way the reviewer can know which parts of the diff were lint and which
were integral to the task at hand. Not a big deal, and an opinion that
may not be shared by others; just something to think about.

review: Approve (code)
Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

Hi JC,

It's a good thing Benji pointed out the odd leading underscore in "_translations_usage" since this is the cause of a serious bug in your queries, and it is probably something that storm or the sqlobject layer on top of it should warn about.

Product.selectBy(translations_usage=ServiceUsage.LAUNCHPAD) actually generates the SQL:
 WHERE FALSE
since Person.translations_usage is a property object and not a storm attribute.

Product.selectBy(_translations_usage=ServiceUsage.LAUNCHPAD) generates the SQL:
 WHERE Product.translations_usage = 20

The need for a property to override the setter can be done in storm with a validator. However, I'm not sure if you will still need to override the getter, which would still require a translations_usage property, and the storm attribute would still need to be renamed _translations_usage.

-Edwin

Revision history for this message
j.c.sackett (jcsackett) wrote :

> I'm curious why _translations_usage has a leading underscore, but that
> wasn't introduced in this branch, so not really relevant.

It's part of the long running effort to migrate to these enums. _translations_usage is the enum column. translations_usage is a property that uses _translations_usage.

At some point there will be a branch that removes the properties, once we're 100% sure we no longer need them.

Revision history for this message
j.c.sackett (jcsackett) wrote :

> Product.selectBy(translations_usage=ServiceUsage.LAUNCHPAD) actually generates the SQL:
> WHERE FALSE
> since Person.translations_usage is a property object and not a storm attribute.
>
> Product.selectBy(_translations_usage=ServiceUsage.LAUNCHPAD) generates the SQL:
> WHERE Product.translations_usage = 20

I got this error in another place, didn't realize I hadn't been caught everywhere. Thanks for the note. I'll fix that.

Revision history for this message
Benji York (benji) wrote :

The fact that the bad query made it through suggests that there's a missing test somewhere.

Revision history for this message
j.c.sackett (jcsackett) wrote :

Actually, it suggests I need to pay more attention to which set of test results I'm looking at. :-P

This test result got mixed up with the other branch I had going of a very similar name.

Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

Hi JC,

It bothers me a little bit that you will be converting queries over to _translations_usage and then back to translations_usage once the property is removed, but I guess that will be a simple search and replace, so it should be fine to land this branch now.

-Edwin

review: Approve (code)
Revision history for this message
j.c.sackett (jcsackett) wrote :

> It bothers me a little bit that you will be converting queries over to _translations_usage and then back to translations_usage once the property is removed, but I guess that will be a simple search and replace, so it should be fine to land this branch now.

It bothers me a little bit too. Perhaps I misunderstood some of your earlier comments--there's a way to have the query use translations_usage rather than _translations_usage and still have it work? The validator stuff I have seen doesn't seem applicable.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/database/launchpadstatistic.py'
--- lib/canonical/launchpad/database/launchpadstatistic.py 2010-11-15 16:44:08 +0000
+++ lib/canonical/launchpad/database/launchpadstatistic.py 2010-11-24 14:50:20 +0000
@@ -105,8 +105,7 @@
105105
106 self.update(106 self.update(
107 'products_using_malone',107 'products_using_malone',
108 Product.selectBy(official_malone=True).count()108 Product.selectBy(official_malone=True).count())
109 )
110 ztm.commit()109 ztm.commit()
111110
112 cur = cursor()111 cur = cursor()
@@ -164,10 +163,14 @@
164 Product.selectBy(license_reviewed=True, active=True).count())163 Product.selectBy(license_reviewed=True, active=True).count())
165164
166 def _updateRosettaStatistics(self, ztm):165 def _updateRosettaStatistics(self, ztm):
166 # XXX j.c.sackett 2010-11-19 bug=677532 It's less than ideal that
167 # this query is using _translations_usage, but there's no cleaner
168 # way to deal with it. Once the bug above is resolved, this should
169 # should be fixed to use translations_usage.
167 self.update(170 self.update(
168 'products_using_rosetta',171 'products_using_rosetta',
169 Product.selectBy(translations_usage=ServiceUsage.LAUNCHPAD).count()172 Product.selectBy(
170 )173 _translations_usage=ServiceUsage.LAUNCHPAD).count())
171 self.update('potemplate_count', POTemplate.select().count())174 self.update('potemplate_count', POTemplate.select().count())
172 ztm.commit()175 ztm.commit()
173 self.update('pofile_count', POFile.select().count())176 self.update('pofile_count', POFile.select().count())
@@ -225,4 +228,3 @@
225 "FROM Question")228 "FROM Question")
226 self.update("projects_with_questions_count", cur.fetchone()[0] or 0)229 self.update("projects_with_questions_count", cur.fetchone()[0] or 0)
227 ztm.commit()230 ztm.commit()
228
229231
=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py 2010-11-21 09:52:02 +0000
+++ lib/lp/registry/model/product.py 2010-11-24 14:50:20 +0000
@@ -1567,12 +1567,16 @@
15671567
1568 def getTranslatables(self):1568 def getTranslatables(self):
1569 """See `IProductSet`"""1569 """See `IProductSet`"""
1570 # XXX j.c.sackett 2010-11-19 bug=677532 It's less than ideal that
1571 # this query is using _translations_usage, but there's no cleaner
1572 # way to deal with it. Once the bug above is resolved, this should
1573 # should be fixed to use translations_usage.
1570 results = IStore(Product).find(1574 results = IStore(Product).find(
1571 (Product, Person),1575 (Product, Person),
1572 Product.active == True,1576 Product.active == True,
1573 Product.id == ProductSeries.productID,1577 Product.id == ProductSeries.productID,
1574 POTemplate.productseriesID == ProductSeries.id,1578 POTemplate.productseriesID == ProductSeries.id,
1575 Product.official_rosetta == True,1579 Product._translations_usage == ServiceUsage.LAUNCHPAD,
1576 Person.id == Product._ownerID).config(1580 Person.id == Product._ownerID).config(
1577 distinct=True).order_by(Product.title)1581 distinct=True).order_by(Product.title)
15781582
15791583
=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py 2010-11-15 22:22:38 +0000
+++ lib/lp/registry/model/projectgroup.py 2010-11-24 14:50:20 +0000
@@ -21,6 +21,7 @@
21from storm.expr import (21from storm.expr import (
22 And,22 And,
23 SQL,23 SQL,
24 Join,
24 )25 )
25from storm.locals import Int26from storm.locals import Int
26from storm.store import Store27from storm.store import Store
@@ -104,6 +105,7 @@
104 )105 )
105from lp.services.worlddata.model.language import Language106from lp.services.worlddata.model.language import Language
106from lp.translations.enums import TranslationPermission107from lp.translations.enums import TranslationPermission
108from lp.translations.model.potemplate import POTemplate
107109
108110
109class ProjectGroup(SQLBase, BugTargetBase, HasSpecificationsMixin,111class ProjectGroup(SQLBase, BugTargetBase, HasSpecificationsMixin,
@@ -184,22 +186,25 @@
184186
185 def translatables(self):187 def translatables(self):
186 """See `IProjectGroup`."""188 """See `IProjectGroup`."""
187 return Product.select('''189 store = Store.of(self)
188 Product.project = %s AND190 origin = [
189 Product.translations_usage = %s AND191 Product,
190 Product.id = ProductSeries.product AND192 Join(ProductSeries, Product.id == ProductSeries.productID),
191 POTemplate.productseries = ProductSeries.id193 Join(POTemplate, ProductSeries.id == POTemplate.productseriesID),
192 ''' % sqlvalues(self, ServiceUsage.LAUNCHPAD),194 ]
193 clauseTables=['ProductSeries', 'POTemplate'],195 # XXX j.c.sackett 2010-11-19 bug=677532 It's less than ideal that
194 distinct=True)196 # this query is using _translations_usage, but there's no cleaner
197 # way to deal with it. Once the bug above is resolved, this should
198 # should be fixed to use translations_usage.
199 return store.using(*origin).find(
200 Product,
201 Product.project == self.id,
202 Product._translations_usage == ServiceUsage.LAUNCHPAD,
203 ).config(distinct=True)
195204
196 def has_translatable(self):205 def has_translatable(self):
197 """See `IProjectGroup`."""206 """See `IProjectGroup`."""
198 # XXX: BradCrittenden 2010-10-12 bug=659078: The test should be207 return not self.translatables().is_empty()
199 # converted to use is_empty but the implementation in storm's
200 # sqlobject wrapper is broken.
201 # return not self.translatables().is_empty()
202 return self.translatables().count() != 0
203208
204 def has_branches(self):209 def has_branches(self):
205 """ See `IProjectGroup`."""210 """ See `IProjectGroup`."""
206211
=== modified file 'lib/lp/translations/model/translationsperson.py'
--- lib/lp/translations/model/translationsperson.py 2010-11-15 17:38:25 +0000
+++ lib/lp/translations/model/translationsperson.py 2010-11-24 14:50:20 +0000
@@ -24,6 +24,7 @@
2424
25from canonical.database.sqlbase import sqlvalues25from canonical.database.sqlbase import sqlvalues
26from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities26from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
27from lp.app.enums import ServiceUsage
27from lp.registry.interfaces.person import IPerson28from lp.registry.interfaces.person import IPerson
28from lp.registry.model.distribution import Distribution29from lp.registry.model.distribution import Distribution
29from lp.registry.model.distroseries import DistroSeries30from lp.registry.model.distroseries import DistroSeries
@@ -277,16 +278,20 @@
277 # translation focus.278 # translation focus.
278 distrojoin_conditions = And(279 distrojoin_conditions = And(
279 Distribution.id == DistroSeries.distributionID,280 Distribution.id == DistroSeries.distributionID,
280 Distribution.official_rosetta == True,281 Distribution._translations_usage == ServiceUsage.LAUNCHPAD,
281 Distribution.translation_focusID == DistroSeries.id)282 Distribution.translation_focusID == DistroSeries.id)
282283
283 DistroJoin = LeftJoin(Distribution, distrojoin_conditions)284 DistroJoin = LeftJoin(Distribution, distrojoin_conditions)
284285
285 ProductSeriesJoin = LeftJoin(286 ProductSeriesJoin = LeftJoin(
286 ProductSeries, ProductSeries.id == POTemplate.productseriesID)287 ProductSeries, ProductSeries.id == POTemplate.productseriesID)
288 # XXX j.c.sackett 2010-11-19 bug=677532 It's less than ideal that
289 # this query is using _translations_usage, but there's no cleaner
290 # way to deal with it. Once the bug above is resolved, this should
291 # should be fixed to use translations_usage.
287 ProductJoin = LeftJoin(Product, And(292 ProductJoin = LeftJoin(Product, And(
288 Product.id == ProductSeries.productID,293 Product.id == ProductSeries.productID,
289 Product.official_rosetta == True))294 Product._translations_usage == ServiceUsage.LAUNCHPAD))
290295
291 ProjectJoin = LeftJoin(296 ProjectJoin = LeftJoin(
292 ProjectGroup, ProjectGroup.id == Product.projectID)297 ProjectGroup, ProjectGroup.id == Product.projectID)
293298
=== modified file 'lib/lp/translations/scripts/tests/test_translations_to_branch.py'
--- lib/lp/translations/scripts/tests/test_translations_to_branch.py 2010-11-18 20:39:59 +0000
+++ lib/lp/translations/scripts/tests/test_translations_to_branch.py 2010-11-24 14:50:20 +0000
@@ -17,6 +17,7 @@
17from canonical.launchpad.scripts.logger import QuietFakeLogger17from canonical.launchpad.scripts.logger import QuietFakeLogger
18from canonical.launchpad.scripts.tests import run_script18from canonical.launchpad.scripts.tests import run_script
19from canonical.testing.layers import ZopelessAppServerLayer19from canonical.testing.layers import ZopelessAppServerLayer
20from lp.app.enums import ServiceUsage
20from lp.registry.interfaces.teammembership import (21from lp.registry.interfaces.teammembership import (
21 ITeamMembershipSet,22 ITeamMembershipSet,
22 TeamMembershipStatus,23 TeamMembershipStatus,
@@ -59,7 +60,7 @@
59 # Set up a translations_branch for the series.60 # Set up a translations_branch for the series.
60 db_branch, tree = self.create_branch_and_tree(product=product)61 db_branch, tree = self.create_branch_and_tree(product=product)
61 removeSecurityProxy(db_branch).last_scanned_id = 'null:'62 removeSecurityProxy(db_branch).last_scanned_id = 'null:'
62 product.official_rosetta = True63 product.translations_usage = ServiceUsage.LAUNCHPAD
63 series.translations_branch = db_branch64 series.translations_branch = db_branch
6465
65 # Set up a template & Dutch translation for the series.66 # Set up a template & Dutch translation for the series.
6667
=== modified file 'lib/lp/translations/scripts/translations_to_branch.py'
--- lib/lp/translations/scripts/translations_to_branch.py 2010-11-20 15:03:58 +0000
+++ lib/lp/translations/scripts/translations_to_branch.py 2010-11-24 14:50:20 +0000
@@ -16,8 +16,8 @@
16from bzrlib.errors import NotBranchError16from bzrlib.errors import NotBranchError
17import pytz17import pytz
18from storm.expr import (18from storm.expr import (
19 And,
19 Join,20 Join,
20 SQL,
21 )21 )
22from zope.component import getUtility22from zope.component import getUtility
2323
@@ -32,6 +32,7 @@
32 MAIN_STORE,32 MAIN_STORE,
33 SLAVE_FLAVOR,33 SLAVE_FLAVOR,
34 )34 )
35from lp.app.enums import ServiceUsage
35from lp.code.interfaces.branchjob import IRosettaUploadJobSource36from lp.code.interfaces.branchjob import IRosettaUploadJobSource
36from lp.code.model.directbranchcommit import (37from lp.code.model.directbranchcommit import (
37 ConcurrentUpdateError,38 ConcurrentUpdateError,
@@ -316,8 +317,10 @@
316 product_join = Join(317 product_join = Join(
317 ProductSeries, Product, ProductSeries.product == Product.id)318 ProductSeries, Product, ProductSeries.product == Product.id)
318 productseries = self.store.using(product_join).find(319 productseries = self.store.using(product_join).find(
319 ProductSeries, SQL(320 ProductSeries,
320 "official_rosetta AND translations_branch IS NOT NULL"))321 And(
322 Product._translations_usage == ServiceUsage.LAUNCHPAD,
323 ProductSeries.translations_branch != None))
321324
322 # Anything deterministic will do, and even that is only for325 # Anything deterministic will do, and even that is only for
323 # testing.326 # testing.
324327
=== modified file 'lib/lp/translations/stories/standalone/xx-products-with-translations.txt'
--- lib/lp/translations/stories/standalone/xx-products-with-translations.txt 2009-09-11 11:07:49 +0000
+++ lib/lp/translations/stories/standalone/xx-products-with-translations.txt 2010-11-24 14:50:20 +0000
@@ -1,5 +1,22 @@
1= Products with translations =1= Products with translations =
22
3We have to do a little set up.
4
5 >>> from zope.component import getUtility
6 >>> from lp.app.enums import ServiceUsage
7 >>> from lp.registry.interfaces.product import IProductSet
8 >>> from lp.testing import (
9 ... login,
10 ... logout,
11 ... )
12 >>> login('admin@canonical.com')
13 >>> evolution = getUtility(IProductSet).getByName('evolution')
14 >>> alsa = getUtility(IProductSet).getByName('alsa-utils')
15 >>> evolution.translations_usage = ServiceUsage.LAUNCHPAD
16 >>> alsa.translations_usage = ServiceUsage.LAUNCHPAD
17 >>> transaction.commit()
18 >>> logout()
19
3The +products-with-translations page lists all translatable products in20The +products-with-translations page lists all translatable products in
4Launchpad.21Launchpad.
522

Subscribers

People subscribed via source and target branches

to status/vote changes: