Merge lp:~rharding/charmworld/bundle-metadata into lp:~juju-jitsu/charmworld/trunk

[Richard Harding]

Implement the /file/xxxx api off of the bundle api space.

This branch implements the /file/xxxx api off of the bundle api space. This is required to pull README content. This duplicates some of the style of the api parsing from the Charm code with some changes to be a little simpler. Things are kept as lists until the last minute, etc.

The general idea is to get back to the idea of an api call with a /bundle/some/id/trailing/bits so that we can implement extra calls on a bundle. We'll need a /related trailing call in the future to provide the bundles that are in the same basket, for instance.

The _charm_icon stuff is a start because we'll need to have the ability to load the bundle icon by default if the charm is not promulgated/has one much like we do with charm icon. It just won't have the category complication part.

makeBundle was updated so that you can force the basket record to be created as well. This was required to be able to load the test file object into gridfs to make sure we could fetch it via the api response.

QA:

pull the changes, if you've got the demo bundles loaded you should be able to hit:

http://charmworld:2464/api/3/bundle/~bac/wiki/wiki/file/icon.svg
http://charmworld:2464/api/3/bundle/~bac/wiki/wiki/file/README

and get the proper response.

[Richard Harding (rharding)]

The big goal was to get back the idea of the trailing. I kept it a [] vs a string so that it's easier to find the bits in it.

One thing I changed to make consistent was the custom json response if the bundle id was invalid. That matched the charm behavior vs just throwing the not found exception. We had agreed during API design that all failures should provide some sort of payload of helpful info if possible.

The _charm_icon stuff is a start because we'll need to have the ability to load the bundle icon by default if the charm is not promulgated/has one much like we do with charm icon. It just won't have the category complication part.

As is the old tests are passing and were very helpful. The next step is to get the readme into the test bundle and have tests against getting it back out again via the url /~owner/basket/bundle/file/README and such.

[Benji York (benji)]

There are a few small things that should be addressed, but otherwise
this looks good.

It would be nice if the comment that includes "Bundle IDs can consist of
up to 4 parts." spelled out explicitly what those four parts are.

Line 96 of the diff: We should capitalize "id" unless we're also talking
about the "ego" and "super-ego". I also feel that we should capitalize
"URL" but that may be more of a personal preference, so I'll leave that
one up to you.

Line 98 of the diff: A "split point" is referenced, but I don't see one.
There is an identical comment earlier in the file; perhaps this is a
copy/paste bug.

The try/except around lines 110 through 119 of the diff seems a little
big and the except is for two unrelated exceptions. I think it would be
better to have less code in the try and handle each exception
independently.

Lines 130 and 162 of the diff: s/id/ID/

make_bundle_file could use a docstring.

Since test_extracting_bundle_id_with_trailing contains three independent
setup/assert blocks it would be better to have it broken into three
separate tests (each with their own customized version of the comment at
the top of the test now).

review: Approve

[Richard Harding (rharding)]

> There are a few small things that should be addressed, but otherwise
> this looks good.
>
> It would be nice if the comment that includes "Bundle IDs can consist of
> up to 4 parts." spelled out explicitly what those four parts are.

Updated the comment with the parts vs the sample urls. I agree that it's clearer
that way.

> Line 96 of the diff: We should capitalize "id" unless we're also talking
> about the "ego" and "super-ego". I also feel that we should capitalize
> "URL" but that may be more of a personal preference, so I'll leave that
> one up to you.

Updated, not a fan of caps URL, but it'll be more consistent.

> Line 98 of the diff: A "split point" is referenced, but I don't see one.
> There is an identical comment earlier in the file; perhaps this is a
> copy/paste bug.

Agreed, updated comment to note the position change vs the 'split point'.

> The try/except around lines 110 through 119 of the diff seems a little
> big and the except is for two unrelated exceptions. I think it would be
> better to have less code in the try and handle each exception
> independently.

Agree in a sense. I moved the try/except around the one line of code that's attempting to find a version number. Both exceptions come out of that line of code though, so left the two exceptions to be caught there. Added a comment on why each was included to help clarify that one can come from the int() call and the other from the [index] use.

> Lines 130 and 162 of the diff: s/id/ID/
>
> make_bundle_file could use a docstring.

Updated! Thanks, saw a lot of methods sans docstrings and trying to find balance. I'm usually a docstring all the things view myself.

