launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> === modified file 'lib/lp/code/configure.zcml'
> --- lib/lp/code/configure.zcml	2015-02-23 14:47:40 +0000
> +++ lib/lp/code/configure.zcml	2015-02-23 14:47:40 +0000
> @@ -865,6 +865,22 @@
>    <adapter factory="lp.code.model.defaultgit.OwnerProjectDefaultGitRepository" />
>    <adapter factory="lp.code.model.defaultgit.OwnerPackageDefaultGitRepository" />
>  
> +  <class class="lp.code.model.gitlookup.GitLookup">
> +    <allow interface="lp.code.interfaces.gitlookup.IGitLookup" />
> +  </class>
> +  <securedutility
> +      class="lp.code.model.gitlookup.GitLookup"
> +      provides="lp.code.interfaces.gitlookup.IGitLookup">
> +    <allow interface="lp.code.interfaces.gitlookup.IGitLookup" />
> +  </securedutility>
> +  <securedutility
> +      class="lp.code.model.gitlookup.DefaultGitTraverser"
> +      provides="lp.code.interfaces.gitlookup.IDefaultGitTraverser">
> +    <allow interface="lp.code.interfaces.gitlookup.IDefaultGitTraverser" />
> +  </securedutility>
> +  <adapter factory="lp.code.model.gitlookup.DistributionGitTraversable" />
> +  <adapter factory="lp.code.model.gitlookup.PersonGitTraversable" />
> +
>    <lp:help-folder folder="help" name="+help-code" />
>  
>    <!-- Diffs -->
> 
> === added file 'lib/lp/code/interfaces/gitlookup.py'
> --- lib/lp/code/interfaces/gitlookup.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/interfaces/gitlookup.py	2015-02-23 14:47:40 +0000
> @@ -0,0 +1,131 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Utility for looking up Git repositories by name."""
> +
> +__metaclass__ = type
> +__all__ = [
> +    'IDefaultGitTraversable',
> +    'IDefaultGitTraverser',
> +    'IGitLookup',
> +    ]
> +
> +from zope.interface import Interface
> +
> +
> +class IDefaultGitTraversable(Interface):
> +    """A thing that can be traversed to find a thing with a default Git
> +    repository."""
> +
> +    def traverse(name, segments):
> +        """Return the object beneath this one that matches 'name'.
> +
> +        :param name: The name of the object being traversed to.
> +        :param segments: Remaining path segments.
> +        :return: An `IDefaultGitTraversable` object if traversing should
> +            continue; an `ICanHasDefaultGitRepository` object otherwise.
> +        """
> +
> +
> +class IDefaultGitTraverser(Interface):
> +    """Utility for traversing to an object that can have a default Git
> +    repository."""
> +
> +    def traverse(path):
> +        """Traverse to the object referred to by 'path'.
> +
> +        :raises InvalidNamespace: If the path cannot be parsed as a
> +            namespace.
> +        :raises InvalidProductName: If the project component of the path is
> +            not a valid name.
> +        :raises NoSuchPerson: If the first segment of the path begins with a
> +            '~', but we can't find a person matching the remainder.
> +        :raises NoSuchProduct: If we can't find a project that matches the
> +            project component of the path.
> +        :raises NoSuchSourcePackageName: If the source package referred to
> +            does not exist.
> +
> +        :return: One of
> +            * `IProduct`
> +            * `IDistributionSourcePackage`
> +            * `IPersonProduct`
> +            * `IPersonDistributionSourcePackage`
> +        """
> +
> +
> +class IGitLookup(Interface):
> +    """Utility for looking up a Git repository by name."""
> +
> +    def get(repository_id, default=None):
> +        """Return the repository with the given id.
> +
> +        Return the default value if there is no such repository.
> +        """
> +
> +    def getByUniqueName(unique_name):
> +        """Find a repository by its unique name.
> +
> +        Unique names have one of the following forms:
> +            ~OWNER/PROJECT/+git/NAME
> +            ~OWNER/DISTRO/+source/SOURCE/+git/NAME
> +            ~OWNER/+git/NAME
> +
> +        :return: An `IGitRepository`, or None.
> +        """
> +
> +    def uriToHostingPath(uri):
> +        """Return the path for the URI, if the URI is on codehosting.
> +
> +        This does not ensure that the path is valid.
> +
> +        :param uri: An instance of lazr.uri.URI
> +        :return: The path if possible; None if the URI is not a valid
> +            codehosting URI.
> +        """
> +
> +    def getByUrl(url):
> +        """Find a repository by URL.
> +
> +        Either from the URL on git.launchpad.net (various schemes) or the
> +        lp: URL (which relies on client-side configuration).
> +        """
> +
> +    def getByPath(path):
> +        """Find a repository by its path.
> +
> +        Any of these forms may be used, with or without a leading slash:
> +            Unique names:
> +                ~OWNER/PROJECT/+git/NAME
> +                ~OWNER/DISTRO/+source/SOURCE/+git/NAME
> +                ~OWNER/+git/NAME
> +            Owner-target default aliases:
> +                ~OWNER/PROJECT
> +                ~OWNER/DISTRO/+source/SOURCE
> +            Official aliases:
> +                PROJECT
> +                DISTRO/+source/SOURCE
> +
> +        :raises InvalidNamespace: If the path looks like a unique repository
> +            name but doesn't have enough segments to be a unique name.
> +        :raises InvalidProductName: If the given project in a project
> +            shortcut is an invalid name for a project.
> +
> +        :raises NoSuchGitRepository: If we can't find a repository that
> +            matches the repository component of the path.
> +        :raises NoSuchPerson: If we can't find a person who matches the
> +            person component of the path.
> +        :raises NoSuchProduct: If we can't find a project that matches the
> +            project component of the path.
> +        :raises NoSuchSourcePackageName: If the source package referred to
> +            does not exist.
> +
> +        :raises NoDefaultGitRepository: If the path refers to an existing
> +            thing that's not a Git repository and has no default repository
> +            associated with it.  For example, a product without a default
> +            repository.
> +        :raises CannotHaveDefaultGitRepository: If the path refers to an
> +            existing thing that cannot have a default Git repository
> +            associated with it.  For example, a distribution.
> +
> +        :return: An `IGitRepository`.
> +        """
> 
> === added file 'lib/lp/code/model/gitlookup.py'
> --- lib/lp/code/model/gitlookup.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/model/gitlookup.py	2015-02-23 14:47:40 +0000
> @@ -0,0 +1,275 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Database implementation of the Git repository lookup utility."""
> +
> +__metaclass__ = type
> +# This module doesn't export anything. If you want to look up Git
> +# repositories by name, then get the IGitLookup utility.
> +__all__ = []
> +
> +from lazr.uri import (
> +    InvalidURIError,
> +    URI,
> +    )
> +from zope.component import (
> +    adapts,
> +    getUtility,
> +    queryMultiAdapter,
> +    )
> +from zope.interface import implements
> +
> +from lp.app.validators.name import valid_name
> +from lp.code.errors import (
> +    CannotHaveDefaultGitRepository,
> +    InvalidNamespace,
> +    NoDefaultGitRepository,
> +    NoSuchGitRepository,
> +    )
> +from lp.code.interfaces.defaultgit import get_default_git_repository
> +from lp.code.interfaces.gitlookup import (
> +    IDefaultGitTraversable,
> +    IDefaultGitTraverser,
> +    IGitLookup,
> +    )
> +from lp.code.interfaces.gitnamespace import IGitNamespaceSet
> +from lp.code.model.gitrepository import GitRepository
> +from lp.registry.errors import NoSuchSourcePackageName
> +from lp.registry.interfaces.distribution import IDistribution
> +from lp.registry.interfaces.person import (
> +    IPerson,
> +    IPersonSet,
> +    NoSuchPerson,
> +    )
> +from lp.registry.interfaces.persondistributionsourcepackage import (
> +    IPersonDistributionSourcePackageFactory,
> +    )
> +from lp.registry.interfaces.personproduct import IPersonProductFactory
> +from lp.registry.interfaces.pillar import IPillarNameSet
> +from lp.registry.interfaces.product import (
> +    InvalidProductName,
> +    NoSuchProduct,
> +    )
> +from lp.services.config import config
> +from lp.services.database.interfaces import IStore
> +
> +
> +def adapt(provided, interface):
> +    """Adapt 'obj' to 'interface', using multi-adapters if necessary."""

