Launchpad itself

Merge lp:~jtv/launchpad/bug-599254 into lp:launchpad/db-devel

  1. bug-599254
  2. Merge into db-devel
Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: no longer in the source branch.
Merged at revision: 9502
Proposed branch: lp:~jtv/launchpad/bug-599254
Merge into: lp:launchpad/db-devel
Diff against target: 90 lines (+21/-15)
2 files modified
lib/lp/translations/model/potmsgset.py (+8/-1)
lib/lp/translations/model/translationmessage.py (+13/-14)
To merge this branch: bzr merge lp:~jtv/launchpad/bug-599254
Reviewer Review Type Date Requested Status
Māris Fogels (community) rc Approve
Henning Eggers (community) code Approve
Review via email: mp+28618@code.launchpad.net

Commit message

Ensure defined flushing order when making TranslationMessages current.

Description of the change

= Bug 599254 =

When translating a given message in a given template to a given language, only one TranslationMessage can have the is_current flag set and only one can have the is_imported flag set. The database ensures this through unique constraints.

In the current code (which we are working to replace entirely) we have validator functions that keep us within the bounds of those constraints. Whenever we set a TM's is_current to True, the corresponding validator first finds the message that previously had the flag set, if any, and clears the flag there. Same for the is_imported flag. They also tell the ORM that the change clearing the old flag needs to be flushed to the database before the change setting the new one.

These validators only look for translations that are for the same template. This is not as simple as it seems: with message sharing, most messages will have their template set to None which means they're shared across templates. Things quickly get confusing when changes to these flags are combined with changes to the TranslationMessage.potemplate field. Bug 599254 involves violations of the unique constraints (i.e, oopses) as the ORM flushes changes in an inconvenient order.

In this branch I'm making a minimal change to define flush order in cases where we were already clearing flags but not running the affected TranslationMessages through the validators. There are no functional changes. It looks probable that re-ordering the changes to TranslationMessages could also have solved the problem, but that would involve a risk of unintended functional changes. Our "recife" project is going to replace the entire cluster of methods that triggers the problem, as well as the validators. Not a good time for finicky unpredictable tweaks.

In the validators I did have to add an extra check against defining flush order between a TranslationMessage and itself. Some of the tests triggered such an error at one point.

I found myself unable to come up with a sensible test for this that doesn't take a lot of work or duplicate lots of other tests. We already have tons of tests covering the methods involved, and those all pass with my change.

To test, run all translations tests but in particular:
{{{
./bin/test -vv -t potmsgset -t translationmessage
}}}

There was one piece of lint in code I haven't touched: an unnecessary assignment. I'm leaving it intact to avoid conflicts with the ongoing Recife work that will touch the same code.

Jeroen

To post a comment you must log in.
Revision history for this message
Henning Eggers (henninge) wrote :

Thanks for the quick fix.

57 + Store.of(self).add_flush_order(
58 + current_translation_message, self)

These two fit on one line, I think.

Cheers,
Henning

review: Approve (code)
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Indeed it does. Thanks for the review!

Revision history for this message
Henning Eggers (henninge) wrote :

Hi Maris,
could you please review this for an r-c? It fixes an oops that would hinder translators in their daily work.

Cheers,
Henning

Revision history for this message
Māris Fogels (mars) wrote :

Great work, nice explanation of the problem and solution. release-critical=mars

