launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Thanks for the comments, William.

Comments inline ...

On Wed, Apr 23, 2014 at 8:00 AM, William Grant <me@xxxxxxxxxxxxxxxxxx> wrote:
>
> 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

Good point.

> 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

Comment adjusted.

>
> 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.
>

Right, load_related() and load_referencing() helped with PU and PUS.
Unfortunately, they do not support nested attributes and could not be
used for SPN.

https://bugs.launchpad.net/launchpad/+bug/1311690

> 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.
>

We discussed this further on IRC and caching PU.sources is fine and
correct because of coincidence of constraints, we only accept
single-source uploads and once created PU contents never change. So,
in this scenario, caching PU.sources outside the upload-processing
period will be always "correct".

That said, there are plans to redesign the package upload workflow
(and queue) entirely and this particular aspect might change and those
callsites will have to be revisited.

>
> 170     + def create_testing_ppa(self):
>
> This isn't a legal method name.


Right, fixed.

[]

-- 
Celso Providelo
celso.providelo@xxxxxxxxxxxxx

https://code.launchpad.net/~cprov/launchpad/bug-1295318/+merge/216731
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.