obj == provided? "provided" also usually indicates an interface, not an implementation.

> +    required = interface(provided, None)
> +    if required is not None:
> +        return required
> +    try:
> +        return queryMultiAdapter(provided, interface)
> +    except TypeError:
> +        return None
> +
> +
> +class RootGitTraversable:
> +    """Root traversable for default Git repository objects.
> +
> +    Corresponds to '/' in the path.  From here, you can traverse to a
> +    person, a distribution, or a project.
> +    """
> +
> +    implements(IDefaultGitTraversable)
> +
> +    def traverse(self, name, segments):
> +        """See `IDefaultGitTraversable`.
> +
> +        :raise InvalidProductName: If 'name' is not a valid name.
> +        :raise NoSuchPerson: If 'name' begins with a '~', but the remainder
> +            doesn't match an existing person.
> +        :raise NoSuchProduct: If 'name' doesn't match an existing pillar.
> +        :return: `IPerson` or `IPillar`.
> +        """
> +        if name.startswith("~"):
> +            person_name = name[1:]
> +            person = getUtility(IPersonSet).getByName(person_name)
> +            if person is None:
> +                raise NoSuchPerson(person_name)
> +            return person
> +        if not valid_name(name):
> +            raise InvalidProductName(name)
> +        pillar = getUtility(IPillarNameSet).getByName(name)
> +        if pillar is None:
> +            # Actually, the pillar is no such *anything*.
> +            raise NoSuchProduct(name)
> +        return pillar
> +
> +
> +class _BaseGitTraversable:
> +    """Base class for traversable implementations.
> +
> +    This just defines a very simple constructor.
> +    """
> +
> +    def __init__(self, context):
> +        self.context = context
> +
> +
> +class DistributionGitTraversable(_BaseGitTraversable):
> +    """Default Git repository traversable for distributions.
> +
> +    From here, you can traverse to a distribution source package.
> +    """
> +
> +    adapts(IDistribution)
> +    implements(IDefaultGitTraversable)
> +
> +    def traverse(self, name, segments):
> +        """See `IDefaultGitTraversable`.
> +
> +        :raise InvalidNamespace: If 'name' is not '+source' or there are no
> +            further segments.
> +        :raise NoSuchSourcePackageName: If the segment after '+source'
> +            doesn't match an existing source package name.
> +        :return: `IDistributionSourcePackage`.
> +        """
> +        if name != "+source" or not segments:
> +            raise InvalidNamespace(name)
> +        spn_name = segments.pop(0)
> +        distro_source_package = self.context.getSourcePackage(spn_name)
> +        if distro_source_package is None:
> +            raise NoSuchSourcePackageName(spn_name)
> +        return distro_source_package
> +
> +
> +class PersonGitTraversable(_BaseGitTraversable):
> +    """Default Git repository traversable for people.
> +
> +    From here, you can traverse to a person-distribution-source-package or a
> +    person-project.
> +    """
> +
> +    adapts(IPerson)
> +    implements(IDefaultGitTraversable)
> +
> +    def traverse(self, name, segments):
> +        """See `IDefaultGitTraversable`.
> +
> +        :raise InvalidNamespace: If 'name' matches an existing distribution,
> +            and the next segment is not '+source' or there are no further
> +            segments.
> +        :raise InvalidProductName: If 'name' is not a valid name.
> +        :raise NoSuchProduct: If 'name' doesn't match an existing pillar.
> +        :raise NoSuchSourcePackageName: If 'name' matches an existing
> +            distribution, and the segment after '+source' doesn't match an
> +            existing source package name.
> +        :return: `IPersonProduct` or `IPersonDistributionSourcePackage`.
> +        """
> +        if not valid_name(name):
> +            raise InvalidProductName(name)
> +        pillar = getUtility(IPillarNameSet).getByName(name)
> +        if pillar is None:
> +            # Actually, the pillar is no such *anything*.
> +            raise NoSuchProduct(name)
> +        # XXX cjwatson 2015-02-23: This would be neater if
> +        # IPersonDistribution existed.
> +        if IDistribution.providedBy(pillar):
> +            if len(segments) < 2:
> +                raise InvalidNamespace(name)
> +            segments.pop(0)
> +            spn_name = segments.pop(0)
> +            distro_source_package = pillar.getSourcePackage(spn_name)
> +            if distro_source_package is None:
> +                raise NoSuchSourcePackageName(spn_name)
> +            return getUtility(IPersonDistributionSourcePackageFactory).create(
> +                self.context, distro_source_package)
> +        else:
> +            return getUtility(IPersonProductFactory).create(
> +                self.context, pillar)

