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
=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py 2010-08-02 02:13:52 +0000
+++ lib/lp/translations/model/translationimportqueue.py 2010-08-06 14:07:48 +0000
@@ -863,35 +863,42 @@
863863
864 return entries[0]864 return entries[0]
865865
866 def addOrUpdateEntry(self, path, content, is_published, importer,866 def _getFormatAndImporter(self, filename, content, format=None):
867 sourcepackagename=None, distroseries=None, productseries=None,867 """Get the appropriate format and importer for this upload.
868 potemplate=None, pofile=None, format=None):868
869 """See ITranslationImportQueue."""869 :param filename: Name of the uploaded file.
870 if ((sourcepackagename is not None or distroseries is not None) and870 :param content: Contents of the uploaded file.
871 productseries is not None):871 :param format: Optional hard choice of format. If none is
872 raise AssertionError(872 given, a format will be divined from the file's name and
873 'The productseries argument cannot be not None if'873 contents.
874 ' sourcepackagename or distroseries is also not None.')874 :return: a tuple of the selected format and its importer.
875 if (sourcepackagename is None and distroseries is None and875 """
876 productseries is None):
877 raise AssertionError('Any of sourcepackagename, distroseries or'
878 ' productseries must be not None.')
879
880 if content is None or content == '':
881 raise AssertionError('The content cannot be empty')
882
883 if path is None or path == '':
884 raise AssertionError('The path cannot be empty')
885
886 filename = os.path.basename(path)
887 root, ext = os.path.splitext(filename)
888 translation_importer = getUtility(ITranslationImporter)
889 if format is None:876 if format is None:
890 # Get it based on the file extension and file content.877 root, ext = os.path.splitext(filename)
878 translation_importer = getUtility(ITranslationImporter)
891 format = translation_importer.getTranslationFileFormat(879 format = translation_importer.getTranslationFileFormat(
892 ext, content)880 ext, content)
893 format_importer = translation_importer.getTranslationFormatImporter(881
894 format)882 translation_importer = getUtility(ITranslationImporter)
883 return (
884 format, translation_importer.getTranslationFormatImporter(format))
885
886 def addOrUpdateEntry(self, path, content, is_published, importer,
887 sourcepackagename=None, distroseries=None,
888 productseries=None, potemplate=None, pofile=None,
889 format=None):
890 """See `ITranslationImportQueue`."""
891 assert (distroseries is None) != (productseries is None), (
892 "An upload must be for a productseries or a distroseries. "
893 "This one has either neither or both.")
894 assert productseries is None or sourcepackagename is None, (
895 "Can't upload to a sourcepackagename in a productseries.")
896 assert content is not None and content != '', "Upload has no content."
897 assert path is not None and path != '', "Upload has no path."
898
899 filename = os.path.basename(path)
900 format, format_importer = self._getFormatAndImporter(
901 filename, content, format=format)
895902
896 # Upload the file into librarian.903 # Upload the file into librarian.
897 size = len(content)904 size = len(content)
@@ -906,6 +913,7 @@
906 pofile, sourcepackagename, distroseries, productseries)913 pofile, sourcepackagename, distroseries, productseries)
907 except TranslationImportQueueConflictError:914 except TranslationImportQueueConflictError:
908 return None915 return None
916
909 if entry is None:917 if entry is None:
910 # It's a new row.918 # It's a new row.
911 entry = TranslationImportQueueEntry(path=path, content=alias,919 entry = TranslationImportQueueEntry(path=path, content=alias,
@@ -1157,52 +1165,65 @@
11571165
1158 return distroseriess + products1166 return distroseriess + products
11591167
1168 def _attemptToSet(self, entry, potemplate=None, pofile=None):
1169 """Set potemplate or pofile on a `TranslationImportQueueEntry`.
1170
1171 This will do nothing if setting potemplate or pofile would clash
1172 with another entry.
1173 """
1174 if potemplate == entry.potemplate and pofile == entry.pofile:
1175 # Nothing to do here.
1176 return False
1177
1178 existing_entry = self._getMatchingEntry(
1179 entry.path, entry.importer, potemplate, pofile,
1180 entry.sourcepackagename, entry.distroseries, entry.productseries)
1181
1182 if existing_entry is None:
1183 entry.potemplate = potemplate
1184 entry.pofile = pofile
1185
1186 def _attemptToApprove(self, entry):
1187 """Attempt to approve one queue entry."""
1188 if entry.status != RosettaImportStatus.NEEDS_REVIEW:
1189 return False
1190
1191 if entry.import_into is None:
1192 # We don't have a place to import this entry. Try to guess it.
1193 importer = getUtility(ITranslationImporter)
1194 if importer.isTranslationName(entry.path):
1195 potemplate = entry.potemplate
1196 pofile = entry.getGuessedPOFile()
1197 else:
1198 # It's a template.
1199 # Check if we can guess where it should be imported.
1200 potemplate = entry.guessed_potemplate
1201 pofile = entry.pofile
1202
1203 self._attemptToSet(entry, potemplate=potemplate, pofile=pofile)
1204
1205 if entry.import_into is None:
1206 # Still no dice.
1207 return False
1208
1209 # Yay! We have a POTemplate or POFile to import this entry
1210 # into. Approve.
1211 entry.setStatus(RosettaImportStatus.APPROVED,
1212 getUtility(ILaunchpadCelebrities).rosetta_experts)
1213
1214 return True
1215
1160 def executeOptimisticApprovals(self, txn=None):1216 def executeOptimisticApprovals(self, txn=None):
1161 """See ITranslationImportQueue."""1217 """See `ITranslationImportQueue`."""
1162 there_are_entries_approved = False1218 approved_entries = False
1163 importer = getUtility(ITranslationImporter)
1164 for entry in self._iterNeedsReview():1219 for entry in self._iterNeedsReview():
1165 if entry.import_into is None:1220 success = self._attemptToApprove(entry)
1166 # We don't have a place to import this entry. Try to guess it.1221 if success:
1167 if importer.isTranslationName(entry.path):1222 approved_entries = True
1168 # Check if we can guess where it should be imported.
1169 guess = entry.getGuessedPOFile()
1170 if guess is None:
1171 # We were not able to guess a place to import it,
1172 # leave the status of this entry as
1173 # RosettaImportStatus.NEEDS_REVIEW and wait for an
1174 # admin to manually review it.
1175 continue
1176 # Set the place where it should be imported.
1177 entry.pofile = guess
1178
1179 else:
1180 # It's a template.
1181 # Check if we can guess where it should be imported.
1182 guess = entry.guessed_potemplate
1183 if guess is None:
1184 # We were not able to guess a place to import it,
1185 # leave the status of this entry as
1186 # RosettaImportStatus.NEEDS_REVIEW and wait for an
1187 # admin to manually review it.
1188 continue
1189 # Set the place where it should be imported.
1190 entry.potemplate = guess
1191
1192 assert not entry.import_into is None
1193
1194 if entry.status != RosettaImportStatus.APPROVED:
1195 there_are_entries_approved = True
1196
1197 # Already know where it should be imported. The entry is approved
1198 # automatically.
1199 entry.setStatus(RosettaImportStatus.APPROVED,
1200 getUtility(ILaunchpadCelebrities).rosetta_experts)
1201
1202 if txn is not None:1223 if txn is not None:
1203 txn.commit()1224 txn.commit()
12041225
1205 return there_are_entries_approved1226 return approved_entries
12061227
1207 def _getSlaveStore(self):1228 def _getSlaveStore(self):
1208 """Return the slave store for the import queue.1229 """Return the slave store for the import queue.
12091230
=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
--- lib/lp/translations/tests/test_autoapproval.py 2010-07-25 23:29:15 +0000
+++ lib/lp/translations/tests/test_autoapproval.py 2010-08-06 14:07:48 +0000
@@ -587,6 +587,37 @@
587587
588 self.assertEqual(template, entry.guessed_potemplate)588 self.assertEqual(template, entry.guessed_potemplate)
589589
590 def test_avoid_clash_with_existing_entry(self):
591 # When trying to approve a template upload that didn't have its
592 # potemplate field set during upload or an earlier approval run,
593 # the approver will fill out the field if it can. But if by
594 # then there's already another entry from the same person and
595 # for the same target that does have the field set, then filling
596 # out the field would make the two entries clash.
597 queue = TranslationImportQueue()
598 template = self.factory.makePOTemplate()
599 old_entry = queue.addOrUpdateEntry(
600 template.path, '# Content here', False, template.owner,
601 productseries=template.productseries)
602 new_entry = queue.addOrUpdateEntry(
603 template.path, '# Content here', False, template.owner,
604 productseries=template.productseries, potemplate=template)
605
606 # Before approval, the two entries differ in that the new one
607 # has a potemplate.
608 self.assertNotEqual(old_entry, new_entry)
609 self.assertEqual(RosettaImportStatus.NEEDS_REVIEW, old_entry.status)
610 self.assertIs(None, old_entry.potemplate)
611 self.assertEqual(template, new_entry.potemplate)
612 IMasterStore(old_entry).flush()
613
614 # The approver deals with the problem by skipping the entry.
615 queue._attemptToApprove(old_entry)
616
617 # So nothing changes.
618 self.assertIs(None, old_entry.potemplate)
619 self.assertEqual(template, new_entry.potemplate)
620
590621
591class TestKdePOFileGuess(TestCaseWithFactory, GardenerDbUserMixin):622class TestKdePOFileGuess(TestCaseWithFactory, GardenerDbUserMixin):
592 """Test auto-approval's `POFile` guessing for KDE uploads.623 """Test auto-approval's `POFile` guessing for KDE uploads.