launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-734573 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #734573 in Launchpad itself: "setting two features rules for the same flag with the same sequence oopses"
  https://bugs.launchpad.net/launchpad/+bug/734573

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-734573/+merge/53572

The rules for a feature flag must form a totally ordered set. This is enforced by the database, resulting in an IntegrityError when a duplicate priority is encountered. This branch fixes it to raise a less unpleasant form validation error.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-734573/+merge/53572
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-734573 into lp:launchpad.

=== modified file 'lib/lp/services/features/browser/edit.py'
--- lib/lp/services/features/browser/edit.py	2011-02-22 17:53:15 +0000
+++ lib/lp/services/features/browser/edit.py	2011-03-16 06:21:46 +0000
@@ -25,6 +25,7 @@
     )
 from lp.app.browser.stringformatter import FormattersAPI
 from lp.services.features.changelog import ChangeLog
+from lp.services.features.rulesource import DuplicatePriorityError
 
 
 class IFeatureControlForm(Interface):
@@ -113,5 +114,5 @@
             # Unfortunately if the field is '', zope leaves it out of data.
             self.request.features.rule_source.parseRules(
                 data.get('feature_rules') or '')
-        except (IndexError, TypeError, ValueError), e:
+        except (IndexError, TypeError, ValueError, DuplicatePriorityError), e:
             self.setFieldError('feature_rules', 'Invalid rule syntax: %s' % e)

=== modified file 'lib/lp/services/features/browser/tests/test_feature_editor.py'
--- lib/lp/services/features/browser/tests/test_feature_editor.py	2011-02-22 17:53:15 +0000
+++ lib/lp/services/features/browser/tests/test_feature_editor.py	2011-03-16 06:21:46 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+from textwrap import dedent
 
 from testtools.matchers import Equals
 from zope.component import getUtility
@@ -205,3 +206,19 @@
         # The action is not available to unauthorized users.
         view = FeatureControlView(None, None)
         self.assertFalse(view.change_action.available())
+
+    def test_error_for_duplicate_priority(self):
+        """Duplicate priority values for a flag result in a nice error."""
+        browser = self.getUserBrowserAsAdmin()
+        browser.open(self.getFeatureRulesEditURL())
+        textarea = browser.getControl(name="field.feature_rules")
+        textarea.value = dedent("""\
+            key foo 10 foo
+            key bar 10 bar
+            """)
+        browser.getControl(name="field.comment").value = 'comment'
+        browser.getControl(name="field.actions.change").click()
+        self.assertThat(
+            browser.contents,
+            Contains(
+                'Invalid rule syntax: duplicate priority for flag "key": 10'))

=== modified file 'lib/lp/services/features/rulesource.py'
--- lib/lp/services/features/rulesource.py	2011-02-09 05:41:36 +0000
+++ lib/lp/services/features/rulesource.py	2011-03-16 06:21:46 +0000
@@ -12,7 +12,10 @@
 __metaclass__ = type
 
 import re
-from collections import namedtuple
+from collections import (
+    defaultdict,
+    namedtuple,
+    )
 
 from storm.locals import Desc
 
@@ -27,6 +30,17 @@
 Rule = namedtuple("Rule", "flag scope priority value")
 
 
+class DuplicatePriorityError(Exception):
+
+    def __init__(self, flag, priority):
+        self.flag = flag
+        self.priority = priority
+
+    def __str__(self):
+        return 'duplicate priority for flag "%s": %d' % (
+            self.flag, self.priority)
+
+
 class FeatureRuleSource(object):
     """Access feature rule sources from the database or elsewhere."""
 
@@ -78,11 +92,17 @@
         errors immediately.
         """
         r = []
+        seen_priorities = defaultdict(set)
         for line in text_form.splitlines():
             if line.strip() == '':
                 continue
             flag, scope, priority_str, value = re.split('[ \t]+', line, 3)
-            r.append((flag, scope, int(priority_str), unicode(value)))
+            priority = int(priority_str)
+            r.append((flag, scope, priority, unicode(value)))
+            if priority in seen_priorities[flag]:
+                raise DuplicatePriorityError(flag, priority)
+            seen_priorities[flag].add(priority)
+
         return r
 
 
@@ -103,7 +123,7 @@
             # have no rules).
             adapter.get_request_remaining_seconds()
         except adapter.RequestExpired:
-            return 
+            return
         store = getFeatureStore()
         rs = (store
                 .find(FeatureFlag)