Merge lp:~benji/charmworld/update-bundle-api into lp:~juju-jitsu/charmworld/trunk

[Benji York]

Add metadata to the bundle API endpoint

Add metadata to the bundle API endpoint

[Aaron Bentley (abentley)]

This looks fine.

If we're generalizing the way we retrieve charms in from_query, I'd prefer to accept a collection, rather than a database. (i.e. 'db.bundles', not 'db'.) That supports temporary collections, like we use in exodus migrations.

I expect we will eventually want to operate at a higher level, e.g. BundleSource.get_bundle(name, basket, revision, promulgated=True). At that point, the code will ensure the queries are not overbroad.

You're changing the contract of json_response, so that it no longer will support lists, strings, or other non-dict values. I'd prefer you didn't do that, but if you do, please change the docs so they don't say just "json-serializable value".

At 188, 'overbroad' is misspelled, and at 197, "convenient".

Thanks for spelling out the precise JSON that the API will return. I'm not sure we need all of it, but it's pre-existing, so that's okay.

It would be nice to update the API docs, but we can do that in another branch.

None of these are blockers, but please address json_response one way or the other.

review: Approve

[Benji York (benji)]

On Thu, Aug 15, 2013 at 2:28 PM, Aaron Bentley <email address hidden> wrote:
> Review: Approve

Thanks for the review.

> This looks fine.
>
> If we're generalizing the way we retrieve charms in from_query, I'd
> prefer to accept a collection, rather than a database. (i.e.
> 'db.bundles', not 'db'.) That supports temporary collections, like we
> use in exodus migrations.

The reason I took "db" as the parameter is that I wanted to hide the
knowledge of which collection holds the bundles in the Bundles class
instead of having it in all the places that load bundles. However I
wasn't aware of the other use case for specifying the collection.

After thinking it over and trying a couple of different things, I added
an optional "collection" parameter to from_query so any special-purpose
code that needs to operate over a particular collection can do so while
most code won't have to know where the bundles are stored. If we don't
end up liking this approach we can change it later.

> You're changing the contract of json_response, so that it no longer
> will support lists, strings, or other non-dict values. I'd prefer you
> didn't do that, but if you do, please change the docs so they don't
> say just "json-serializable value".

That's a good catch. I have changed json_response so that if an object
instance is passed in, it is "cast" to dict() and serialized to JSON.

> At 188, 'overbroad' is misspelled, and at 197, "convenient".

Thanks. I am not the best speller.

> It would be nice to update the API docs, but we can do that in another branch.

I didn't even know we had API docs. :) I added the beginnings of bundle
documentation, but we could use more. It would be nice if the docs were
tested so we will know if the code and docs ever get out of sync. Maybe
we can do that down the road.
--
Benji York

[Charmworld Lander (charmworld-lander)]

No commit message specified.

[Charmworld Lander (charmworld-lander)]

No commit message specified.

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-08-15 19:57:31 +0000
+++ charmworld/models.py	2013-08-16 14:42:35 +0000
@@ -1259,7 +1259,6 @@
         'owner': '',
         'basket': None,
         'name': '',
-        'inherits': None,
         'title': '',
         'description': '',
         'data': dict(),
@@ -1268,16 +1267,37 @@
     }
 
     def __init__(self, data):
+        assert 'inherits' not in data, (
+            'inheriting from other bundles is not supported, the bundle '
+            'must be preprocessed to incorporate any inheritance')
         self._raw_representation = data
         self._representation = dict(self._COMMON_REPRESENTATION)
         self._representation.update(data)
 
+    @classmethod
+    def from_query(cls, query, db=None, collection=None):
+        """Given a query and a database or collection, find a bundle.
+
+        If the query doesn't match any bundles, None is returned.  If the
+        query matches more than one bundle, an exception is raised.
+        """
+        assert db is not None or collection is not None, (
+            'either "db" or "collection" must be provided')
+        assert db is None or collection is None, (
+            'only one of "db" or "collection" can be provided')
+        if collection is None:
+            collection = db.bundles
+        cursor = collection.find(query, limit=2)
+        if cursor.count() > 1:
+            raise ValueError('more than one matching bundle')
+        elif cursor.count() == 0:
+            # No bundles match the query.
+            return None
+        return cls(cursor.next())
+
     def __eq__(self, other):
-        if (isinstance(other, self.__class__)
-                and self.permanent_url == other.permanent_url):
-            # These two bundles are the same.
-            return True
-        return False
+        return (isinstance(other, self.__class__)
+                and self.permanent_url == other.permanent_url)
 
     def __getattr__(self, name):
         if name in self._COMMON_REPRESENTATION:
@@ -1308,6 +1328,17 @@
         """The permanent url for the bundle."""
         return "jc:{}".format(self.id)
 
