Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-initial-name-bzr 
into lp:launchpad with lp:~cjwatson/launchpad/better-bzr-mp-diffs as a 
prerequisite.

Commit message:
Extract initial Snap.store_name from snapcraft.yaml for Bazaar as well as Git.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-initial-name-bzr/+merge/345757

The potential timeout issues will be at least as bad as with git, but 
BranchHostingClient has all the same sort of timeout management code as we put 
together for GitHostingClient, which should mitigate that.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
lp:~cjwatson/launchpad/snap-initial-name-bzr into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2018-05-17 14:25:53 +0000
+++ lib/lp/code/interfaces/branch.py	2018-05-17 14:25:53 +0000
@@ -765,6 +765,15 @@
         :param launchbag: `ILaunchBag`.
         """
 
+    def getBlob(filename, revision_id=None):
+        """Get a blob by file name from this branch.
+
+        :param filename: Relative path of a file in the branch.
+        :param revision_id: An optional revision ID.  Defaults to the last
+            scanned revision ID of the branch.
+        :return: The blob content as a byte string.
+        """
+
     def getDiff(new, old):
         """Get the diff between two revisions in this branch.
 

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2018-05-17 14:25:53 +0000
+++ lib/lp/code/model/branch.py	2018-05-17 14:25:53 +0000
@@ -10,12 +10,15 @@
 
 from datetime import datetime
 from functools import partial
+import json
 import operator
+import os.path
 
 from bzrlib import urlutils
 from bzrlib.revision import NULL_REVISION
 from lazr.lifecycle.event import ObjectCreatedEvent
 import pytz
+from six.moves.urllib_parse import urlsplit
 from sqlobject import (
     ForeignKey,
     IntCol,
@@ -84,6 +87,7 @@
     )
 from lp.code.errors import (
     AlreadyLatestFormat,
+    BranchFileNotFound,
     BranchMergeProposalExists,
     BranchTargetError,
     BranchTypeError,
@@ -172,10 +176,12 @@
     ArrayAgg,
     ArrayIntersects,
     )
+from lp.services.features import getFeatureFlag
 from lp.services.helpers import shortlist
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.mail.notificationrecipientset import NotificationRecipientSet
+from lp.services.memcache.interfaces import IMemcacheClient
 from lp.services.propertycache import (
     cachedproperty,
     get_property_cache,
@@ -783,6 +789,63 @@
                 RevisionAuthor, revisions, ['revision_author_id'])
         return DecoratedResultSet(result, pre_iter_hook=eager_load)
 
+    def getBlob(self, filename, revision_id=None, enable_memcache=None,
+                logger=None):
+        """See `IBranch`."""
+        hosting_client = getUtility(IBranchHostingClient)
+        if enable_memcache is None:
+            enable_memcache = not getFeatureFlag(
+                u'code.bzr.blob.disable_memcache')
+        if revision_id is None:
+            revision_id = self.last_scanned_id
+        if revision_id is None:
+            # revision_id may still be None if the branch scanner hasn't
+            # scanned this branch yet.  In this case, we won't be able to
+            # guarantee that subsequent calls to this method within the same
+            # transaction with revision_id=None will see the same revision,
+            # and we won't be able to cache file lists.  Neither is fatal,
+            # and this should be relatively rare.
+            enable_memcache = False
+        dirname = os.path.dirname(filename)
+        unset = object()
+        file_list = unset
+        if enable_memcache:
+            memcache_client = getUtility(IMemcacheClient)
+            instance_name = urlsplit(
+                config.codehosting.internal_bzr_api_endpoint).hostname
+            memcache_key = '%s:bzr-file-list:%s:%s:%s' % (
+                instance_name, self.id, revision_id, dirname)
+            if not isinstance(memcache_key, bytes):
+                memcache_key = memcache_key.encode('UTF-8')
+            cached_file_list = memcache_client.get(memcache_key)
+            if cached_file_list is not None:
+                try:
+                    file_list = json.loads(cached_file_list)
+                except Exception:
+                    logger.exception(
+                        'Cannot load cached file list for %s:%s:%s; deleting' %
+                        (self.unique_name, revision_id, dirname))
+                    memcache_client.delete(memcache_key)
+        if file_list is unset:
+            try:
+                inventory = hosting_client.getInventory(
+                    self.unique_name, dirname, rev=revision_id)
+                file_list = {
+                    entry['filename']: entry['file_id']
+                    for entry in inventory['filelist']}
+            except BranchFileNotFound:
+                file_list = None
+            if enable_memcache:
+                # Cache the file list in case there's a request for another
+                # file in the same directory.
+                memcache_client.set(memcache_key, json.dumps(file_list))
+        file_id = (file_list or {}).get(os.path.basename(filename))
+        if file_id is None:
+            raise BranchFileNotFound(
+                self.unique_name, filename=filename, rev=revision_id)
+        return hosting_client.getBlob(
+            self.unique_name, file_id, rev=revision_id)
+
     def getDiff(self, new, old=None):
         """See `IBranch`."""
         hosting_client = getUtility(IBranchHostingClient)

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2018-03-06 00:59:06 +0000
+++ lib/lp/code/model/tests/test_branch.py	2018-05-17 14:25:53 +0000
@@ -11,6 +11,7 @@
     datetime,
     timedelta,
     )
