Merge lp:~sinzui/charmworld/support-gui-charm-urls into lp:~juju-jitsu/charmworld/trunk

[Curtis Hovey]

Support juju-gui URLs in charmworld.

Support juju-gui URLs in charmworld.

RULES

    Pre-implementation: gary, mark, abentley
    * Juju-gui wants to delegate requests for charm data when the client
      is not supported -- juju-gui supports a small set of JS-enabled
      browsers.
      * Users need to see charm (and bundle) details to learn what they
        do and how to use them. We expect users and sometimes bots to
        find these links and need access to discover juju or develop
        with it.
      * The gui or it's front-end server will forward the request to
        charmworld. Charmworld needs to understand the URL.
    * Charmworld supports unversioned user urls now. It must support:
      * -{rev} in any charm url.
      * /{series}/
    * ^ These urls do not conflict with current urls so moving the old urls
      can be postponed.

QA

    * Visit http://staging.jujucharms.com/precise/apache2-11
    * Visit http://staging.jujucharms.com/precise/apache2-HEAD
    * Visit http://staging.jujucharms.com/precise/apache2
    * Visit http://staging.jujucharms.com/~charmers/precise/apache2-11
    * Visit http://staging.jujucharms.com/~charmers/precise/apache2-HEAD
    * Visit http://staging.jujucharms.com/~charmers/precise/apache2
    * Visit http://staging.jujucharms.com/~gnuoy/precise/apache2-1
    * Visit http://staging.jujucharms.com/~gnuoy/precise/apache2-HEAD
    * Visit http://staging.jujucharms.com/~gnuoy/precise/apache2
    * Visit http://staging.jujucharms.com/~abentley/bundles/wiki-bundle/1/wiki
    * Visit http://staging.jujucharms.com/~abentley/bundles/wiki-bundle/1/wiki/json
    * Visit http://staging.jujucharms.com/~abentley/bundles/wiki-bundle/wiki

IMPLEMENTATION

I added routing tests for charms before I changed code. I discovered that
the charm json views were missing a content type. While content_type was
passed to the Response, it was clobbered by the headerlist. The fix was
to include the content-type in the header list.

I extracted a helper from a pattern I saw in several charm views.
_reconcile_revision() knows to correct the charm name when it is
mistaken for a revision. I added a rule to handle the HEAD case that
juju gui's apache adds.

The main difference between charmworld URLs and juju-gui URLs is the
revision and the lack of namespacing. Adding revision to personal branches
and bundles was trivial. Reviewed charms was tricky. Making the charms
the last rules ensures the current urls are not misconstrued to be charm
series. Since bundles is both a namespace and a series, there was no conflict.
I did add a revision to the official bundle json view to avoid conflicts with
the route I believe the GUI will use; this make the official json view
consistent with the person json view.

[Benji York (benji)]

The branch looks good. The tests especially nice.

On lines 233, 255, and 262 you could use self.assertIn and get slightly
better diagnostics if the assertion ever fails.

Unpacking the comments in the routing tests would be nice. I can pretty
much understand them in the context of the branch, but I suspect someone
coming in cold would have to ruminate a bit to see what they are saying.
For example, test_route_personal_charm_with_revision's comment of

    # /~owner/series/charm-1 route find the charm.

Might be expanded to

    # A non-promulgated charm's route of the form /~owner/series/charm-1
    # specifies the charm with a charm store ID of
    # cs:~owner/series/charm.

review: Approve

[lp:~sinzui/charmworld/support-gui-charm-urls]

383. By Curtis Hovey on 2013-08-29

Updated tests to better report the failing condition.

384. By Curtis Hovey on 2013-08-29

Updated comments.

[Curtis Hovey (sinzui)]

Self-approving changes made per review.

review: Approve (code)

=== modified file 'charmworld/routes.py'
--- charmworld/routes.py	2013-08-23 12:47:09 +0000
+++ charmworld/routes.py	2013-08-29 18:36:30 +0000
@@ -33,6 +33,8 @@
     # Personal Charms
     config.add_route("personal-collection", "/~{owner}")
     config.add_route("personal-series", "/~{owner}/{series}")
+    config.add_route("personal-charm-revision",
+                     "/~{owner}/{series}/{charm}-{revision}")
     config.add_route("personal-charm", "/~{owner}/{series}/{charm}")
     config.add_route("personal-charm-revision-json",
                      "/~{owner}/{series}/{charm}-{revision}/json")
@@ -50,6 +52,8 @@
     config.add_route("interface-collection-map", "/interface-charm-map")
 
     # Personal Bundles
