Launchpad itself

Code review comment for lp:~henninge/launchpad/bug-545354-enable-sharing

  1. bug-545354-enable-sharing
  2. Merge into recife
Revision history for this message
Henning Eggers (henninge) wrote :

Am 16.06.2010 23:24, schrieb Edwin Grubbs:
> This looks good. I just have a few comments below.

Thanks for the review. I explained one away and fixed two. ;)

>> === modified file 'lib/lp/translations/model/potemplate.py'
>> --- lib/lp/translations/model/potemplate.py 2010-01-12 21:29:03 +0000
>> +++ lib/lp/translations/model/potemplate.py 2010-06-16 19:25:05 +0000
>> @@ -184,6 +184,21 @@
>>
>> _uses_english_msgids = None
>>
>> + @cachedproperty
>> + def _sharing_ids(self):
>> + """Return the IDs of all sharing templates including this one."""
>> + subset = getUtility(IPOTemplateSet).getSharingSubset(
>> + product=self.product,
>> + distribution=self.distribution,
>> + sourcepackagename=self.sourcepackagename)
>> + # Convert to a list for caching.
>> + result = list(subset.getSharingPOTemplateIDs(self.name))
>> + if len(result) == 0:
>> + # Always return at least the template itself.
>> + return [self.id]
>> + else:
>> + return result
>
>
> Should you also make sure that self.id is in result even if len != 0?

No. Actually, the len == 0 case is basically a "no sharing possible case" and
with a single id in the list no sharing happens. If we get any ids back, those
are the sharing ones and we can trust them to include all we need.

[...]

>> def groupEquivalentPOTemplates(self, name_pattern=None): """See IPOTemplateSharingSubset.""" equivalents = {}
>>
>> - for template in self._iterate_potemplates(name_pattern):
>> + if name_pattern is None:
>> + templatename_clause = "1=1"
>> + else:
>> + templatename_clause = "potemplate.name ~ '%s'" % name_pattern
>
>
> I think you should either escape the name_pattern with sqlvalues,
> or you should convert it to a storm conditional.

Yes, thank you. I fixed that by using sqlvalues. Also I replaced "1=1" with
True since we are still in storm land here. I also pulled this out into
another variant of getSharingPOTemplates and added tests for it.

>
>
>> +
>> + for template in self._queryPOTemplates(templatename_clause):
>> key = self._get_potemplate_equivalence_class(template)
>> if key not in equivalents:
>> equivalents[key] = []
>> === modified file 'lib/lp/translations/tests/test_shared_potemplate.py'
>> --- lib/lp/translations/tests/test_shared_potemplate.py 2010-05-18 14:00:10 +0000
>> +++ lib/lp/translations/tests/test_shared_potemplate.py 2010-06-16 19:25:05 +0000
>> @@ -177,5 +176,398 @@
>> self.assertEquals(potmsgset, shared_potmsgset)
>>
>>
>> +class TestMessageSharingProductPackage(TestCaseWithFactory):
>> + """Test message sharing between a product and a package.
>> +
>> + Each test uses assertStatementCount to make sure the number of SQL
>> + queries does not change. This was integrated here to avoid having
>> + a second test case just for statement counts.
>> + The current implementation is good and only needs one statement."""
>
>
> Ending triple-quotes belongs by itself on the following line, since
> this is a multiline docstring.
>

True, thanks.

Changes pushed, incremental diff attached.

Henning

