Merge lp:~free.ekanayaka/txaws/api-method into lp:txaws

[Free Ekanayaka]

This branch adds a Method abstraction for encapsulating the logic that handles specific actions and versions. It also adds a Registry for registering the methods of a QueryAPI. The former QueryAPI.actions attribute is still supported but deprecated in favor of Registry.

You will need the python-venusian package from oneiric:

http://archive.ubuntu.com/ubuntu//pool/universe/p/python-venusian/python-venusian_0.9-1_all.deb

[lp:~free.ekanayaka/txaws/api-method]

93. By Free Ekanayaka on 2011-08-19

Move Registry to txaws.server.registry

[Thomas Herve (therve)]

Nice branch!

[1] The tests should be skipped if venusian is not present.

[2] It may be worth modifying setup.py to specify the optional dependency. Although it seems we're behind already.

[3]
+ if hasattr(self, "actions"):

Please use getattr(self, "actions", None) is not None.

[4] I guess backward compatibility is offered because you had to override the execute method?

[5]
+ actions = method_class.actions or [name]
+ versions = method_class.versions or [None]

Please use explicit conditionals here.

[6] I'm not sure I see the value of Method.actions being a list. Do you have a use case in mind?

[7] Having the version on the method is nice, but it's not really convenient. I'd like to have maybe the version introduced, but then have the method automatically available to newer versions. I guess we could make versions a property and implement some logic, but I wanted your point of view.

Thanks!

review: Needs Fixing

[lp:~free.ekanayaka/txaws/api-method]

94. By Free Ekanayaka on 2011-08-22

Add registry module, skip tests if venusian is not there

[Free Ekanayaka (free.ekanayaka)]

Thanks for the careful review.

[1] Fixed.

[2] Yeah, not even sure how to do that, it seems we don't have dependencies at all there? I'd make that another (possibly trivial) branch.

[3] Fixed.

[4] Not really, the only reason for backward compatibility is to not break Landscape and Clouddeck code when this is merged and the integration branches that I'm preparing are not yet merged as well. Also I don't want to break other people's code, though I believe we're the only ones using this stuff. This logic can be eventually dropped.

[5] Fixed.