> Since test_extracting_bundle_id_with_trailing contains three independent
> setup/assert blocks it would be better to have it broken into three
> separate tests (each with their own customized version of the comment at
> the top of the test now).

Updated.

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://10.55.32.130:8080/job/charmworld-autoland-lxc/1/
Executed test runs:

review: Needs Fixing (continuous-integration)

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
No commit message was specified in the merge proposal. Hit 'Add commit message' on the merge proposal web page or follow the link below. You can approve the merge proposal yourself to rerun.
https://code.launchpad.net/~rharding/charmworld/bundle-metadata/+merge/188905/+edit-commit-message

review: Needs Fixing (continuous-integration)

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://10.55.32.130:8080/job/charmworld-autoland-lxc/5/
Executed test runs:

review: Needs Fixing (continuous-integration)

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://10.55.32.130:8080/job/charmworld-autoland-lxc/6/
Executed test runs:

review: Needs Fixing (continuous-integration)

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://10.55.32.130:8080/job/charmworld-autoland-lxc/12/
Executed test runs:

review: Needs Fixing (continuous-integration)

[Juju Gui Bot (juju-gui-bot)]

review: Approve (continuous-integration)

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-09-19 21:54:54 +0000
+++ charmworld/testing/factory.py	2013-10-03 17:14:08 +0000
@@ -11,8 +11,10 @@
     datetime,
 )
 import hashlib
+import magic
 import os
 from random import randint
+import re
 import random
 import stat
 import string
@@ -34,7 +36,11 @@
     make_bundle_doc,
     options_to_storage,
 )
-from charmworld.utils import write_locked
+from charmworld.utils import (
+    quote_yaml,
+    unquote_yaml,
+    write_locked,
+)
 
 
 def random_string(length=None):
@@ -335,10 +341,22 @@
 
 
 def makeBundle(db, *args, **kwargs):
+    with_basket = False
+    if 'with_basket' in kwargs:
+        with_basket = True
+        del kwargs['with_basket']
+
     bundle_data = get_bundle_data(*args, **kwargs)
     bundle = Bundle(bundle_data)
     bundle_data['_id'] = bundle.id
     db.bundles.insert(bundle_data)
+
+    if with_basket:
+        basket_data = {
+            '_id': re.sub('\/[^/]*$', '', bundle.id),
+            'file_hashes': {}
+        }
+        db.baskets.save(basket_data)
     return bundle, bundle_data
 
 
@@ -352,6 +370,30 @@
     return charm_file
 
 
