launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14947
[Merge] lp:~stevenk/launchpad/use-specification-aag into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/use-specification-aag into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1067616 in Launchpad itself: "Blueprints could use a denormalized schema for improved performance"
https://bugs.launchpad.net/launchpad/+bug/1067616
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/use-specification-aag/+merge/143630
Destroy visible_specification_query, and write get_specification_privacy_filter as a replacement, which backs onto the denormalized columns Specification.access_{policy,grants}.
--
https://code.launchpad.net/~stevenk/launchpad/use-specification-aag/+merge/143630
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/use-specification-aag into lp:launchpad.
=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py 2012-12-26 01:04:05 +0000
+++ lib/lp/blueprints/model/specification.py 2013-01-17 04:07:29 +0000
@@ -1,9 +1,10 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
__all__ = [
'get_specification_filters',
+ 'get_specification_privacy_filter',
'HasSpecificationsMixin',
'recursive_blocked_query',
'recursive_dependent_query',
@@ -12,7 +13,6 @@
'SPECIFICATION_POLICY_DEFAULT_TYPES',
'SpecificationSet',
'spec_started_clause',
- 'visible_specification_query',
]
from lazr.lifecycle.event import (
@@ -30,6 +30,7 @@
)
from storm.expr import (
And,
+ Coalesce,
In,
Join,
LeftJoin,
@@ -111,7 +112,12 @@
SQLBase,
sqlvalues,
)
-from lp.services.database.stormexpr import fti_search
+from lp.services.database.stormexpr import (
+ Array,
+ ArrayAgg,
+ ArrayIntersects,
+ fti_search,
+ )
from lp.services.mail.helpers import get_contact_email_addresses
from lp.services.propertycache import (
cachedproperty,
@@ -1058,7 +1064,6 @@
method.
:return: A DecoratedResultSet with Person precaching setup.
"""
- # Circular import.
if isinstance(clauses, basestring):
clauses = [SQL(clauses)]
@@ -1157,39 +1162,32 @@
prejoin_people=True):
store = IStore(Specification)
- # Take the visibility due to privacy into account.
- privacy_tables, clauses = visible_specification_query(user)
-
if not filter:
# Default to showing incomplete specs
filter = [SpecificationFilter.INCOMPLETE]
- spec_clauses = get_specification_filters(filter)
- clauses.extend(spec_clauses)
+ tables, clauses = get_specification_privacy_filter(user)
+ clauses.extend(get_specification_filters(filter))
- # sort by priority descending, by default
+ # Sort by priority descending, by default.
if sort is None or sort == SpecificationSort.PRIORITY:
- order = [Desc(Specification.priority),
- Specification.definition_status,
- Specification.name]
-
+ order = [
+ Desc(Specification.priority), Specification.definition_status,
+ Specification.name]
elif sort == SpecificationSort.DATE:
if SpecificationFilter.COMPLETE in filter:
- # if we are showing completed, we care about date completed
- order = [Desc(Specification.date_completed),
- Specification.id]
+ # If we are showing completed, we care about date completed.
+ order = [Desc(Specification.date_completed), Specification.id]
else:
- # if not specially looking for complete, we care about date
- # registered
+ # If not specially looking for complete, we care about date
+ # registered.
order = [Desc(Specification.datecreated), Specification.id]
if prejoin_people:
- results = self._preload_specifications_people(
- privacy_tables, clauses)
+ results = self._preload_specifications_people(tables, clauses)
else:
- results = store.using(*privacy_tables).find(
- Specification, *clauses)
- return results.order_by(*order)[:quantity]
+ results = store.using(*tables).find(Specification, *clauses)
+ return results.order_by(*order).config(limit=quantity)
def getByURL(self, url):
"""See ISpecificationSet."""
@@ -1266,46 +1264,45 @@
return Specification.get(spec_id)
-def visible_specification_query(user):
- """Return a Storm expression and list of tables for filtering
- specifications by privacy.
-
- :param user: A Person ID or a column reference.
- :return: A tuple of tables, clauses to filter out specifications that the
- user cannot see.
- """
+def get_specification_privacy_filter(user):
+ # Circular imports.
+ from lp.registry.model.accesspolicy import AccessPolicyGrant
from lp.registry.model.product import Product
- from lp.registry.model.accesspolicy import (
- AccessArtifact,
- AccessPolicy,
- AccessPolicyGrantFlat,
- )
tables = [
- Specification,
- LeftJoin(Product, Specification.productID == Product.id),
- LeftJoin(AccessPolicy, And(
- Or(Specification.productID == AccessPolicy.product_id,
- Specification.distributionID ==
- AccessPolicy.distribution_id),
- Specification.information_type == AccessPolicy.type)),
- LeftJoin(AccessPolicyGrantFlat,
- AccessPolicy.id == AccessPolicyGrantFlat.policy_id),
- LeftJoin(
- TeamParticipation,
- And(AccessPolicyGrantFlat.grantee == TeamParticipation.teamID,
- TeamParticipation.person == user)),
- LeftJoin(AccessArtifact,
- AccessPolicyGrantFlat.abstract_artifact_id ==
- AccessArtifact.id)
- ]
- clauses = [
- Or(Specification.information_type.is_in(PUBLIC_INFORMATION_TYPES),
- And(AccessPolicyGrantFlat.id != None,
- TeamParticipation.personID != None,
- Or(AccessPolicyGrantFlat.abstract_artifact == None,
- AccessArtifact.specification_id == Specification.id))),
- Or(Specification.product == None, Product.active == True)]
- return tables, clauses
+ Specification, LeftJoin(
+ Product, Specification.productID == Product.id)]
+ active_products = (
+ Or(Specification.product == None, Product.active == True))
+ public_spec_filter = (
+ Specification.information_type.is_in(PUBLIC_INFORMATION_TYPES))
+
+ if user is None:
+ return tables, [active_products, public_spec_filter]
+
+ artifact_grant_query = Coalesce(
+ ArrayIntersects(
+ SQL('Specification.access_grants'),
+ Select(
+ ArrayAgg(TeamParticipation.teamID),
+ tables=TeamParticipation,
+ where=(TeamParticipation.person == user)
+ )), False)
+
+ policy_grant_query = Coalesce(
+ ArrayIntersects(
+ Array(SQL('Specification.access_policy')),
+ Select(
+ ArrayAgg(AccessPolicyGrant.policy_id),
+ tables=(AccessPolicyGrant,
+ Join(TeamParticipation,
+ TeamParticipation.teamID ==
+ AccessPolicyGrant.grantee_id)),
+ where=(TeamParticipation.person == user)
+ )), False)
+
+ return tables, [
+ active_products, Or(public_spec_filter, artifact_grant_query,
+ policy_grant_query)]
def get_specification_filters(filter):
=== modified file 'lib/lp/blueprints/model/sprint.py'
--- lib/lp/blueprints/model/sprint.py 2013-01-07 02:40:55 +0000
+++ lib/lp/blueprints/model/sprint.py 2013-01-17 04:07:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -39,8 +39,8 @@
)
from lp.blueprints.model.specification import (
get_specification_filters,
+ get_specification_privacy_filter,
HasSpecificationsMixin,
- visible_specification_query,
)
from lp.blueprints.model.sprintattendance import SprintAttendance
from lp.blueprints.model.sprintspecification import SprintSpecification
@@ -118,14 +118,14 @@
specifications() method because we want to reuse this query in the
specificationLinks() method.
"""
- # import here to avoid circular deps
+ # Avoid circular imports.
from lp.blueprints.model.specification import Specification
- tables, query = visible_specification_query(user)
+ tables, query = get_specification_privacy_filter(user)
tables.append(Join(
SprintSpecification,
- SprintSpecification.specification == Specification.id
- ))
- query.extend([SprintSpecification.sprintID == self.id])
+ SprintSpecification.specification == Specification.id))
+ query.append(SprintSpecification.sprintID == self.id)
+
if not filter:
# filter could be None or [] then we decide the default
# which for a sprint is to show everything approved
=== modified file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py 2012-12-26 01:04:05 +0000
+++ lib/lp/blueprints/tests/test_specification.py 2013-01-17 04:07:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Unit tests for Specification."""
@@ -42,8 +42,8 @@
from lp.blueprints.errors import TargetAlreadyHasSpecification
from lp.blueprints.interfaces.specification import ISpecificationSet
from lp.blueprints.model.specification import (
+ get_specification_privacy_filter,
Specification,
- visible_specification_query,
)
from lp.registry.enums import (
SharingPermission,
@@ -407,48 +407,44 @@
specification.target.owner, specification,
error_expected=False, attribute='name', value='foo')
- def test_visible_specification_query(self):
- # visible_specification_query returns a Storm expression
- # that can be used to filter specifications by their visibility-
+ def _fetch_specs_visible_for_user(self, user):
+ tables, query = get_specification_privacy_filter(user)
+ return Store.of(self.product).using(*tables).find(
+ Specification,
+ Specification.productID == self.product.id, *query)
+
+ def test_get_specification_privacy_filter(self):
+ # get_specification_privacy_filter returns a Storm expression
+ # that can be used to filter specifications by their visibility.
owner = self.factory.makePerson()
- product = self.factory.makeProduct(
+ self.product = self.factory.makeProduct(
owner=owner,
specification_sharing_policy=(
SpecificationSharingPolicy.PUBLIC_OR_PROPRIETARY))
- public_spec = self.factory.makeSpecification(product=product)
+ public_spec = self.factory.makeSpecification(product=self.product)
proprietary_spec_1 = self.factory.makeSpecification(
- product=product, information_type=InformationType.PROPRIETARY)
+ product=self.product, information_type=InformationType.PROPRIETARY)
proprietary_spec_2 = self.factory.makeSpecification(
- product=product, information_type=InformationType.PROPRIETARY)
+ product=self.product, information_type=InformationType.PROPRIETARY)
all_specs = [
public_spec, proprietary_spec_1, proprietary_spec_2]
- store = Store.of(product)
- tables, query = visible_specification_query(None)
- specs_for_anon = store.using(*tables).find(
- Specification,
- Specification.productID == product.id, *query)
- self.assertContentEqual([public_spec],
- specs_for_anon.config(distinct=True))
+ specs_for_anon = self._fetch_specs_visible_for_user(None)
+ self.assertContentEqual(
+ [public_spec], specs_for_anon.config(distinct=True))
# Product owners havae grants on the product, the privacy
# filter returns thus all specifications for them.
- tables, query = visible_specification_query(owner.id)
- specs_for_owner = store.using(*tables).find(
- Specification, Specification.productID == product.id, *query)
+ specs_for_owner = self._fetch_specs_visible_for_user(owner)
self.assertContentEqual(all_specs, specs_for_owner)
# The filter returns only public specs for ordinary users.
user = self.factory.makePerson()
- tables, query = visible_specification_query(user.id)
- specs_for_other_user = store.using(*tables).find(
- Specification, Specification.productID == product.id, *query)
+ specs_for_other_user = self._fetch_specs_visible_for_user(user)
self.assertContentEqual([public_spec], specs_for_other_user)
# If the user has a grant for a specification, the filter returns
# this specification too.
with person_logged_in(owner):
getUtility(IService, 'sharing').ensureAccessGrants(
[user], owner, specifications=[proprietary_spec_1])
- tables, query = visible_specification_query(user.id)
- specs_for_other_user = store.using(*tables).find(
- Specification, Specification.productID == product.id, *query)
+ specs_for_other_user = self._fetch_specs_visible_for_user(user)
self.assertContentEqual(
[public_spec, proprietary_spec_1], specs_for_other_user)
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py 2012-11-15 20:54:45 +0000
+++ lib/lp/registry/model/distribution.py 2013-01-17 04:07:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Database classes for implementing distribution items."""
@@ -952,15 +952,13 @@
constraint)
if prejoin_people:
- results = self._preload_specifications_people([Specification],
- query)
+ results = self._preload_specifications_people(
+ [Specification], query)
else:
- results = Store.of(self).find(
- Specification,
- SQL(query))
+ results = Store.of(self).find(Specification, SQL(query))
results.order_by(order)
if quantity is not None:
- results = results[:quantity]
+ results.config(limit=quantity)
return results
def getSpecification(self, name):
=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py 2013-01-07 02:40:55 +0000
+++ lib/lp/registry/model/milestone.py 2013-01-17 04:07:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Milestone model classes."""
@@ -39,8 +39,8 @@
from lp.app.enums import InformationType
from lp.app.errors import NotFoundError
from lp.blueprints.model.specification import (
+ get_specification_privacy_filter,
Specification,
- visible_specification_query,
)
from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
from lp.bugs.interfaces.bugsummary import IBugSummaryDimension
@@ -155,14 +155,11 @@
def getSpecifications(self, user):
"""See `IMilestoneData`"""
from lp.registry.model.person import Person
- store = Store.of(self.target)
- origin, clauses = visible_specification_query(user)
- origin.extend([
- LeftJoin(Person, Specification._assigneeID == Person.id),
- ])
+ origin, clauses = get_specification_privacy_filter(user)
+ origin.append(LeftJoin(Person, Specification._assigneeID == Person.id))
milestones = self._milestone_ids_expr(user)
- results = store.using(*origin).find(
+ results = Store.of(self.target).using(*origin).find(
(Specification, Person),
Specification.id.is_in(
Union(
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2013-01-14 06:13:52 +0000
+++ lib/lp/registry/model/person.py 2013-01-17 04:07:29 +0000
@@ -131,10 +131,10 @@
)
from lp.blueprints.model.specification import (
get_specification_filters,
+ get_specification_privacy_filter,
HasSpecificationsMixin,
spec_started_clause,
Specification,
- visible_specification_query,
)
from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
from lp.bugs.interfaces.bugtarget import IBugTarget
@@ -879,7 +879,7 @@
Select(SpecificationSubscription.specificationID,
[SpecificationSubscription.person == self]
)))
- tables, clauses = visible_specification_query(user)
+ tables, clauses = get_specification_privacy_filter(user)
clauses.append(Or(*role_clauses))
# Defaults for completeness: if nothing is said about completeness
# then we want to show INCOMPLETE.
@@ -901,7 +901,7 @@
results = results.order_by(sort)
results.config(distinct=True)
if quantity is not None:
- results = results[:quantity]
+ results.config(limit=quantity)
return results
# XXX: Tom Berger 2008-04-14 bug=191799:
@@ -1482,20 +1482,19 @@
from lp.registry.model.distribution import Distribution
store = Store.of(self)
WorkItem = SpecificationWorkItem
- origin, query = visible_specification_query(user)
+ origin, query = get_specification_privacy_filter(user)
origin.extend([
Join(WorkItem, WorkItem.specification == Specification.id),
# WorkItems may not have a milestone and in that case they inherit
# the one from the spec.
Join(Milestone,
Coalesce(WorkItem.milestone_id,
- Specification.milestoneID) == Milestone.id),
- ])
+ Specification.milestoneID) == Milestone.id)])
today = datetime.today().date()
query.extend([
Milestone.dateexpected <= date, Milestone.dateexpected >= today,
WorkItem.deleted == False,
- OR(WorkItem.assignee_id.is_in(self.participant_ids),
+ Or(WorkItem.assignee_id.is_in(self.participant_ids),
Specification._assigneeID.is_in(self.participant_ids))])
result = store.using(*origin).find(WorkItem, *query)
result.config(distinct=True)
@@ -1680,6 +1679,12 @@
requester=reviewer)
return (status_changed, tm.status)
+ def _accept_or_decline_membership(self, team, status, comment):
+ tm = TeamMembership.selectOneBy(person=self, team=team)
+ assert tm is not None
+ assert tm.status == TeamMembershipStatus.INVITED
+ tm.setStatus(status, getUtility(ILaunchBag).user, comment=comment)
+
# The three methods below are not in the IPerson interface because we want
# to protect them with a launchpad.Edit permission. We could do that by
# defining explicit permissions for all IPerson methods/attributes in
@@ -1691,12 +1696,8 @@
the INVITED status. The status of this TeamMembership will be changed
to APPROVED.
"""
- tm = TeamMembership.selectOneBy(person=self, team=team)
- assert tm is not None
- assert tm.status == TeamMembershipStatus.INVITED
- tm.setStatus(
- TeamMembershipStatus.APPROVED, getUtility(ILaunchBag).user,
- comment=comment)
+ self._accept_or_decline_membership(
+ team, TeamMembershipStatus.APPROVED, comment)
def declineInvitationToBeMemberOf(self, team, comment):
"""Decline an invitation to become a member of the given team.
@@ -1705,12 +1706,8 @@
the INVITED status. The status of this TeamMembership will be changed
to INVITATION_DECLINED.
"""
- tm = TeamMembership.selectOneBy(person=self, team=team)
- assert tm is not None
- assert tm.status == TeamMembershipStatus.INVITED
- tm.setStatus(
- TeamMembershipStatus.INVITATION_DECLINED,
- getUtility(ILaunchBag).user, comment=comment)
+ self._accept_or_decline_membership(
+ team, TeamMembershipStatus.INVITATION_DECLINED, comment)
def retractTeamMembership(self, team, user, comment=None):
"""See `IPerson`"""
@@ -1765,14 +1762,11 @@
def getOwnedTeams(self, user=None):
"""See `IPerson`."""
query = And(
- get_person_visibility_terms(user),
- Person.teamowner == self.id,
+ get_person_visibility_terms(user), Person.teamowner == self.id,
Person.merged == None)
- store = IStore(Person)
- results = store.find(
+ return IStore(Person).find(
Person, query).order_by(
Upper(Person.displayname), Upper(Person.name))
- return results
@cachedproperty
def administrated_teams(self):
=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py 2013-01-03 05:00:59 +0000
+++ lib/lp/registry/model/product.py 2013-01-17 04:07:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Database classes including and related to Product."""
@@ -92,11 +92,11 @@
from lp.blueprints.enums import SpecificationFilter
from lp.blueprints.model.specification import (
get_specification_filters,
+ get_specification_privacy_filter,
HasSpecificationsMixin,
Specification,
SPECIFICATION_POLICY_ALLOWED_TYPES,
SPECIFICATION_POLICY_DEFAULT_TYPES,
- visible_specification_query,
)
from lp.blueprints.model.sprint import HasSprintsMixin
from lp.bugs.interfaces.bugsummary import IBugSummaryDimension
@@ -1469,14 +1469,14 @@
# - completeness.
# - informational.
#
- tables, clauses = visible_specification_query(user)
+ tables, clauses = get_specification_privacy_filter(user)
clauses.append(Specification.product == self)
clauses.extend(get_specification_filters(filter))
if prejoin_people:
results = self._preload_specifications_people(tables, clauses)
else:
- tableset = Store.of(self).using(*tables)
- results = tableset.find(Specification, *clauses)
+ results = Store.of(self).using(*tables).find(
+ Specification, *clauses)
results.order_by(order).config(distinct=True)
if quantity is not None:
results = results[:quantity]
=== modified file 'lib/lp/registry/model/sharingjob.py'
--- lib/lp/registry/model/sharingjob.py 2012-11-16 20:30:12 +0000
+++ lib/lp/registry/model/sharingjob.py 2013-01-17 04:07:29 +0000
@@ -1,7 +1,6 @@
-# Copyright 2012 Canonical Ltd. This software is licensed under the
+# Copyright 2012-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
-
"""Job classes related to the sharing feature are in here."""
__metaclass__ = type
@@ -44,8 +43,8 @@
from lp.app.enums import InformationType
from lp.blueprints.interfaces.specification import ISpecification
from lp.blueprints.model.specification import (
+ get_specification_privacy_filter,
Specification,
- visible_specification_query,
)
from lp.blueprints.model.specificationsubscription import (
SpecificationSubscription,
@@ -439,14 +438,17 @@
sub.branch.unsubscribe(
sub.person, self.requestor, ignore_permissions=True)
if specification_filters:
- specification_filters.append(
- spec_not_visible(SpecificationSubscription.personID))
+ _, privacy_query = get_specification_privacy_filter(
+ SpecificationSubscription.personID)
+ specification_filters.append(Not(Or(*privacy_query)))
tables = (
SpecificationSubscription,
Join(
Specification,
Specification.id ==
- SpecificationSubscription.specificationID))
+ SpecificationSubscription.specificationID),
+ Join(
+ Product, Product.id == Specification.productID))
specifications_subscriptions = IStore(
SpecificationSubscription).using(*tables).find(
SpecificationSubscription, *specification_filters).config(
@@ -454,10 +456,3 @@
for sub in specifications_subscriptions:
sub.specification.unsubscribe(
sub.person, self.requestor, ignore_permissions=True)
-
-
-def spec_not_visible(person_id):
- """Return an expression for finding specs not visible to the person."""
- tables, clauses = visible_specification_query(person_id)
- subselect = Select(Specification.id, tables=tables, where=And(clauses))
- return Not(Specification.id.is_in(subselect))