I suspect this whole module might be cleaner if the traversers returned the person and target dimensions separately. All this duplicated code would disappear, as a person wouldn't be traversable -- they'd just be an element in the tuple returned from RootGitTraversable.

In that case this could probably also be used by GitNamespaceSet.traverse, rather than further duplicating most of that code here.

> +
> +
> +class DefaultGitTraverser:
> +    """Utility for traversing to objects that can have default repositories."""
> +
> +    implements(IDefaultGitTraverser)
> +
> +    def traverse(self, path):
> +        """See `IDefaultGitTraverser`."""
> +        segments = path.split("/")
> +        traversable = RootGitTraversable()
> +        while segments:
> +            name = segments.pop(0)
> +            context = traversable.traverse(name, segments)
> +            traversable = adapt(context, IDefaultGitTraversable)
> +            if traversable is None:
> +                break
> +        if segments:
> +            raise InvalidNamespace(path)
> +        return context
> +
> +
> +class GitLookup:
> +    """Utility for looking up Git repositories."""
> +
> +    implements(IGitLookup)
> +
> +    def get(self, repository_id, default=None):
> +        """See `IGitLookup`."""
> +        repository = IStore(GitRepository).get(GitRepository, repository_id)
> +        if repository is None:
> +            return default
> +        return repository
> +
> +    @staticmethod
> +    def uriToHostingPath(uri):
> +        """See `IGitLookup`."""
> +        schemes = ('git', 'git+ssh', 'https', 'ssh')
> +        codehosting_host = URI(config.codehosting.git_anon_root).host
> +        if ((uri.scheme in schemes and uri.host == codehosting_host) or
> +            (uri.scheme == "lp" and uri.host is None)):
> +            return uri.path.lstrip("/")
> +        else:
> +            return None
> +
> +    def getByUrl(self, url):
> +        """See `IGitLookup`."""
> +        if url is None:
> +            return None
> +        url = url.rstrip("/")
> +        try:
> +            uri = URI(url)
> +        except InvalidURIError:
> +            return None
> +
> +        path = self.uriToHostingPath(uri)
> +        if path is None:
> +            return None
> +        try:
> +            return self.getByPath(path)
> +        except (
> +            CannotHaveDefaultGitRepository, InvalidNamespace,
> +            InvalidProductName, NoDefaultGitRepository, NoSuchGitRepository,
> +            NoSuchPerson, NoSuchProduct, NoSuchSourcePackageName):
> +            return None
> +
> +    def getByUniqueName(self, unique_name):
> +        """See `IGitLookup`."""
> +        try:
> +            if unique_name.startswith("~"):
> +                path = unique_name.lstrip("~")
> +                namespace_set = getUtility(IGitNamespaceSet)
> +                segments = iter(path.split("/"))
> +                repository = namespace_set.traverse(segments)
> +                if list(segments):
> +                    raise InvalidNamespace(path)
> +                return repository
> +        except InvalidNamespace:
> +            pass
> +        return None
> +
> +    def getByPath(self, path):
> +        """See `IGitLookup`."""
> +        # Try parsing as a unique name.
> +        repository = self.getByUniqueName(path)
> +        if repository is not None:
> +            return repository
> +
> +        # Try parsing as a shortcut.
> +        object_with_git_repository_default = getUtility(
> +            IDefaultGitTraverser).traverse(path)
> +        default = get_default_git_repository(
> +            object_with_git_repository_default)
> +        return default.repository