+    config.add_route("personal-bundle-revision",
+                     "/~{owner}/bundles/{basket}/{rev}/{bundle}")
     config.add_route("personal-bundle",
                      "/~{owner}/bundles/{basket}/{bundle}")
     config.add_route("personal-bundle-json",
@@ -57,7 +61,9 @@
 
     # Official (promulgated) Bundles
     config.add_route("official-bundle-json",
-                     "/bundles/{basket}/{bundle}/json")
+                     "/bundles/{basket}/{rev}/{bundle}/json")
+    config.add_route("official-bundle-revision",
+                     "/bundles/{basket}/{rev}/{bundle}")
 
     # Tools
     config.add_route("tools-core-review", "/tools/core-review-queue")
@@ -105,4 +111,10 @@
 
     # Monitoring support.
     config.add_route('heartbeat', '/heartbeat')
+
+    # Unnamespaced promulgated charms.
+    config.add_route("promulgated-charm-revision",
+                     "/{series}/{charm}-{revision}")
+    config.add_route("promulgated-charm", "/{series}/{charm}")
+
     return config
 
=== modified file 'charmworld/views/bundles.py'
--- charmworld/views/bundles.py	2013-08-23 19:19:21 +0000
+++ charmworld/views/bundles.py	2013-08-29 18:36:30 +0000
@@ -100,6 +100,9 @@
 
 
 @cached_view_config(
+    route_name="personal-bundle-revision",
+    renderer="charmworld:templates/bundle.pt")
+@cached_view_config(
     route_name="personal-bundle",
     renderer="charmworld:templates/bundle.pt")
 def personal_bundle(request):
@@ -127,6 +130,14 @@
 
 
 @cached_view_config(
+    route_name="official-bundle-revision",
+    renderer="charmworld:templates/bundle.pt")
+def official_bundle(request):
+    bundle = find_bundle(request, promulgated=True)
+    return _bundle_view(request, found(bundle))
+
+
+@cached_view_config(
     route_name="official-bundle-json")
 def official_bundle_json(request):
     bundle = find_bundle(request, promulgated=True)
 
=== modified file 'charmworld/views/charms.py'
--- charmworld/views/charms.py	2013-08-22 16:23:51 +0000
+++ charmworld/views/charms.py	2013-08-29 18:36:30 +0000
@@ -185,8 +185,30 @@
         json.dumps(charm, sort_keys=True, indent=2),
         headerlist=[
             ("Access-Control-Allow-Origin", "*"),
-            ("Access-Control-Allow-Headers", "X-Requested-With")],
-        content_type=u"application/json")
+            ("Access-Control-Allow-Headers", "X-Requested-With"),
+            ("Content-Type", "application/json")])
+
+
+def _reconcile_revision(request):
+    """Check and reconcile the revision munged with the charm name.
+
+    Charm names may contain dashes, but the charm store appends the
+    charm revision revision with a dash. Routing rules may match a revision
+    that is actually a part of the charm name. The juju-gui apache front-end
+    will append -HEAD to versionless charm urls.
+    """
+    match = request.matchdict
+    if 'revision' not in match or match['revision'].isdigit():
+        # The charm name and revision look sane.
+        return
+    if match['revision'].upper() == 'HEAD':
+        # Remove the false revision created by the juju-gui apache front-end.
+        # Charmworld always shows head when no revision is specified.
+        del match['revision']
+    else:
+        # The charm name was wrongly matched as a revision.
+        match['charm'] = "%s-%s" % (match['charm'], match['revision'])
+        del match['revision']
 
 
 def found_charm_collection(db, query, sub_only, only_promulgated=False):
@@ -275,7 +297,11 @@
 @cached_view_config(
     route_name="personal-charm",
     renderer="charmworld:templates/charm.pt")
+@cached_view_config(
+    route_name="personal-charm-revision",
+    renderer="charmworld:templates/charm.pt")
 def personal_charm(request):
+    _reconcile_revision(request)
     return _charm_view(request, find_charm(request))
 
 
@@ -284,18 +310,22 @@
 @cached_view_config(
     route_name="personal-charm-json")
 def person_charm_json(request):
-    match = request.matchdict
-    if 'revision' in match and not match['revision'].isdigit():
-        match['charm'] = "%s-%s" % (
-            match['charm'], match['revision'])
+    _reconcile_revision(request)
     charm = Charm(find_charm(request))
     return _json_charm(charm)
 
 
 @cached_view_config(
+    route_name="promulgated-charm-revision",
+    renderer="charmworld:templates/charm.pt")
+@cached_view_config(
+    route_name="promulgated-charm",
+    renderer="charmworld:templates/charm.pt")
+@cached_view_config(
     route_name="charm",
     renderer="charmworld:templates/charm.pt")
 def distro_charm(request):
