launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14330
[Merge] lp:~abentley/launchpad/fix-bp-timeouts into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/fix-bp-timeouts into lp:launchpad.
Commit message:
Fix timeouts checking blueprint visiblity
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1075569 in Launchpad itself: "Timeout loading milestone because of blueprints"
https://bugs.launchpad.net/launchpad/+bug/1075569
Bug #1077980 in Launchpad itself: "viewing project page leads to timeout when private blueprints exists"
https://bugs.launchpad.net/launchpad/+bug/1077980
Bug #1078239 in Launchpad itself: "loading team blueprints leads to timeout Person:+index"
https://bugs.launchpad.net/launchpad/+bug/1078239
Bug #1079390 in Launchpad itself: "Person:+upcomingwork timeout querying specifiations"
https://bugs.launchpad.net/launchpad/+bug/1079390
For more details, see:
https://code.launchpad.net/~abentley/launchpad/fix-bp-timeouts/+merge/134744
= Summary =
Fix the following bugs:
Bug #1075569: Timeout loading milestone because of blueprints
Bug #1077980: viewing project page leads to timeout when private blueprints exists
Bug #1078239: loading team blueprints leads to timeout Person:+index
Bug #1079390: Person:+upcomingwork timeout querying specifiations
== Proposed fix ==
Switch from get_specification_privacy_filter to visible_specification_query everywhere.
== Pre-implementation notes ==
None
== LOC Rationale ==
Part of private products.
== Implementation details ==
visible_specification_query does not guarantee distinct results, so added config(distinct=True) as needed.
Since get_specification_privacy_filter is no longer used, it is deleted, and its tests migrated to visible_specification_query.
TestPersonUpcomingWork.test_container_progressbar assumed that WorkItem containers were in a specific order, based on the id of their initial WorkItem. This work changed the ordering, but a consistent ordering makes sense, so I've made the ordering explicit.
All the call sites of get_specification_filters now use visible_specification_query, so its assume_product_active behaviour can now become mandatory.
_preload_specifications_people is used by both Product and Distribution, so tweaked Distribution to supply tables.
In sharingjob, replaced get_specification_privacy_filter by introducing a sub-select, because otherwise, negating visible_specification_query would be very tricky.
Fixed lint in SpecificationWorkItem.__init__.
Fixed long line by assigning LatestPersonSourcePackageReleaseCache.upload_distroseries_id to a local variable.
== Tests ==
bin/test -v
== Demo and Q/A ==
Go to the staging versions of all pages linked in the bugs and see whether they consistently time out.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/blueprints/model/specificationworkitem.py
lib/lp/registry/model/sharingjob.py
lib/lp/blueprints/tests/test_specification.py
lib/lp/blueprints/model/specification.py
lib/lp/blueprints/model/sprint.py
lib/lp/registry/model/product.py
lib/lp/registry/model/person.py
lib/lp/registry/model/milestone.py
lib/lp/registry/model/distribution.py
--
https://code.launchpad.net/~abentley/launchpad/fix-bp-timeouts/+merge/134744
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/fix-bp-timeouts into lp:launchpad.
=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py 2012-11-12 16:40:28 +0000
+++ lib/lp/blueprints/model/specification.py 2012-11-16 20:58:25 +0000
@@ -4,7 +4,6 @@
__metaclass__ = type
__all__ = [
'get_specification_filters',
- 'get_specification_privacy_filter',
'HasSpecificationsMixin',
'recursive_blocked_query',
'recursive_dependent_query',
@@ -960,7 +959,7 @@
elif sort == SpecificationSort.DATE:
return (Desc(Specification.datecreated), Specification.id)
- def _preload_specifications_people(self, clauses):
+ def _preload_specifications_people(self, tables, clauses):
"""Perform eager loading of people and their validity for query.
:param query: a string query generated in the 'specifications'
@@ -1001,7 +1000,7 @@
index += 1
decorator(person, column)
- results = Store.of(self).find(Specification, *clauses)
+ results = Store.of(self).using(*tables).find(Specification, *clauses)
return DecoratedResultSet(results, pre_iter_hook=cache_people)
@property
@@ -1230,58 +1229,6 @@
return Specification.get(spec_id)
-def get_specification_privacy_filter(user):
- """Return a Storm expression for filtering specifications by privacy.
-
- :param user: A Person ID or a column reference.
- :return: A Storm expression to check if a peron has access grants
- for a specification.
- """
- # Avoid circular imports.
- from lp.registry.model.accesspolicy import (
- AccessArtifact,
- AccessPolicy,
- AccessPolicyGrantFlat,
- )
- public_specification_filter = (
- Specification.information_type.is_in(PUBLIC_INFORMATION_TYPES))
- if user is None:
- return public_specification_filter
- return Or(
- public_specification_filter,
- Specification.id.is_in(
- Select(
- Specification.id,
- tables=(
- Specification,
- Join(
- AccessPolicy,
- And(
- Or(
- Specification.productID ==
- AccessPolicy.product_id,
- Specification.distributionID ==
- AccessPolicy.distribution_id),
- Specification.information_type ==
- AccessPolicy.type)),
- Join(
- AccessPolicyGrantFlat,
- AccessPolicy.id == AccessPolicyGrantFlat.policy_id),
- LeftJoin(
- AccessArtifact,
- AccessPolicyGrantFlat.abstract_artifact_id ==
- AccessArtifact.id),
- Join(
- TeamParticipation,
- And(
- TeamParticipation.team ==
- AccessPolicyGrantFlat.grantee_id,
- TeamParticipation.person == user))),
- where=Or(
- AccessPolicyGrantFlat.abstract_artifact_id == None,
- AccessArtifact.specification_id == Specification.id))))
-
-
def visible_specification_query(user):
"""Return a Storm expression and list of tables for filtering
specifications by privacy.
@@ -1324,21 +1271,13 @@
return tables, clauses
-def get_specification_filters(filter, assume_product_active=False):
+def get_specification_filters(filter):
"""Return a list of Storm expressions for filtering Specifications.
:param filters: A collection of SpecificationFilter and/or strings.
Strings are used for text searches.
- :param assume_product_active: If True, assume the Product is active,
- instead of ensuring it is active.
"""
- from lp.registry.model.product import Product
clauses = []
- # If Product is used, it must be active.
- if not assume_product_active:
- clauses.extend([Or(Specification.product == None,
- Not(Specification.productID.is_in(Select(Product.id,
- Product.active == False))))])
# ALL is the trump card.
if SpecificationFilter.ALL in filter:
return clauses
=== modified file 'lib/lp/blueprints/model/specificationworkitem.py'
--- lib/lp/blueprints/model/specificationworkitem.py 2012-04-05 13:05:04 +0000
+++ lib/lp/blueprints/model/specificationworkitem.py 2012-11-16 20:58:25 +0000
@@ -29,6 +29,7 @@
implements(ISpecificationWorkItem)
__storm_table__ = 'SpecificationWorkItem'
+ __storm_order__ = 'id'
id = Int(primary=True)
title = Unicode(allow_none=False)
@@ -53,12 +54,12 @@
def __init__(self, title, status, specification, assignee, milestone,
sequence):
- self.title=title
- self.status=status
+ self.title = title
+ self.status = status
self.specification = specification
- self.assignee=assignee
- self.milestone=milestone
- self.sequence=sequence
+ self.assignee = assignee
+ self.milestone = milestone
+ self.sequence = sequence
@property
def is_complete(self):
=== modified file 'lib/lp/blueprints/model/sprint.py'
--- lib/lp/blueprints/model/sprint.py 2012-11-12 16:40:28 +0000
+++ lib/lp/blueprints/model/sprint.py 2012-11-16 20:58:25 +0000
@@ -155,8 +155,7 @@
if len(statuses) > 0:
query.append(Or(*statuses))
# Filter for specification text
- query.extend(
- get_specification_filters(filter, assume_product_active=True))
+ query.extend(get_specification_filters(filter))
return tables, query
def all_specifications(self, user):
=== modified file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py 2012-10-16 14:47:54 +0000
+++ lib/lp/blueprints/tests/test_specification.py 2012-11-16 20:58:25 +0000
@@ -33,8 +33,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,
@@ -397,8 +397,8 @@
specification.target.owner, specification,
error_expected=False, attribute='name', value='foo')
- def test_get_specification_privacy_filter(self):
- # get_specification_privacy_filter() returns a Storm expression
+ def test_visible_specification_query(self):
+ # visible_specification_query returns a Storm expression
# that can be used to filter specifications by their visibility-
owner = self.factory.makePerson()
product = self.factory.makeProduct(
@@ -413,30 +413,32 @@
all_specs = [
public_spec, proprietary_spec_1, proprietary_spec_2]
store = Store.of(product)
- specs_for_anon = store.find(
- Specification, get_specification_privacy_filter(None),
- Specification.productID == product.id)
- self.assertContentEqual([public_spec], specs_for_anon)
+ 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))
# Product owners havae grants on the product, the privacy
# filter returns thus all specifications for them.
- specs_for_owner = store.find(
- Specification, get_specification_privacy_filter(owner.id),
- Specification.productID == product.id)
+ tables, query = visible_specification_query(owner.id)
+ specs_for_owner = store.using(*tables).find(
+ Specification, Specification.productID == product.id, *query)
self.assertContentEqual(all_specs, specs_for_owner)
# The filter returns only public specs for ordinary users.
user = self.factory.makePerson()
- specs_for_other_user = store.find(
- Specification, get_specification_privacy_filter(user.id),
- Specification.productID == product.id)
+ tables, query = visible_specification_query(user.id)
+ specs_for_other_user = store.using(*tables).find(
+ Specification, Specification.productID == product.id, *query)
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])
- specs_for_other_user = store.find(
- Specification, get_specification_privacy_filter(user.id),
- Specification.productID == product.id)
+ tables, query = visible_specification_query(user.id)
+ specs_for_other_user = store.using(*tables).find(
+ Specification, Specification.productID == product.id, *query)
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-02 00:53:58 +0000
+++ lib/lp/registry/model/distribution.py 2012-11-16 20:58:25 +0000
@@ -952,7 +952,8 @@
constraint)
if prejoin_people:
- results = self._preload_specifications_people(query)
+ results = self._preload_specifications_people([Specification],
+ query)
else:
results = Store.of(self).find(
Specification,
=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py 2012-10-26 08:59:24 +0000
+++ lib/lp/registry/model/milestone.py 2012-11-16 20:58:25 +0000
@@ -39,8 +39,8 @@
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,10 +155,10 @@
"""See `IMilestoneData`"""
from lp.registry.model.person import Person
store = Store.of(self.target)
- origin = [
- Specification,
+ origin, clauses = visible_specification_query(user)
+ origin.extend([
LeftJoin(Person, Specification.assigneeID == Person.id),
- ]
+ ])
milestones = self._milestone_ids_expr(user)
results = store.using(*origin).find(
@@ -176,12 +176,12 @@
milestones),
SpecificationWorkItem.deleted == False)),
all=True)),
- get_specification_privacy_filter(user))
- results.config(distinct=True)
+ *clauses)
ordered_results = results.order_by(Desc(Specification.priority),
Specification.definition_status,
Specification.implementation_status,
Specification.title)
+ ordered_results.config(distinct=True)
mapper = lambda row: row[0]
return DecoratedResultSet(ordered_results, mapper)
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2012-11-14 02:03:32 +0000
+++ lib/lp/registry/model/person.py 2012-11-16 20:58:25 +0000
@@ -130,10 +130,10 @@
)
from lp.blueprints.model.specification import (
get_specification_filters,
- get_specification_privacy_filter,
HasSpecificationsMixin,
Specification,
spec_started_clause,
+ visible_specification_query,
)
from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
from lp.bugs.interfaces.bugtarget import IBugTarget
@@ -857,7 +857,8 @@
Select(SpecificationSubscription.specificationID,
[SpecificationSubscription.person == self]
)))
- clauses = [Or(*role_clauses), get_specification_privacy_filter(user)]
+ tables, clauses = visible_specification_query(user)
+ clauses.append(Or(*role_clauses))
# Defaults for completeness: if nothing is said about completeness
# then we want to show INCOMPLETE.
if SpecificationFilter.COMPLETE not in filter:
@@ -867,7 +868,7 @@
filter.add(SpecificationFilter.INCOMPLETE)
clauses.extend(get_specification_filters(filter))
- results = Store.of(self).find(Specification, *clauses)
+ results = Store.of(self).using(*tables).find(Specification, *clauses)
# The default sort is priority descending, so only explictly sort for
# DATE.
if sort == SpecificationSort.DATE:
@@ -876,6 +877,7 @@
sort = None
if sort is not None:
results = results.order_by(sort)
+ results.config(distinct=True)
if quantity is not None:
results = results[:quantity]
return results
@@ -1466,23 +1468,23 @@
from lp.registry.model.distribution import Distribution
store = Store.of(self)
WorkItem = SpecificationWorkItem
- origin = [
- WorkItem,
- Join(Specification, WorkItem.specification == Specification.id),
+ origin, query = visible_specification_query(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),
- ]
+ ])
today = datetime.today().date()
- query = And(
- get_specification_privacy_filter(user),
+ query.extend([
Milestone.dateexpected <= date, Milestone.dateexpected >= today,
WorkItem.deleted == False,
OR(WorkItem.assignee_id.is_in(self.participant_ids),
- Specification.assigneeID.is_in(self.participant_ids)))
- result = store.using(*origin).find(WorkItem, query)
+ Specification.assigneeID.is_in(self.participant_ids))])
+ result = store.using(*origin).find(WorkItem, *query)
+ result.config(distinct=True)
def eager_load(workitems):
specs = bulk.load_related(
@@ -2844,6 +2846,8 @@
pass
elif uploader_only:
lpspr = ClassAlias(LatestPersonSourcePackageReleaseCache, 'lpspr')
+ upload_distroseries_id = (
+ LatestPersonSourcePackageReleaseCache.upload_distroseries_id)
clauses.append(Not(Exists(Select(1,
where=And(
lpspr.sourcepackagename_id ==
@@ -2851,7 +2855,7 @@
lpspr.upload_archive_id ==
LatestPersonSourcePackageReleaseCache.upload_archive_id,
lpspr.upload_distroseries_id ==
- LatestPersonSourcePackageReleaseCache.upload_distroseries_id,
+ upload_distroseries_id,
lpspr.archive_purpose != ArchivePurpose.PPA,
lpspr.maintainer_id == self.id),
tables=lpspr))))
=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py 2012-11-15 23:05:42 +0000
+++ lib/lp/registry/model/product.py 2012-11-16 20:58:25 +0000
@@ -97,11 +97,11 @@
)
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
@@ -1408,14 +1408,15 @@
# - completeness.
# - informational.
#
- clauses = [Specification.product == self,
- get_specification_privacy_filter(user)]
+ tables, clauses = visible_specification_query(user)
+ clauses.append(Specification.product == self)
clauses.extend(get_specification_filters(filter))
if prejoin_people:
- results = self._preload_specifications_people(clauses)
+ results = self._preload_specifications_people(tables, clauses)
else:
- results = Store.of(self).find(Specification, *clauses)
- results.order_by(order)
+ tableset = Store.of(self).using(*tables)
+ results = tableset.find(Specification, *clauses)
+ results.order_by(order).config(distinct=True)
if quantity is not None:
results = results[:quantity]
return results
=== modified file 'lib/lp/registry/model/sharingjob.py'
--- lib/lp/registry/model/sharingjob.py 2012-09-28 06:15:58 +0000
+++ lib/lp/registry/model/sharingjob.py 2012-11-16 20:58:25 +0000
@@ -44,8 +44,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,
@@ -440,8 +440,7 @@
sub.person, self.requestor, ignore_permissions=True)
if specification_filters:
specification_filters.append(
- Not(get_specification_privacy_filter(
- SpecificationSubscription.personID)))
+ spec_not_visible(SpecificationSubscription.personID))
tables = (
SpecificationSubscription,
Join(
@@ -455,3 +454,10 @@
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))
Follow ups
