launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-fix-rule-ordering into lp:launchpad.

Commit message:
Canonicalise expected rule ordering in GitRepository.setRules.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1815431 in Launchpad itself: "git repository setRule() triggers Internal Server Error following an Assert"
  https://bugs.launchpad.net/launchpad/+bug/1815431

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-fix-rule-ordering/+merge/362978
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-fix-rule-ordering into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/gitrule.py'
--- lib/lp/code/interfaces/gitrule.py	2018-10-31 14:56:02 +0000
+++ lib/lp/code/interfaces/gitrule.py	2019-02-11 12:57:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2018-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Git repository access rules."""
@@ -12,6 +12,7 @@
     'IGitNascentRuleGrant',
     'IGitRule',
     'IGitRuleGrant',
+    'is_rule_exact',
     ]
 
 from lazr.restful.fields import Reference
@@ -43,6 +44,18 @@
     )
 
 
+def is_rule_exact(rule):
+    """Is this an exact-match rule?
+
+    :param rule: An `IGitRule` or `IGitNascentRule`.
+    :return: True if the rule is for an exact reference name, or False if it
+        is for a wildcard.
+    """
+    # turnip's glob_to_re only treats * as special, so any rule whose
+    # pattern does not contain * must be an exact-match rule.
+    return "*" not in rule.ref_pattern
+
+
 class IGitRuleView(Interface):
     """`IGitRule` attributes that require launchpad.View."""
 
@@ -71,12 +84,6 @@
         title=_("Date last modified"), required=True, readonly=True,
         description=_("The time when this rule was last modified."))
 
-    is_exact = Bool(
-        title=_("Is this an exact-match rule?"), required=True, readonly=True,
-        description=_(
-            "True if this rule is for an exact reference name, or False if "
-            "it is for a wildcard."))
-
     grants = Attribute("The access grants for this rule.")
 
 

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2019-02-01 14:08:34 +0000
+++ lib/lp/code/model/gitrepository.py	2019-02-11 12:57:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -123,7 +123,10 @@
     IGitRepositorySet,
     user_has_special_git_repository_access,
     )
-from lp.code.interfaces.gitrule import describe_git_permissions
+from lp.code.interfaces.gitrule import (
+    describe_git_permissions,
+    is_rule_exact,
+    )
 from lp.code.interfaces.revision import IRevisionSet
 from lp.code.mail.branch import send_git_repository_modified_notifications
 from lp.code.model.branchmergeproposal import BranchMergeProposal
@@ -1171,17 +1174,22 @@
         return Store.of(self).find(
             GitRule, GitRule.repository == self).order_by(GitRule.position)
 
+    def _canonicaliseRuleOrdering(self, rules):
+        """Canonicalise rule ordering.
+
+        Exact-match rules come first in lexicographical order, followed by
+        wildcard rules in the requested order.  (Note that `sorted` is
+        guaranteed to be stable.)
+        """
+        return sorted(rules, key=lambda rule: (
+            (0, rule.ref_pattern) if is_rule_exact(rule) else (1,)))
+
     def _syncRulePositions(self, rules):
         """Synchronise rule positions with their order in a provided list.
 
         :param rules: A sequence of `IGitRule`s in the desired order.
         """
-        # Canonicalise rule ordering: exact-match rules come first in
-        # lexicographical order, followed by wildcard rules in the requested
-        # order.  (Note that `sorted` is guaranteed to be stable.)
-        rules = sorted(
-            rules,
-            key=lambda rule: (0, rule.ref_pattern) if rule.is_exact else (1,))
+        rules = self._canonicaliseRuleOrdering(rules)
         # Ensure the correct position of all rules, which may involve more
         # work than necessary, but is simple and tends to be
         # self-correcting.  This works because the unique constraint on
@@ -1283,7 +1291,9 @@
         """See `IGitRepository`."""
         self._validateRules(rules)
         existing_rules = {rule.ref_pattern: rule for rule in self.rules}
-        new_rules = OrderedDict((rule.ref_pattern, rule) for rule in rules)
+        new_rules = OrderedDict(
+            (rule.ref_pattern, rule)
+            for rule in self._canonicaliseRuleOrdering(rules))
         GitRule.preloadGrantsForRules(existing_rules.values())
 
         # Remove old rules first so that we don't generate unnecessary move

=== modified file 'lib/lp/code/model/gitrule.py'
--- lib/lp/code/model/gitrule.py	2018-10-31 14:56:02 +0000
+++ lib/lp/code/model/gitrule.py	2019-02-11 12:57:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2018-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Git repository access rules."""
@@ -127,13 +127,6 @@
         return "<GitRule '%s' for %s>" % (
             self.ref_pattern, self.repository.unique_name)
 
-    @property
-    def is_exact(self):
-        """See `IGitRule`."""
-        # turnip's glob_to_re only treats * as special, so any rule whose
-        # pattern does not contain * must be an exact-match rule.
-        return "*" not in self.ref_pattern
-
     def toDataForJSON(self, media_type):
         """See `IJSONPublishable`."""
         if media_type != "application/json":
@@ -253,8 +246,29 @@
         removeSecurityProxy(grant).date_last_modified = UTC_NOW
 
 