+    _reconcile_revision(request)
     return _charm_view(request, find_charm(request, promulgated=True))
 
 
@@ -306,10 +336,7 @@
     route_name="charm-json",
 )
 def distro_charm_json(request):
-    match = request.matchdict
-    if 'revision' in match and not match['revision'].isdigit():
-        match['charm'] = "%s-%s" % (
-            match['charm'], match['revision'])
+    _reconcile_revision(request)
     charm = Charm(find_charm(request, promulgated=True))
     return _json_charm(charm)
 
 
=== modified file 'charmworld/views/tests/test_bundles.py'
--- charmworld/views/tests/test_bundles.py	2013-08-27 14:39:13 +0000
+++ charmworld/views/tests/test_bundles.py	2013-08-29 18:36:30 +0000
@@ -250,6 +250,16 @@
         lp_long_url = 'https://code.launchpad.net/{path}'.format(path=lp_path)
         self.assertIn(lp_long_url, resp.body)
 
+    def test_personal_bundle_route_with_revision(self):
+        bundle, bundle_data = factory.makeBundle(
+            self.db, name='gigis', owner='ladonna',
+            basket_with_rev='basket/1', series='raring',
+        )
+        path = '/~ladonna/bundles/basket/1/gigis'
+        response = self.app.get(path, status=200)
+        self.assertIn(
+            'lp:~ladonna/charms/bundles/basket/bundle', response.body)
+
     def test_personal_bundle_route_404(self):
         bundle, bundle_data = factory.makeBundle(
             self.db, name='gigis', owner='ladonna',
@@ -282,12 +292,22 @@
             self.db, name='wiki', owner='charmers',
             basket_with_rev='wiki-basket/1', series='raring', promulgated=True,
         )
-        path = '/bundles/wiki-basket/wiki/json'
+        path = '/bundles/wiki-basket/1/wiki/json'
         resp = self.app.get(path, status=200)
         self.assertIn(bundle.data['series'], resp.body)
         self.assertIn(str(bundle.data['relations']), resp.body)
         self.assertIn(str(bundle.data['services']), resp.body)
 
