On Fri, Aug 7, 2009 at 10:05 AM, William Grant<email address hidden> wrote:
> William Grant has proposed merging lp:~wgrant/launchpad/export-archive-dependencies into lp:launchpad/devel.
>
> Requested reviews:
>    Celso Providelo (cprov)
>
> = Summary =
>
> Bug #385129 requests that archive dependencies be exported on the webservice.
> This branch gives read-only access to ArchiveDependency records through the
> API.
>
>
> == Implementation details ==
>
> I've given IArchiveDependency a URL and navigation
> (<dependent>/+dependency/<dependency ID>). To get hold of the dependencies,
> I also moved IArchive.{dependencies,getArchiveDependency} from IArchivePublic
> to IArchiveView and exported them. This required some interface patches in
> c.l.i._schema_circular_imports.
>
> IComponent isn't exposed, so IArchiveDependency.component could not be
> exposed directly. I followed the example of similar interfaces, instead
> creating and exporting IArchiveDependency.component_name.
>
> The other changes to IArchiveDependency are just decoration, with Choices
> and Objects replaced with ReferenceChoices and References to coerce
> lazr.restful into working.

Hi Willian,

Thanks for working on this, You've done a fantastic job!

Comments inline.

> === modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'

Nice that you've found your way through the monkey-patches :)

> === modified file 'lib/lp/soyuz/browser/archive.py'
> --- lib/lp/soyuz/browser/archive.py	2009-08-05 11:33:16 +0000
> +++ lib/lp/soyuz/browser/archive.py	2009-08-07 14:05:54 +0000
> @@ -34,6 +34,8 @@
>  from zope.schema import Choice, List
>  from zope.schema.vocabulary import SimpleVocabulary, SimpleTerm
>
> +from sqlobject import SQLObjectNotFound
> +
>  from canonical.cachedproperty import cachedproperty
>  from canonical.launchpad import _
>  from lp.soyuz.browser.build import BuildRecordsView
> @@ -285,6 +287,20 @@
>          else:
>              return None
>
> +    @stepthrough('+dependency')
> +    def traverse_dependency(self, id):
> +        try:
> +            id = int(id)
> +        except ValueError:
> +            # Not a number.
> +            return None
> +
> +        try:
> +            archive = getUtility(IArchiveSet).get(id)
> +        except SQLObjectNotFound:
> +            return None
> +
> +        return self.context.getArchiveDependency(archive)

Okay, there are few subtle details in this travesal that I'd like to
clarify with you.

1. The protection against leaking dependencies of P3As exists because
IArchive.getArchiveDependencies() requires launchpad.View.

2. By design we don't allow public PPAs to depend on P3As, so attempts
to reach a P3A by querying its ID as a dependency of a public PPA will
never be an option (not that it would expose much of the dependant P3A
either, but ...)

Can you summarize those aspects in the traversal docstring ?

It is still possible, though, that a P3A B added as dependency of
another P3A A isn't accessible to all people with launchpad.View on A.
This situation would break most of the code in this area, including
the page rendering ... We should file a bug about it.

>  class ArchiveContextMenu(ContextMenu):
>      """Overview Menu for IArchive."""
>
> === modified file 'lib/lp/soyuz/browser/configure.zcml'

Nice one!