+class GitRuleGrantMixin:
+    """Properties common to GitRuleGrant and GitNascentRuleGrant."""
+
+    @property
+    def permissions(self):
+        permissions = set()
+        if self.can_create:
+            permissions.add(GitPermissionType.CAN_CREATE)
+        if self.can_push:
+            permissions.add(GitPermissionType.CAN_PUSH)
+        if self.can_force_push:
+            permissions.add(GitPermissionType.CAN_FORCE_PUSH)
+        return frozenset(permissions)
+
+    @permissions.setter
+    def permissions(self, value):
+        self.can_create = GitPermissionType.CAN_CREATE in value
+        self.can_push = GitPermissionType.CAN_PUSH in value
+        self.can_force_push = GitPermissionType.CAN_FORCE_PUSH in value
+
+
 @implementer(IGitRuleGrant, IJSONPublishable)
-class GitRuleGrant(StormBase):
+class GitRuleGrant(StormBase, GitRuleGrantMixin):
     """See `IGitRuleGrant`."""
 
     __storm_table__ = 'GitRuleGrant'
@@ -326,23 +340,6 @@
             ", ".join(describe_git_permissions(self.permissions)),
             grantee_name, self.repository.unique_name, self.rule.ref_pattern)
 
-    @property
-    def permissions(self):
-        permissions = set()
-        if self.can_create:
-            permissions.add(GitPermissionType.CAN_CREATE)
-        if self.can_push:
-            permissions.add(GitPermissionType.CAN_PUSH)
-        if self.can_force_push:
-            permissions.add(GitPermissionType.CAN_FORCE_PUSH)
-        return frozenset(permissions)
-
-    @permissions.setter
-    def permissions(self, value):
-        self.can_create = GitPermissionType.CAN_CREATE in value
-        self.can_push = GitPermissionType.CAN_PUSH in value
-        self.can_force_push = GitPermissionType.CAN_FORCE_PUSH in value
-
     def toDataForJSON(self, media_type):
         """See `IJSONPublishable`."""
         if media_type != "application/json":
@@ -367,6 +364,9 @@
         self.ref_pattern = ref_pattern
         self.grants = grants
 
+    def __repr__(self):
+        return "<GitNascentRule '%s'>" % self.ref_pattern
+
 
 @adapter(dict)
 @implementer(IGitNascentRule)
@@ -375,7 +375,7 @@
 
 
 @implementer(IGitNascentRuleGrant)
-class GitNascentRuleGrant:
+class GitNascentRuleGrant(GitRuleGrantMixin):
 
     def __init__(self, grantee_type, grantee=None, can_create=False,
                  can_push=False, can_force_push=False):
@@ -385,6 +385,15 @@
         self.can_push = can_push
         self.can_force_push = can_force_push
 
+    def __repr__(self):
+        if self.grantee_type == GitGranteeType.PERSON:
+            grantee_name = "~%s" % self.grantee.name
+        else:
+            grantee_name = self.grantee_type.title.lower()
+        return "<GitNascentRuleGrant [%s] to %s>" % (
+            ", ".join(describe_git_permissions(self.permissions)),
+            grantee_name)
+
 
 @adapter(dict)
 @implementer(IGitNascentRuleGrant)

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2019-01-28 18:09:21 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2019-02-11 12:57:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for Git repositories."""
@@ -2848,6 +2848,20 @@
                 date_last_modified=Equals(date_created)),
             ]))
 
+    def test_setRules_canonicalises_expected_ordering(self):
+        repository = self.factory.makeGitRepository()
+        with person_logged_in(repository.owner):
+            repository.setRules([
+                IGitNascentRule({
+                    "ref_pattern": "refs/heads/master-next",
+                    "grants": [],
+                    }),
+                IGitNascentRule({
+                    "ref_pattern": "refs/heads/master",
+                    "grants": [],
+                    }),
+                ], repository.owner)
+
     def test_setRules_modify_grants(self):
         owner = self.factory.makeTeam()
         members = [

=== modified file 'lib/lp/code/model/tests/test_gitrule.py'
--- lib/lp/code/model/tests/test_gitrule.py	2018-10-31 14:56:02 +0000
+++ lib/lp/code/model/tests/test_gitrule.py	2019-02-11 12:57:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2018-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for Git repository access rules."""
@@ -28,6 +28,7 @@
     IGitNascentRuleGrant,
     IGitRule,
     IGitRuleGrant,
+    is_rule_exact,
     )
 from lp.services.database.sqlbase import get_transaction_timestamp
 from lp.services.webapp.snapshot import notify_modified
@@ -69,7 +70,7 @@
             "<GitRule 'refs/heads/*' for %s>" % repository.unique_name,
             repr(rule))
 
-    def test_is_exact(self):
+    def test_is_rule_exact(self):
         repository = self.factory.makeGitRepository()
         for ref_pattern, is_exact in (
                 ("refs/heads/master", True),
@@ -83,9 +84,9 @@
                 ):
             self.assertEqual(
                 is_exact,
-                self.factory.makeGitRule(
+                is_rule_exact(self.factory.makeGitRule(
                     repository=repository,
-                    ref_pattern=ref_pattern).is_exact)
+                    ref_pattern=ref_pattern)))
 
     def test_grants(self):
         rule = self.factory.makeGitRule()