+    def test_official_bundle_with_revision_route(self):
+        bundle, bundle_data = factory.makeBundle(
+            self.db, name='wiki', owner='charmers',
+            basket_with_rev='wiki-basket/1', series='raring', promulgated=True,
+        )
+        path = '/bundles/wiki-basket/1/wiki'
+        response = self.app.get(path, status=200)
+        self.assertIn(
+            'lp:~charmers/charms/bundles/wiki-basket/bundle', response.body)
+
     def test_official_bundle_route_404(self):
         bundle, bundle_data = factory.makeBundle(
             self.db, name='gigis', owner='ladonna',
 
=== modified file 'charmworld/views/tests/test_charms.py'
--- charmworld/views/tests/test_charms.py	2013-08-22 16:23:51 +0000
+++ charmworld/views/tests/test_charms.py	2013-08-29 18:36:30 +0000
@@ -28,6 +28,7 @@
 from charmworld.views.api import API3
 from charmworld.views.charms import (
     _charm_view,
+    _reconcile_revision,
     charm_collection,
     config,
     distro_charm,
@@ -475,6 +476,49 @@
         self.assertEqual(expect, response['icon_path'])
 
 
+class CharmsViewHelpersTestCase(ViewTestBase):
+
+    def test_reconcile_revision_with_dashed_name(self):
+        # The charm name is reconstructed when a revision is wrongly matched.
+        request = self.getRequest()
+        request.matchdict = {
+            'charm': 'charm',
+            'revision': 'name',
+        }
+        _reconcile_revision(request)
+        self.assertEqual('charm-name', request.matchdict['charm'])
+        self.assertNotIn('revision', request.matchdict)
+
+    def test_reconcile_revision_with_revision(self):
+        # The charm name is reconstructed when a revision is wrongly matched.
+        request = self.getRequest()
+        request.matchdict = {
+            'charm': 'charm',
+            'revision': '1',
+        }
+        _reconcile_revision(request)
+        self.assertEqual('charm', request.matchdict['charm'])
+        self.assertEqual('1', request.matchdict['revision'])
+
+    def test_reconcile_revision_with_head_revision(self):
+        # The HEAD revision is dropped.
+        request = self.getRequest()
+        request.matchdict = {
+            'charm': 'charm',
+            'revision': 'HEAD',
+        }
+        _reconcile_revision(request)
+        self.assertEqual('charm', request.matchdict['charm'])
+        self.assertNotIn('revision', request.matchdict)
+        request.matchdict = {
+            'charm': 'charm',
+            'revision': 'head',
+        }
+        _reconcile_revision(request)
+        self.assertEqual('charm', request.matchdict['charm'])
+        self.assertNotIn('revision', request.matchdict)
+
+
 class CharmsWebTestCase(WebTestBase):
 
     def test_charm_collection_with_missing_data(self):
@@ -487,6 +531,86 @@
         resp = self.app.get('/charms/precise', status=200)
         self.assertIn(two['short_url'], resp.body)
 
+    def test_route_personal_charm_with_revision(self):
+        # The route /~owner/series/charm-1 finds charms using owner,
+        # series, charm name, and revision. The deploy instruction
+        # does not include the revision.
+        ignore, charm = factory.makeCharm(
+            self.db, owner='owner', series='series', name='charm', files={})
+        response = self.app.get('/~owner/series/charm-1', status=200)
+        self.assertIn('juju deploy cs:~owner/series/charm', response.body)
+
+    def test_route_personal_charm_with_dash_and_revision(self):
+        # /~owner/series/charm-name-1 route find the charm.
+        # This is a corner-case where the revision and charm name are
+        # ambigous because they are munged.
+        ignore, charm = factory.makeCharm(
+            self.db, owner='owner', series='series', name='charm-name',
+            files={})
+        response = self.app.get('/~owner/series/charm-name-1', status=200)
+        self.assertIn('juju deploy cs:~owner/series/charm-name', response.body)
+
+    def test_route_personal_charm_with_head_revision(self):
+        # the /~owner/series/charm-name-HEAD route finds the charm.
+        # The juju-gui apache rewrite versionless charm urls to help the GUI.
+        ignore, charm = factory.makeCharm(
+            self.db, owner='owner', series='series', name='charm', files={})
+        response = self.app.get('/~owner/series/charm-HEAD', status=200)
+        self.assertIn('juju deploy cs:~owner/series/charm', response.body)
+
+    def test_route_personal_charm_without_revision(self):
+        # The route /~owner/series/charm route finds the charm using owner
+        # series, and charm name. Revision is ignored. The deploy instruction
+        # matches the route.
+        ignore, charm = factory.makeCharm(
+            self.db, owner='owner', series='series', name='charm', files={})
+        response = self.app.get('/~owner/series/charm', status=200)
+        self.assertIn('juju deploy cs:~owner/series/charm', response.body)
+
+    def test_route_personal_charm_json_with_revision(self):
+        # The route /~owner/series/charm-1/json finds charms using owner,
+        # series, charm name, and revision. The address key does not contain
+        # the revision.
+        ignore, charm = factory.makeCharm(
+            self.db, owner='owner', series='series', name='charm', files={})
+        response = self.app.get('/~owner/series/charm-1/json', status=200)
+        self.assertEqual('cs:~owner/series/charm', response.json['address'])
+
+    def test_route_personal_charm_json_without_revision(self):
+        # The route /~owner/series/charm/json finds the charm using owner
+        # series, and charm name. Revision is ignored. The address key matches
+        # the route
+        ignore, charm = factory.makeCharm(
+            self.db, owner='owner', series='series', name='charm', files={})
+        response = self.app.get('/~owner/series/charm/json', status=200)
+        self.assertEqual('cs:~owner/series/charm', response.json['address'])
+
+    def test_route_promulgated_charm_with_revision(self):
+        # The route /series/charm-1 route finds promulgated charms using
+        # The series, charm name and version. The deploy instruction does not
+        # include the version.
+        ignore, charm = factory.makeCharm(
+            self.db, promulgated=True, series='series', name='charm', files={})
+        response = self.app.get('/series/charm-1', status=200)
+        self.assertIn('juju deploy cs:series/charm', response.body)
+
+    def test_route_promulgated_charm_with_head_revision(self):
+        # The route /series/charm-HEAD finds promulgated charms using
+        # series, charm name, and head revision. The revision is not included
+        # in the deploy instruction.
+        ignore, charm = factory.makeCharm(
+            self.db, promulgated=True, series='series', name='charm', files={})
+        response = self.app.get('/series/charm-HEAD', status=200)
+        self.assertIn('juju deploy cs:series/charm', response.body)
+
+    def test_route_promulgated_charm_without_revision(self):
+        # The route /series/charm finds the promulgated charms using just
+        # the series and charm name. The deploy instruction matches the route.
+        ignore, charm = factory.makeCharm(
+            self.db, promulgated=True, series='series', name='charm', files={})
+        response = self.app.get('/series/charm', status=200)
+        self.assertIn('juju deploy cs:series/charm', response.body)
+
 
 class TestQAForm(ViewTestBase):