[6] I was thinking to API changes, like you change the name of an action but you still want it to be handled by the same method class, or you have to different actions but with very similar behavior and you want to have a single method for them (say it's simpler or easier to maintain then two different methods with a base class). We don't have a use case in Landscape for now, so I can drop it if you prefer.

[7] There are several options to have the method available to newer versions. One could just specify None as version, or you can have a base class for your methods (something that in practice you're going to need anyway) and list all your default versions there, and override them for some particular methods. That's what I do in the integration branches that I'm preparing. Also, yes, you can make versions a property for greater control. In general, I think the supported versions is really an information that should be attached to methods, for fine grained control (when you need it, otherwise the patterns above work fine).

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Oops, auto-approved by mistake.

review: Abstain

[lp:~free.ekanayaka/txaws/api-method]

95. By Free Ekanayaka on 2011-08-22

Fix review points

[Thomas Herve (therve)]

Looks good, +1!

review: Approve

=== added file 'txaws/server/method.py'
--- txaws/server/method.py	1970-01-01 00:00:00 +0000
+++ txaws/server/method.py	2011-08-22 14:16:29 +0000
@@ -0,0 +1,45 @@
+def method(method_class):
+    """Decorator to use to mark an API method.
+
+    When invoking L{Registry.scan} the classes marked with this decorator
+    will be added to the registry.
+
+    @param method_class: The L{Method} class to register.
+    """
+
+    def callback(scanner, name, method_class):
+        if method_class.actions is not None:
+            actions = method_class.actions
+        else:
+            actions = [name]
+
+        if method_class.versions is not None:
+            versions = method_class.versions
+        else:
+            versions = [None]
+
+        for action in actions:
+            for version in versions:
+                scanner.registry.add(method_class,
+                                     action=action,
+                                     version=version)
+
+    from venusian import attach
+    attach(method_class, callback)
+    return method_class
+
+
+class Method(object):
+    """Handle a single HTTP request to an API resource.
+
+    @cvar actions: List of actions that the Method can handle, if C{None}
+        the class name will be used as only supported action.
+    @cvar versions: List of versions that the Method can handle, if C{None}
+        all versions will be supported.
+    """
+    actions = None
+    versions = None
+
+    def invoke(self, call):
+        """Invoke this method for executing the given C{call}."""
+        raise NotImplemented("Sub-classes have to implement the invoke method")

=== added file 'txaws/server/registry.py'
--- txaws/server/registry.py	1970-01-01 00:00:00 +0000
+++ txaws/server/registry.py	2011-08-22 14:16:29 +0000
@@ -0,0 +1,50 @@
+from txaws.server.exception import APIError
+
+
+class Registry(object):
+    """Register API L{Method}s. for handling specific actions and versions"""
+
+    def __init__(self):
+        self._by_action = {}
+
+    def add(self, method_class, action, version):
+        """Add a method class to the regitry.
+
+        @param method_class: The method class to add
+        @param action: The action that the method class can handle
+        @param version: The version that the method class can handle
+        """
+        by_version = self._by_action.setdefault(action, {})
+        if version in by_version:
+            raise RuntimeError("A method was already registered for action"
+                               " %s in version %s" % (action, version))
+        by_version[version] = method_class
+
+    def check(self, action, version):
+        """Check if the given action is supported in the given version.
+
+        @raises APIError: If there's no method class registered for handling
+            the given action or version.
+        """
+        if action not in self._by_action:
+            raise APIError(400, "InvalidAction", "The action %s is not valid "
+                           "for this web service." % action)
+        by_version = self._by_action[action]
+        if None not in by_version:
+            # There's no catch-all method, let's try the version-specific one
+            if version not in by_version:
+                raise APIError(400, "InvalidVersion", "Invalid API version.")
+
+    def get(self, action, version):
+        """Get the method class handing the given action and version."""
+        by_version = self._by_action[action]
+        if version in by_version:
+            return by_version[version]
+        else:
+            return by_version[None]
+
+    def scan(self, module):
+        """Scan the given module object for L{Method}s and register them."""
+        from venusian import Scanner
+        scanner = Scanner(registry=self)
+        scanner.scan(module)

=== modified file 'txaws/server/resource.py'
--- txaws/server/resource.py	2011-05-20 07:57:00 +0000
+++ txaws/server/resource.py	2011-08-22 14:16:29 +0000
@@ -19,6 +19,8 @@
 class QueryAPI(Resource):
     """Base class for  EC2-like query APIs.
 
+    @param registry: The L{Registry} to use to look up L{Method}s for handling
+        the API requests.
     @param path: Optionally, the actual resource path the clients are using
         when sending HTTP requests to this API, to take into account when
         validating the signature. This can differ from the one in the HTTP
@@ -29,8 +31,6 @@
 
     The following class variables must be defined by sub-classes:
 
-    @ivar actions: The actions that the API supports. The 'Action' field of
-        the request must contain one of these.
     @ivar signature_versions: A list of allowed values for 'SignatureVersion'.
     @cvar content_type: The content type to set the 'Content-Type' header to.
     """
@@ -48,9 +48,19 @@
         Unicode("Signature"),
         Integer("SignatureVersion", optional=True, default=2))
 
-    def __init__(self, path=None):
+    def __init__(self, registry=None, path=None):
         Resource.__init__(self)
         self.path = path
+        self.registry = registry
+
+    def get_method(self, call, *args, **kwargs):
+        """Return the L{Method} instance to invoke for the given L{Call}.
+
+        @param args: Positional arguments to pass to the method constructor.
+        @param kwargs: Keyword arguments to pass to the method constructor.
+        """
+        method_class = self.registry.get(call.action, call.version)
+        return method_class(*args, **kwargs)
 
     def get_principal(self, access_key):
         """Return a principal object by access key.
@@ -108,6 +118,16 @@
         """
         raise NotImplementedError("Must be implemented by subclass.")
 
+    def dump_result(self, result):
+        """Serialize the result of the method invokation.
+
+        @param result: The L{Method} result to serialize.
+        """
+        return result
+
+    def authorize(self, method, call):
+        """Authorize to invoke the given L{Method} with the given L{Call}."""
+
     def execute(self, call):
         """Execute an API L{Call}.
 
@@ -118,7 +138,10 @@
         @raises: An L{APIError} in case the execution fails, sporting an error
             message the HTTP status code to return.
         """
-        raise NotImplementedError()
+        method = self.get_method(call)
+        deferred = maybeDeferred(self.authorize, method, call)
+        deferred.addCallback(lambda _: method.invoke(call))
+        return deferred.addCallback(self.dump_result)
 
     def get_utc_time(self):
         """Return a C{datetime} object with the current time in UTC."""
@@ -142,7 +165,7 @@
         params = dict((k, v[-1]) for k, v in request.args.iteritems())
         args, rest = self.schema.extract(params)
 
-        self._validate_generic_parameters(args, self.get_utc_time())
+        self._validate_generic_parameters(args)
 
         def create_call(principal):
             self._validate_principal(principal, args)
@@ -157,11 +180,10 @@
         deferred.addCallback(create_call)
         return deferred
 
-    def _validate_generic_parameters(self, args, utc_now):
+    def _validate_generic_parameters(self, args):
         """Validate the generic request parameters.
 
         @param args: Parsed schema arguments.
-        @param utc_now: The current UTC time in datetime format.
         @raises APIError: In the following cases:
             - Action is not included in C{self.actions}
             - SignatureVersion is not included in C{self.signature_versions}
@@ -169,9 +191,15 @@
             - Expires is before the current time
             - Timestamp is older than 15 minutes.
         """
-        if not args.Action in self.actions:
-            raise APIError(400, "InvalidAction", "The action %s is not valid "
-                           "for this web service." % args.Action)
+        utc_now = self.get_utc_time()
+
+        if getattr(self, "actions", None) is not None:
+            # Check the deprecated 'actions' attribute
+            if not args.Action in self.actions:
+                raise APIError(400, "InvalidAction", "The action %s is not "
+                               "valid for this web service." % args.Action)
+        else:
+            self.registry.check(args.Action, args.Version)
 
         if not args.SignatureVersion in self.signature_versions:
             raise APIError(403, "InvalidSignature", "SignatureVersion '%s' "

=== added file 'txaws/server/tests/test_method.py'
--- txaws/server/tests/test_method.py	1970-01-01 00:00:00 +0000
+++ txaws/server/tests/test_method.py	2011-08-22 14:16:29 +0000
@@ -0,0 +1,18 @@
+from twisted.trial.unittest import TestCase
+
+from txaws.server.method import Method
+
+
+class MethodTest(TestCase):
+
+    def setUp(self):
+        super(MethodTest, self).setUp()
+        self.method = Method()
+
+    def test_defaults(self):
+        """
+        By default a L{Method} applies to all API versions and handles a
+        single action matching its class name.
+        """
+        self.assertIdentical(None, self.method.actions)
+        self.assertIdentical(None, self.method.versions)

=== added file 'txaws/server/tests/test_registry.py'
--- txaws/server/tests/test_registry.py	1970-01-01 00:00:00 +0000
+++ txaws/server/tests/test_registry.py	2011-08-22 14:16:29 +0000
@@ -0,0 +1,97 @@
+from twisted.trial.unittest import TestCase
+
+from txaws.server.method import Method
+from txaws.server.registry import Registry
+from txaws.server.exception import APIError
+
+try:
+    import venusian
+except ImportError:
+    method = lambda function: function
+    has_venusian = False
+else:
+    from txaws.server.method import method
+    has_venusian = True
+
+
+@method
+class TestMethod(Method):
+    pass
+
+
+class RegistryTest(TestCase):
+
+    def setUp(self):
+        super(RegistryTest, self).setUp()
+        self.registry = Registry()
+
+    def test_add(self):
+        """
+        L{MehtodRegistry.add} registers a method class for the given action
+        and version.
+        """
+        self.registry.add(TestMethod, "test", "1.0")
+        self.registry.add(TestMethod, "test", "2.0")
+        self.registry.check("test", "1.0")
+        self.registry.check("test", "2.0")
+        self.assertIdentical(TestMethod, self.registry.get("test", "1.0"))
+        self.assertIdentical(TestMethod, self.registry.get("test", "2.0"))
+
+    def test_add_duplicate_method(self):
+        """
+        L{MehtodRegistry.add} fails if a method class for the given action
+        and version was already registered.
+        """
+
+        class TestMethod2(Method):
+            pass
+
+        self.registry.add(TestMethod, "test", "1.0")
+        self.assertRaises(RuntimeError, self.registry.add, TestMethod2,
+                          "test", "1.0")
+
+    def test_get(self):
+        """
+        L{MehtodRegistry.get} returns the method class registered for the
+        given action and version.
+        """
+
+        class TestMethod2(Method):
+            pass
+
+        self.registry.add(TestMethod, "test", "1.0")
+        self.registry.add(TestMethod, "test", "2.0")
+        self.registry.add(TestMethod2, "test", "3.0")
+        self.assertIdentical(TestMethod, self.registry.get("test", "1.0"))
+        self.assertIdentical(TestMethod, self.registry.get("test", "2.0"))
+        self.assertIdentical(TestMethod2, self.registry.get("test", "3.0"))
+
+    def test_check_with_missing_action(self):
+        """
+        L{MehtodRegistry.get} fails if the given action is not registered.
+        """
+        error = self.assertRaises(APIError, self.registry.check, "boom", "1.0")
+        self.assertEqual(400, error.status)
+        self.assertEqual("InvalidAction", error.code)
+        self.assertEqual("The action boom is not valid for this web service.",
+                         error.message)
+
+    def test_check_with_missing_version(self):
+        """
+        L{MehtodRegistry.get} fails if the given action is not registered.
+        """
+        self.registry.add(TestMethod, "test", "1.0")
+        error = self.assertRaises(APIError, self.registry.check, "test", "2.0")
+        self.assertEqual(400, error.status)
+        self.assertEqual("InvalidVersion", error.code)
+        self.assertEqual("Invalid API version.", error.message)
+
+    def test_scan(self):
+        """
+        L{MehtodRegistry.scan} registers the L{Method}s decorated with L{api}.
+        """
+        self.registry.scan(__import__(__name__))
+        self.assertIdentical(TestMethod, self.registry.get("TestMethod", None))
+
+    if not has_venusian:
+        test_scan.skip = "venusian module not available"

=== modified file 'txaws/server/tests/test_resource.py'
--- txaws/server/tests/test_resource.py	2011-05-19 15:45:27 +0000
+++ txaws/server/tests/test_resource.py	2011-08-22 14:16:29 +0000
@@ -1,3 +1,4 @@
+from json import dumps, loads
 from pytz import UTC
 from cStringIO import StringIO
 from datetime import datetime
@@ -7,6 +8,8 @@
 from txaws.credentials import AWSCredentials
 from txaws.service import AWSServiceEndpoint
 from txaws.ec2.client import Query
+from txaws.server.method import Method
+from txaws.server.registry import Registry
 from txaws.server.resource import QueryAPI
 
 
@@ -55,6 +58,12 @@
         return self.written.getvalue()
 
 
+class TestMethod(Method):
+
+    def invoke(self, call):
+        return "data"
+
+
 class TestPrincipal(object):
 
     def __init__(self, creds):
@@ -71,7 +80,6 @@
 
 class TestQueryAPI(QueryAPI):
 
-    actions = ["SomeAction"]
     signature_versions = (1, 2)
     content_type = "text/plain"
 
@@ -79,9 +87,6 @@
         QueryAPI.__init__(self, *args, **kwargs)
         self.principal = None
 
-    def execute(self, call):
-        return "data"
-
     def get_principal(self, access_key):
         if self.principal and self.principal.access_key == access_key:
             return self.principal
@@ -94,7 +99,9 @@
 
     def setUp(self):
         super(QueryAPITest, self).setUp()
-        self.api = TestQueryAPI()
+        self.registry = Registry()
+        self.registry.add(TestMethod, action="SomeAction", version=None)
+        self.api = TestQueryAPI(registry=self.registry)
 
     def test_handle(self):
         """
@@ -116,11 +123,46 @@
         self.api.principal = TestPrincipal(creds)
         return self.api.handle(request).addCallback(check)
 
+    def test_handle_with_dump_result(self):
+        """
+        L{QueryAPI.handle} serializes the action result with C{dump_result}.
+        """
+        creds = AWSCredentials("access", "secret")
+        endpoint = AWSServiceEndpoint("http://uri")
+        query = Query(action="SomeAction", creds=creds, endpoint=endpoint)
+        query.sign()
+        request = FakeRequest(query.params, endpoint)
+
+        def check(ignored):
+            self.assertEqual("data", loads(request.response))
+
+        self.api.dump_result = dumps
+        self.api.principal = TestPrincipal(creds)
+        return self.api.handle(request).addCallback(check)
+
+    def test_handle_with_deprecated_actions(self):
+        """
+        L{QueryAPI.handle} supports the legacy 'actions' attribute.
+        """
+        self.api.actions = ["SomeAction"]
+        creds = AWSCredentials("access", "secret")
+        endpoint = AWSServiceEndpoint("http://uri")
+        query = Query(action="SomeAction", creds=creds, endpoint=endpoint)
+        query.sign()
+        request = FakeRequest(query.params, endpoint)
+
+        def check(ignored):
+            self.assertEqual("data", request.response)
+
+        self.api.principal = TestPrincipal(creds)
+        return self.api.handle(request).addCallback(check)
+
     def test_handle_pass_params_to_call(self):
         """
         L{QueryAPI.handle} creates a L{Call} object with the correct
         parameters.
         """
+        self.registry.add(TestMethod, "SomeAction", "1.2.3")
         creds = AWSCredentials("access", "secret")
         endpoint = AWSServiceEndpoint("http://uri")
         query = Query(action="SomeAction", creds=creds, endpoint=endpoint,
@@ -250,7 +292,27 @@
         return self.api.handle(request).addCallback(check)
 
     def test_handle_with_unsupported_action(self):
-        """Only actions listed in L{QueryAPI.actions} are supported."""
+        """Only actions registered in the L{Registry} are supported."""
+        creds = AWSCredentials("access", "secret")
+        endpoint = AWSServiceEndpoint("http://uri")
+        query = Query(action="FooBar", creds=creds, endpoint=endpoint)
+        query.sign()
+        request = FakeRequest(query.params, endpoint)
+
+        def check(ignored):
+            self.flushLoggedErrors()
+            self.assertEqual("InvalidAction - The action FooBar is not valid"
+                             " for this web service.", request.response)
+            self.assertEqual(400, request.code)
+
+        return self.api.handle(request).addCallback(check)
+
+    def test_handle_with_deprecated_actions_and_unsupported_action(self):
+        """
+        If the deprecated L{QueryAPI.actions} attribute is set, it will be
+        used for looking up supported actions.
+        """
+        self.api.actions = ["SomeAction"]
         creds = AWSCredentials("access", "secret")
         endpoint = AWSServiceEndpoint("http://uri")
         query = Query(action="FooBar", creds=creds, endpoint=endpoint)