getByPath can raise a tonne of exceptions, while getByUrl and getByUniqueName just return None.

> 
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py	2015-02-19 23:57:34 +0000
> +++ lib/lp/code/model/gitrepository.py	2015-02-23 14:47:40 +0000
> @@ -24,21 +24,30 @@
>      Reference,
>      Unicode,
>      )
> -from zope.component import getUtility
> +from zope.component import (
> +    getAdapter,
> +    getUtility,
> +    )
>  from zope.interface import implements
> +from zope.security.proxy import removeSecurityProxy
>  
>  from lp.app.enums import (
>      InformationType,
>      PRIVATE_INFORMATION_TYPES,
>      PUBLIC_INFORMATION_TYPES,
>      )
> +from lp.app.errors import NotFoundError
>  from lp.app.interfaces.informationtype import IInformationType
>  from lp.app.interfaces.launchpad import IPrivacy
> +from lp.app.interfaces.security import IAuthorization
>  from lp.app.interfaces.services import IService
>  from lp.code.errors import (
>      GitDefaultConflict,
>      GitTargetError,
> +    InvalidGitRepositoryException,
> +    InvalidNamespace,
>      )
> +from lp.code.interfaces.gitlookup import IGitLookup
>  from lp.code.interfaces.gitnamespace import (
>      get_git_namespace,
>      IGitNamespacePolicy,
> @@ -59,8 +68,14 @@
>      IDistributionSourcePackage,
>      )
>  from lp.registry.interfaces.person import IPerson
> -from lp.registry.interfaces.product import IProduct
> -from lp.registry.interfaces.role import IHasOwner
> +from lp.registry.interfaces.product import (
> +    InvalidProductName,
> +    IProduct,
> +    )
> +from lp.registry.interfaces.role import (
> +    IHasOwner,
> +    IPersonRoles,
> +    )
>  from lp.registry.interfaces.sharingjob import (
>      IRemoveArtifactSubscriptionsJobSource,
>      )
> @@ -355,8 +370,18 @@
>  
>      def getByPath(self, user, path):
>          """See `IGitRepositorySet`."""
> -        # XXX cjwatson 2015-02-06: Fill this in once IGitLookup is in place.
> -        raise NotImplementedError
> +        try:
> +            repository = getUtility(IGitLookup).getByPath(path)
> +        except (InvalidGitRepositoryException, InvalidNamespace,
> +                InvalidProductName, NotFoundError):
> +            return None
> +        authz = getAdapter(
> +            removeSecurityProxy(repository), IAuthorization, 'launchpad.View')
> +        if ((user is None and authz.checkUnauthenticated()) or
> +            (user is not None and authz.checkAuthenticated(
> +                IPersonRoles(user)))):
> +            return repository