+    def __iter__(self):
+        """Iterate over the keys and values of this bundle.
+
+        Among other uses, this makes it easy to get a dictionary
+        representation of a bundle by "casting" with dict(bundle).
+        """
+        yield 'id', self.id
+        yield 'permanent_url', self.permanent_url
+        for key in self._COMMON_REPRESENTATION:
+            yield key, self.__getattr__(key)
+
 
 def acquire_session_secret(database):
     """Return a secret to use for AuthTkt sessions.

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-08-14 15:12:04 +0000
+++ charmworld/testing/factory.py	2013-08-16 14:42:35 +0000
@@ -310,10 +310,9 @@
     return record_id, charm
 
 
-def get_bundle_data(name=None, owner=None, basket=None, inherits=None,
-                    series='precise', title='', description='',
-                    services=dict(), relations=dict(), promulgated=False,
-                    branch_deleted=False):
+def get_bundle_data(name=None, owner=None, basket=None, series='precise',
+                    title='', description='', services=dict(),
+                    relations=dict(), promulgated=False, branch_deleted=False):
     if name is None:
         name = random_string(10)
     if owner is None:
@@ -327,7 +326,6 @@
                 branch_deleted=branch_deleted,
                 data=data,
                 description=description,
-                inherits=inherits,
                 name=name,
                 owner=owner,
                 promulgated=promulgated,

=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-08-15 19:57:31 +0000
+++ charmworld/tests/test_models.py	2013-08-16 14:42:35 +0000
@@ -1603,9 +1603,24 @@
         self.assertTrue(FauxElasticSearchClient.index_bundles_called)
 
 
-class BundleTestCase(TestCase):
+class BundleTestCase(MongoTestBase):
     """Unit tests for the Bundle model that wraps bundle representations."""
 
+    bundle_data = {
+        'owner': 'sinzui',
+        'basket': 'mysql',
+        'name': 'tiny',
+        'title': 'Tiny bundle',
+        'description': 'My Tiny Bundle',
+        'data': {
+            'services': dict(apache=1),
+            'relations': dict(a=1),
+            'series': 'series',
+        },
+        'promulgated': True,
+        'branch_deleted': True,
+    }
+
     def test_init_with_empty_representation(self):
         bundle_data = {}
         bundle = Bundle(bundle_data)
@@ -1617,7 +1632,6 @@
             'owner': 'sinzui',
             'basket': 'mysql',
             'name': 'tiny',
-            'inherits': 'main-mysql',
             'title': 'Tiny bundle',
             'description': 'My Tiny Bundle',
             'data': {
@@ -1638,32 +1652,24 @@
             bundle.not_a_real_attribute
 
     def test_is_equal(self):
-        bundle1 = Bundle(
-            {'name': 'tiny',
-             'owner': 'bac',
-             'basket': 'apache/1',
-             'inherits': 'big-apache',
-             })
-        bundle2 = Bundle(
-            {'name': 'tiny',
-             'owner': 'bac',
-             'basket': 'apache/1',
-             'inherits': 'moderately-big-apache',
-             'branch_deleted': True,
-             })
+        # If two bundles have the same permanent_url, they are considered
+        # equal, even if their data differ.
+        common = {
+            'name': 'tiny',
+            'owner': 'bac',
+            'basket': 'apache/1',
+        }
+        bundle1 = Bundle(dict(common, title='Tiny Apache'))
+        bundle2 = Bundle(dict(common, title='Itty Bitty Apache'))
         self.assertEqual(bundle1, bundle2)
 
     def test_not_equal(self):
-        bundle1 = Bundle(
-            {'name': 'tiny',
-             'owner': 'bac',
-             'basket': 'apache/4',
-             })
-        bundle2 = Bundle(
-            {'name': 'tiny',
-             'owner': 'abc',
-             'basket': 'apache/4',
-             })
+        common = {
+            'name': 'tiny',
+            'basket': 'apache/4',
+        }
+        bundle1 = Bundle(dict(common, owner='bac'))
+        bundle2 = Bundle(dict(common, owner='abc'))
         self.assertNotEqual(bundle1, bundle2)
 
     def test_id(self):
@@ -1693,6 +1699,46 @@
         self.assertEqual(
             "/bundle/~bac/apache/6/tiny", bundle.short_url)
 
+    def test_loading_a_bundle_from_the_database(self):
+        # A bundle can be loaded from the database with a query.
+        self.db.bundles.save(self.bundle_data)
+        bundle = Bundle.from_query({'owner': 'sinzui'}, self.db)
+        self.assertIsNotNone(bundle)
+        self.assertEqual(bundle.basket, 'mysql')
+        self.assertEqual(bundle.name, 'tiny')
+
+    def test_loading_a_bundle_from_a_given_collection(self):
+        # A bundle can be loaded from a specific collection.
+        collection = self.db.create_collection(factory.random_string(10))
+        collection.save(self.bundle_data)
+        bundle = Bundle.from_query({'owner': 'sinzui'}, collection=collection)
+        self.assertIsNotNone(bundle)
+        self.assertEqual(bundle.basket, 'mysql')
+        self.assertEqual(bundle.name, 'tiny')
+
+    def test_loading_a_nonexistent_bundle_returns_none(self):
+        # If a query does not match any bundles, None is returned.
+        bundle = Bundle.from_query({'owner': 'nunyas'}, self.db)
+        self.assertIsNone(bundle)
+
+    def test_overbroad_query_raises_error(self):
+        # If a query matches multiple bundles, an exception is raised.
+        self.db.bundles.save({})
+        self.db.bundles.save({})
+        with self.assertRaises(Exception):
+            Bundle.from_query({}, self.db)
+
+    def test_bundles_are_iterable(self):
+        # When a bundle is iterated over it generates a series of key, value
+        # pairs.  This is convenient for generating a dictionary from a
+        # bundle (e.g., dict(bundle)).
+        bundle = Bundle({'name': 'bundle-name'})
+        self.assertIn(('name', 'bundle-name'), list(bundle))
+        # The important properties are also represented in the output.
+        bundle_data = dict(bundle)
+        self.assertIn('id', bundle_data)
+        self.assertIn('permanent_url', bundle_data)
+
 
 class TestSlurpFiles(MongoTestBase):
 

=== modified file 'charmworld/views/api.py'
--- charmworld/views/api.py	2013-08-15 17:54:48 +0000
+++ charmworld/views/api.py	2013-08-16 14:42:35 +0000
@@ -39,22 +39,33 @@
 )
 
 
-def json_response(status, params, headers=[]):
+def json_response(status, value, headers=[]):
     """Return a JSON API response.
 
     :param status: The HTTP status code to use.