review: Approve
Revision history for this message
Māris Fogels (mars) :
review: Approve (rc)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py 2010-03-22 17:18:28 +0000
+++ lib/lp/translations/model/potmsgset.py 2010-06-28 12:21:37 +0000
@@ -668,6 +668,9 @@
668 imported_message.is_imported = False668 imported_message.is_imported = False
669 imported_message.is_current = False669 imported_message.is_current = False
670 imported_message.potemplate = None670 imported_message.potemplate = None
671 if imported_message != new_message:
672 Store.of(imported_message).add_flush_order(
673 imported_message, new_message)
671 if not (force_diverged or force_shared):674 if not (force_diverged or force_shared):
672 # If there was an imported message, keep the same675 # If there was an imported message, keep the same
673 # divergence/shared state unless something was forced.676 # divergence/shared state unless something was forced.
@@ -682,10 +685,14 @@
682 # Change actual is_current flag only if it validates ok.685 # Change actual is_current flag only if it validates ok.
683 if new_message.validation_status == TranslationValidationStatus.OK:686 if new_message.validation_status == TranslationValidationStatus.OK:
684 if make_current:687 if make_current:
685 # Deactivate previous diverged message.
686 if (current_message is not None and688 if (current_message is not None and
687 current_message.potemplate is not None):689 current_message.potemplate is not None):
690 # Deactivate previous diverged message.
688 current_message.is_current = False691 current_message.is_current = False
692 if current_message != new_message:
693 Store.of(current_message).add_flush_order(
694 current_message, new_message)
695
689 # Do not "converge" a diverged imported message since696 # Do not "converge" a diverged imported message since
690 # there might be another shared imported message.697 # there might be another shared imported message.
691 if not current_message.is_imported:698 if not current_message.is_imported:
692699
=== modified file 'lib/lp/translations/model/translationmessage.py'
--- lib/lp/translations/model/translationmessage.py 2010-03-12 15:13:14 +0000
+++ lib/lp/translations/model/translationmessage.py 2010-06-28 12:21:37 +0000
@@ -167,23 +167,23 @@
167 assert value is not None, 'is_current field cannot be None.'167 assert value is not None, 'is_current field cannot be None.'
168168
169 if value and not self.is_current:169 if value and not self.is_current:
170 # We are setting this message as the current one. We need to170 # We are setting this message as the current one.
171 # change current one to non current before.
172 current_translation_message = (171 current_translation_message = (
173 self.potmsgset.getCurrentTranslationMessage(172 self.potmsgset.getCurrentTranslationMessage(
174 self.potemplate,173 self.potemplate,
175 self.language, self.variant))174 self.language, self.variant))
176 if (current_translation_message is not None and175 if (current_translation_message is not None and
176 current_translation_message != self and
177 current_translation_message.potemplate == self.potemplate):177 current_translation_message.potemplate == self.potemplate):
178 # Clear flag on the previous current message.
178 current_translation_message.is_current = False179 current_translation_message.is_current = False
179 # We need to flush the old current message before the180 # Flush changes in the right order so we don't get two
180 # new one because the database constraints prevent two181 # current messages in the same place.
181 # current messages.182 Store.of(self).add_flush_order(current_translation_message, self)
182 Store.of(self).add_flush_order(current_translation_message,
183 self)
184183
185 return value184 return value
186185
186
187def validate_is_imported(self, attr, value):187def validate_is_imported(self, attr, value):
188 """Unset current imported message before setting this as imported.188 """Unset current imported message before setting this as imported.
189189
@@ -195,20 +195,19 @@
195 assert value is not None, 'is_imported field cannot be None.'195 assert value is not None, 'is_imported field cannot be None.'
196196
197 if value and not self.is_imported:197 if value and not self.is_imported:
198 # We are setting this message as the current one. We need to198 # We are setting this message as the imported one.
199 # change current one to non current before.
200 imported_translation_message = (199 imported_translation_message = (
201 self.potmsgset.getImportedTranslationMessage(200 self.potmsgset.getImportedTranslationMessage(
202 self.potemplate,201 self.potemplate,
203 self.language, self.variant))202 self.language, self.variant))
204 if (imported_translation_message is not None and203 if (imported_translation_message is not None and
204 imported_translation_message != self and
205 imported_translation_message.potemplate == self.potemplate):205 imported_translation_message.potemplate == self.potemplate):
206 # Clear flag on the previous imported message.
206 imported_translation_message.is_imported = False207 imported_translation_message.is_imported = False
207 # We need to flush the old imported message before the208 # Flush changes in the right order so we don't get two
208 # new one because the database constraints prevent two209 # current messages in the same place.
209 # imported messages.210 Store.of(self).add_flush_order(imported_translation_message, self)
210 Store.of(self).add_flush_order(imported_translation_message,
211 self)
212211
213 return value212 return value
214213

Subscribers

People subscribed via source and target branches

to status/vote changes: