Launchpad itself

Merge lp:~jtv/launchpad/bug-613821 into lp:launchpad

  1. bug-613821
  2. Merge into devel
Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Данило Шеган
Approved revision: no longer in the source branch.
Merged at revision: 11312
Proposed branch: lp:~jtv/launchpad/bug-613821
Merge into: lp:launchpad
Diff against target: 229 lines (+119/-67)
2 files modified
lib/lp/translations/model/translationimportqueue.py (+88/-67)
lib/lp/translations/tests/test_autoapproval.py (+31/-0)
To merge this branch: bzr merge lp:~jtv/launchpad/bug-613821
Reviewer Review Type Date Requested Status
Данило Шеган (community) code Approve
Review via email: mp+31957@code.launchpad.net

Commit message

Check for potential translation approval conflicts before hitting the DB constraint.

Description of the change

= Bug 613821 =

This fixes a complicated bug where sometimes two template uploads on the translation import queue can clash in a way that breaks auto-approval. You'll find the scenario described in the test. A unique constraint gets violated.

I did some small cleanups along the way. I separated the big approval loop from the loop body which tries to approve an entry, which makes it a bit easier to test approval of a specific entry. I also extracted the part that looks up a format and its importer, just so that it'd stop distracting me while reading the surrounding code.

The check as it is now is a bit over-careful for the current form of the database constraint: it checks both when setting an entry's pofile and when setting an entry's potemplate. That's not actually necessary for the pofile field, since it's not part of the constraint. If there's a clash there, it occurs during upload. But checking in both cases makes the higher-level code simpler and leaves us freer to change the constraint later if we like. It does add one query to POFile approval, but not to the most performance-sensitive path (which is where we try to approve a file but fail).

I also sanitized the ordering of the approval method a bit. Depending on the details of the ORM and/or database language binding, and how result-set batching may interact with transaction boundaries, it may be possible to query all needs-review entries but actually get some that are already approved. Instead of checking at the very end whether an entry might already have been approved, I now do that check up-front where it doesn't look so crazy.

Test:
{{{
./bin/test -vvc -m lp.translations.tests.test_autoapproval
}}}

To Q/A, run the updated approver on staging and see that it doesn't barf. The production glitch that this fixes has been around long enough that it should be in the staging database prior to release.

Jeroen

To post a comment you must log in.
Revision history for this message
Данило Шеган (danilo) wrote :

<danilos> jtv, did addOrUpdateEntry signature change? (or is it just whitespaces?)
<jtv> danilos: whitespace.
<danilos> jtv, cool
<jtv> I promise.
* danilos rechecks
<danilos> jtv, does _getMatchingEntry return only an already approved entry? (i.e. it will not return "itself")
<jtv> danilos: it's only called when looking for the more specific match that the approver's pofile/potemplate guess will produce.
<jtv> So it won't produce the entry that you're already looking at: you'd be looking for an entry with the same pofile/potemplate, and in that case the caller shortcuts.
<danilos> jtv, it'd be nice to return an explicit False in _attemptToApprove when entry.status != NEEDS_REVIEW
<jtv> danilos: whoops, sorry :)
<danilos> jtv, also, your comment about the 'checking at the very end if it was already approved' confused me a bit :)
<danilos> jtv, old code was not even considering that problem, it was checking if approval succeeded instead
<danilos> jtv, basically what you do with a check on the returned value from _attemptToApprove
<danilos> jtv, anyway, looks good other than that minor point above, r=danilo
<jtv> danilos: thanks. I meant the part where it checked at the end whether the entry's status was APPROVED. Not too sane. :-)
 danilos: could you land it for me?
