launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code

52	+ def _getPublishedSources(name=None, version=None, status=None,
53	+ distroseries=None, pocket=None,
54	+ exact_match=False, created_since_date=None,
55	+ component_name=None):
56	+ """Wrapper for getPublishedSources.

This should be something like api_getPublishedSources to indicate its purpose.

92	+ # Pre-cache related `PackageUpload`s, needed for security checks.

The PackageUploads are needed for the SPPH.packageupload field. We pre-populate the sources attribute of each PackageUpload to avoid the security check.

93	+ pu_ids = set(map(attrgetter('packageuploadID'), rows))
94	+ list(store.find(PackageUpload, PackageUpload.id.is_in(pu_ids)))
95	+ # Pre-cache related `SourcePackageName`s, needed for the published
96	+ # record title (materialized in the API).
97	+ spn_ids = set(map(
98	+ attrgetter('sourcepackagerelease.sourcepackagenameID'), rows))
99	+ list(store.find(SourcePackageName,
100	+ SourcePackageName.id.is_in(spn_ids)))

These should be able to be simplified using lp.services.database.bulk.load_related.

116	+ get_property_cache(spr).published_archives = [self]
117	+ # Fill `PackageUpload.sources`.
118	+ if pub_source.packageupload is not None:
119	+ upload = pub_source.packageupload
120	+ get_property_cache(upload).sources = [
121	+ pu_sources_dict.get(upload.id)]

Setting published_archives and sources to just the elements that we know are sufficient here is fine for security purposes. But the attributes aren't limited to security checks, so this may introduce confusing correctness bugs -- they'll magically be erroneously non-exhaustive when they happen to have been touched by this API method.

I'd consider renaming the attributes to make it clear that they're only good for security checks. This might mean creating a new cachedproperty around PU.sources, as that attribute is used for more than just security.

170	+ def create_testing_ppa(self):

This isn't a legal method name.
-- 
https://code.launchpad.net/~cprov/launchpad/bug-1295318/+merge/216731
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.