launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02286
[Merge] lp:~henninge/launchpad/db-devel-697845-import-failure into lp:launchpad/db-devel
Henning Eggers has proposed merging lp:~henninge/launchpad/db-devel-697845-import-failure into lp:launchpad/db-devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#697845 Translations import script on staging fails
https://bugs.launchpad.net/bugs/697845
For more details, see:
https://code.launchpad.net/~henninge/launchpad/db-devel-697845-import-failure/+merge/45422
= Summary =
Boy, how do I explain this... In the recife branch, the method to
find sharing translation templates was extended to include templates
on the other side of packaging links. The developer who did that
tried to be overly smart and this caused the import script to fail on
real-world data. Wonder who that guy was ... This is actually a
regression compared to current production code so we really want that
fixed.
== Proposed fix ==
Make the selection algorithm much simpler by simply selecting all
templates on this side (distribution source package or product,
denpending on the request) and all sharing templates on the other
side of any packaging link that might exist.
Due to the weirdest linkage and naming combination in the real-world,
this approach might produce a different subset for each side of a
packaging link but in normal sane cases that shouldn't happen. The
old code tried to avoid that but ended up forbidding more
translations sharing than finding as many sharing opportunities as
possible. The latter is the paradigm we are trying to follow.
If you did not understand all of that, please ask me and i'll try to
explain...
== Pre-implementation notes ==
Explained the situation to Jeroen and he understood it and agreed
that the old solution was dumb and this is the best way to go. He
also reviewed the SQL queries that were the basis for the storm
queries you see in the code now.
== Implementation details ==
In _queryByProduct you see SQL() being used. The only reason for this
is to preserve the () around the tuple/row which are needed to make
the "IN" comparison to the subquery. Using a python tuple instead is
not possible as storm loses the () around it. This may be a storm
bug.
The test case lost a few method which tested for the old behavior and
gained a couple of others, one of which represents roughly the
situation that is causing the poimport script to fail currently.
== Tests ==
bin/test -vvcm lp.translations.tests.test_shared_potemplate \
-t TestMessageSharingProductPackage
== Demo and Q/A ==
The import script on staging should work now. I will try out asap.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/translations/interfaces/potemplate.py
lib/lp/testing/factory.py
lib/lp/translations/tests/test_shared_potemplate.py
lib/lp/translations/model/potemplate.py
--
https://code.launchpad.net/~henninge/launchpad/db-devel-697845-import-failure/+merge/45422
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/db-devel-697845-import-failure into lp:launchpad/db-devel.
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2011-01-05 08:10:10 +0000
+++ lib/lp/testing/factory.py 2011-01-06 18:08:19 +0000
@@ -2479,7 +2479,8 @@
def makePOTemplate(self, productseries=None, distroseries=None,
sourcepackagename=None, owner=None, name=None,
- translation_domain=None, path=None):
+ translation_domain=None, path=None,
+ copy_pofiles=True):
"""Make a new translation template."""
if productseries is None and distroseries is None:
# No context for this template; set up a productseries.
@@ -2506,7 +2507,7 @@
if path is None:
path = 'messages.pot'
- return subset.new(name, translation_domain, path, owner)
+ return subset.new(name, translation_domain, path, owner, copy_pofiles)
def makePOTemplateAndPOFiles(self, language_codes, **kwargs):
"""Create a POTemplate and associated POFiles.
=== modified file 'lib/lp/translations/interfaces/potemplate.py'
--- lib/lp/translations/interfaces/potemplate.py 2010-12-18 09:10:37 +0000
+++ lib/lp/translations/interfaces/potemplate.py 2011-01-06 18:08:19 +0000
@@ -565,7 +565,7 @@
def __getitem__(name):
"""Get a POTemplate by its name."""
- def new(name, translation_domain, path, owner):
+ def new(name, translation_domain, path, owner, copy_pofiles=True):
"""Create a new template for the context of this Subset."""
def getPOTemplateByName(name):
=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py 2010-12-09 19:49:36 +0000
+++ lib/lp/translations/model/potemplate.py 2011-01-06 18:08:19 +0000
@@ -32,9 +32,12 @@
from storm.expr import (
And,
Desc,
+ In,
Join,
LeftJoin,
Or,
+ Select,
+ SQL,
)
from storm.info import ClassAlias
from storm.store import (
@@ -1138,7 +1141,7 @@
# Do not continue, else it would trigger an existingpo assertion.
return
- def new(self, name, translation_domain, path, owner):
+ def new(self, name, translation_domain, path, owner, copy_pofiles=True):
"""See `IPOTemplateSubset`."""
header_params = {
'origin': 'PACKAGE VERSION',
@@ -1154,7 +1157,8 @@
path=path,
owner=owner,
header=standardTemplateHeader % header_params)
- self._copyPOFilesFromSharingTemplates(template)
+ if copy_pofiles:
+ self._copyPOFilesFromSharingTemplates(template)
return template
def getPOTemplateByName(self, name):
@@ -1426,53 +1430,10 @@
assert distribution or not sourcepackagename, (
"Picking a source package only makes sense with a distribution.")
self.potemplateset = potemplateset
- self.share_by_name = False
- self.subset = None
-
- if distribution is not None:
- self.distribution = distribution
- self.sourcepackagename = sourcepackagename
- if sourcepackagename is not None:
- product = self._findUpstreamProduct(
- distribution, sourcepackagename)
- if product is None:
- self.share_by_name = self._canShareByName(
- distribution, sourcepackagename)
- if product is not None:
- self.product = product
-
- def _findUpstreamProduct(self, distribution, sourcepackagename):
- """Find the upstream product by looking at the translation focus.
-
- The translation focus is used to pick a distroseries, so a source
- package instance can be created. If no translation focus is set,
- the distribution's current series is used."""
-
- from lp.registry.model.sourcepackage import SourcePackage
- distroseries = distribution.translation_focus
- if distroseries is None:
- distroseries = distribution.currentseries
- sourcepackage = SourcePackage(sourcepackagename, distroseries)
- if sourcepackage.productseries is None:
- return None
- return sourcepackage.productseries.product
-
- def _canShareByName(self, distribution, sourcepackagename):
- """Determine if sharing by sourcepackagename is a wise thing.
-
- Without a product, the linkage between sharing packages can only be
- determined by their name. This is only (fairly) safe if none of these
- is packaged elsewhere.
- """
- from lp.registry.model.distroseries import DistroSeries
- origin = Join(
- Packaging, DistroSeries,
- Packaging.distroseries == DistroSeries.id)
- matches = Store.of(distribution).using(origin).find(
- Packaging,
- And(DistroSeries.distribution == distribution.id,
- Packaging.sourcepackagename == sourcepackagename.id))
- return not bool(matches.any())
+
+ self.distribution = distribution
+ self.sourcepackagename = sourcepackagename
+ self.product = product
def _get_potemplate_equivalence_class(self, template):
"""Return whatever we group `POTemplate`s by for sharing purposes."""
@@ -1494,27 +1455,37 @@
:return: A ResultSet for the query.
"""
# Avoid circular imports.
+ from lp.registry.model.distroseries import DistroSeries
from lp.registry.model.productseries import ProductSeries
- ProductSeries1 = ClassAlias(ProductSeries)
+ subquery = Select(
+ (DistroSeries.distributionID, Packaging.sourcepackagenameID),
+ tables=(Packaging, ProductSeries, DistroSeries),
+ where=And(
+ Packaging.productseriesID == ProductSeries.id,
+ Packaging.distroseriesID == DistroSeries.id,
+ ProductSeries.product == self.product),
+ distinct=True
+ )
origin = LeftJoin(
LeftJoin(
POTemplate, ProductSeries,
POTemplate.productseriesID == ProductSeries.id),
- Join(
- Packaging, ProductSeries1,
- Packaging.productseriesID == ProductSeries1.id),
- And(
- Packaging.distroseriesID == POTemplate.distroseriesID,
- Packaging.sourcepackagenameID == (
- POTemplate.sourcepackagenameID)))
+ DistroSeries,
+ POTemplate.distroseriesID == DistroSeries.id
+ )
return Store.of(self.product).using(origin).find(
POTemplate,
And(
+ templatename_clause,
Or(
- ProductSeries.productID == self.product.id,
- ProductSeries1.productID == self.product.id),
- templatename_clause))
+ ProductSeries.product == self.product,
+ In(
+ SQL("(DistroSeries.distribution, "
+ "POTemplate.sourcepackagename)"),
+ subquery)
+ ))
+ )
def _queryBySourcepackagename(self, templatename_clause):
"""Build the query that finds POTemplates by their names.
@@ -1524,16 +1495,36 @@
name.
:return: A ResultSet for the query.
"""
+ # Avoid circular imports.
from lp.registry.model.distroseries import DistroSeries
- origin = Join(
- POTemplate, DistroSeries,
- POTemplate.distroseries == DistroSeries.id)
+ from lp.registry.model.productseries import ProductSeries
+
+ subquery = Select(
+ ProductSeries.productID,
+ tables=(Packaging, ProductSeries, DistroSeries),
+ where=And(
+ Packaging.productseriesID == ProductSeries.id,
+ Packaging.distroseriesID == DistroSeries.id,
+ DistroSeries.distribution == self.distribution,
+ Packaging.sourcepackagename == self.sourcepackagename),
+ distinct=True
+ )
+ origin = LeftJoin(
+ LeftJoin(
+ POTemplate, ProductSeries,
+ POTemplate.productseriesID == ProductSeries.id),
+ DistroSeries,
+ POTemplate.distroseriesID == DistroSeries.id
+ )
return Store.of(self.distribution).using(origin).find(
POTemplate,
And(
- DistroSeries.distributionID == self.distribution.id,
- POTemplate.sourcepackagename == self.sourcepackagename,
- templatename_clause))
+ templatename_clause,
+ Or(
+ And(
+ DistroSeries.distribution == self.distribution,
+ POTemplate.sourcepackagename == self.sourcepackagename),
+ In(ProductSeries.productID, subquery))))
def _queryByDistribution(self, templatename_clause):
"""Special case when templates are searched across a distribution."""
@@ -1544,7 +1535,7 @@
"""Select the right query to be used."""
if self.product is not None:
return self._queryByProduct(templatename_clause)
- elif self.share_by_name:
+ elif self.sourcepackagename is not None:
return self._queryBySourcepackagename(templatename_clause)
elif self.distribution is not None and self.sourcepackagename is None:
return self._queryByDistribution(templatename_clause)
=== modified file 'lib/lp/translations/tests/test_shared_potemplate.py'
--- lib/lp/translations/tests/test_shared_potemplate.py 2010-12-02 16:13:51 +0000
+++ lib/lp/translations/tests/test_shared_potemplate.py 2011-01-06 18:08:19 +0000
@@ -319,13 +319,11 @@
def test_getSharingPOTemplates_product_multiple_series(self):
# Sharing templates for a product include the same templates from
- # a linked source package, even from multiple product series.
- # But templates for the same sourcepackagename are not returned
- # if they are not linked.
+ # a linked source package, even from multiple product series and
+ # multiple distro series.
stable_template = self.factory.makePOTemplate(
productseries=self.stable, name=self.templatename)
- # This will not be returned.
- self.factory.makePOTemplate(
+ warty_template = self.factory.makePOTemplate(
distroseries=self.warty, sourcepackagename=self.packagename,
name=self.templatename)
self.factory.makeSourcePackagePublishingHistory(
@@ -336,15 +334,17 @@
templates = self._assertStatements(
1, subset.getSharingPOTemplates(self.templatename))
- self.assertContentEqual(
- [self.trunk_template, self.hoary_template, stable_template],
- templates)
+ expected_templates = [
+ self.trunk_template,
+ self.hoary_template,
+ stable_template,
+ warty_template,
+ ]
+ self.assertContentEqual(expected_templates, templates)
def test_getSharingPOTemplates_package_multiple_series(self):
# Sharing templates for a source package include the same templates
# from a linked product, even with multiple product series.
- # To include templates from other source packages, the product must
- # be linked to that one, too.
stable_template = self.factory.makePOTemplate(
productseries=self.stable, name=self.templatename)
warty_template = self.factory.makePOTemplate(
@@ -362,34 +362,13 @@
templates = self._assertStatements(
1, subset.getSharingPOTemplates(self.templatename))
- self.assertContentEqual(
- [self.trunk_template, self.hoary_template,
- stable_template, warty_template],
- templates)
-
- def test_getSharingPOTemplates_package_name_changed(self):
- # When the name of a package changes (but not the name of the
- # template), it will still share translations if it is linked
- # to the same product.
- changed_name = self.factory.makeSourcePackageName()
- warty_template = self.factory.makePOTemplate(
- distroseries=self.warty, sourcepackagename=changed_name,
- name=self.templatename)
- hoary_sourcepackage = self.factory.makeSourcePackage(
- self.packagename, self.hoary)
- hoary_sourcepackage.setPackaging(self.trunk, self.owner)
- warty_sourcepackage = self.factory.makeSourcePackage(
- changed_name, self.warty)
- warty_sourcepackage.setPackaging(self.stable, self.owner)
- subset = self.potemplateset.getSharingSubset(
- distribution=self.ubuntu,
- sourcepackagename=self.packagename)
- templates = self._assertStatements(
- 1, subset.getSharingPOTemplates(self.templatename))
-
- self.assertContentEqual(
- [self.trunk_template, self.hoary_template, warty_template],
- templates)
+ expected_templates = [
+ self.trunk_template,
+ self.hoary_template,
+ stable_template,
+ warty_template,
+ ]
+ self.assertContentEqual(expected_templates, templates)
def test_getSharingPOTemplates_many_series(self):
# The number of queries for a call to getSharingPOTemplates must
@@ -489,6 +468,84 @@
[self.trunk_template, self.hoary_template, warty_template],
templates)
+ def test_getSharingPOTemplates_package_different_products(self):
+ # A package from different distroseries can be packaging different
+ # products. These products may also have templates in different
+ # series which are not linked to the package but have the same name.
+ warty_template = self.factory.makePOTemplate(
+ distroseries=self.warty, sourcepackagename=self.packagename,
+ name=self.templatename)
+ warty_sourcepackage = self.factory.makeSourcePackage(
+ self.packagename, self.warty)
+ warty_productseries = self.factory.makeProductSeries()
+ warty_productseries_template = self.factory.makePOTemplate(
+ productseries=warty_productseries, name=self.templatename)
+ warty_sourcepackage.setPackaging(warty_productseries, self.owner)
+
+ other_productseries = self.factory.makeProductSeries(
+ product=warty_productseries.product)
+ other_productseries_template = self.factory.makePOTemplate(
+ productseries=other_productseries, name=self.templatename)
+
+ hoary_sourcepackage = self.factory.makeSourcePackage(
+ self.packagename, self.hoary)
+ hoary_sourcepackage.setPackaging(self.trunk, self.owner)
+
+ subset = self.potemplateset.getSharingSubset(
+ distribution=self.ubuntu, sourcepackagename=self.packagename)
+ templates = self._assertStatements(
+ 1, subset.getSharingPOTemplates(self.templatename))
+
+ expected_templates = [
+ self.hoary_template,
+ self.trunk_template,
+ warty_template,
+ warty_productseries_template,
+ other_productseries_template,
+ ]
+ self.assertContentEqual(expected_templates, templates)
+
+ def test_getSharingPOTemplates_product_different_packages(self):
+ # A product can be packaged in different packages which may also have
+ # sharing templates in series that are not linked to this product.
+ other_sourcepackagename = self.factory.makeSourcePackageName()
+ self.factory.makeSourcePackagePublishingHistory(
+ sourcepackagename=other_sourcepackagename,
+ distroseries=self.warty)
+ self.stable.setPackaging(
+ self.warty, other_sourcepackagename, self.owner)
+ other_warty_sourcepackage_template = self.factory.makePOTemplate(
+ distroseries=self.warty,
+ sourcepackagename=other_sourcepackagename,
+ name=self.templatename)
+ stable_template = self.factory.makePOTemplate(
+ productseries=self.stable, name=self.templatename)
+
+ other_distroseries = self.factory.makeDistroSeries(
+ distribution=self.ubuntu)
+ other_distroseries_template = self.factory.makePOTemplate(
+ distroseries=other_distroseries,
+ sourcepackagename=other_sourcepackagename,
+ name=self.templatename)
+
+ self.factory.makeSourcePackagePublishingHistory(
+ sourcepackagename=self.packagename, distroseries=self.hoary)
+ self.trunk.setPackaging(
+ self.hoary, self.packagename, self.owner)
+
+ subset = self.potemplateset.getSharingSubset(product=self.product)
+ templates = self._assertStatements(
+ 1, subset.getSharingPOTemplates(self.templatename))
+
+ expected_templates = [
+ self.hoary_template,
+ self.trunk_template,
+ other_warty_sourcepackage_template,
+ stable_template,
+ other_distroseries_template,
+ ]
+ self.assertContentEqual(expected_templates, templates)
+
def test_getSharingPOTemplates_product_different_names_same_series(self):
# A product may be packaged into differently named packages even in
# the same distroseries. Must use different product series, though.
@@ -532,40 +589,6 @@
[self.trunk_template, self.hoary_template],
templates)
- def test_getSharingPOTemplates_package_unrelated_template_linked(self):
- # Sharing templates for a source package must not include templates
- # from sourcepackages of the same name that are linked to a different
- # product.
- other_productseries = self.factory.makeProductSeries()
- other_sourcepackage = self.factory.makeSourcePackage(
- self.packagename, self.warty)
- other_sourcepackage.setPackaging(other_productseries, self.owner)
- other_template = self.factory.makePOTemplate(
- productseries=other_productseries, name=self.templatename)
-
- sourcepackage = self.factory.makeSourcePackage(
- self.packagename, self.hoary)
- sourcepackage.setPackaging(self.trunk, self.owner)
- subset = self.potemplateset.getSharingSubset(
- distribution=self.ubuntu,
- sourcepackagename=self.packagename)
- templates = self._assertStatements(
- 1, subset.getSharingPOTemplates(self.templatename))
-
- self.assertContentEqual(
- [self.trunk_template, self.hoary_template], templates)
-
- # The behavior is controlled by the translation focus of the
- # distribution. The series in focus will be selected.
- self.ubuntu.translation_focus = self.warty
- subset = self.potemplateset.getSharingSubset(
- distribution=self.ubuntu,
- sourcepackagename=self.packagename)
- templates = self._assertStatements(
- 1, subset.getSharingPOTemplates(self.templatename))
-
- self.assertContentEqual([other_template], templates)
-
def test_getSharingPOTemplates_package_only(self):
# Sharing templates for a source package only, is done by the
# sourcepackagename.
@@ -585,29 +608,6 @@
self.assertContentEqual(
[self.hoary_template, other_template, warty_template], templates)
- def test_getSharingPOTemplates_package_one_linked(self):
- # Once one a sourcepackage in a distroseries that is neither the
- # translation focus nor the current series is linked to a product,
- # no sharing by name is possible anymore.
- self.factory.makePOTemplate(
- distroseries=self.warty, sourcepackagename=self.packagename,
- name=self.templatename)
- other_series = self.factory.makeDistroSeries(self.ubuntu)
- self.factory.makePOTemplate(
- distroseries=other_series, sourcepackagename=self.packagename,
- name=self.templatename)
- other_sourcepackage = self.factory.makeSourcePackage(
- self.packagename, other_series)
- other_sourcepackage.setPackaging(self.trunk, self.owner)
-
- subset = self.potemplateset.getSharingSubset(
- distribution=self.ubuntu,
- sourcepackagename=self.packagename)
- templates = self._assertStatements(
- 0, subset.getSharingPOTemplates(self.templatename))
-
- self.assertEqual([], templates)
-
def test_getOrCreateSharedPOTMsgSet_product(self):
# Trying to create an identical POTMsgSet in a product as exists
# in a linked sourcepackage will return the existing POTMsgset.
@@ -626,17 +626,16 @@
def test_getOrCreateSharedPOTMsgSet_package(self):
# Trying to create an identical POTMsgSet in a product as exists
# in a linked sourcepackage will return the existing POTMsgset.
- self.factory.makeSourcePackagePublishingHistory(
- sourcepackagename=self.packagename,
- distroseries=self.hoary)
- self.trunk.setPackaging(self.hoary, self.packagename, self.owner)
- hoary_potmsgset = self.factory.makePOTMsgSet(
- potemplate=self.hoary_template, sequence=1)
+ sourcepackage = self.factory.makeSourcePackage(
+ self.packagename, self.hoary)
+ sourcepackage.setPackaging(self.trunk, self.owner)
+ trunk_potmsgset = self.factory.makePOTMsgSet(
+ potemplate=self.trunk_template, sequence=1)
- trunk_potmsgset = self.trunk_template.getOrCreateSharedPOTMsgSet(
- singular_text=hoary_potmsgset.singular_text,
- plural_text=hoary_potmsgset.plural_text)
- self.assertEqual(hoary_potmsgset, trunk_potmsgset)
+ hoary_potmsgset = self.trunk_template.getOrCreateSharedPOTMsgSet(
+ singular_text=trunk_potmsgset.singular_text,
+ plural_text=trunk_potmsgset.plural_text)
+ self.assertEqual(trunk_potmsgset, hoary_potmsgset)
def test_suite():