Any reason to not use visibleByUser directly?

> +        return None
>  
>      def getDefaultRepository(self, target, owner=None):
>          """See `IGitRepositorySet`."""
> 
> === added file 'lib/lp/code/model/tests/test_gitlookup.py'
> --- lib/lp/code/model/tests/test_gitlookup.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/model/tests/test_gitlookup.py	2015-02-23 14:47:40 +0000
> @@ -0,0 +1,338 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Tests for the IGitLookup implementation."""
> +
> +__metaclass__ = type
> +
> +from lazr.uri import URI
> +from zope.component import getUtility
> +
> +from lp.code.errors import (
> +    CannotHaveDefaultGitRepository,
> +    InvalidNamespace,
> +    NoDefaultGitRepository,
> +    )
> +from lp.code.interfaces.gitlookup import (
> +    IDefaultGitTraverser,
> +    IGitLookup,
> +    )
> +from lp.registry.errors import NoSuchSourcePackageName
> +from lp.registry.interfaces.person import NoSuchPerson
> +from lp.registry.interfaces.persondistributionsourcepackage import (
> +    IPersonDistributionSourcePackageFactory,
> +    )
> +from lp.registry.interfaces.personproduct import IPersonProductFactory
> +from lp.registry.interfaces.product import (
> +    InvalidProductName,
> +    NoSuchProduct,
> +    )
> +from lp.services.config import config
> +from lp.testing import (
> +    person_logged_in,
> +    TestCaseWithFactory,
> +    )
> +from lp.testing.layers import DatabaseFunctionalLayer
> +
> +
> +class TestGetByUniqueName(TestCaseWithFactory):
> +    """Tests for `IGitLookup.getByUniqueName`."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        super(TestGetByUniqueName, self).setUp()
> +        self.lookup = getUtility(IGitLookup)
> +
> +    def test_not_found(self):
> +        unused_name = self.factory.getUniqueString()
> +        self.assertIsNone(self.lookup.getByUniqueName(unused_name))
> +
> +    def test_project(self):
> +        repository = self.factory.makeGitRepository()
> +        self.assertEqual(
> +            repository, self.lookup.getByUniqueName(repository.unique_name))
> +
> +    def test_package(self):
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        repository = self.factory.makeGitRepository(target=dsp)
> +        self.assertEqual(
> +            repository, self.lookup.getByUniqueName(repository.unique_name))
> +
> +    def test_personal(self):
> +        owner = self.factory.makePerson()
> +        repository = self.factory.makeGitRepository(owner=owner, target=owner)
> +        self.assertEqual(
> +            repository, self.lookup.getByUniqueName(repository.unique_name))
> +
> +
> +class TestGetByPath(TestCaseWithFactory):
> +    """Test `IGitLookup.getByPath`."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        super(TestGetByPath, self).setUp()
> +        self.lookup = getUtility(IGitLookup)
> +
> +    def test_project(self):
> +        repository = self.factory.makeGitRepository()
> +        self.assertEqual(
> +            repository, self.lookup.getByPath(repository.unique_name))
> +
> +    def test_project_default(self):
> +        repository = self.factory.makeGitRepository()
> +        with person_logged_in(repository.target.owner):
> +            repository.target.setDefaultGitRepository(repository)
> +        self.assertEqual(
> +            repository, self.lookup.getByPath(repository.shortened_path))
> +
> +    def test_package(self):
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        repository = self.factory.makeGitRepository(target=dsp)
> +        self.assertEqual(
> +            repository, self.lookup.getByPath(repository.unique_name))
> +
> +    def test_package_default(self):
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        repository = self.factory.makeGitRepository(target=dsp)
> +        with person_logged_in(repository.target.distribution.owner):
> +            repository.target.setDefaultGitRepository(repository)
> +        self.assertEqual(
> +            repository, self.lookup.getByPath(repository.shortened_path))
> +
> +    def test_personal(self):
> +        owner = self.factory.makePerson()
> +        repository = self.factory.makeGitRepository(owner=owner, target=owner)
> +        self.assertEqual(
> +            repository, self.lookup.getByPath(repository.unique_name))
> +
> +    def test_cannot_have_default_git_repository(self):
> +        # If `getByPath` is given a path to something with no default Git
> +        # repository, such as a distribution, it raises
> +        # CannotHaveDefaultGitRepository.
> +        distro = self.factory.makeDistribution()
> +        self.assertRaises(
> +            CannotHaveDefaultGitRepository, self.lookup.getByPath, distro.name)
> +
> +    def test_no_default_git_repository(self):
> +        # If `getByPath` is given a path to something that could have a Git
> +        # repository but doesn't, it raises NoDefaultGitRepository.
> +        project = self.factory.makeProduct()
> +        self.assertRaises(
> +            NoDefaultGitRepository, self.lookup.getByPath, project.name)
> +
> +
> +class TestGetByUrl(TestCaseWithFactory):
> +    """Test `IGitLookup.getByUrl`."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        super(TestGetByUrl, self).setUp()
> +        self.lookup = getUtility(IGitLookup)
> +
> +    def makeProjectRepository(self):
> +        owner = self.factory.makePerson(name="aa")
> +        project = self.factory.makeProduct(name="bb")
> +        return self.factory.makeGitRepository(
> +            owner=owner, target=project, name=u"cc")
> +
> +    def test_getByUrl_with_none(self):
> +        # getByUrl returns None if given None.
> +        self.assertIsNone(self.lookup.getByUrl(None))
> +
> +    def assertUrlMatches(self, url, repository):
> +        self.assertEqual(repository, self.lookup.getByUrl(url))
> +
> +    def test_getByUrl_with_trailing_slash(self):
> +        # Trailing slashes are stripped from the URL prior to searching.
> +        repository = self.makeProjectRepository()
> +        self.assertUrlMatches(
> +            "git://git.launchpad.dev/~aa/bb/+git/cc/", repository)
> +
> +    def test_getByUrl_with_git(self):
> +        # getByUrl recognises LP repositories for git URLs.
> +        repository = self.makeProjectRepository()
> +        self.assertUrlMatches(
> +            "git://git.launchpad.dev/~aa/bb/+git/cc", repository)
> +
> +    def test_getByUrl_with_git_ssh(self):
> +        # getByUrl recognises LP repositories for git+ssh URLs.
> +        repository = self.makeProjectRepository()
> +        self.assertUrlMatches(
> +            "git+ssh://git.launchpad.dev/~aa/bb/+git/cc", repository)
> +
> +    def test_getByUrl_with_https(self):
> +        # getByUrl recognises LP repositories for https URLs.
> +        repository = self.makeProjectRepository()
> +        self.assertUrlMatches(
> +            "https://git.launchpad.dev/~aa/bb/+git/cc";, repository)
> +
> +    def test_getByUrl_with_ssh(self):
> +        # getByUrl recognises LP repositories for ssh URLs.
> +        repository = self.makeProjectRepository()
> +        self.assertUrlMatches(
> +            "ssh://git.launchpad.dev/~aa/bb/+git/cc", repository)
> +
> +    def test_getByUrl_with_ftp(self):
> +        # getByUrl does not recognise LP repositories for ftp URLs.
> +        self.makeProjectRepository()
> +        self.assertIsNone(
> +            self.lookup.getByUrl("ftp://git.launchpad.dev/~aa/bb/+git/cc";))
> +
> +    def test_getByUrl_with_lp(self):
> +        # getByUrl supports lp: URLs.
> +        url = "lp:~aa/bb/+git/cc"
> +        self.assertIsNone(self.lookup.getByUrl(url))
> +        repository = self.makeProjectRepository()
> +        self.assertUrlMatches(url, repository)
> +
> +    def test_getByUrl_with_default(self):
> +        # getByUrl honours default repositories when looking up URLs.
> +        repository = self.makeProjectRepository()
> +        with person_logged_in(repository.target.owner):
> +            repository.target.setDefaultGitRepository(repository)
> +        self.assertUrlMatches("lp:bb", repository)
> +
> +    def test_uriToHostingPath(self):
> +        # uriToHostingPath only supports our own URLs with certain schemes.
> +        uri = URI(config.codehosting.git_anon_root)
> +        uri.path = "/~foo/bar/baz"
> +        # Test valid schemes.
> +        for scheme in ("git", "git+ssh", "https", "ssh"):
> +            uri.scheme = scheme
> +            self.assertEqual("~foo/bar/baz", self.lookup.uriToHostingPath(uri))
> +        # Test an invalid scheme.
> +        uri.scheme = "ftp"
> +        self.assertIsNone(self.lookup.uriToHostingPath(uri))
> +        # Test valid scheme but invalid domain.
> +        uri.scheme = 'sftp'
> +        uri.host = 'example.com'
> +        self.assertIsNone(self.lookup.uriToHostingPath(uri))
> +
> +
> +class TestDefaultGitTraverser(TestCaseWithFactory):
> +    """Tests for the default repository traverser."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        super(TestDefaultGitTraverser, self).setUp()
> +        self.traverser = getUtility(IDefaultGitTraverser)
> +
> +    def assertTraverses(self, path, result):
> +        self.assertEqual(result, self.traverser.traverse(path))
> +
> +    def test_nonexistent_project(self):
> +        # `traverse` raises `NoSuchProduct` when resolving a path of
> +        # 'project' if the project doesn't exist.
> +        self.assertRaises(NoSuchProduct, self.traverser.traverse, "bb")
> +
> +    def test_invalid_project(self):
> +        # `traverse` raises `InvalidProductName` when resolving a path for a
> +        # completely invalid default project repository.
> +        self.assertRaises(InvalidProductName, self.traverser.traverse, "b")
> +
> +    def test_project(self):
> +        # `traverse` resolves the name of a project to the project itself.
> +        project = self.factory.makeProduct()
> +        self.assertTraverses(project.name, project)
> +
> +    def test_no_such_distribution(self):
> +        # `traverse` raises `NoSuchProduct` if the distribution doesn't
> +        # exist.  That's because it can't tell the difference between the
> +        # name of a project that doesn't exist and the name of a
> +        # distribution that doesn't exist.
> +        self.assertRaises(
> +            NoSuchProduct, self.traverser.traverse, "distro/+source/package")
> +
> +    def test_missing_sourcepackagename(self):
> +        # `traverse` raises `InvalidNamespace` if there are no segments
> +        # after '+source'.
> +        self.factory.makeDistribution(name="distro")
> +        self.assertRaises(
> +            InvalidNamespace, self.traverser.traverse, "distro/+source")
> +
> +    def test_no_such_sourcepackagename(self):
> +        # `traverse` raises `NoSuchSourcePackageName` if the package in
> +        # distro/+source/package doesn't exist.
> +        self.factory.makeDistribution(name="distro")
> +        self.assertRaises(
> +            NoSuchSourcePackageName, self.traverser.traverse,
> +            "distro/+source/nonexistent")
> +
> +    def test_package(self):
> +        # `traverse` resolves 'distro/+source/package' to the distribution
> +        # source package.
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        path = "%s/+source/%s" % (
> +            dsp.distribution.name, dsp.sourcepackagename.name)
> +        self.assertTraverses(path, dsp)
> +
> +    def test_nonexistent_person(self):
> +        # `traverse` raises `NoSuchPerson` when resolving a path of
> +        # '~person/project' if the person doesn't exist.
> +        self.assertRaises(NoSuchPerson, self.traverser.traverse, "~person/bb")
> +
> +    def test_nonexistent_person_project(self):
> +        # `traverse` raises `NoSuchProduct` when resolving a path of
> +        # '~person/project' if the project doesn't exist.
> +        self.factory.makePerson(name="person")
> +        self.assertRaises(NoSuchProduct, self.traverser.traverse, "~person/bb")
> +
> +    def test_invalid_person_project(self):
> +        # `traverse` raises `InvalidProductName` when resolving a path for a
> +        # person and a completely invalid default project repository.
> +        self.factory.makePerson(name="person")
> +        self.assertRaises(
> +            InvalidProductName, self.traverser.traverse, "~person/b")
> +
> +    def test_person_project(self):
> +        # `traverse` resolves '~person/project' to a PersonProduct.
> +        person = self.factory.makePerson()
> +        project = self.factory.makeProduct()
> +        person_project = getUtility(IPersonProductFactory).create(
> +            person, project)
> +        self.assertTraverses(
> +            "~%s/%s" % (person.name, project.name), person_project)
> +
> +    def test_no_such_person_distribution(self):
> +        # `traverse` raises `NoSuchProduct` when resolving a path of
> +        # '~person/distro' if the distribution doesn't exist.  That's
> +        # because it can't tell the difference between the name of a project
> +        # that doesn't exist and the name of a distribution that doesn't
> +        # exist.
> +        self.factory.makePerson(name="person")
> +        self.assertRaises(
> +            NoSuchProduct, self.traverser.traverse,
> +            "~person/distro/+source/package")
> +
> +    def test_missing_person_sourcepackagename(self):
> +        # `traverse` raises `InvalidNamespace` if there are no segments
> +        # after '+source' in a person-DSP path.
> +        self.factory.makePerson(name="person")
> +        self.factory.makeDistribution(name="distro")
> +        self.assertRaises(
> +            InvalidNamespace, self.traverser.traverse,
> +            "~person/distro/+source")
> +
> +    def test_no_such_person_sourcepackagename(self):
> +        # `traverse` raises `NoSuchSourcePackageName` if the package in
> +        # ~person/distro/+source/package doesn't exist.
> +        self.factory.makePerson(name="person")
> +        self.factory.makeDistribution(name="distro")
> +        self.assertRaises(
> +            NoSuchSourcePackageName, self.traverser.traverse,
> +            "~person/distro/+source/nonexistent")
> +
> +    def test_person_package(self):
> +        # `traverse` resolves '~person/distro/+source/package' to a
> +        # PersonDistributionSourcePackage.
> +        person = self.factory.makePerson()
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        person_dsp = getUtility(
> +            IPersonDistributionSourcePackageFactory).create(person, dsp)
> +        path = "~%s/%s/+source/%s" % (
> +            person.name, dsp.distribution.name, dsp.sourcepackagename.name)
> +        self.assertTraverses(path, person_dsp)
> 
> === modified file 'lib/lp/code/model/tests/test_gitrepository.py'
> --- lib/lp/code/model/tests/test_gitrepository.py	2015-02-23 14:47:40 +0000
> +++ lib/lp/code/model/tests/test_gitrepository.py	2015-02-23 14:47:40 +0000
> @@ -29,7 +29,10 @@
>      IGitNamespacePolicy,
>      IGitNamespaceSet,
>      )
> -from lp.code.interfaces.gitrepository import IGitRepository
> +from lp.code.interfaces.gitrepository import (
> +    IGitRepository,
> +    IGitRepositorySet,
> +    )
>  from lp.registry.enums import BranchSharingPolicy
>  from lp.registry.interfaces.persondistributionsourcepackage import (
>      IPersonDistributionSourcePackageFactory,
> @@ -486,3 +489,27 @@
>              self.assertRaises(
>                  GitTargetError, repository.setTarget,
>                  target=commercial_project, user=owner)
> +
> +
> +class TestGitRepositorySet(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_provides_IGitRepositorySet(self):
> +        # GitRepositorySet instances provide IGitRepositorySet.
> +        verifyObject(IGitRepositorySet, getUtility(IGitRepositorySet))
> +
> +    def test_getByPath(self):
> +        # getByPath returns a repository matching the path that it's given.
> +        a = self.factory.makeGitRepository()
> +        self.factory.makeGitRepository()
> +        repository = getUtility(IGitRepositorySet).getByPath(
> +            a.owner, a.shortened_path)
> +        self.assertEqual(a, repository)
> +
> +    def test_getByPath_not_found(self):
> +        # If a repository cannot be found for a path, then getByPath returns
> +        # None.
> +        person = self.factory.makePerson()
> +        self.assertIsNone(
> +            getUtility(IGitRepositorySet).getByPath(person, "nonexistent"))
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-lookup/+merge/250628
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.