launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04976
[Merge] lp:~wallyworld/launchpad/branch-transitively_private-attribute into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/branch-transitively_private-attribute into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #849850 in Launchpad itself: "Branch needs a transitively_private attribute"
https://bugs.launchpad.net/launchpad/+bug/849850
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/branch-transitively_private-attribute/+merge/75325
This branch can only be deployed after the following db-devel branches have been deployed to lp.net:
lp:~wallyworld/launchpad/branch-privacy-trigger
lp:~wallyworld/launchpad/branch-transitively-private-column
Bug 568121 requires that public branches stacked on private branches are shown as private. This mp introduces a new Branch attribute: transitively_private. The attribute is maintained by a db trigger and allows both queries and application code to use a common attribute for determining whether a branch is truly private. This branch allows some previously deployed code to recursively look at stacked on branches to populate the "private" attribute to be replaced with a delegated call to transitively_private.
== Implementation ==
Essentially: simply replace uses of Branch.private with Branch.transitively_private in Storm queries and also hand coded SQL.
Also a bit of code to ensure that when branch.setPrivate() is called, branch.transitively_private is invalidated on that instance so that it is reloaded from the db.
== Tests ==
Existing tests using private branches were altered so that some of the private branches were made transitively private.
Changed tests in:
- test_branchcollection
- test_branchlookup
- test_revision
- test_branchmergeproposal
- test_branch
Plus added tests:
TestUpdateRevisionCacheForBranch
- test_revisions_for_transitive_private_branch_marked_private
- test_existing_transitive_private_revisions_with_public_branch
TestGetByLPPath
- test_transitive_private_branch
- test_transitive_private_linked_branch
== Lint ==
Linting changed files:
lib/lp/code/model/branch.py
lib/lp/code/model/branchcollection.py
lib/lp/code/model/branchlookup.py
lib/lp/code/model/branchmergeproposal.py
lib/lp/code/model/revision.py
lib/lp/code/model/tests/test_branch.py
lib/lp/code/model/tests/test_branchcollection.py
lib/lp/code/model/tests/test_branchlookup.py
lib/lp/code/model/tests/test_branchmergeproposal.py
lib/lp/code/model/tests/test_revision.py
lib/lp/registry/model/distribution.py
lib/lp/registry/tests/test_person.py
lib/lp/testing/factory.py
./lib/lp/code/model/tests/test_branchcollection.py
759: local variable 'mp' is assigned to but never used
770: local variable 'mp' is assigned to but never used
./lib/lp/code/model/tests/test_revision.py
312: local variable 'b1' is assigned to but never used
506: local variable 'rev2' is assigned to but never used
529: local variable 'rev2' is assigned to but never used
537: local variable 'rev3' is assigned to but never used
556: local variable 'rev1' is assigned to but never used
565: local variable 'rev2' is assigned to but never used
635: local variable 'last_scanned_id' is assigned to but never used
--
https://code.launchpad.net/~wallyworld/launchpad/branch-transitively_private-attribute/+merge/75325
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/branch-transitively_private-attribute into lp:launchpad.
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2011-09-12 07:29:31 +0000
+++ lib/lp/code/model/branch.py 2011-09-14 11:51:42 +0000
@@ -175,22 +175,14 @@
# of the state of any stacked on branches.
explicitly_private = BoolCol(
default=False, notNull=True, dbName='private')
+ # A branch is transitively private if it is private or it is stacked on a
+ # transitively private branch. The value of this attribute is maintained
+ # by a database trigger.
+ transitively_private = BoolCol(dbName='transitively_private')
@property
def private(self):
- return self.explicitly_private or self._isStackedOnPrivate()
-
- def _isStackedOnPrivate(self, checked_branches=None):
- # Return True if any of this branch's stacked_on branches is private.
- is_stacked_on_private = False
- if self.stacked_on is not None:
- checked_branches = checked_branches or []
- checked_branches.append(self)
- if self.stacked_on not in checked_branches:
- is_stacked_on_private = (
- self.stacked_on.explicitly_private or
- self.stacked_on._isStackedOnPrivate(checked_branches))
- return is_stacked_on_private
+ return self.transitively_private
def setPrivate(self, private, user):
"""See `IBranch`."""
@@ -205,6 +197,12 @@
if not private and not policy.canBranchesBePublic():
raise BranchCannotBePublic()
self.explicitly_private = private
+ # If this branch is private, then it is also transitively_private
+ # otherwise we need to reload the value.
+ if private:
+ self.transitively_private = True
+ else:
+ self.transitively_private = AutoReload
registrant = ForeignKey(
dbName='registrant', foreignKey='Person',
@@ -1419,6 +1417,7 @@
naked_branch.unique_name = AutoReload
naked_branch.owner_name = AutoReload
naked_branch.target_suffix = AutoReload
+ naked_branch.transitively_private = AutoReload
def branch_modified_subscriber(branch, event):
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2011-09-13 05:23:16 +0000
+++ lib/lp/code/model/branchcollection.py 2011-09-14 11:51:42 +0000
@@ -368,7 +368,7 @@
scope_tables = [Branch] + self._tables.values()
scope_expressions = self._branch_filter_expressions
select = self.store.using(*scope_tables).find(
- (Branch.id, Branch.explicitly_private, Branch.ownerID),
+ (Branch.id, Branch.transitively_private, Branch.ownerID),
*scope_expressions)
branches_query = select._get_select()
with_expr = [With("scope_branches", branches_query)
@@ -719,7 +719,7 @@
def _getBranchVisibilityExpression(self, branch_class=Branch):
"""Return the where clauses for visibility."""
- return [branch_class.explicitly_private == False]
+ return [branch_class.transitively_private == False]
def _getCandidateBranchesWith(self):
"""Return WITH clauses defining candidate branches.
@@ -730,7 +730,8 @@
# Anonymous users get public branches only.
return [
With("candidate_branches",
- SQL("select id from scope_branches where not private"))
+ SQL("""select id from scope_branches
+ where not transitively_private"""))
]
@@ -818,7 +819,7 @@
BranchSubscription.person ==
TeamParticipation.teamID,
TeamParticipation.person == person,
- Branch.explicitly_private == True)))
+ Branch.transitively_private == True)))
return private_branches
def _getBranchVisibilityExpression(self, branch_class=Branch):
@@ -827,7 +828,7 @@
:param branch_class: The Branch class to use - permits using
ClassAliases.
"""
- public_branches = branch_class.explicitly_private == False
+ public_branches = branch_class.transitively_private == False
if self._private_branch_ids is None:
# Public only.
return [public_branches]
@@ -848,21 +849,24 @@
# Really an anonymous sitation
return [
With("candidate_branches",
- SQL("select id from scope_branches where not private"))
+ SQL("""
+ select id from scope_branches
+ where not transitively_private"""))
]
return [
With("teams", self.store.find(TeamParticipation.teamID,
TeamParticipation.personID == person.id)._get_select()),
With("private_branches", SQL("""
SELECT scope_branches.id FROM scope_branches WHERE
- scope_branches.private AND (
+ scope_branches.transitively_private AND (
(scope_branches.owner in (select team from teams) OR
EXISTS(SELECT true from BranchSubscription, teams WHERE
branchsubscription.branch = scope_branches.id AND
branchsubscription.person = teams.team)))""")),
With("candidate_branches", SQL("""
(SELECT id FROM private_branches) UNION
- (select id FROM scope_branches WHERE not private)"""))
+ (select id FROM scope_branches
+ WHERE not transitively_private)"""))
]
def visibleByUser(self, person):
=== modified file 'lib/lp/code/model/branchlookup.py'
--- lib/lp/code/model/branchlookup.py 2011-08-29 07:55:48 +0000
+++ lib/lp/code/model/branchlookup.py 2011-09-14 11:51:42 +0000
@@ -299,7 +299,7 @@
result = store.find(
(Branch.id),
Branch.id == branch_id,
- Branch.explicitly_private == False).one()
+ Branch.transitively_private == False).one()
if result is None:
return None, None
else:
@@ -316,7 +316,7 @@
result = store.find(
(Branch.id, Branch.unique_name),
Branch.unique_name.is_in(prefixes),
- Branch.explicitly_private == False).one()
+ Branch.transitively_private == False).one()
if result is None:
return None, None
else:
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2011-08-26 00:43:31 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2011-09-14 11:51:42 +0000
@@ -184,9 +184,10 @@
@property
def private(self):
return (
- self.source_branch.private or self.target_branch.private or
+ self.source_branch.transitively_private or
+ self.target_branch.transitively_private or
(self.prerequisite_branch is not None and
- self.prerequisite_branch.private))
+ self.prerequisite_branch.transitively_private))
reviewer = ForeignKey(
dbName='reviewer', foreignKey='Person',
=== modified file 'lib/lp/code/model/revision.py'
--- lib/lp/code/model/revision.py 2011-08-29 07:55:48 +0000
+++ lib/lp/code/model/revision.py 2011-09-14 11:51:42 +0000
@@ -162,7 +162,7 @@
self.id == BranchRevision.revision_id,
BranchRevision.branch_id == Branch.id)
if not allow_private:
- query = And(query, Not(Branch.explicitly_private))
+ query = And(query, Not(Branch.transitively_private))
if not allow_junk:
query = And(
query,
@@ -478,7 +478,7 @@
Revision,
And(revision_time_limit(day_limit),
person_condition,
- Not(Branch.explicitly_private)))
+ Not(Branch.transitively_private)))
result_set.config(distinct=True)
return result_set.order_by(Desc(Revision.revision_date))
@@ -497,7 +497,7 @@
]
conditions = And(revision_time_limit(day_limit),
- Not(Branch.explicitly_private))
+ Not(Branch.transitively_private))
if IProduct.providedBy(obj):
conditions = And(conditions, Branch.product == obj)
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py 2011-09-06 05:58:25 +0000
+++ lib/lp/code/model/tests/test_branch.py 2011-09-14 11:51:42 +0000
@@ -2240,6 +2240,7 @@
stacked_on = self.factory.makeBranch(private=True)
branch = self.factory.makeBranch(stacked_on=stacked_on, private=False)
self.assertTrue(branch.private)
+ self.assertTrue(removeSecurityProxy(branch).transitively_private)
self.assertFalse(branch.explicitly_private)
def test_private_stacked_on_public_is_private(self):
@@ -2247,6 +2248,7 @@
stacked_on = self.factory.makeBranch(private=False)
branch = self.factory.makeBranch(stacked_on=stacked_on, private=True)
self.assertTrue(branch.private)
+ self.assertTrue(removeSecurityProxy(branch).transitively_private)
self.assertTrue(branch.explicitly_private)
@@ -2265,6 +2267,8 @@
self.assertFalse(branch.private)
branch.setPrivate(False, branch.owner)
self.assertFalse(branch.private)
+ self.assertFalse(removeSecurityProxy(branch).transitively_private)
+ self.assertFalse(branch.explicitly_private)
def test_public_to_private_allowed(self):
# If there is a privacy policy allowing the branch owner to have
@@ -2274,6 +2278,8 @@
branch.owner, BranchVisibilityRule.PRIVATE)
branch.setPrivate(True, branch.owner)
self.assertTrue(branch.private)
+ self.assertTrue(removeSecurityProxy(branch).transitively_private)
+ self.assertTrue(branch.explicitly_private)
def test_public_to_private_not_allowed(self):
# If there are no privacy policies allowing private branches, then
@@ -2292,6 +2298,8 @@
admins = getUtility(ILaunchpadCelebrities).admin
branch.setPrivate(True, admins.teamowner)
self.assertTrue(branch.private)
+ self.assertTrue(removeSecurityProxy(branch).transitively_private)
+ self.assertTrue(branch.explicitly_private)
def test_private_to_private(self):
# Setting a private branch to be private is a no-op.
@@ -2299,6 +2307,8 @@
self.assertTrue(branch.private)
branch.setPrivate(True, branch.owner)
self.assertTrue(branch.private)
+ self.assertTrue(removeSecurityProxy(branch).transitively_private)
+ self.assertTrue(branch.explicitly_private)
def test_private_to_public_allowed(self):
# If the namespace policy allows public branches, then changing from
@@ -2306,6 +2316,8 @@
branch = self.factory.makeProductBranch(private=True)
branch.setPrivate(False, branch.owner)
self.assertFalse(branch.private)
+ self.assertFalse(removeSecurityProxy(branch).transitively_private)
+ self.assertFalse(branch.explicitly_private)
def test_private_to_public_not_allowed(self):
# If the namespace policy does not allow public branches, attempting
=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py 2011-08-30 19:25:53 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py 2011-09-14 11:51:42 +0000
@@ -529,8 +529,14 @@
TestCaseWithFactory.setUp(self)
remove_all_sample_data_branches()
self.public_branch = self.factory.makeAnyBranch(name='public')
+ # We make private branch by stacking a public branch on top of a
+ # private one.
+ self.private_stacked_on_branch = self.factory.makeAnyBranch(
+ private=True)
+ self.public_stacked_on_branch = self.factory.makeAnyBranch(
+ stacked_on=self.private_stacked_on_branch)
self.private_branch1 = self.factory.makeAnyBranch(
- private=True, name='private1')
+ stacked_on=self.public_stacked_on_branch, name='private1')
self.private_branch2 = self.factory.makeAnyBranch(
private=True, name='private2')
self.all_branches = getUtility(IAllBranches)
@@ -540,7 +546,8 @@
# collection.
self.assertEqual(
sorted([self.public_branch, self.private_branch1,
- self.private_branch2]),
+ self.private_branch2, self.public_stacked_on_branch,
+ self.private_stacked_on_branch]),
sorted(self.all_branches.getBranches()))
def test_anonymous_sees_only_public(self):
=== modified file 'lib/lp/code/model/tests/test_branchlookup.py'
--- lib/lp/code/model/tests/test_branchlookup.py 2011-08-12 11:37:08 +0000
+++ lib/lp/code/model/tests/test_branchlookup.py 2011-09-14 11:51:42 +0000
@@ -141,6 +141,18 @@
result = self.branch_set.getIdAndTrailingPath(path)
self.assertEqual((None, None), result)
+ def test_branch_id_alias_transitive_private(self):
+ # Transitively private branches are not found at all
+ # (this is for anonymous access)
+ owner = self.factory.makePerson()
+ private_branch = self.factory.makeAnyBranch(
+ owner=owner, private=True)
+ branch = self.factory.makeAnyBranch(stacked_on=private_branch)
+ with person_logged_in(owner):
+ path = branch_id_alias(branch)
+ result = self.branch_set.getIdAndTrailingPath(path)
+ self.assertEqual((None, None), result)
+
def test_branch_id_alias_public(self):
# Public branches can be accessed.
branch = self.factory.makeAnyBranch()
@@ -539,6 +551,15 @@
self.assertRaises(
NoSuchBranch, self.branch_lookup.getByLPPath, path)
+ def test_transitive_private_branch(self):
+ # If the unique name refers to an invisible branch, getByLPPath raises
+ # NoSuchBranch, just as if the branch weren't there at all.
+ private_branch = self.factory.makeAnyBranch(private=True)
+ branch = self.factory.makeAnyBranch(stacked_on=private_branch)
+ path = removeSecurityProxy(branch).unique_name
+ self.assertRaises(
+ NoSuchBranch, self.branch_lookup.getByLPPath, path)
+
def test_resolve_product_branch_unique_name(self):
# getByLPPath returns the branch, no trailing path and no series if
# given the unique name of an existing product branch.
@@ -626,6 +647,17 @@
self.assertRaises(
NoLinkedBranch, self.branch_lookup.getByLPPath, product.name)
+ def test_transitive_private_linked_branch(self):
+ # If the given path refers to an object with an invisible linked
+ # branch, then getByLPPath raises `NoLinkedBranch`, as if the branch
+ # weren't there at all.
+ private_branch = self.factory.makeProductBranch(private=True)
+ branch = self.factory.makeProductBranch(stacked_on=private_branch)
+ product = removeSecurityProxy(branch).product
+ removeSecurityProxy(product).development_focus.branch = branch
+ self.assertRaises(
+ NoLinkedBranch, self.branch_lookup.getByLPPath, product.name)
+
def test_no_official_branch(self):
sourcepackage = self.factory.makeSourcePackage()
exception = self.assertRaises(
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2011-08-29 07:55:48 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2011-09-14 11:51:42 +0000
@@ -1043,9 +1043,11 @@
product = getUtility(IProductSet).getByName(product_name)
if product is None:
product = self.factory.makeProduct(name=product_name)
+ stacked_on_branch = self.factory.makeProductBranch(
+ product=product, owner=owner, registrant=registrant)
branch = self.factory.makeProductBranch(
product=product, owner=owner, registrant=registrant,
- name=branch_name)
+ name=branch_name, stacked_on=stacked_on_branch)
if registrant is None:
registrant = owner
bmp = branch.addLandingTarget(
@@ -1202,8 +1204,10 @@
proposal = self._make_merge_proposal(
'xray', 'november', 'work', registrant=albert)
- # Mark the source branch private.
- removeSecurityProxy(proposal.source_branch).explicitly_private = True
+ # Mark the source branch private by making it's stacked on branch
+ # private.
+ removeSecurityProxy(
+ proposal.source_branch.stacked_on).explicitly_private = True
november = getUtility(IProductSet).getByName('november')
# The proposal is visible to charles.
=== modified file 'lib/lp/code/model/tests/test_revision.py'
--- lib/lp/code/model/tests/test_revision.py 2011-08-29 07:55:48 +0000
+++ lib/lp/code/model/tests/test_revision.py 2011-09-14 11:51:42 +0000
@@ -821,6 +821,18 @@
self.assertTrue(branch.private)
self.assertTrue(cached.private)
+ def test_revisions_for_transitive_private_branch_marked_private(self):
+ # If the branch is stacked on a private branch, then the revisions in
+ # the cache will be marked private too.
+ private_branch = self.factory.makeAnyBranch(private=True)
+ branch = self.factory.makeAnyBranch(stacked_on=private_branch)
+ revision = self.factory.makeRevision()
+ branch.createBranchRevision(1, revision)
+ RevisionSet.updateRevisionCacheForBranch(branch)
+ [cached] = self._getRevisionCache()
+ self.assertTrue(branch.private)
+ self.assertTrue(cached.private)
+
def test_product_branch_revisions(self):
# The revision cache knows the product for revisions in product
# branches.
@@ -897,6 +909,26 @@
# But the privacy flags are different.
self.assertNotEqual(rev1.private, rev2.private)
+ def test_existing_transitive_private_revisions_with_public_branch(self):
+ # If a revision is in both public and private branches, there is a
+ # revision cache row for both public and private. A branch is private
+ # if it is stacked on a private branch.
+ stacked_on_branch = self.factory.makeAnyBranch(private=True)
+ private_branch = self.factory.makeAnyBranch(
+ stacked_on=stacked_on_branch)
+ public_branch = self.factory.makeAnyBranch(private=False)
+ revision = self.factory.makeRevision()
+ private_branch.createBranchRevision(1, revision)
+ RevisionSet.updateRevisionCacheForBranch(private_branch)
+ public_branch.createBranchRevision(1, revision)
+ RevisionSet.updateRevisionCacheForBranch(public_branch)
+ [rev1, rev2] = self._getRevisionCache()
+ # Both revisions point to the same underlying revision.
+ self.assertEqual(rev1.revision, revision)
+ self.assertEqual(rev2.revision, revision)
+ # But the privacy flags are different.
+ self.assertNotEqual(rev1.private, rev2.private)
+
class TestPruneRevisionCache(RevisionCacheTestCase):
"""Tests for RevisionSet.pruneRevisionCache."""
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py 2011-08-29 00:13:22 +0000
+++ lib/lp/registry/model/distribution.py 2011-09-14 11:51:42 +0000
@@ -655,7 +655,7 @@
Branch.last_scanned_id,
SPBDS.name AS distro_series_name,
Branch.id,
- Branch.private,
+ Branch.transitively_private,
Branch.owner
FROM Branch
JOIN DistroSeries
@@ -675,7 +675,7 @@
# Now we see just a touch of privacy concerns.
# If the current user is anonymous, they cannot see any private
# branches.
- base_query += (' AND NOT Branch.private\n')
+ base_query += (' AND NOT Branch.transitively_private\n')
# We want to order the results, in part for easier grouping at the
# end.
base_query += 'ORDER BY unique_name, last_scanned_id'
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2011-08-11 20:37:16 +0000
+++ lib/lp/registry/tests/test_person.py 2011-09-14 11:51:42 +0000
@@ -941,7 +941,7 @@
duplicate, mergee = self._do_merge(duplicate, mergee)
branches = [b.name for b in mergee.getBranches()]
self.assertEqual(2, len(branches))
- self.assertEqual([u'foo', u'foo-1'], branches)
+ self.assertContentEqual([u'foo', u'foo-1'], branches)
def test_merge_moves_recipes(self):
# When person/teams are merged, recipes owned by the from person are
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2011-09-13 22:43:21 +0000
+++ lib/lp/testing/factory.py 2011-09-14 11:51:42 +0000
@@ -1125,6 +1125,7 @@
url=url, **optional_branch_args)
if private:
removeSecurityProxy(branch).explicitly_private = True
+ removeSecurityProxy(branch).transitively_private = True
if stacked_on is not None:
removeSecurityProxy(branch).stacked_on = stacked_on
if reviewer is not None:
Follow ups