inc.diff
1=== modified file 'lib/lp/translations/interfaces/potemplate.py'
2--- lib/lp/translations/interfaces/potemplate.py 2010-06-09 07:36:35 +0000
3+++ lib/lp/translations/interfaces/potemplate.py 2010-06-18 10:16:50 +0000
4@@ -695,6 +695,17 @@
5 :return: A list of all potemplates of the same name from all series.
6 """
7
8+ def getSharingPOTemplatesRegex(name_pattern=None):
9+ """Find all sharing templates with names matching the given pattern.
10+
11+ If name_pattern is None, match is performed on the template name.
12+ Use with care as it may return all templates in a distribution!
13+
14+ :param name_pattern: A POSIX regular expression that the template
15+ is matched against.
16+ :return: A list of all potemplates matching the pattern.
17+ """
18+
19 def getSharingPOTemplateIDs(potemplate_name):
20 """Find database ids of all sharing templates of the given name.
21
22
23=== modified file 'lib/lp/translations/model/potemplate.py'
24--- lib/lp/translations/model/potemplate.py 2010-06-16 16:31:06 +0000
25+++ lib/lp/translations/model/potemplate.py 2010-06-18 11:05:58 +0000
26@@ -1506,6 +1506,16 @@
27 return self._queryPOTemplates(
28 POTemplate.name == potemplate_name)
29
30+ def getSharingPOTemplatesRegex(self, name_pattern=None):
31+ """See IPOTemplateSharingSubset."""
32+ if name_pattern is None:
33+ templatename_clause = True
34+ else:
35+ templatename_clause = (
36+ "potemplate.name ~ %s" % sqlvalues(name_pattern))
37+
38+ return self._queryPOTemplates(templatename_clause)
39+
40 def getSharingPOTemplateIDs(self, potemplate_name):
41 """See IPOTemplateSharingSubset."""
42 return self.getSharingPOTemplates(potemplate_name).values(
43@@ -1515,12 +1525,7 @@
44 """See IPOTemplateSharingSubset."""
45 equivalents = {}
46
47- if name_pattern is None:
48- templatename_clause = "1=1"
49- else:
50- templatename_clause = "potemplate.name ~ '%s'" % name_pattern
51-
52- for template in self._queryPOTemplates(templatename_clause):
53+ for template in self.getSharingPOTemplatesRegex(name_pattern):
54 key = self._get_potemplate_equivalence_class(template)
55 if key not in equivalents:
56 equivalents[key] = []
57
58=== modified file 'lib/lp/translations/tests/test_shared_potemplate.py'
59--- lib/lp/translations/tests/test_shared_potemplate.py 2010-06-09 09:16:54 +0000
60+++ lib/lp/translations/tests/test_shared_potemplate.py 2010-06-18 11:15:54 +0000
61@@ -7,6 +7,7 @@
62
63 import unittest
64
65+from storm.exceptions import DataError
66 from zope.component import getUtility
67 from zope.security.proxy import removeSecurityProxy
68
69@@ -176,13 +177,69 @@
70 self.assertEquals(potmsgset, shared_potmsgset)
71
72
73+class TestSharingPOTemplatesRegex(TestCaseWithFactory):
74+ """Isolate tests for regular expression use in SharingSubset."""
75+
76+ layer = ZopelessDatabaseLayer
77+
78+ def setUp(self):
79+ super(TestSharingPOTemplatesRegex, self).setUp()
80+ self.product = self.factory.makeProduct()
81+ self.product.official_rosetta = True
82+ self.trunk = self.product.getSeries('trunk')
83+ self.potemplateset = getUtility(IPOTemplateSet)
84+
85+ def _makeTemplates(self, names):
86+ # Create some templates with the given names.
87+ return [
88+ self.factory.makePOTemplate(productseries=self.trunk, name=name)
89+ for name in names]
90+
91+ def test_getSharingPOTemplatesRegex_baseline(self):
92+ # Baseline test.
93+ templates = self._makeTemplates(['foo', 'foo-bar', 'foo-two'])
94+ subset = self.potemplateset.getSharingSubset(product=self.product)
95+ self.assertContentEqual(
96+ templates, subset.getSharingPOTemplatesRegex('foo.*'))
97+
98+ def test_getSharingPOTemplatesRegex_not_all(self):
99+ # A template may not match.
100+ templates = self._makeTemplates(['foo', 'foo-bar', 'foo-two'])
101+ subset = self.potemplateset.getSharingSubset(product=self.product)
102+ self.assertContentEqual(
103+ templates[1:], subset.getSharingPOTemplatesRegex('foo-.*'))
104+
105+ def test_getSharingPOTemplatesRegex_all(self):
106+ # Not passing a pattern returns all templates.
107+ templates = self._makeTemplates(['foo', 'foo-bar', 'foo-two'])
108+ subset = self.potemplateset.getSharingSubset(product=self.product)
109+ self.assertContentEqual(
110+ templates, subset.getSharingPOTemplatesRegex())
111+
112+ def test_getSharingPOTemplatesRegex_robustness_quotes(self):
113+ # Quotes in the pattern can be dangerous.
114+ subset = self.potemplateset.getSharingSubset(product=self.product)
115+ self.assertContentEqual(
116+ [], subset.getSharingPOTemplatesRegex("'\""))
117+
118+ def test_getSharingPOTemplatesRegex_robustness_backslash(self):
119+ # A backslash at the end could escape enclosing quotes without
120+ # proper escaping, leading to a SyntaxError or even a successful
121+ # exploit. Instead, storm should complain about an invalid expression
122+ # by raising DataError.
123+ subset = self.potemplateset.getSharingSubset(product=self.product)
124+ self.assertRaises(
125+ DataError, list, subset.getSharingPOTemplatesRegex("foo.*\\"))
126+
127+
128 class TestMessageSharingProductPackage(TestCaseWithFactory):
129 """Test message sharing between a product and a package.
130
131 Each test uses assertStatementCount to make sure the number of SQL
132 queries does not change. This was integrated here to avoid having
133 a second test case just for statement counts.
134- The current implementation is good and only needs one statement."""
135+ The current implementation is good and only needs one statement.
136+ """
137
138 layer = ZopelessDatabaseLayer
139

« Back to merge proposal