-    :param params: A json-serializable value to use as the body, or None for
+    :param value: A json-serializable value to use as the body, or None for
         no body.
     """
-    if params is None:
+    # If the value is an object, then it must be representable as a mapping.
+    if value is None:
         body = ''
     else:
-        body = json.dumps(params, sort_keys=True, indent=2)
+        while True:
+            try:
+                body = json.dumps(value, sort_keys=True, indent=2)
+            except TypeError:
+                # If the serialization wasn't possible, and the value wasn't
+                # already a dictionary, convert it to one and try again.
+                if not isinstance(value, dict):
+                    value = dict(value)
+                    continue
+            break
+
     return Response(body,
                     headerlist=[
                         ('Content-Type', 'application/json'),
-                        ("Access-Control-Allow-Origin", "*"),
-                        ("Access-Control-Allow-Headers", "X-Requested-With"),
+                        ('Access-Control-Allow-Origin', '*'),
+                        ('Access-Control-Allow-Headers', 'X-Requested-With'),
                     ] + headers,
                     status_code=status)
 
@@ -363,12 +374,14 @@
             query = {'basket': basket_id, 'name': path[2], 'promulgated': True}
         else:
             raise HTTPNotFound(self.request.path)
-        bundle_data = self.request.db.bundles.find_one(query)
-        if bundle_data is None:
-            return json_response(
-                404, {'type': 'no_such_bundle', 'bundle_id': fullpath})
-        bundle = Bundle(bundle_data)
-        return {bundle.name: bundle.data}
+        bundle = Bundle.from_query(query, self.request.db)
+        if bundle is None:
+            status = 404
+            result = {'type': 'no_such_bundle', 'bundle_id': fullpath}
+        else:
+            status = 200
+            result = bundle
+        return json_response(status, result)
 
     @staticmethod
     def _get_file_headers(md5sum, content_type=None):

=== modified file 'charmworld/views/tests/test_api.py'
--- charmworld/views/tests/test_api.py	2013-08-15 17:54:48 +0000
+++ charmworld/views/tests/test_api.py	2013-08-16 14:42:35 +0000
@@ -58,27 +58,72 @@
 
 class TestJSONResponse(ViewTestBase):
 
-    def test_json_response(self):
-        """json_response produces expected response."""
-        error = json_response(200, {'type': 'not_an_error'})
-        self.assertEqual(200, error.status_code)
-        self.assertEqual({'type': 'not_an_error'}, error.json_body)
-        self.assertEqual('application/json', error.content_type)
-        self.assertIn(("Access-Control-Allow-Origin", "*"), error.headerlist)
-        self.assertIn(
-            ("Access-Control-Allow-Headers", "X-Requested-With"),
-            error.headerlist)
+    def test_json_response_status(self):
+        # The status passed in will be used to generate the HTTP response.
+        response = json_response(200, None)
+        self.assertEqual(200, response.status_code)
+
+    def test_json_response_body(self):
+        # The value passed in will be serialized to JSON and returned as the
+        # body of the response.
+        response = json_response(200, {'type': 'not_an_error'})
+        self.assertEqual({'type': 'not_an_error'}, response.json_body)
+        self.assertEqual('application/json', response.content_type)
+
+    def test_content_type(self):
+        # The content type is set as JSON.
+        response = json_response(200, {})
+        self.assertEqual('application/json', response.content_type)
+
+    def test_cross_origin_headers(self):
+        # Access control headers are set that allow cross origin resource
+        # sharing (CORS).
+        response = json_response(200, {})
+        self.assertIn(
+            ('Access-Control-Allow-Origin', '*'),
+            response.headerlist)
+        self.assertIn(
+            ('Access-Control-Allow-Headers', 'X-Requested-With'),
+            response.headerlist)
 
     def test_json_response_none(self):
-        response = json_response(201, None)
-        self.assertEqual(201, response.status_code)
+        # If given None as the body value, an empty body is generated.
+        response = json_response(200, None)
         self.assertEqual(0, response.content_length)
+        self.assertEqual('', response.body)
+        # Despite the empty body, the JSON content type is set.
         self.assertEqual('application/json', response.content_type)
-        self.assertIn(("Access-Control-Allow-Origin", "*"),
-                      response.headerlist)
-        self.assertIn(
-            ("Access-Control-Allow-Headers", "X-Requested-With"),
-            response.headerlist)
+
+    def test_serializing_mappings(self):
+        response = json_response(200, {'a': 1, 'b': 2})
+        self.assertEqual(response.json_body, {'a': 1, 'b': 2})
+
+    def test_serializing_lists(self):
+        response = json_response(200, [1, 2, 3])
+        self.assertEqual(response.json_body, [1, 2, 3])
+
+    def test_serializing_strings(self):
+        response = json_response(200, 'test')
+        self.assertEqual(response.json_body, 'test')
+
+    def test_serializing_ints(self):
+        response = json_response(200, 42)
+        self.assertEqual(response.json_body, 42)
+
+    def test_serializing_objects(self):
+        # We want to be able to pass in objects as a value and have them
+        # serialized to JSON, so any value that is an instance will be "cast"
+        # to a dict() in order to retrieve the values to be seriallized.  The
+        # easiest way to make an object behave correctly in that scenario is
+        # to give it an __iter__ method that is a generator yielding key,
+        # value pairs.
+        class Model:
+            def __iter__(self):
+                yield ('first', 1)
+                yield ('second', 2)
+
+        response = json_response(200, Model())
+        self.assertEqual(response.json_body, {'first': 1, 'second': 2})
 
 
 class APITestBase(ViewTestBase):
@@ -651,7 +696,6 @@
             name=bundle.name,
             owner=bundle.owner,
             basket=bundle.basket,
-            inherits=None,
             title='',
             data=dict(services=dict(),
                       relations=dict(),
@@ -706,11 +750,27 @@
         self.assertEqual(200, response.status_code)
 
     def test_results_match(self):
-        bundle = self.makeBundle(name='bat', owner='bac', basket='byobu/4')
+        self.makeBundle(name='bat', owner='bac', basket='byobu/4')
         response = self.get_response('bundle', '~bac/byobu/4/bat')
         self.assertEqual(200, response.status_code)
-        json = dict(response.json)
-        self.assertEqual(json['bat'], bundle.data)
+        self.assertEqual(
+            response.json,
+            {
+                u'basket': u'byobu/4',
+                u'branch_deleted': False,
+                u'data': {
+                    u'series': u'precise',
+                    u'services': {},
+                    u'relations': {},
+                },
+                u'description': u'',
+                u'id': u'~bac/byobu/4/bat',
+                u'name': u'bat',
+                u'owner': u'bac',
+                u'permanent_url': u'jc:~bac/byobu/4/bat',
+                u'promulgated': False,
+                u'title': u'',
+            })
 
 
 class TestAPI3Bundles(TestAPIBundles, API3Mixin):

=== modified file 'docs/api.rst'
--- docs/api.rst	2013-07-17 14:38:00 +0000
+++ docs/api.rst	2013-08-16 14:42:35 +0000
@@ -340,3 +340,11 @@
         }
       }
     }
+
+
+Retrieving a bundle
+===================
+A bundle can be retrieved by invoking GET on ``bundle/$bundle-id``,
+where the bundle-id is appended to the path without escaping.  So if the
+bundle-id is ``apache2/with-squid``, the path is
+``bundle/apache2/with-squid``, not ``bundle/apache2%2fwith-squid``.