launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/maas-cli-grok-anon-handlers into lp:maas with lp:~allenap/maas/break-on-invalid-oauth as a prerequisite.

Commit message:
The command-line interface now understands the new resource-centric API description format.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1058316 in MAAS: "maas-cli reports unknown operation if you're logged in with the wrong credentials"
  https://bugs.launchpad.net/maas/+bug/1058316

For more details, see:
https://code.launchpad.net/~allenap/maas/maas-cli-grok-anon-handlers/+merge/128381
-- 
https://code.launchpad.net/~allenap/maas/maas-cli-grok-anon-handlers/+merge/128381
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/maas-cli-grok-anon-handlers into lp:maas.

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2012-10-05 18:44:46 +0000
+++ src/maascli/api.py	2012-10-07 11:55:26 +0000
@@ -15,11 +15,13 @@
     "register_cli_commands",
     ]
 
+from collections import defaultdict
 from email.message import Message
 from getpass import getpass
 import httplib
 from itertools import chain
 import json
+from operator import itemgetter
 import sys
 from urlparse import (
     urljoin,
@@ -366,15 +368,47 @@
             execute=action_class(action_parser))
 
 
-def register_handlers(profile, parser):
-    """Register a profile's handlers."""
+def register_handler(profile, handler, parser):
+    """Register a resource's handler."""
+    help_title, help_body = parse_docstring(handler["doc"])
+    handler_name = handler_command_name(handler["name"])
+    handler_parser = parser.subparsers.add_parser(
+        handler_name, help=help_title, description=help_body)
+    register_actions(profile, handler, handler_parser)
+
+
+def register_resources(profile, parser):
+    """Register a profile's resources."""
+    anonymous = profile["credentials"] is None
     description = profile["description"]
-    for handler in description["handlers"]:
-        help_title, help_body = parse_docstring(handler["doc"])
-        handler_name = handler_command_name(handler["name"])
-        handler_parser = parser.subparsers.add_parser(
-            handler_name, help=help_title, description=help_body)
-        register_actions(profile, handler, handler_parser)
+    resources = description["resources"]
+    for resource in sorted(resources, key=itemgetter("name")):
+        # Don't consider the authenticated handler if this profile has no
+        # credentials associated with it.
+        if anonymous:
+            handlers = [resource["anon"]]
+        else:
+            handlers = [resource["auth"], resource["anon"]]
+        # Merge actions from the active handlers. This could be slightly
+        # simpler using a dict and going through the handlers in reverse, but
+        # doing it forwards with a defaultdict(list) leaves an easier-to-debug
+        # structure, and ought to be easier to understand.
+        actions = defaultdict(list)
+        for handler in handlers:
+            if handler is not None:
+                for action in handler["actions"]:
+                    action_name = action["name"]
+                    actions[action_name].append(action)
+        # Always represent this resource using the authenticated handler, if
+        # defined, before the fall-back anonymous handler, even if this
+        # profile does not have credentials.
+        represent_as = resource["auth"] or resource["anon"]
+        represent_as = dict(represent_as, name=resource["name"], actions=[])
+        # Register the handler using the first actions discovered.
+        if len(actions) != 0:
+            represent_as["actions"].extend(
+                value[0] for value in actions.values())
+            register_handler(profile, represent_as, parser)
 
 
 commands = {
@@ -401,4 +435,4 @@
             profile = config[profile_name]
             profile_parser = parser.subparsers.add_parser(
                 profile["name"], help="Interact with %(url)s" % profile)
-            register_handlers(profile, profile_parser)
+            register_resources(profile, profile_parser)

=== modified file 'src/maascli/tests/test_api.py'
--- src/maascli/tests/test_api.py	2012-10-05 09:41:33 +0000
+++ src/maascli/tests/test_api.py	2012-10-07 11:55:26 +0000
@@ -26,6 +26,10 @@
     )
 from maascli.command import CommandError
 from maascli.config import ProfileConfig
+from maascli.utils import (
+    handler_command_name,
+    safe_name,
+    )
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
 from mock import sentinel
@@ -49,37 +53,69 @@
 class TestRegisterAPICommands(TestCase):
     """Tests for `register_api_commands`."""
 
+    def make_handler(self):
+        return {
+            'name': factory.make_name('handler'),
+            'doc': "Short\n\nLong",
+            'params': [],
+            'actions': [
+                {'name': factory.make_name('action'),
+                 'doc': "Doc\n\nstring"},
+                ],
+            }
+
+    def make_resource(self, anon=True, auth=True):
+        auth = self.make_handler() if auth else None
+        anon = self.make_handler() if anon else None
+        name = factory.make_name("resource")
+        return {"auth": auth, "anon": anon, "name": name}
+
     def make_profile(self):
         """Fake a profile."""
-        profile = factory.make_name('profile')
-        url = 'http://%s.example.com/' % profile
+        profile_name = factory.make_name('profile')
+        profile = {
+            'name': profile_name,
+            'url': 'http://%s.example.com/' % profile_name,
+            'credentials': factory.make_name("credentials"),
+            'description': {
+                'resources': [
+                    self.make_resource(),
+                    self.make_resource(),
+                    ],
+                },
+            }
         fake_open = self.patch(ProfileConfig, 'open')
-        fake_open.return_value = FakeConfig({
-            profile: {
-                'name': profile,
-                'url': url,
-                'description': {
-                    'handlers': [{
-                        'name': factory.make_name('handler'),
-                        'doc': "Short\n\nLong",
-                        'params': [],
-                        'actions': [{
-                            'name': factory.make_name('action'),
-                            'doc': "Doc\n\nstring",
-                        }],
-                    }],
-                },
-            },
-        })
+        fake_open.return_value = FakeConfig({profile_name: profile})
         return profile
 
     def test_registers_subparsers(self):
+        profile_name = self.make_profile()["name"]
+        parser = ArgumentParser()
+        self.assertIsNone(parser._subparsers)
+        api.register_api_commands(parser)
+        self.assertIsNotNone(parser._subparsers)
+        self.assertIsNotNone(parser.subparsers.choices[profile_name])
+
+    def test_handlers_registered_using_correct_names(self):
         profile = self.make_profile()
         parser = ArgumentParser()
-        self.assertIsNone(parser._subparsers)
         api.register_api_commands(parser)
-        self.assertIsNotNone(parser._subparsers)
-        self.assertIsNotNone(parser.subparsers.choices[profile])
+        for resource in profile["description"]["resources"]:
+            for action in resource["auth"]["actions"]:
+                # Profile names are matched as-is.
+                profile_name = profile["name"]
+                # Handler names are processed with handler_command_name before
+                # being added to the argument parser tree.
+                handler_name = handler_command_name(resource["name"])
+                # Action names are processed with safe_name before being added
+                # to the argument parser tree.
+                action_name = safe_name(action["name"])
+                # Parsing these names as command-line arguments yields an
+                # options object. Its execute attribute is an instance of
+                # Action (or a subclass thereof).
+                options = parser.parse_args(
+                    (profile_name, handler_name, action_name))
+                self.assertIsInstance(options.execute, api.Action)
 
 
 class TestRegisterCLICommands(TestCase):