<danilos> jtv, right, but that's your "if success" check, roughly the same thing

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/translations/model/translationimportqueue.py'
2--- lib/lp/translations/model/translationimportqueue.py 2010-08-02 02:13:52 +0000
3+++ lib/lp/translations/model/translationimportqueue.py 2010-08-06 14:07:48 +0000
4@@ -863,35 +863,42 @@
5
6 return entries[0]
7
8- def addOrUpdateEntry(self, path, content, is_published, importer,
9- sourcepackagename=None, distroseries=None, productseries=None,
10- potemplate=None, pofile=None, format=None):
11- """See ITranslationImportQueue."""
12- if ((sourcepackagename is not None or distroseries is not None) and
13- productseries is not None):
14- raise AssertionError(
15- 'The productseries argument cannot be not None if'
16- ' sourcepackagename or distroseries is also not None.')
17- if (sourcepackagename is None and distroseries is None and
18- productseries is None):
19- raise AssertionError('Any of sourcepackagename, distroseries or'
20- ' productseries must be not None.')
21-
22- if content is None or content == '':
23- raise AssertionError('The content cannot be empty')
24-
25- if path is None or path == '':
26- raise AssertionError('The path cannot be empty')
27-
28- filename = os.path.basename(path)
29- root, ext = os.path.splitext(filename)
30- translation_importer = getUtility(ITranslationImporter)
31+ def _getFormatAndImporter(self, filename, content, format=None):
32+ """Get the appropriate format and importer for this upload.
33+
34+ :param filename: Name of the uploaded file.
35+ :param content: Contents of the uploaded file.
36+ :param format: Optional hard choice of format. If none is
37+ given, a format will be divined from the file's name and
38+ contents.
39+ :return: a tuple of the selected format and its importer.
40+ """
41 if format is None:
42- # Get it based on the file extension and file content.
43+ root, ext = os.path.splitext(filename)
44+ translation_importer = getUtility(ITranslationImporter)
45 format = translation_importer.getTranslationFileFormat(
46 ext, content)
47- format_importer = translation_importer.getTranslationFormatImporter(
48- format)
49+
50+ translation_importer = getUtility(ITranslationImporter)
51+ return (
52+ format, translation_importer.getTranslationFormatImporter(format))
53+
54+ def addOrUpdateEntry(self, path, content, is_published, importer,
55+ sourcepackagename=None, distroseries=None,
56+ productseries=None, potemplate=None, pofile=None,
57+ format=None):
58+ """See `ITranslationImportQueue`."""
59+ assert (distroseries is None) != (productseries is None), (
60+ "An upload must be for a productseries or a distroseries. "
61+ "This one has either neither or both.")
62+ assert productseries is None or sourcepackagename is None, (
63+ "Can't upload to a sourcepackagename in a productseries.")
64+ assert content is not None and content != '', "Upload has no content."
65+ assert path is not None and path != '', "Upload has no path."
66+
67+ filename = os.path.basename(path)
68+ format, format_importer = self._getFormatAndImporter(
69+ filename, content, format=format)
70
71 # Upload the file into librarian.
72 size = len(content)
73@@ -906,6 +913,7 @@
74 pofile, sourcepackagename, distroseries, productseries)
75 except TranslationImportQueueConflictError:
76 return None
77+
78 if entry is None:
79 # It's a new row.
80 entry = TranslationImportQueueEntry(path=path, content=alias,
81@@ -1157,52 +1165,65 @@
82
83 return distroseriess + products
84
85+ def _attemptToSet(self, entry, potemplate=None, pofile=None):
86+ """Set potemplate or pofile on a `TranslationImportQueueEntry`.
87+
88+ This will do nothing if setting potemplate or pofile would clash
89+ with another entry.
90+ """
91+ if potemplate == entry.potemplate and pofile == entry.pofile:
92+ # Nothing to do here.
93+ return False
94+
95+ existing_entry = self._getMatchingEntry(
96+ entry.path, entry.importer, potemplate, pofile,
97+ entry.sourcepackagename, entry.distroseries, entry.productseries)
98+
99+ if existing_entry is None:
100+ entry.potemplate = potemplate
101+ entry.pofile = pofile
102+
103+ def _attemptToApprove(self, entry):
104+ """Attempt to approve one queue entry."""
105+ if entry.status != RosettaImportStatus.NEEDS_REVIEW:
106+ return False
107+
108+ if entry.import_into is None:
109+ # We don't have a place to import this entry. Try to guess it.
110+ importer = getUtility(ITranslationImporter)
111+ if importer.isTranslationName(entry.path):
112+ potemplate = entry.potemplate
113+ pofile = entry.getGuessedPOFile()
114+ else:
115+ # It's a template.
116+ # Check if we can guess where it should be imported.
117+ potemplate = entry.guessed_potemplate
118+ pofile = entry.pofile
119+
120+ self._attemptToSet(entry, potemplate=potemplate, pofile=pofile)
121+
122+ if entry.import_into is None:
123+ # Still no dice.
124+ return False
125+
126+ # Yay! We have a POTemplate or POFile to import this entry
127+ # into. Approve.
128+ entry.setStatus(RosettaImportStatus.APPROVED,
129+ getUtility(ILaunchpadCelebrities).rosetta_experts)
130+
131+ return True
132+
133 def executeOptimisticApprovals(self, txn=None):
134- """See ITranslationImportQueue."""
135- there_are_entries_approved = False
136- importer = getUtility(ITranslationImporter)
137+ """See `ITranslationImportQueue`."""
138+ approved_entries = False
139 for entry in self._iterNeedsReview():
140- if entry.import_into is None:
141- # We don't have a place to import this entry. Try to guess it.
142- if importer.isTranslationName(entry.path):
143- # Check if we can guess where it should be imported.
144- guess = entry.getGuessedPOFile()
145- if guess is None:
146- # We were not able to guess a place to import it,
147- # leave the status of this entry as
148- # RosettaImportStatus.NEEDS_REVIEW and wait for an
149- # admin to manually review it.
150- continue
151- # Set the place where it should be imported.
152- entry.pofile = guess
153-
154- else:
155- # It's a template.
156- # Check if we can guess where it should be imported.
157- guess = entry.guessed_potemplate
158- if guess is None:
159- # We were not able to guess a place to import it,
160- # leave the status of this entry as
161- # RosettaImportStatus.NEEDS_REVIEW and wait for an
162- # admin to manually review it.
163- continue
164- # Set the place where it should be imported.
165- entry.potemplate = guess
166-
167- assert not entry.import_into is None
168-
169- if entry.status != RosettaImportStatus.APPROVED:
170- there_are_entries_approved = True
171-
172- # Already know where it should be imported. The entry is approved
173- # automatically.
174- entry.setStatus(RosettaImportStatus.APPROVED,
175- getUtility(ILaunchpadCelebrities).rosetta_experts)
176-
177+ success = self._attemptToApprove(entry)
178+ if success:
179+ approved_entries = True
180 if txn is not None:
181 txn.commit()
182
183- return there_are_entries_approved
184+ return approved_entries
185
186 def _getSlaveStore(self):
187 """Return the slave store for the import queue.
188
189=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
190--- lib/lp/translations/tests/test_autoapproval.py 2010-07-25 23:29:15 +0000
191+++ lib/lp/translations/tests/test_autoapproval.py 2010-08-06 14:07:48 +0000
192@@ -587,6 +587,37 @@
193
194 self.assertEqual(template, entry.guessed_potemplate)
195
196+ def test_avoid_clash_with_existing_entry(self):
197+ # When trying to approve a template upload that didn't have its
198+ # potemplate field set during upload or an earlier approval run,
199+ # the approver will fill out the field if it can. But if by
200+ # then there's already another entry from the same person and
201+ # for the same target that does have the field set, then filling
202+ # out the field would make the two entries clash.
203+ queue = TranslationImportQueue()
204+ template = self.factory.makePOTemplate()
205+ old_entry = queue.addOrUpdateEntry(
206+ template.path, '# Content here', False, template.owner,
207+ productseries=template.productseries)
208+ new_entry = queue.addOrUpdateEntry(
209+ template.path, '# Content here', False, template.owner,
210+ productseries=template.productseries, potemplate=template)
211+
212+ # Before approval, the two entries differ in that the new one
213+ # has a potemplate.
214+ self.assertNotEqual(old_entry, new_entry)
215+ self.assertEqual(RosettaImportStatus.NEEDS_REVIEW, old_entry.status)
216+ self.assertIs(None, old_entry.potemplate)
217+ self.assertEqual(template, new_entry.potemplate)
218+ IMasterStore(old_entry).flush()
219+
220+ # The approver deals with the problem by skipping the entry.
221+ queue._attemptToApprove(old_entry)
222+
223+ # So nothing changes.
224+ self.assertIs(None, old_entry.potemplate)
225+ self.assertEqual(template, new_entry.potemplate)
226+
227
228 class TestKdePOFileGuess(TestCaseWithFactory, GardenerDbUserMixin):
229 """Test auto-approval's `POFile` guessing for KDE uploads.