+def make_bundle_file(db, bundle, path, content=None):
+    """Create a file in gridfs for a bundle.
+
+    Note that these are actually keyed off the basket the bundle is in and so
+    the supplied bundle must have a basket entry for the file to be created
+    successfully.
+
+    """
+    fs = Bundle.getfs(db)
+    if content is None:
+        content = random_string(10).encode('utf-8')
+    _id = hashlib.sha256(content).hexdigest()
+    content_type = magic.from_buffer(content, mime=True)
+
+    fs.put(content, contentType=content_type, _id=_id)
+
+    # The list of files is actually stored on the basket object.
+    basket = db.baskets.find_one(bundle.basket_id)
+    hashes = unquote_yaml(basket.get('file_hashes', {}))
+    hashes[path] = _id
+    basket['file_hashes'] = quote_yaml(hashes)
+    db.baskets.save(basket)
+
+
 def makeTestResult(db, name, branch_spec, status, provider,
                    branch_revision=None, timestamp=None, test_number=None):
     """Create a test record.
 
=== modified file 'charmworld/views/api.py'
--- charmworld/views/api.py	2013-10-02 15:33:27 +0000
+++ charmworld/views/api.py	2013-10-03 17:14:08 +0000
@@ -149,7 +149,7 @@
 
     @staticmethod
     def _get_api_id(charm):
-        """Return the API id for a Mongo-formatted charm."""
+        """Return the API ID for a Mongo-formatted charm."""
         return re.sub('^cs:', '', charm.store_url)
 
     @staticmethod
@@ -309,8 +309,77 @@
         return charm_id, trailing
 
     @staticmethod
+    def _parse_bundle_id(path):
+        """Extract the bundle API ID from a given URL path.
+
+        Bundle API IDs beginning with a '~' are considered to include an owner.
+        Bundle IDs can consist of up to 4 parts.
+
+        - owner (optional)
+        - basket (required)
+        - version (optional)
+        - bundle (required)
+
+        """
+        if len(path) < 2:
+            raise ValueError('The bundle ID does not contain enough elements')
+
+        # Determine if the path includes an owner and adjust the backet index
+        # appropriately.
+        owner = None
+        if unquote(path[0])[0] == '~':
+            owner = path[0]
+            basket = path[1]
+        else:
+            basket = path[0]
+
+        # Now check if there's an optional revision section in there after the
+        # basket name.
+        version_index = 2 if owner else 1
+        try:
+            version = int(path[version_index])
+        except (IndexError, ValueError):
+            # If the url isn't long enough to have a version value, or the
+            # value in that slot is not an integer, then no version was
+            # provided.
+            version = None
+
+        if version:
+            if owner:
+                # URL includes an owner, a basket, and a version.
+                bundle = path[3]
+            else:
+                # URL does not include an owner and only has a basket,
+                # version, and bundle name.
+                bundle = path[2]
+        else:
+            if owner:
+                # URL includes an owner, a basket, and a bundle name.
+                bundle = path[2]
+            else:
+                # URL only includes a basket name and a bundle name.
+                bundle = path[1]
+
+        # So now we can pull out the parts that are the ID and the rest will
+        # be the trailing.
+        id_length = 2
+        if owner:
+            id_length = id_length + 1
+        if version:
+            id_length = id_length + 1
+        parsedId = '/'.join(path[0:id_length])
+        trailing = path[id_length:]
+
+        return parsedId, trailing, {
+            'owner': owner,
+            'basket': basket,
+            'version': version,
+            'bundle': bundle
+        }
+
+    @staticmethod
     def _parse_charm_id(charm_id):
-        """Split a charm id into its component parts.
+        """Split a charm ID into its component parts.
 
         Returns a tuple of (owner, series, name, revision).
         """
@@ -358,6 +427,34 @@
                 query, sort=[('store_data.revision', pymongo.DESCENDING)])
         return charm_id, trailing, charm
 
+    def _find_bundle(self, path):
+        try:
+            bundle_id, trailing, bundle_bits = self._parse_bundle_id(path)
+        except ValueError:
+            bundle = bundle_id = trailing = bundle_bits = None
+        else:
+            if bundle_bits['owner'] and bundle_bits['version']:
+                # We have all 4 parts needed, just use the ID.
+                query = {'_id': bundle_id}
+            elif bundle_bits['owner']:
+                # This URL includes the owner, basket name, and bundle name.
+                # The specified bundle may be promulgated or not.
+                owner = bundle_bits['owner'][1:]  # Strip the tilde.
+                query = {
+                    'owner': owner,
+                    'basket_name': bundle_bits['basket'],
+                    'name': bundle_bits['bundle']
+                }
+            else:
+                query = {
+                    'basket_name': bundle_bits['basket'],
+                    'name': bundle_bits['bundle'],
+                    'promulgated': True
+                }
+
+            bundle = Bundle.from_query(query, self.request.db)
+        return bundle_id, trailing, bundle
+
     def charm(self, path=None):
         """Retrieve a charm according to its API ID (the path prefix)."""
         if path is None:
@@ -370,7 +467,7 @@
         if trailing is None:
             return self._charm_details(charm_data)
         elif trailing == self.ICON_TRAILING:
-            return self._icon(charm)
+            return self._charm_icon(charm)
         elif trailing.startswith('file/'):
             return self._charm_file(charm, trailing)
         elif trailing == 'qa':
@@ -393,46 +490,26 @@
             status_code=200)
 
     def bundle(self, path=None):
-        """Retrieve a bundle based on id."""
+        """Retrieve a bundle based on ID."""
         if path is None:
             raise HTTPNotFound(self.request.path)
+        path = list(path)
+        bundle_id, trailing, bundle = self._find_bundle(path)
+
         filename = None