+import json
 
 from bzrlib.branch import Branch
 from bzrlib.bzrdir import BzrDir
@@ -61,6 +62,7 @@
     AlreadyLatestFormat,
     BranchCreatorNotMemberOfOwnerTeam,
     BranchCreatorNotOwner,
+    BranchFileNotFound,
     BranchTargetError,
     CannotDeleteBranch,
     CannotUpgradeNonHosted,
@@ -109,7 +111,10 @@
 from lp.code.model.branchrevision import BranchRevision
 from lp.code.model.codereviewcomment import CodeReviewComment
 from lp.code.model.revision import Revision
-from lp.code.tests.helpers import add_revision_to_branch
+from lp.code.tests.helpers import (
+    add_revision_to_branch,
+    BranchHostingFixture,
+    )
 from lp.codehosting.safe_open import BadUrl
 from lp.codehosting.vfs.branchfs import get_real_branch_path
 from lp.registry.enums import (
@@ -136,6 +141,7 @@
     block_on_job,
     monitor_celery,
     )
+from lp.services.memcache.interfaces import IMemcacheClient
 from lp.services.osutils import override_environ
 from lp.services.propertycache import clear_property_cache
 from lp.services.webapp.authorization import check_permission
@@ -159,6 +165,7 @@
     )
 from lp.testing.dbuser import dbuser
 from lp.testing.factory import LaunchpadObjectFactory
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import (
     CeleryBranchWriteJobLayer,
     CeleryBzrsyncdJobLayer,
@@ -3293,6 +3300,150 @@
         self.assertRaises(BadUrl, db_stacked.getBzrBranch)
 
 
+class TestBranchGetBlob(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_default_rev_unscanned(self):
+        branch = self.factory.makeBranch()
+        hosting_fixture = self.useFixture(BranchHostingFixture(
+            file_list={'README.txt': 'some-file-id'}, blob=b'Some text'))
+        blob = branch.getBlob('src/README.txt')
+        self.assertEqual('Some text', blob)
+        self.assertEqual(
+            [((branch.unique_name, 'src'), {'rev': None})],
+            hosting_fixture.getInventory.calls)
+        self.assertEqual(
+            [((branch.unique_name, 'some-file-id'), {'rev': None})],
+            hosting_fixture.getBlob.calls)
+        self.assertEqual({}, getUtility(IMemcacheClient)._cache)
+
+    def test_default_rev_scanned(self):
+        branch = self.factory.makeBranch()
+        removeSecurityProxy(branch).last_scanned_id = 'scanned-id'
+        hosting_fixture = self.useFixture(BranchHostingFixture(
+            file_list={'README.txt': 'some-file-id'}, blob=b'Some text'))
+        blob = branch.getBlob('src/README.txt')
+        self.assertEqual('Some text', blob)
+        self.assertEqual(
+            [((branch.unique_name, 'src'), {'rev': 'scanned-id'})],
+            hosting_fixture.getInventory.calls)
+        self.assertEqual(
+            [((branch.unique_name, 'some-file-id'), {'rev': 'scanned-id'})],
+            hosting_fixture.getBlob.calls)
+        key = (
+            'bazaar.launchpad.dev:bzr-file-list:%s:scanned-id:src' % branch.id)
+        self.assertEqual(
+            json.dumps({'README.txt': 'some-file-id'}),
+            getUtility(IMemcacheClient).get(key.encode('UTF-8')))
+
+    def test_with_rev(self):
+        branch = self.factory.makeBranch()
+        hosting_fixture = self.useFixture(BranchHostingFixture(
+            file_list={'README.txt': 'some-file-id'}, blob=b'Some text'))
+        blob = branch.getBlob('src/README.txt', revision_id='some-rev')
+        self.assertEqual('Some text', blob)
+        self.assertEqual(
+            [((branch.unique_name, 'src'), {'rev': 'some-rev'})],
+            hosting_fixture.getInventory.calls)
+        self.assertEqual(
+            [((branch.unique_name, 'some-file-id'), {'rev': 'some-rev'})],
+            hosting_fixture.getBlob.calls)
+        key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:src' % branch.id
+        self.assertEqual(
+            json.dumps({'README.txt': 'some-file-id'}),
+            getUtility(IMemcacheClient).get(key.encode('UTF-8')))
+
+    def test_cached_inventory(self):
+        branch = self.factory.makeBranch()
+        hosting_fixture = self.useFixture(BranchHostingFixture(
+            blob=b'Some text'))
+        key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:src' % branch.id
+        getUtility(IMemcacheClient).set(
+            key.encode('UTF-8'), json.dumps({'README.txt': 'some-file-id'}))
+        blob = branch.getBlob('src/README.txt', revision_id='some-rev')
+        self.assertEqual('Some text', blob)
+        self.assertEqual([], hosting_fixture.getInventory.calls)
+        self.assertEqual(
+            [((branch.unique_name, 'some-file-id'), {'rev': 'some-rev'})],
+            hosting_fixture.getBlob.calls)
+
+    def test_disable_memcache(self):
+        self.useFixture(FeatureFixture(
+            {'code.bzr.blob.disable_memcache': 'on'}))
+        branch = self.factory.makeBranch()
+        hosting_fixture = self.useFixture(BranchHostingFixture(
+            file_list={'README.txt': 'some-file-id'}, blob=b'Some text'))
+        key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:src' % branch.id
+        getUtility(IMemcacheClient).set(key.encode('UTF-8'), '{}')
+        blob = branch.getBlob('src/README.txt', revision_id='some-rev')
+        self.assertEqual('Some text', blob)
+        self.assertEqual(
+            [((branch.unique_name, 'src'), {'rev': 'some-rev'})],
+            hosting_fixture.getInventory.calls)
+        self.assertEqual(
+            '{}', getUtility(IMemcacheClient).get(key.encode('UTF-8')))
+
+    def test_file_at_root_of_branch(self):
+        branch = self.factory.makeBranch()
+        hosting_fixture = self.useFixture(BranchHostingFixture(
+            file_list={'README.txt': 'some-file-id'}, blob=b'Some text'))
+        blob = branch.getBlob('README.txt', revision_id='some-rev')
+        self.assertEqual('Some text', blob)
+        self.assertEqual(
+            [((branch.unique_name, ''), {'rev': 'some-rev'})],
+            hosting_fixture.getInventory.calls)
+        self.assertEqual(
+            [((branch.unique_name, 'some-file-id'), {'rev': 'some-rev'})],
+            hosting_fixture.getBlob.calls)
+        key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:' % branch.id
+        self.assertEqual(
+            json.dumps({'README.txt': 'some-file-id'}),
+            getUtility(IMemcacheClient).get(key.encode('UTF-8')))
+
+    def test_file_not_in_directory(self):
+        branch = self.factory.makeBranch()
+        hosting_fixture = self.useFixture(BranchHostingFixture(file_list={}))
+        self.assertRaises(
+            BranchFileNotFound, branch.getBlob,
+            'src/README.txt', revision_id='some-rev')
+        self.assertEqual(
+            [((branch.unique_name, 'src'), {'rev': 'some-rev'})],
+            hosting_fixture.getInventory.calls)
+        self.assertEqual([], hosting_fixture.getBlob.calls)
+        key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:src' % branch.id
+        self.assertEqual(
+            '{}', getUtility(IMemcacheClient).get(key.encode('UTF-8')))
+
+    def test_missing_directory(self):
+        branch = self.factory.makeBranch()
+        hosting_fixture = self.useFixture(BranchHostingFixture())
+        hosting_fixture.getInventory = FakeMethod(
+            failure=BranchFileNotFound(
+                branch.unique_name, filename='src', rev='some-rev'))
+        self.assertRaises(
+            BranchFileNotFound, branch.getBlob,
+            'src/README.txt', revision_id='some-rev')
+        self.assertEqual(
+            [((branch.unique_name, 'src'), {'rev': 'some-rev'})],
+            hosting_fixture.getInventory.calls)
+        self.assertEqual([], hosting_fixture.getBlob.calls)
+        key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:src' % branch.id
+        self.assertEqual(
+            'null', getUtility(IMemcacheClient).get(key.encode('UTF-8')))
+
+    def test_cached_missing_directory(self):
+        branch = self.factory.makeBranch()
+        hosting_fixture = self.useFixture(BranchHostingFixture())
+        key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:src' % branch.id
+        getUtility(IMemcacheClient).set(key.encode('UTF-8'), 'null')
+        self.assertRaises(
+            BranchFileNotFound, branch.getBlob,
+            'src/README.txt', revision_id='some-rev')
+        self.assertEqual([], hosting_fixture.getInventory.calls)
+        self.assertEqual([], hosting_fixture.getBlob.calls)
+
+
 class TestBranchUnscan(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/code/tests/helpers.py'
--- lib/lp/code/tests/helpers.py	2018-05-17 14:25:53 +0000
+++ lib/lp/code/tests/helpers.py	2018-05-17 14:25:53 +0000
@@ -306,14 +306,33 @@
 class BranchHostingFixture(fixtures.Fixture):
     """A fixture that temporarily registers a fake Bazaar hosting client."""
 
-    def __init__(self, diff=None, inventory=None, blob=None):
+    def __init__(self, diff=None, inventory=None, file_list=None, blob=None,
+                 disable_memcache=True):
         self.create = FakeMethod()
         self.getDiff = FakeMethod(result=diff or {})
-        self.getInventory = FakeMethod(result=inventory or {})
+        if inventory is None:
+            if file_list is not None:
+                # Simple common case.
+                inventory = {
+                    "filelist": [
+                        {"filename": filename, "file_id": file_id}
+                        for filename, file_id in file_list.items()],
+                    }
+            else:
+                inventory = {"filelist": []}
+        self.getInventory = FakeMethod(result=inventory)
         self.getBlob = FakeMethod(result=blob)
+        self.disable_memcache = disable_memcache
 
     def _setUp(self):
         self.useFixture(ZopeUtilityFixture(self, IBranchHostingClient))
+        if self.disable_memcache:
+            # Most tests that involve Branch.getBlob don't want to cache the
+            # result: doing so requires more time-consuming test setup and
+            # makes it awkward to repeat the same call with different
+            # responses.  For convenience, we make it easy to disable that
+            # here.
+            self.memcache_fixture = self.useFixture(MemcacheFixture())
 
 
 class GitHostingFixture(fixtures.Fixture):

=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py	2018-04-21 10:01:22 +0000
+++ lib/lp/snappy/browser/snap.py	2018-05-17 14:25:53 +0000
@@ -23,7 +23,6 @@
     copy_field,
     use_template,
     )
-import yaml
 from zope.component import getUtility
 from zope.error.interfaces import IErrorReportingUtility
 from zope.interface import Interface
@@ -44,6 +43,7 @@
 from lp.app.browser.lazrjs import InlinePersonEditPickerWidget
 from lp.app.browser.tales import format_link
 from lp.app.enums import PRIVATE_INFORMATION_TYPES
+from lp.app.errors import NotFoundError
 from lp.app.interfaces.informationtype import IInformationType
 from lp.app.widgets.itemswidgets import (
     LabeledMultiCheckBoxWidget,
@@ -52,10 +52,6 @@
     )
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.code.browser.widgets.gitref import GitRefWidget
-from lp.code.errors import (
-    GitRepositoryBlobNotFound,
-    GitRepositoryScanFault,
-    )
 from lp.code.interfaces.gitref import IGitRef
 from lp.registry.enums import VCSType
 from lp.registry.interfaces.pocket import PackagePublishingPocket
@@ -83,6 +79,8 @@
 from lp.snappy.browser.widgets.storechannels import StoreChannelsWidget
 from lp.snappy.interfaces.snap import (
     CannotAuthorizeStoreUploads,
+    CannotFetchSnapcraftYaml,
+    CannotParseSnapcraftYaml,
     ISnap,
     ISnapSet,
     NoSuchSnap,
@@ -424,34 +422,19 @@
     @property
     def initial_values(self):
         store_name = None
-        if self.has_snappy_distro_series and IGitRef.providedBy(self.context):
+        if self.has_snappy_distro_series:
             # Try to extract Snap store name from snapcraft.yaml file.
             try:
-                paths = (
-                    'snap/snapcraft.yaml',
-                    'snapcraft.yaml',
-                    '.snapcraft.yaml',
-                    )
-                for i, path in enumerate(paths):
-                    try:
-                        blob = self.context.repository.getBlob(
-                            path, self.context.name)
-                        break
-                    except GitRepositoryBlobNotFound:
-                        if i == len(paths) - 1:
-                            raise
-                # Beware of unsafe yaml.load()!
-                store_name = yaml.safe_load(blob).get('name')
-            except GitRepositoryScanFault:
-                log.exception("Failed to get Snap manifest from Git %s",
-                              self.context.unique_name)
-            except (AttributeError, yaml.YAMLError):
-                # Ignore parsing errors from invalid, user-supplied YAML
+                snapcraft_data = getUtility(ISnapSet).getSnapcraftYaml(
+                    self.context, logger=log)
+            except (NotFoundError, CannotFetchSnapcraftYaml,
+                    CannotParseSnapcraftYaml):
                 pass
-            except Exception as e:
-                log.exception(
-                    "Failed to extract name from Snap manifest at Git %s: %s",
-                    self.context.unique_name, unicode(e))
+            else:
+                try:
+                    store_name = snapcraft_data.get('name')
+                except AttributeError:
+                    pass
 
         store_series = getUtility(ISnappySeriesSet).getAll().first()
         if store_series.preferred_distro_series is not None:

=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py	2018-05-07 05:25:27 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py	2018-05-17 14:25:53 +0000
@@ -25,7 +25,6 @@
     HTTMock,
     )
 from mechanize import LinkNotFoundError
-import mock
 from pymacaroons import Macaroon
 import pytz
 import soupmatchers
@@ -43,10 +42,13 @@
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.code.errors import (
-    GitRepositoryBlobNotFound,
+    BranchHostingFault,
     GitRepositoryScanFault,
     )
-from lp.code.tests.helpers import GitHostingFixture
+from lp.code.tests.helpers import (
+    BranchHostingFixture,
+    GitHostingFixture,
+    )
 from lp.registry.enums import PersonVisibility
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
@@ -260,7 +262,7 @@
             MatchesTagText(content, "store_upload"))
 
     def test_create_new_snap_git(self):
-        self.useFixture(GitHostingFixture(blob=""))
+        self.useFixture(GitHostingFixture(blob=b""))
         [git_ref] = self.factory.makeGitRefs()
         source_display = git_ref.display_name
         browser = self.getViewBrowser(
@@ -508,74 +510,57 @@
         self.assertContentEqual(
             ["386", "amd64"], [proc.name for proc in snap.processors])
 
-    def test_initial_name_extraction_git_snap_snapcraft_yaml(self):
-        def getBlob(filename, *args, **kwargs):
-            if filename == "snap/snapcraft.yaml":
-                return "name: test-snap"
-            else:
-                raise GitRepositoryBlobNotFound("dummy", filename)
-
-        [git_ref] = self.factory.makeGitRefs()
-        git_ref.repository.getBlob = getBlob
-        view = create_initialized_view(git_ref, "+new-snap")
-        initial_values = view.initial_values
-        self.assertIn('store_name', initial_values)
-        self.assertEqual('test-snap', initial_values['store_name'])
-
-    def test_initial_name_extraction_git_plain_snapcraft_yaml(self):
-        def getBlob(filename, *args, **kwargs):
-            if filename == "snapcraft.yaml":
-                return "name: test-snap"
-            else:
-                raise GitRepositoryBlobNotFound("dummy", filename)
-
-        [git_ref] = self.factory.makeGitRefs()
-        git_ref.repository.getBlob = getBlob
-        view = create_initialized_view(git_ref, "+new-snap")
-        initial_values = view.initial_values
-        self.assertIn('store_name', initial_values)
-        self.assertEqual('test-snap', initial_values['store_name'])
-
-    def test_initial_name_extraction_git_dot_snapcraft_yaml(self):
-        def getBlob(filename, *args, **kwargs):
-            if filename == ".snapcraft.yaml":
-                return "name: test-snap"
-            else:
-                raise GitRepositoryBlobNotFound("dummy", filename)
-
-        [git_ref] = self.factory.makeGitRefs()
-        git_ref.repository.getBlob = getBlob
-        view = create_initialized_view(git_ref, "+new-snap")
-        initial_values = view.initial_values
-        self.assertIn('store_name', initial_values)
-        self.assertEqual('test-snap', initial_values['store_name'])
-
-    def test_initial_name_extraction_git_repo_error(self):
-        [git_ref] = self.factory.makeGitRefs()
-        git_ref.repository.getBlob = FakeMethod(failure=GitRepositoryScanFault)
-        view = create_initialized_view(git_ref, "+new-snap")
-        initial_values = view.initial_values
-        self.assertIn('store_name', initial_values)
-        self.assertIsNone(initial_values['store_name'])
-
-    def test_initial_name_extraction_git_invalid_data(self):
-        for invalid_result in (None, 123, '', '[][]', '#name:test', ']'):
-            [git_ref] = self.factory.makeGitRefs()
-            git_ref.repository.getBlob = FakeMethod(result=invalid_result)
-            view = create_initialized_view(git_ref, "+new-snap")
-            initial_values = view.initial_values
-            self.assertIn('store_name', initial_values)
-            self.assertIsNone(initial_values['store_name'])
-
-    def test_initial_name_extraction_git_safe_yaml(self):
-        [git_ref] = self.factory.makeGitRefs()
-        git_ref.repository.getBlob = FakeMethod(result='Malicious YAML!')
-        view = create_initialized_view(git_ref, "+new-snap")
-        with mock.patch('yaml.load') as unsafe_load:
-            with mock.patch('yaml.safe_load') as safe_load:
-                view.initial_values
-        self.assertEqual(0, unsafe_load.call_count)
-        self.assertEqual(1, safe_load.call_count)
+    def test_initial_name_extraction_bzr_success(self):
+        self.useFixture(BranchHostingFixture(
+            file_list={"snapcraft.yaml": "file-id"}, blob=b"name: test-snap"))
+        branch = self.factory.makeBranch()
+        view = create_initialized_view(branch, "+new-snap")
+        initial_values = view.initial_values
+        self.assertIn('store_name', initial_values)
+        self.assertEqual('test-snap', initial_values['store_name'])
+
+    def test_initial_name_extraction_bzr_error(self):
+        self.useFixture(BranchHostingFixture()).getInventory = FakeMethod(
+            failure=BranchHostingFault)
+        branch = self.factory.makeBranch()
+        view = create_initialized_view(branch, "+new-snap")
+        initial_values = view.initial_values
+        self.assertIn('store_name', initial_values)
+        self.assertIsNone(initial_values['store_name'])
+
+    def test_initial_name_extraction_bzr_no_name(self):
+        self.useFixture(BranchHostingFixture(
+            file_list={"snapcraft.yaml": "file-id"}, blob=b"some: nonsense"))
+        branch = self.factory.makeBranch()
+        view = create_initialized_view(branch, "+new-snap")
+        initial_values = view.initial_values
+        self.assertIn('store_name', initial_values)
+        self.assertIsNone(initial_values['store_name'])
+
+    def test_initial_name_extraction_git_success(self):
+        self.useFixture(GitHostingFixture(blob=b"name: test-snap"))
+        [git_ref] = self.factory.makeGitRefs()
+        view = create_initialized_view(git_ref, "+new-snap")
+        initial_values = view.initial_values
+        self.assertIn('store_name', initial_values)
+        self.assertEqual('test-snap', initial_values['store_name'])
+
+    def test_initial_name_extraction_git_error(self):
+        self.useFixture(GitHostingFixture()).getBlob = FakeMethod(
+            failure=GitRepositoryScanFault)
+        [git_ref] = self.factory.makeGitRefs()
+        view = create_initialized_view(git_ref, "+new-snap")
+        initial_values = view.initial_values
+        self.assertIn('store_name', initial_values)
+        self.assertIsNone(initial_values['store_name'])
+
+    def test_initial_name_extraction_git_no_name(self):
+        self.useFixture(GitHostingFixture(blob=b"some: nonsense"))
+        [git_ref] = self.factory.makeGitRefs()
+        view = create_initialized_view(git_ref, "+new-snap")
+        initial_values = view.initial_values
+        self.assertIn('store_name', initial_values)
+        self.assertIsNone(initial_values['store_name'])
 
 
 class TestSnapAdminView(BaseTestSnapView):

=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py	2018-05-07 05:25:27 +0000
+++ lib/lp/snappy/interfaces/snap.py	2018-05-17 14:25:53 +0000
@@ -9,7 +9,9 @@
     'BadSnapSearchContext',
     'BadSnapSource',
     'CannotAuthorizeStoreUploads',
+    'CannotFetchSnapcraftYaml',
     'CannotModifySnapProcessor',
+    'CannotParseSnapcraftYaml',
     'CannotRequestAutoBuilds',
     'DuplicateSnapName',
     'ISnap',
@@ -237,6 +239,14 @@
             "because %s is not set." % field)
 
 
+class CannotFetchSnapcraftYaml(Exception):
+    """Launchpad cannot fetch this snap package's snapcraft.yaml."""
+
+
+class CannotParseSnapcraftYaml(Exception):
+    """Launchpad cannot parse this snap package's snapcraft.yaml."""
+
+
 class ISnapView(Interface):
     """`ISnap` attributes that require launchpad.View permission."""
 
@@ -787,6 +797,22 @@
     def preloadDataForSnaps(snaps, user):
         """Load the data related to a list of snap packages."""
 
+    def getSnapcraftYaml(context, logger=None):
+        """Fetch a package's snapcraft.yaml from code hosting, if possible.
+
+        :param context: Either an `ISnap` or the source branch for a snap
+            package.
+        :param logger: An optional logger.
+
+        :return: The package's parsed snapcraft.yaml.
+        :raises NotFoundError: if this package has no snapcraft.yaml.
+        :raises CannotFetchSnapcraftYaml: if it was not possible to fetch
+            snapcraft.yaml from the code hosting backend for some other
+            reason.
+        :raises CannotParseSnapcraftYaml: if the fetched snapcraft.yaml
+            cannot be parsed.
+        """
+
     def makeAutoBuilds(logger=None):
         """Create and return automatic builds for stale snap packages.
 

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2018-04-21 10:01:22 +0000
+++ lib/lp/snappy/model/snap.py	2018-05-17 14:25:53 +0000
@@ -32,6 +32,7 @@
     Storm,
     Unicode,
     )
+import yaml
 from zope.component import (
     getAdapter,
     getUtility,
@@ -42,7 +43,10 @@
 
 from lp.app.browser.tales import DateTimeFormatterAPI
 from lp.app.enums import PRIVATE_INFORMATION_TYPES
-from lp.app.errors import IncompatibleArguments
+from lp.app.errors import (
+    IncompatibleArguments,
+    NotFoundError,
+    )
 from lp.app.interfaces.security import IAuthorization
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
@@ -50,6 +54,12 @@
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.buildmaster.model.processor import Processor
+from lp.code.errors import (
+    BranchFileNotFound,
+    BranchHostingFault,
+    GitRepositoryBlobNotFound,
+    GitRepositoryScanFault,
+    )
 from lp.code.interfaces.branch import IBranch
 from lp.code.interfaces.branchcollection import (
     IAllBranches,
@@ -110,7 +120,9 @@
     BadSnapSearchContext,
     BadSnapSource,
     CannotAuthorizeStoreUploads,
+    CannotFetchSnapcraftYaml,
     CannotModifySnapProcessor,
+    CannotParseSnapcraftYaml,
     CannotRequestAutoBuilds,
     DuplicateSnapName,
     ISnap,
@@ -923,6 +935,52 @@
         list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
             person_ids, need_validity=True))
 
+    def getSnapcraftYaml(self, context, logger=None):
+        """See `ISnapSet`."""
+        if ISnap.providedBy(context):
+            context = context.source
+        try:
+            paths = (
+                "snap/snapcraft.yaml",
+                "snapcraft.yaml",
+                ".snapcraft.yaml",
+                )
+            for path in paths:
+                try:
+                    if IBranch.providedBy(context):
+                        blob = context.getBlob(path)
+                    else:
+                        blob = context.repository.getBlob(path, context.name)
+                    break
+                except (BranchFileNotFound, GitRepositoryBlobNotFound):
+                    pass
+            else:
+                msg = "Cannot find snapcraft.yaml in %s"
+                if logger is not None:
+                    logger.exception(msg, context.unique_name)
+                raise NotFoundError(msg % context.unique_name)
+        except (BranchHostingFault, GitRepositoryScanFault) as e:
+            msg = "Failed to get snap manifest from %s"
+            if logger is not None:
+                logger.exception(msg, context.unique_name)
+            raise CannotFetchSnapcraftYaml(
+                "%s: %s" % (msg % context.unique_name, e))
+
+        try:
+            snapcraft_data = yaml.safe_load(blob)
+        except Exception as e:
+            # Don't bother logging parsing errors from user-supplied YAML.
+            raise CannotParseSnapcraftYaml(
+                "Cannot parse snapcraft.yaml from %s: %s" %
+                (context.unique_name, e))
+
+        if not isinstance(snapcraft_data, dict):
+            raise CannotParseSnapcraftYaml(
+                "The top level of snapcraft.yaml from %s is not a mapping" %
+                context.unique_name)
+
+        return snapcraft_data
+
     @staticmethod
     def _findStaleSnaps():
         """See `ISnapSet`."""

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2018-04-21 10:01:22 +0000
+++ lib/lp/snappy/tests/test_snap.py	2018-05-17 14:25:53 +0000
@@ -14,6 +14,7 @@
 import json
 from urlparse import urlsplit
 
+from fixtures import MockPatch
 from httmock import (
     all_requests,
     HTTMock,
@@ -46,6 +47,16 @@
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.buildqueue import BuildQueue
+from lp.code.errors import (
+    BranchFileNotFound,
+    BranchHostingFault,
+    GitRepositoryBlobNotFound,
+    GitRepositoryScanFault,
+    )
+from lp.code.tests.helpers import (
+    BranchHostingFixture,
+    GitHostingFixture,
+    )
 from lp.registry.enums import PersonVisibility
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
@@ -65,7 +76,9 @@
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.snappy.interfaces.snap import (
     BadSnapSearchContext,
+    CannotFetchSnapcraftYaml,
     CannotModifySnapProcessor,
+    CannotParseSnapcraftYaml,
     ISnap,
     ISnapSet,
     ISnapView,
@@ -978,6 +991,145 @@
             [snaps[0], snaps[2], snaps[4], snaps[6]],
             getUtility(ISnapSet).findByURLPrefixes(prefixes, owner=owners[0]))
 
+    def test_getSnapcraftYaml_bzr_snap_snapcraft_yaml(self):
+        def getInventory(unique_name, dirname, *args, **kwargs):
+            if dirname == "snap":
+                return {"filelist": [{
+                    "filename": "snapcraft.yaml", "file_id": "some-file-id",
+                    }]}
+            else:
+                raise BranchFileNotFound("dummy", dirname)
+
+        self.useFixture(BranchHostingFixture(
+            blob=b"name: test-snap")).getInventory = getInventory
+        branch = self.factory.makeBranch()
+        self.assertEqual(
+            {"name": "test-snap"},
+            getUtility(ISnapSet).getSnapcraftYaml(branch))
+
+    def test_getSnapcraftYaml_bzr_plain_snapcraft_yaml(self):
+        def getInventory(unique_name, dirname, *args, **kwargs):
+            if dirname == "":
+                return {"filelist": [{
+                    "filename": "snapcraft.yaml", "file_id": "some-file-id",
+                    }]}
+            else:
+                raise BranchFileNotFound("dummy", dirname)
+
+        self.useFixture(BranchHostingFixture(
+            blob=b"name: test-snap")).getInventory = getInventory
+        branch = self.factory.makeBranch()
+        self.assertEqual(
+            {"name": "test-snap"},
+            getUtility(ISnapSet).getSnapcraftYaml(branch))
+
+    def test_getSnapcraftYaml_bzr_dot_snapcraft_yaml(self):
+        def getInventory(unique_name, dirname, *args, **kwargs):
+            if dirname == "":
+                return {"filelist": [{
+                    "filename": ".snapcraft.yaml", "file_id": "some-file-id",
+                    }]}
+            else:
+                raise BranchFileNotFound("dummy", dirname)
+
+        self.useFixture(BranchHostingFixture(
+            blob=b"name: test-snap")).getInventory = getInventory
+        branch = self.factory.makeBranch()
+        self.assertEqual(
+            {"name": "test-snap"},
+            getUtility(ISnapSet).getSnapcraftYaml(branch))
+
+    def test_getSnapcraftYaml_bzr_error(self):
+        self.useFixture(BranchHostingFixture()).getInventory = FakeMethod(
+            failure=BranchHostingFault)
+        branch = self.factory.makeBranch()
+        self.assertRaises(
+            CannotFetchSnapcraftYaml,
+            getUtility(ISnapSet).getSnapcraftYaml, branch)
+
+    def test_getSnapcraftYaml_git_snap_snapcraft_yaml(self):
+        def getBlob(path, filename, *args, **kwargs):
+            if filename == "snap/snapcraft.yaml":
+                return b"name: test-snap"
+            else:
+                raise GitRepositoryBlobNotFound("dummy", filename)
+
+        self.useFixture(GitHostingFixture()).getBlob = getBlob
+        [git_ref] = self.factory.makeGitRefs()
+        self.assertEqual(
+            {"name": "test-snap"},
+            getUtility(ISnapSet).getSnapcraftYaml(git_ref))
+
+    def test_getSnapcraftYaml_git_plain_snapcraft_yaml(self):
+        def getBlob(path, filename, *args, **kwargs):
+            if filename == "snapcraft.yaml":
+                return b"name: test-snap"
+            else:
+                raise GitRepositoryBlobNotFound("dummy", filename)
+
+        self.useFixture(GitHostingFixture()).getBlob = getBlob
+        [git_ref] = self.factory.makeGitRefs()
+        self.assertEqual(
+            {"name": "test-snap"},
+            getUtility(ISnapSet).getSnapcraftYaml(git_ref))
+
+    def test_getSnapcraftYaml_git_dot_snapcraft_yaml(self):
+        def getBlob(path, filename, *args, **kwargs):
+            if filename == ".snapcraft.yaml":
+                return b"name: test-snap"
+            else:
+                raise GitRepositoryBlobNotFound("dummy", filename)
+
+        self.useFixture(GitHostingFixture()).getBlob = getBlob
+        [git_ref] = self.factory.makeGitRefs()
+        self.assertEqual(
+            {"name": "test-snap"},
+            getUtility(ISnapSet).getSnapcraftYaml(git_ref))
+
+    def test_getSnapcraftYaml_git_error(self):
+        self.useFixture(GitHostingFixture()).getBlob = FakeMethod(
+            failure=GitRepositoryScanFault)
+        [git_ref] = self.factory.makeGitRefs()
+        self.assertRaises(
+            CannotFetchSnapcraftYaml,
+            getUtility(ISnapSet).getSnapcraftYaml, git_ref)
+
+    def test_getSnapcraftYaml_snap_bzr(self):
+        self.useFixture(BranchHostingFixture(
+            file_list={"snapcraft.yaml": "some-file-id"},
+            blob=b"name: test-snap"))
+        branch = self.factory.makeBranch()
+        snap = self.factory.makeSnap(branch=branch)
+        self.assertEqual(
+            {"name": "test-snap"}, getUtility(ISnapSet).getSnapcraftYaml(snap))
+
+    def test_getSnapcraftYaml_snap_git(self):
+        self.useFixture(GitHostingFixture(blob=b"name: test-snap"))
+        [git_ref] = self.factory.makeGitRefs()
+        snap = self.factory.makeSnap(git_ref=git_ref)
+        self.assertEqual(
+            {"name": "test-snap"}, getUtility(ISnapSet).getSnapcraftYaml(snap))
+
+    def test_getSnapcraftYaml_invalid_data(self):
+        hosting_fixture = self.useFixture(GitHostingFixture())
+        for invalid_result in (None, 123, b"", b"[][]", b"#name:test", b"]"):
+            [git_ref] = self.factory.makeGitRefs()
+            hosting_fixture.getBlob = FakeMethod(result=invalid_result)
+            self.assertRaises(
+                CannotParseSnapcraftYaml,
+                getUtility(ISnapSet).getSnapcraftYaml, git_ref)
+
+    def test_getSnapcraftYaml_safe_yaml(self):
+        self.useFixture(GitHostingFixture(blob=b"Malicious YAML!"))
+        [git_ref] = self.factory.makeGitRefs()
+        unsafe_load = self.useFixture(MockPatch("yaml.load"))
+        safe_load = self.useFixture(MockPatch("yaml.safe_load"))
+        self.assertRaises(
+            CannotParseSnapcraftYaml,
+            getUtility(ISnapSet).getSnapcraftYaml, git_ref)
+        self.assertEqual(0, unsafe_load.mock.call_count)
+        self.assertEqual(1, safe_load.mock.call_count)
+
     def test__findStaleSnaps(self):
         # Stale; not built automatically.
         self.factory.makeSnap(is_stale=True)

_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to     : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

Reply via email to