> === modified file 'lib/lp/soyuz/interfaces/archive.py'
> --- lib/lp/soyuz/interfaces/archive.py	2009-07-30 00:20:01 +0000
> +++ lib/lp/soyuz/interfaces/archive.py	2009-08-07 14:05:54 +0000
> @@ -55,8 +55,9 @@
>      REQUEST_USER, call_with, export_as_webservice_entry, exported,
>      export_read_operation, export_factory_operation, export_operation_as,
>      export_write_operation, operation_parameters,
> -    operation_returns_collection_of, rename_parameters_as, webservice_error)
> -from lazr.restful.fields import Reference
> +    operation_returns_collection_of, operation_returns_entry,
> +    rename_parameters_as, webservice_error)
> +from lazr.restful.fields import CollectionField, Reference
>
>
>  class ArchiveDependencyError(Exception):
> @@ -184,10 +185,6 @@
>      signing_key = Object(
>          title=_('Repository sigining key.'), required=False, schema=IGPGKey)
>
> -    dependencies = Attribute(
> -        "Archive dependencies recorded for this archive and ordered by owner "
> -        "displayname.")
> -
>      expanded_archive_dependencies = Attribute(
>          "The expanded list of archive dependencies. It includes the implicit "
>          "PRIMARY archive dependency for PPAs.")
> @@ -310,15 +307,6 @@
>          not found.
>          """
>
> -    def getArchiveDependency(dependency):
> -        """Return the `IArchiveDependency` object for the given dependency.
> -
> -        :param dependency: is an `IArchive` object.
> -
> -        :return: `IArchiveDependency` or None if a corresponding object
> -            could not be found.
> -        """
> -
>      def removeArchiveDependency(dependency):
>          """Remove the `IArchiveDependency` record for the given dependency.
>
> @@ -761,6 +749,12 @@
>          description=_("The password used by the builder to access the "
>                        "archive."))
>
> +    dependencies = exported(
> +        CollectionField(
> +            title=_("Archive dependencies recorded for this archive."),
> +            value_type=Reference(schema=Interface), #Really IArchiveDependency
> +            readonly=True))
> +
>      description = exported(
>          Text(
>              title=_("Archive contents description"), required=False,
> @@ -920,6 +914,19 @@
>              given ids that belong in the archive.
>          """
>
> +    @operation_parameters(
> +        dependency=Reference(schema=Interface)) #Really IArchive. See below.
> +    @operation_returns_entry(schema=Interface) #Really IArchiveDependency.
> +    @export_read_operation()
> +    def getArchiveDependency(dependency):
> +        """Return the `IArchiveDependency` object for the given dependency.
> +
> +        :param dependency: is an `IArchive` object.
> +
> +        :return: `IArchiveDependency` or None if a corresponding object
> +            could not be found.
> +        """
> +
>
>  class IArchiveAppend(Interface):
>      """Archive interface for operations restricted by append privilege."""
>

Right, you've exposed READ operations on archive dependencies
requiring launchpad.View on the context archive.

So, when you fetch an IArchive it already comes with a link to a
collection of read-only (I will get into this later)
IArchiveDependencies.

If an user needs to check if a specific archive is already used as
dependency, instead of iterating over the collection, a named GET
operation can return the precise answer.

It's very similar to the way PPAs are exposed for Person/Team. Very good!

> === modified file 'lib/lp/soyuz/interfaces/archivedependency.py'
> --- lib/lp/soyuz/interfaces/archivedependency.py	2009-06-25 04:06:00 +0000
> +++ lib/lp/soyuz/interfaces/archivedependency.py	2009-08-07 14:05:54 +0000
> @@ -11,38 +11,54 @@
>      'IArchiveDependency',
>      ]
>
> -from zope.interface import Attribute, Interface
> -from zope.schema import Choice, Datetime, Int, Object
> +from zope.interface import Interface
> +from zope.schema import Choice, Datetime, Int, TextLine
>
>  from canonical.launchpad import _
>  from lp.soyuz.interfaces.archive import IArchive
>  from lp.soyuz.interfaces.publishing import PackagePublishingPocket
> +from lazr.restful.fields import Reference, ReferenceChoice
> +from lazr.restful.declarations import (
> +    export_as_webservice_entry, exported)
>
>
>  class IArchiveDependency(Interface):
>      """ArchiveDependency interface."""
> +    export_as_webservice_entry()
>
>      id = Int(title=_("The archive ID."), readonly=True)
>
> -    date_created = Datetime(
> -        title=_("Instant when the dependency was created."),
> -        required=False, readonly=True)
> -
> -    archive = Choice(
> -        title=_('Target archive'),
> -        required=True,
> -        vocabulary='PPA',
> -        description=_("The PPA affected by this dependecy."))
> -
> -    dependency = Object(
> -        schema=IArchive,
> -        title=_("The archive set as a dependency."),
> -        required=False)
> -
> -    pocket = Choice(
> -        title=_("Pocket"), required=True, vocabulary=PackagePublishingPocket)
> +    date_created = exported(
> +        Datetime(
> +            title=_("Instant when the dependency was created."),
> +            required=False, readonly=True))
> +
> +    archive = exported(
> +        ReferenceChoice(
> +            title=_('Target archive'),
> +            required=True,
> +            vocabulary='PPA',
> +            schema=IArchive,
> +            description=_("The PPA affected by this dependecy.")))
> +

This was a previous design slip, not your fault, but you can probably fix it.

Any archive purpose can contain dependencies, not only PPAs. The
restriction can remain in the form interface
(IArchiveEditDependenciesForm), so you don't need to modify any test.

I believe you can test this by creating a dependency for a COPY
(rebuild) archive. In the best case, I think ReferenceChoice() is
doing nothing and could be replaced with a simple Reference() mainly
for simplicity.

> +    dependency = exported(
> +        Reference(
> +            schema=IArchive,
> +            title=_("The archive set as a dependency."),
> +            required=False))
> +
> +    pocket = exported(
> +        Choice(
> +            title=_("Pocket"), required=True,
> +            vocabulary=PackagePublishingPocket))
>
>      component = Choice(
>          title=_("Component"), required=True, vocabulary='Component')
>
> -    title = Attribute("Archive dependency title.")
> +    # We don't want to export IComponent, so the name is exported specially.
> +    component_name = exported(
> +        TextLine(
> +            title=_("Component name"),
> +            required=True))
> +
> +    title = exported(TextLine(title=_("Archive dependency title.")))
>
> === modified file 'lib/lp/soyuz/model/archivedependency.py'
> --- lib/lp/soyuz/model/archivedependency.py	2009-06-25 04:06:00 +0000
> +++ lib/lp/soyuz/model/archivedependency.py	2009-08-07 14:05:54 +0000
> @@ -48,6 +48,14 @@
>          foreignKey='Component', dbName='component')
>
>      @property
> +    def component_name(self):
> +        """See `IArchiveDependency`"""
> +        if self.component:
> +            return self.component.name
> +        else:
> +            return None
> +
> +    @property
>      def title(self):
>          """See `IArchiveDependency`."""
>          if self.dependency.is_ppa:
>

All good! as we discussed on IRC, IArchiveDependency.component_name
needs test/documentation -> doc/archive.txt (which opens a bad
precedent by mixing 2 content classes, but I think it's okay).

> === modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt'
> --- lib/lp/soyuz/stories/webservice/xx-archive.txt	2009-07-18 11:51:59 +0000
> +++ lib/lp/soyuz/stories/webservice/xx-archive.txt	2009-08-07 14:05:54 +0000
> @@ -15,6 +15,7 @@
>
>      >>> from lazr.restful.testing.webservice import pprint_entry
>      >>> pprint_entry(cprov_archive)
> +    dependencies_collection_link: u'http://.../~cprov/+archive/ppa/dependencies'
>      description: u'packages to help my friends.'
>      displayname: u'PPA for Celso Providelo'
>      distribution_link: u'http://.../ubuntu'
> @@ -77,6 +78,7 @@
>      >>> main_archive = webservice.get(
>      ...     ubuntutest['main_archive_link']).jsonBody()
>      >>> pprint_entry(main_archive)
> +    dependencies_collection_link: u'http://.../ubuntutest/+archive/primary/dependencies'
>      description: None
>      displayname: u'Primary Archive for Ubuntu Test'
>      distribution_link: u'http://.../ubuntutest'
> @@ -704,6 +706,7 @@
>  the IArchive context, in this case only Celso has it.
>
>      >>> pprint_entry(user_webservice.get("/~cprov/+archive/ppa").jsonBody())
> +    dependencies_collection_link: u'http://.../~cprov/+archive/ppa/dependencies'
>      description: u'tag:launchpad.net:2008:redacted'
>      displayname: u'PPA for Celso Providelo'
>      distribution_link: u'http://.../ubuntu'
> @@ -715,6 +718,7 @@
>      signing_key_fingerprint: u'tag:launchpad.net:2008:redacted'
>
>      >>> pprint_entry(cprov_webservice.get("/~cprov/+archive/ppa").jsonBody())
> +    dependencies_collection_link: u'http://.../~cprov/+archive/ppa/dependencies'
>      description: u'packages to help my friends.'
>      displayname: u'PPA for Celso Providelo'
>      distribution_link: u'http://.../ubuntu'
> @@ -878,3 +882,154 @@
>      ...
>      CannotCopy: private 1.1 in hoary
>      (same version already uploaded and waiting in ACCEPTED queue)
> +
> +
> += Archive dependencies =
> +

Just a small note, I think that testing ArchiveDependency API here is
not wrong, since dealing with them only makes sense in an IArchive
context.

Also, fyi, we are in process of migrating the doctest syntax from Moin
to ReST, so new files should use the later. It's not a problem for
this file, don't worry, keep using Moin.

> +Archives can specify dependencies on pockets and components of other
> +archives. Found at <dependentarchive.id>/+dependency/<dependencyarchive.id>,
> +these IArchiveDependency records can be retrieved through the API.
> +
> +First we'll add an explicit dependency on the primary archive to
> +cprov's PPA. We can't do this through the webservice yet.
> +
> +    >>> from lp.soyuz.interfaces.component import IComponentSet
> +    >>> from lp.soyuz.interfaces.publishing import PackagePublishingPocket
> +    >>> login('<email address hidden>')
> +    >>> cprov_ppa_db = getUtility(IPersonSet).getByName('cprov').archive
> +    >>> dep = cprov_ppa_db.addArchiveDependency(
> +    ...     cprov_ppa_db.distribution.main_archive,
> +    ...     PackagePublishingPocket.RELEASE,
> +    ...     component=getUtility(IComponentSet)['universe'])
> +    >>> logout()
> +
> +We can then request that dependency, and see that we get all of its
> +attributes.
> +
> +    >>> cprov_main_dependency = webservice.named_get(
> +    ...     '/~cprov/+archive/ppa', 'getArchiveDependency',
> +    ...     dependency=ubuntu['main_archive_link']).jsonBody()
> +    >>> pprint_entry(cprov_main_dependency)
> +    archive_link: u'http://.../~cprov/+archive/ppa'
> +    component_name: u'universe'
> +    date_created: ...
> +    dependency_link: u'http://.../ubuntu/+archive/primary'
> +    pocket: u'Release'
> +    resource_type_link: u'http://.../#archive_dependency'
> +    self_link: u'http://.../~cprov/+archive/ppa/+dependency/1'
> +    title: u'Primary Archive for Ubuntu Linux - RELEASE (main, universe)'
> +
> +Asking for an archive on which there is no dependency returns None.
> +
> +    >>> debian = webservice.get('/debian').jsonBody()
> +    >>> webservice.named_get(
> +    ...     '/~cprov/+archive/ppa', 'getArchiveDependency',
> +    ...     dependency=debian['main_archive_link']).jsonBody()
> +
> +Archives will also give us a list of their custom dependencies.
> +
> +    >>> print_self_link_of_entries(
> +    ...     webservice.get('/~cprov/+archive/ppa/dependencies').jsonBody())
> +    http://.../~cprov/+archive/ppa/+dependency/1
> +
> +Crafting a URL to a non-dependency 404s:
> +
> +    >>> print webservice.get(
> +    ...     '/~cprov/+archive/ppa/+dependency/2')
> +    HTTP/1.1 404 Not Found
> +    ...
> +
> +A 404 also occurs if we ask for an archive that doesn't exist.
> +
> +    >>> print webservice.get(
> +    ...     '/~cprov/+archive/ppa/+dependency/123456')
> +    HTTP/1.1 404 Not Found
> +    ...
> +

I think you need to test the code path that fails when traversing
something that isn't an 'int'.

> +== Security ==
> +

It don't thing you need the subsection here, it's not adding much, is it ?

Your sentences are super-clear and nice to read, no need to add
subsections with coding jargon.

> +Any user can retrieve a public PPA's dependencies.
> +
> +    >>> login('<email address hidden>')
> +    >>> cprov_ppa_db.private = False
> +    >>> logout()
> +
> +    >>> print user_webservice.get(
> +    ...     '/~cprov/+archive/ppa/dependencies')
> +    HTTP/1.1 200 Ok
> +    ...
> +
> +    >>> print user_webservice.get(
> +    ...     '/~cprov/+archive/ppa/+dependency/1')
> +    HTTP/1.1 200 Ok
> +    ...
> +
> +The dependencies of a private archive are private. Let's make
> +Celso's PPA private.
> +
> +    >>> login('<email address hidden>')
> +    >>> cprov_ppa_db.private = True
> +    >>> logout()
> +
> +Now our unprivileged user can't get a list of the dependencies.
> +
> +    >>> print user_webservice.get(
> +    ...     '/~cprov/+archive/ppa/dependencies')
> +    HTTP/1.1 401 Unauthorized
> +    ...
> +    Unauthorized: (<Archive at ...>, 'dependencies', 'launchpad.View')
> +    <BLANKLINE>
> +
> +Nor can said user craft a URL to a dependency.
> +
> +    >>> print user_webservice.get(
> +    ...     '/~cprov/+archive/ppa/+dependency/1')
> +    HTTP/1.1 401 Unauthorized
> +    ...
> +    Unauthorized: (<Archive at ...>, 'getArchiveDependency', 'launchpad.View')
> +    <BLANKLINE>
> +
> +Celso can still see them if we grant private permissions, of course.
> +
> +    >>> cprov_webservice = webservice_for_person(
> +    ...     cprov, permission=OAuthPermission.WRITE_PRIVATE)
> +    >>> print cprov_webservice.get(
> +    ...     '/~cprov/+archive/ppa/dependencies')
> +    HTTP/1.1 200 Ok
> +    ...
> +    >>> print cprov_webservice.get(
> +    ...     '/~cprov/+archive/ppa/+dependency/1')
> +    HTTP/1.1 200 Ok
> +    ...
> +
> +    >>> login('<email address hidden>')
> +    >>> cprov_ppa_db.private = False
> +    >>> logout()
> +

That's where "dealing with dependencies for an IArchive" documentation
ends and become "How ArchiveDependency records behave".

I think this last snippet would be better placed in
xx-archive-dependency.txt, even if it requires a little bit of setup
duplication.

This code basically documents that IArchiveDependency is READ-ONLY
atm, we don't allow any type of write/edit (note the lack of security
adapters).

> +But even he can't write to a dependency.
> +
> +    >>> sabdfl_ppa = cprov_webservice.get(
> +    ...     '/~sabdfl/+archive/ppa').jsonBody()
> +    >>> print cprov_webservice.patch(
> +    ...     '/~cprov/+archive/ppa/+dependency/1', 'application/json',
> +    ...     simplejson.dumps({'archive_link': sabdfl_ppa['self_link']}))
> +    HTTP/1.1 ...
> +    ...
> +    ForbiddenAttribute: ('archive', <ArchiveDependency at ...>)
> +    <BLANKLINE>
> +
> +    >>> print cprov_webservice.patch(
> +    ...     '/~cprov/+archive/ppa/+dependency/1', 'application/json',
> +    ...     simplejson.dumps({'dependency_link': sabdfl_ppa['self_link']}))
> +    HTTP/1.1 ...
> +    ...
> +    ForbiddenAttribute: ('dependency', <ArchiveDependency at ...>)
> +    <BLANKLINE>
> +
> +    >>> print cprov_webservice.patch(
> +    ...     '/~cprov/+archive/ppa/+dependency/1', 'application/json',
> +    ...     simplejson.dumps({'pocket': 'Security'}))
> +    HTTP/1.1 ...
> +    ...
> +    ForbiddenAttribute: ('pocket', <ArchiveDependency at ...>)
> +    <BLANKLINE>
>

It's, indeed, a very well designed change, nothing problematic. Thanks
for making this review easy :-)

If you agree with my points, you need to:

1. Document and test the ArchiveDependency traversal
2. ArchiveDependency.archive probably doesn't need to be a ReferenceChoice()
3. Test IArchiveDependency.component_name
4. Move the ArchiveDependency specific webservice tests to a separate file.

... and it will ready for landing.

Thank you!
[]




-- 
Celso Providelo <email address hidden>
IRC: cprov,  Jabber: <email address hidden>, Skype: cprovidelo
1024D/681B6469 C858 2652 1A6E F6A6 037B  B3F7 9FF2 583E 681B 6469