-        path = list(path)
-        # If this is a request for the bundle's icon, we need to remove the
-        # file name from the path so the code below can use the remainder of
-        # the path to find the bundle.
-        if path[-1] == 'icon.svg':
-            filename = path.pop()
-        fullpath = '/'.join(path)
-        if len(path) == 4:
-            # We have an owner, basket name, bundle name, and revision, all
-            # those together are enough to build the bundle's ID.
-            query = {'_id': fullpath}
-        elif len(path) == 3:
-            # This URL includes the owner, basket name, and bundle name.  The
-            # specified bundle may be promulgated or not.
-            owner = path[0][1:]  # Strip the tilde.
-            query = {'owner': owner, 'basket_name': path[1],
-                     'name': path[2]}
-        elif len(path) == 2:
-            # Since there is no owner in the URL, this is a request for a
-            # promulgated bundle.
-            query = {'basket_name': path[0], 'name': path[1],
-                     'promulgated': True}
-        else:
-            raise HTTPNotFound(self.request.path)
-        bundle = Bundle.from_query(query, self.request.db)
+
         if bundle is None:
             status = 404
-            result = {'type': 'no_such_bundle', 'bundle_id': fullpath}
+            result = {'type': 'no_such_bundle', 'bundle_id': '/'.join(path)}
+            return json_response(status, result)
+        if not trailing:
+            return json_response(200, bundle)
+        elif trailing and trailing[0] == self.ICON_TRAILING:
+            return self._bundle_file(bundle, filename)
+        elif trailing and trailing[0] == 'file':
+            return self._bundle_file(bundle, '/'.join(trailing[1:]))
         else:
-            # If the URL specified one of the bundle's files (as opposed to
-            # the bundle itself), return that file's contents.
-            if filename is not None:
-                return self._bundle_file(bundle, filename)
-            status = 200
-            result = bundle
-        return json_response(status, result)
+            raise HTTPNotFound('/'.join(path))
 
     @staticmethod
     def _get_file_headers(md5sum, content_type=None):
@@ -478,7 +555,7 @@
             headerlist=headerlist,
             status_code=200)
 
-    def _icon(self, charm):
+    def _charm_icon(self, charm):
         if (charm.files and
                 charm.files.get(quote_key('icon.svg')) and
                 charm.promulgated):
 
=== modified file 'charmworld/views/tests/test_api.py'
--- charmworld/views/tests/test_api.py	2013-09-30 15:26:01 +0000
+++ charmworld/views/tests/test_api.py	2013-10-03 17:14:08 +0000
@@ -745,8 +745,8 @@
 
     def test_invalid_path(self):
         self.makeBundle(name='bat')
-        with self.assertRaises(HTTPNotFound):
-            self.get_response('bundle', 'foo')
+        response = self.get_response('bundle', 'foo')
+        self.assertEqual(404, response.status_code)
 
     def test_not_found(self):
         self.makeBundle(name='bat')
@@ -802,6 +802,37 @@
                 u'title': u'',
             })
 
+    def test_extracting_bundle_id_with_trailing_full_id(self):
+        # If there are trailing characters after the bundle ID,
+        # they are returned in "trailing".
+        bundle_id, trailing, bits = self.api_class._parse_bundle_id(
+            '~bac/byobu/4/bat/file/README'.split('/'))
+        self.assertEqual('~bac/byobu/4/bat', bundle_id)
+        self.assertEqual(['file', 'README'], trailing)
+
+    def test_extracting_bundle_id_with_trailing_no_owner(self):
+        bundle_id, trailing, bits = self.api_class._parse_bundle_id(
+            'byobu/4/bat/file/README'.split('/'))
+        self.assertEqual('byobu/4/bat', bundle_id)
+        self.assertEqual(['file', 'README'], trailing)
+
+    def test_extracting_bundle_id_with_trailing_no_owner_or_version(self):
+        bundle_id, trailing, bits = self.api_class._parse_bundle_id(
+            'byobu/bat/file/README'.split('/'))
+        self.assertEqual('byobu/bat', bundle_id)
+        self.assertEqual(['file', 'README'], trailing)
+
+    def test_bundle_readme(self):
+        # Test that we can store a readme file and get it back out in a
+        # response with the url bundle/id/file/README or something like that.
+        bundle = self.makeBundle(
+            name='bat', owner='bac',
+            basket_with_rev='byobu/4', with_basket=True)
+        factory.make_bundle_file(self.db, bundle, u'README', 'Test Content')
+        response = self.get_response('bundle', '~bac/byobu/4/bat/file/README')
+        self.assertEqual('Test Content', response.body)
+        self.assertEqual(u'text/plain', response.content_type)
+
 
 class TestAPI3Bundles(TestAPIBundles, API3Mixin):
     """Test API 3 bundle endpoint."""