Merged in jmchilton/galaxy-central-fork-1 (pull request #294)

New API Decorator

lib/galaxy/exceptions/__init__.py

@@ -6,33 +6,66 @@ from galaxy import eggs
 eggs.require( "Paste" )
 
 from paste import httpexceptions
+from ..exceptions import error_codes
+
 
 class MessageException( Exception ):
     """
-    Exception to make throwing errors from deep in controllers easier
+    Exception to make throwing errors from deep in controllers easier.
     """
-    def __init__( self, err_msg, type="info" ):
-        self.err_msg = err_msg
+    # status code to be set when used with API.
+    status_code = 400
+    # Error code information embedded into API json responses.
+    err_code = error_codes.UNKNOWN
+
+    def __init__( self, err_msg=None, type="info", **extra_error_info ):
+        self.err_msg = err_msg or self.err_code.default_error_message
         self.type = type
+        self.extra_error_info = extra_error_info
+
     def __str__( self ):
         return self.err_msg
 
+
 class ItemDeletionException( MessageException ):
     pass
 
+
 class ItemAccessibilityException( MessageException ):
-    pass
+    status_code = 403
+    err_code = error_codes.USER_CANNOT_ACCESS_ITEM
+
 
 class ItemOwnershipException( MessageException ):
-    pass
+    status_code = 403
+    err_code = error_codes.USER_DOES_NOT_OWN_ITEM
+
+
+class DuplicatedSlugException( MessageException ):
+    status_code = 400
+    err_code = error_codes.USER_SLUG_DUPLICATE
+
+
+class ObjectAttributeInvalidException( MessageException ):
+    status_code = 400
+    err_code = error_codes.USER_OBJECT_ATTRIBUTE_INVALID
+
+
+class ObjectAttributeMissingException( MessageException ):
+    status_code = 400
+    err_code = error_codes.USER_OBJECT_ATTRIBUTE_MISSING
+
 
 class ActionInputError( MessageException ):
     def __init__( self, err_msg, type="error" ):
         super( ActionInputError, self ).__init__( err_msg, type )
 
-class ObjectNotFound( Exception ):
+
+class ObjectNotFound( MessageException ):
     """ Accessed object was not found """
-    pass
+    status_code = 404
+    err_code = error_codes.USER_OBJECT_NOT_FOUND
+
 
 class ObjectInvalid( Exception ):
     """ Accessed object store ID is invalid """

lib/galaxy/exceptions/error_codes.json

@@ -0,0 +1,57 @@
+[
+   {
+    "name": "UNKNOWN",
+    "code": 0,
+    "message": "Unknown error occurred while processing request."
+    },
+   {
+    "name": "USER_CANNOT_RUN_AS",
+    "code": 400001,
+    "message": "User does not have permissions to run jobs as another user."
+    },
+   {
+    "name": "USER_INVALID_RUN_AS",
+    "code": 400002,
+    "message": "Invalid run_as request - run_as user does not exist."
+    },
+   {
+    "name": "USER_INVALID_JSON",
+    "code": 400003,
+    "message": "Your request did not appear to be valid JSON, please consult the API documentation."
+    },
+   {
+    "name": "USER_OBJECT_ATTRIBUTE_INVALID",
+    "code": 400004,
+    "message": "Attempted to create or update object with invalid attribute value."
+    },
+   {
+    "name": "USER_OBJECT_ATTRIBUTE_MISSING",
+    "code": 400005,
+    "message": "Attempted to create object without required attribute."
+    },
+   {
+    "name": "USER_SLUG_DUPLICATE",
+    "code": 400006,
+    "message": "Slug must be unique per user."
+    },
+   {
+    "name": "USER_NO_API_KEY",
+    "code": 403001,
+    "message": "API Authentication Required for this request"
+    },
+   {
+    "name": "USER_CANNOT_ACCESS_ITEM",
+    "code": 403002,
+    "message": "User cannot access specified item."
+    },
+   {
+    "name": "USER_DOES_NOT_OWN_ITEM",
+    "code": 403003,
+    "message": "User does not own specified item."
+   },
+   {
+    "name": "USER_OBJECT_NOT_FOUND",
+    "code": 404001,
+    "message": "No such object not found."
+   }
+]

lib/galaxy/exceptions/error_codes.py

@@ -0,0 +1,32 @@
+from pkg_resources import resource_string
+from json import loads
+
+# Error codes are provided as a convience to Galaxy API clients, but at this
+# time they do represent part of the more stable interface. They can change
+# without warning between releases.
+UNKNOWN_ERROR_MESSAGE = "Unknown error occurred while processing request."
+
+
+class ErrorCode( object ):
+
+    def __init__( self, code, default_error_message ):
+        self.code = code
+        self.default_error_message = default_error_message or UNKNOWN_ERROR_MESSAGE
+
+    def __str__( self ):
+        return str( self.default_error_message )
+
+    def __int__( self ):
+        return int( self.code )
+
+    @staticmethod
+    def from_dict( entry ):
+        name = entry.get("name")
+        code = entry.get("code")
+        message = entry.get("message")
+        return ( name, ErrorCode( code, message ) )
+
+error_codes_json = resource_string( __name__, 'error_codes.json' )
+for entry in loads( error_codes_json ):
+    name, error_code_obj = ErrorCode.from_dict( entry )
+    globals()[ name ] = error_code_obj

lib/galaxy/web/__init__.py

@@ -15,3 +15,7 @@ from framework import expose_api_anonymous
 from framework import expose_api_raw
 from framework import expose_api_raw_anonymous
 from framework.base import httpexceptions
+
+# TODO: Drop and make these the default.
+from framework import _future_expose_api
+from framework import _future_expose_api_anonymous

lib/galaxy/web/framework/__init__.py

@@ -10,9 +10,9 @@ import random
 import socket
 import string
 import time
-
-from functools import wraps
+from traceback import format_exc
 from Cookie import CookieError
+from functools import wraps
 
 pkg_resources.require( "Cheetah" )
 from Cheetah.Template import Template
@@ -23,6 +23,7 @@ import helpers
 
 from galaxy import util
 from galaxy.exceptions import MessageException
+from galaxy.exceptions import error_codes
 from galaxy.util import asbool
 from galaxy.util import safe_str_cmp
 from galaxy.util.backports.importlib import import_module
@@ -103,6 +104,35 @@ def require_login( verb="perform this action", use_panels=False, webapp='galaxy'
         return decorator
     return argcatcher
 
+
+def __extract_payload_from_request(trans, func, kwargs):
+    content_type = trans.request.headers['content-type']
+    if content_type.startswith('application/x-www-form-urlencoded') or content_type.startswith('multipart/form-data'):
+        # If the content type is a standard type such as multipart/form-data, the wsgi framework parses the request body
+        # and loads all field values into kwargs. However, kwargs also contains formal method parameters etc. which
+        # are not a part of the request body. This is a problem because it's not possible to differentiate between values
+        # which are a part of the request body, and therefore should be a part of the payload, and values which should not be
+        # in the payload. Therefore, the decorated method's formal arguments are discovered through reflection and removed from
+        # the payload dictionary. This helps to prevent duplicate argument conflicts in downstream methods.
+        payload = kwargs.copy()
+        named_args, _, _, _ = inspect.getargspec(func)
+        for arg in named_args:
+            payload.pop(arg, None)
+        for k, v in payload.iteritems():
+            if isinstance(v, (str, unicode)):
+                try:
+                    payload[k] = from_json_string(v)
+                except:
+                    # may not actually be json, just continue
+                    pass
+        payload = util.recursively_stringify_dictionary_keys( payload )
+    else:
+        # Assume application/json content type and parse request body manually, since wsgi won't do it. However, the order of this check
+        # should ideally be in reverse, with the if clause being a check for application/json and the else clause assuming a standard encoding
+        # such as multipart/form-data. Leaving it as is for backward compatibility, just in case.
+        payload = util.recursively_stringify_dictionary_keys( from_json_string( trans.request.body ) )
+    return payload
+
 def expose_api_raw( func ):
     """
     Expose this function via the API but don't dump the results
@@ -140,35 +170,8 @@ def expose_api( func, to_json=True, user_required=True ):
             error_message = "API Authentication Required for this request"
             return error
         if trans.request.body:
-            def extract_payload_from_request(trans, func, kwargs):
-                content_type = trans.request.headers['content-type']
-                if content_type.startswith('application/x-www-form-urlencoded') or content_type.startswith('multipart/form-data'):
-                    # If the content type is a standard type such as multipart/form-data, the wsgi framework parses the request body
-                    # and loads all field values into kwargs. However, kwargs also contains formal method parameters etc. which
-                    # are not a part of the request body. This is a problem because it's not possible to differentiate between values
-                    # which are a part of the request body, and therefore should be a part of the payload, and values which should not be
-                    # in the payload. Therefore, the decorated method's formal arguments are discovered through reflection and removed from
-                    # the payload dictionary. This helps to prevent duplicate argument conflicts in downstream methods.
-                    payload = kwargs.copy()
-                    named_args, _, _, _ = inspect.getargspec(func)
-                    for arg in named_args:
-                        payload.pop(arg, None)
-                    for k, v in payload.iteritems():
-                        if isinstance(v, (str, unicode)):
-                            try:
-                                payload[k] = from_json_string(v)
-                            except:
-                                # may not actually be json, just continue
-                                pass
-                    payload = util.recursively_stringify_dictionary_keys( payload )
-                else:
-                    # Assume application/json content type and parse request body manually, since wsgi won't do it. However, the order of this check
-                    # should ideally be in reverse, with the if clause being a check for application/json and the else clause assuming a standard encoding
-                    # such as multipart/form-data. Leaving it as is for backward compatibility, just in case.
-                    payload = util.recursively_stringify_dictionary_keys( from_json_string( trans.request.body ) )
-                return payload
             try:
-                kwargs['payload'] = extract_payload_from_request(trans, func, kwargs)
+                kwargs['payload'] = __extract_payload_from_request(trans, func, kwargs)
             except ValueError:
                 error_status = '400 Bad Request'
                 error_message = 'Your request did not appear to be valid JSON, please consult the API documentation'
@@ -210,6 +213,143 @@ def expose_api( func, to_json=True, user_required=True ):
     decorator.exposed = True
     return decorator
 
+API_RESPONSE_CONTENT_TYPE = "application/json"
+
+
+def __api_error_message( trans, **kwds ):
+    exception = kwds.get( "exception", None )
+    if exception:
+        # If we are passed a MessageException use err_msg.
+        default_error_code = getattr( exception, "err_code", error_codes.UNKNOWN )
+        default_error_message = getattr( exception, "err_msg", default_error_code.default_error_message )
+        extra_error_info = getattr( exception, 'extra_error_info', {} )
+        if not isinstance( extra_error_info, dict ):
+            extra_error_info = {}
+    else:
+        default_error_message = "Error processing API request."
+        default_error_code = error_codes.UNKNOWN
+        extra_error_info = {}
+    traceback_string = kwds.get( "traceback", "No traceback available." )
+    err_msg = kwds.get( "err_msg", default_error_message )
+    error_code_object = kwds.get( "err_code", default_error_code )
+    try:
+        error_code = error_code_object.code
+    except AttributeError:
+        # Some sort of bad error code sent in, logic failure on part of
+        # Galaxy developer.
+        error_code = error_codes.UNKNOWN.code
+    # Would prefer the terminology of error_code and error_message, but
+    # err_msg used a good number of places already. Might as well not change
+    # it?
+    error_response = dict( err_msg=err_msg, err_code=error_code, **extra_error_info )
+    if trans.debug:  # TODO: Should admins get to see traceback as well?
+        error_response[ "traceback" ] = traceback_string
+    return error_response
+
+
+def __api_error_response( trans, **kwds ):
+    error_dict = __api_error_message( trans, **kwds )
+    exception = kwds.get( "exception", None )
+    # If we are given an status code directly - use it - otherwise check
+    # the exception for a status_code attribute.
+    if "status_code" in kwds:
+        status_code = int( kwds.get( "status_code" ) )
+    elif hasattr( exception, "status_code" ):
+        status_code = int( exception.status_code )
+    else:
+        status_code = 500
+    response = trans.response
+    if not response.status or str(response.status).startswith("20"):
+        # Unset status code appears to be string '200 OK', if anything
+        # non-success (i.e. not 200 or 201) has been set, do not override
+        # underlying controller.
+        response.status = status_code
+    return to_json_string( error_dict )
+
+
+# TODO: rename as expose_api and make default.
+def _future_expose_api_anonymous( func, to_json=True ):
+    """
+    Expose this function via the API but don't require a set user.
+    """
+    return _future_expose_api( func, to_json=to_json, user_required=False )
+
+
+# TODO: rename as expose_api and make default.
+def _future_expose_api( func, to_json=True, user_required=True ):
+    """
+    Expose this function via the API.
+    """
+    @wraps(func)
+    def decorator( self, trans, *args, **kwargs ):
+        if trans.error_message:
+            # TODO: Document this branch, when can this happen,
+            # I don't understand it.
+            return __api_error_response( trans, err_msg=trans.error_message )
+        if user_required and trans.anonymous:
+            error_code = error_codes.USER_NO_API_KEY
+            # Use error codes default error message.
+            return __api_error_response( trans, err_code=error_code, status_code=403 )
+        if trans.request.body:
+            try:
+                kwargs['payload'] = __extract_payload_from_request(trans, func, kwargs)
+            except ValueError:
+                error_code = error_codes.USER_INVALID_JSON
+                return __api_error_response( trans, status_code=400, err_code=error_code )
+
+        trans.response.set_content_type( API_RESPONSE_CONTENT_TYPE )
+        # send 'do not cache' headers to handle IE's caching of ajax get responses
+        trans.response.headers[ 'Cache-Control' ] = "max-age=0,no-cache,no-store"
+        # TODO: Refactor next block out into a helper procedure.
+        # Perform api_run_as processing, possibly changing identity
+        if 'payload' in kwargs and 'run_as' in kwargs['payload']:
+            if not trans.user_can_do_run_as():
+                error_code = error_codes.USER_CANNOT_RUN_AS
+                return __api_error_response( trans, err_code=error_code, status_code=403 )
+            try:
+                decoded_user_id = trans.security.decode_id( kwargs['payload']['run_as'] )
+            except TypeError:
+                error_message = "Malformed user id ( %s ) specified, unable to decode." % str( kwargs['payload']['run_as'] )
+                error_code = error_codes.USER_INVALID_RUN_AS
+                return __api_error_response( trans, err_code=error_code, err_msg=error_message, status_code=400)
+            try:
+                user = trans.sa_session.query( trans.app.model.User ).get( decoded_user_id )
+                trans.api_inherit_admin = trans.user_is_admin()
+                trans.set_user(user)
+            except:
+                error_code = error_codes.USER_INVALID_RUN_AS
+                return __api_error_response( trans, err_code=error_code, status_code=400 )
+        try:
+            rval = func( self, trans, *args, **kwargs)
+            if to_json and trans.debug:
+                rval = to_json_string( rval, indent=4, sort_keys=True )
+            elif to_json:
+                rval = to_json_string( rval )
+            return rval
+        except MessageException as e:
+            traceback_string = format_exc()
+            return __api_error_response( trans, exception=e, traceback=traceback_string )
+        except paste.httpexceptions.HTTPException:
+            # TODO: Allow to pass or format for the API???
+            raise  # handled
+        except Exception as e:
+            traceback_string = format_exc()
+            error_message = 'Uncaught exception in exposed API method:'
+            log.exception( error_message )
+            return __api_error_response(
+                trans,
+                status_code=500,
+                exception=e,
+                traceback=traceback_string,
+                err_msg=error_message,
+                err_code=error_codes.UNKNOWN
+            )
+    if not hasattr(func, '_orig'):
+        decorator._orig = func
+    decorator.exposed = True
+    return decorator
+
+
 def require_admin( func ):
     @wraps(func)
     def decorator( self, trans, *args, **kwargs ):

lib/galaxy/webapps/galaxy/api/histories.py

@@ -9,6 +9,8 @@ pkg_resources.require( "Paste" )
 from paste.httpexceptions import HTTPBadRequest, HTTPForbidden, HTTPInternalServerError, HTTPException
 
 from galaxy import web
+from galaxy.web import _future_expose_api as expose_api
+from galaxy.web import _future_expose_api_anonymous as expose_api_anonymous
 from galaxy.util import string_as_bool, restore_text
 from galaxy.util.sanitize_html import sanitize_html
 from galaxy.web.base.controller import BaseAPIController, UsesHistoryMixin, UsesTagsMixin
@@ -20,7 +22,7 @@ log = logging.getLogger( __name__ )
 
 class HistoriesController( BaseAPIController, UsesHistoryMixin, UsesTagsMixin ):
 
-    @web.expose_api_anonymous
+    @expose_api_anonymous
     def index( self, trans, deleted='False', **kwd ):
         """
         index( trans, deleted='False' )
@@ -152,7 +154,7 @@ class HistoriesController( BaseAPIController, UsesHistoryMixin, UsesTagsMixin ):
 
         return history_data
 
-    @web.expose_api
+    @expose_api
     def create( self, trans, payload, **kwd ):
         """
         create( trans, payload )

lib/galaxy/webapps/galaxy/api/page_revisions.py

@@ -2,8 +2,9 @@
 API for updating Galaxy Pages
 """
 import logging
-from galaxy import web
+from galaxy.web import _future_expose_api as expose_api
 from galaxy.web.base.controller import SharableItemSecurityMixin, BaseAPIController, SharableMixin
+from galaxy import exceptions
 from galaxy.model.item_attrs import UsesAnnotations
 from galaxy.util.sanitize_html import sanitize_html
 
@@ -12,7 +13,7 @@ log = logging.getLogger( __name__ )
 
 class PageRevisionsController( BaseAPIController, SharableItemSecurityMixin, UsesAnnotations, SharableMixin ):
 
-    @web.expose_api
+    @expose_api
     def index( self, trans, page_id, **kwd ):
         """
         index( self, trans, page_id, **kwd )
@@ -24,14 +25,16 @@ class PageRevisionsController( BaseAPIController, SharableItemSecurityMixin, Use
         :rtype:     list
         :returns:   dictionaries containing different revisions of the page
         """
+        page = self._get_page( trans, page_id )
+        self._verify_page_ownership( trans, page )
+
         r = trans.sa_session.query( trans.app.model.PageRevision ).filter_by( page_id=trans.security.decode_id(page_id) )
         out = []
         for page in r:
-            if self.security_check( trans, page, True, True ):
-                out.append( self.encode_all_ids( trans, page.to_dict(), True) )
+            out.append( self.encode_all_ids( trans, page.to_dict(), True) )
         return out
 
-    @web.expose_api
+    @expose_api
     def create( self, trans, page_id, payload, **kwd ):
         """
         create( self, trans, page_id, payload **kwd )
@@ -46,39 +49,42 @@ class PageRevisionsController( BaseAPIController, SharableItemSecurityMixin, Use
         :rtype:     dictionary
         :returns:   Dictionary with 'success' or 'error' element to indicate the result of the request
         """
-        error_str = ""
+        content = payload.get("content", None)
+        if not content:
+            raise exceptions.ObjectAttributeMissingException("content undefined or empty")
 
-        if not page_id:
-            error_str = "page_id is required"
-        elif not payload.get("content", None):
-            error_str = "content is required"
+        page = self._get_page( trans, page_id )
+        self._verify_page_ownership( trans, page )
+
+        if 'title' in payload:
+            title = payload['title']
         else:
+            title = page.title
 
-            # Create the new stored page
+        content = sanitize_html( content, 'utf-8', 'text/html' )
+
+        page_revision = trans.app.model.PageRevision()
+        page_revision.title = title
+        page_revision.page = page
+        page.latest_revision = page_revision
+        page_revision.content = content
+
+        # Persist
+        session = trans.sa_session
+        session.flush()
+
+        return page_revision.to_dict( view="element" )
+
+    def _get_page( self, trans, page_id ):
+        page = None
+        try:
             page = trans.sa_session.query( trans.app.model.Page ).get( trans.security.decode_id(page_id) )
-            if page is None:
-                return { "error" : "page not found"}
+        except Exception:
+            pass
+        if not page:
+            raise exceptions.ObjectNotFound()
+        return page
 
-            if not self.security_check( trans, page, True, True ):
-                return { "error" : "page not found"}
-
-            if 'title' in payload:
-                title = payload['title']
-            else:
-                title = page.title
-
-            content = payload.get("content", "")
-            content = sanitize_html( content, 'utf-8', 'text/html' )
-
-            page_revision = trans.app.model.PageRevision()
-            page_revision.title = title
-            page_revision.page = page
-            page.latest_revision = page_revision
-            page_revision.content = content
-            # Persist
-            session = trans.sa_session
-            session.flush()
-
-            return { "success" : "revision posted" }
-
-        return { "error" : error_str }
+    def _verify_page_ownership( self, trans, page ):
+        if not self.security_check( trans, page, True, True ):
+            raise exceptions.ItemOwnershipException()

lib/galaxy/webapps/galaxy/api/pages.py

@@ -2,8 +2,9 @@
 API for updating Galaxy Pages
 """
 import logging
-from galaxy import web
+from galaxy.web import _future_expose_api as expose_api
 from galaxy.web.base.controller import SharableItemSecurityMixin, BaseAPIController, SharableMixin
+from galaxy import exceptions
 from galaxy.model.item_attrs import UsesAnnotations
 from galaxy.util.sanitize_html import sanitize_html
 
@@ -12,7 +13,7 @@ log = logging.getLogger( __name__ )
 
 class PagesController( BaseAPIController, SharableItemSecurityMixin, UsesAnnotations, SharableMixin ):
 
-    @web.expose_api
+    @expose_api
     def index( self, trans, deleted=False, **kwd ):
         """
         index( self, trans, deleted=False, **kwd )
@@ -47,7 +48,7 @@ class PagesController( BaseAPIController, SharableItemSecurityMixin, UsesAnnotat
 
         return out
 
-    @web.expose_api
+    @expose_api
     def create( self, trans, payload, **kwd ):
         """
         create( self, trans, payload, **kwd )
@@ -64,45 +65,41 @@ class PagesController( BaseAPIController, SharableItemSecurityMixin, UsesAnnotat
         :returns:   Dictionary return of the Page.to_dict call
         """
         user = trans.get_user()
-        error_str = ""
 
         if not payload.get("title", None):
-            error_str = "Page name is required"
+            raise exceptions.ObjectAttributeMissingException( "Page name is required" )
         elif not payload.get("slug", None):
-            error_str = "Page id is required"
+            raise exceptions.ObjectAttributeMissingException( "Page id is required" )
         elif not self._is_valid_slug( payload["slug"] ):
-            error_str = "Page identifier must consist of only lowercase letters, numbers, and the '-' character"
+            raise exceptions.ObjectAttributeInvalidException( "Page identifier must consist of only lowercase letters, numbers, and the '-' character" )
         elif trans.sa_session.query( trans.app.model.Page ).filter_by( user=user, slug=payload["slug"], deleted=False ).first():
-            error_str = "Page id must be unique"
-        else:
+            raise exceptions.DuplicatedSlugException( "Page slug must be unique" )
 
-            content = payload.get("content", "")
-            content = sanitize_html( content, 'utf-8', 'text/html' )
+        content = payload.get("content", "")
+        content = sanitize_html( content, 'utf-8', 'text/html' )
 
-            # Create the new stored page
-            page = trans.app.model.Page()
-            page.title = payload['title']
-            page.slug = payload['slug']
-            page_annotation = sanitize_html( payload.get( "annotation", "" ), 'utf-8', 'text/html' )
-            self.add_item_annotation( trans.sa_session, trans.get_user(), page, page_annotation )
-            page.user = user
-            # And the first (empty) page revision
-            page_revision = trans.app.model.PageRevision()
-            page_revision.title = payload['title']
-            page_revision.page = page
-            page.latest_revision = page_revision
-            page_revision.content = content
-            # Persist
-            session = trans.sa_session
-            session.add( page )
-            session.flush()
+        # Create the new stored page
+        page = trans.app.model.Page()
+        page.title = payload['title']
+        page.slug = payload['slug']
+        page_annotation = sanitize_html( payload.get( "annotation", "" ), 'utf-8', 'text/html' )
+        self.add_item_annotation( trans.sa_session, trans.get_user(), page, page_annotation )
+        page.user = user
+        # And the first (empty) page revision
+        page_revision = trans.app.model.PageRevision()
+        page_revision.title = payload['title']
+        page_revision.page = page
+        page.latest_revision = page_revision
+        page_revision.content = content
+        # Persist
+        session = trans.sa_session
+        session.add( page )
+        session.flush()
 
-            rval = self.encode_all_ids( trans, page.to_dict(), True )
-            return rval
+        rval = self.encode_all_ids( trans, page.to_dict(), True )
+        return rval
 
-        return { "error" : error_str }
-
-    @web.expose_api
+    @expose_api
     def delete( self, trans, id, **kwd ):
         """
         delete( self, trans, id, **kwd )
@@ -114,22 +111,14 @@ class PagesController( BaseAPIController, SharableItemSecurityMixin, UsesAnnotat
         :rtype:     dict
         :returns:   Dictionary with 'success' or 'error' element to indicate the result of the request
         """
-        page_id = id
-        try:
-            page = trans.sa_session.query(self.app.model.Page).get(trans.security.decode_id(page_id))
-        except Exception, e:
-            return { "error" : "Page with ID='%s' can not be found\n Exception: %s" % (page_id, str( e )) }
+        page = self._get_page( trans, id )
 
-        # check to see if user has permissions to selected workflow
-        if page.user != trans.user and not trans.user_is_admin():
-            return { "error" : "Workflow is not owned by or shared with current user" }
-
-        #Mark a workflow as deleted
+        #Mark a page as deleted
         page.deleted = True
         trans.sa_session.flush()
-        return { "success" : "Deleted", "id" : page_id }
+        return ''  # TODO: Figure out what to return on DELETE, document in guidelines!
 
-    @web.expose_api
+    @expose_api
     def show( self, trans, id, **kwd ):
         """
         show( self, trans, id, **kwd )
@@ -141,8 +130,22 @@ class PagesController( BaseAPIController, SharableItemSecurityMixin, UsesAnnotat
         :rtype:     dict
         :returns:   Dictionary return of the Page.to_dict call with the 'content' field populated by the most recent revision
         """
-        page = trans.sa_session.query( trans.app.model.Page ).get( trans.security.decode_id( id ) )
+        page = self._get_page( trans, id )
         self.security_check( trans, page, check_ownership=False, check_accessible=True)
         rval = self.encode_all_ids( trans, page.to_dict(), True )
         rval['content'] = page.latest_revision.content
         return rval
+
+    def _get_page( self, trans, id ):  # Fetches page object and verifies security.
+        try:
+            page = trans.sa_session.query( trans.app.model.Page ).get( trans.security.decode_id( id ) )
+        except Exception:
+            page = None
+
+        if not page:
+            raise exceptions.ObjectNotFound()
+
+        if page.user != trans.user and not trans.user_is_admin():
+            raise exceptions.ItemOwnershipException()
+
+        return page

test/base/api.py

@@ -60,6 +60,12 @@ class ApiTestCase( TwillTestCase ):
         for key in keys:
             assert key in response, "Response [%s] does not contain key [%s]" % ( response, key )
 
+    def _assert_error_code_is( self, response, error_code ):
+        if hasattr( response, "json" ):
+            response = response.json()
+        self._assert_has_keys( response, "err_code" )
+        self.assertEquals( response[ "err_code" ], int( error_code ) )
+
     def _random_key( self ):  # Used for invalid request testing...
         return "1234567890123456"
 

test/functional/api/test_page_revisions.py

@@ -0,0 +1,35 @@
+from galaxy.exceptions import error_codes
+from functional.api.pages import BasePageApiTestCase
+
+
+class PageRevisionsApiTestCase( BasePageApiTestCase ):
+
+    def test_create( self ):
+        page_json = self._create_valid_page_with_slug( "pr1" )
+        revision_data = dict( content="<p>NewContent!</p>" )
+        page_revision_response = self._post( "pages/%s/revisions" % page_json[ 'id' ], data=revision_data )
+        self._assert_status_code_is( page_revision_response, 200 )
+        page_revision_json = page_revision_response.json()
+        self._assert_has_keys( page_revision_json, 'id', 'content' )
+
+    def test_403_if_create_revision_on_unowned_page( self ):
+        page_json = self._create_valid_page_as( "pr2@bx.psu.edu", "pr2" )
+        revision_data = dict( content="<p>NewContent!</p>" )
+        page_revision_response = self._post( "pages/%s/revisions" % page_json[ 'id' ], data=revision_data )
+        self._assert_status_code_is( page_revision_response, 403 )
+
+    def test_revision_index( self ):
+        page_json = self._create_valid_page_with_slug( "pr3" )
+        revision_data = dict( content="<p>NewContent!</p>" )
+        revisions_url = "pages/%s/revisions" % page_json[ 'id' ]
+        self._post( revisions_url, data=revision_data )
+        revisions_response = self._get( revisions_url )
+        self._assert_status_code_is( revisions_response, 200 )
+        revisions_json = revisions_response.json()
+        assert len( revisions_json ) == 2  # Original revision and new one
+
+    def test_404_if_index_unknown_page( self ):
+        revisions_url = "pages/%s/revisions" % self._random_key()
+        revisions_response = self._get( revisions_url )
+        self._assert_status_code_is( revisions_response, 404 )
+        self._assert_error_code_is( revisions_response, error_codes.USER_OBJECT_NOT_FOUND )

test/functional/api/test_pages.py

@@ -0,0 +1,105 @@
+from galaxy.exceptions import error_codes
+from base import api
+from base.interactor import delete_request
+
+from operator import itemgetter
+
+
+class BasePageApiTestCase( api.ApiTestCase ):
+
+    def _create_valid_page_with_slug( self, slug ):
+        page_request = self._test_page_payload( slug=slug )
+        page_response = self._post( "pages", page_request )
+        self._assert_status_code_is( page_response, 200 )
+        return page_response.json()
+
+    def _create_valid_page_as( self, other_email, slug ):
+        run_as_user = self._setup_user( other_email )
+        page_request = self._test_page_payload( slug=slug )
+        page_request[ "run_as" ] = run_as_user[ "id" ]
+        page_response = self._post( "pages", page_request, admin=True )
+        self._assert_status_code_is( page_response, 200 )
+        return page_response.json()
+
+    def _test_page_payload( self, **kwds ):
+        request = dict(
+            slug="mypage",
+            title="MY PAGE",
+            content="<p>Page!</p>",
+        )
+        request.update( **kwds )
+        return request
+
+
+class PageApiTestCase( BasePageApiTestCase ):
+
+    def test_create( self ):
+        response_json = self._create_valid_page_with_slug( "mypage" )
+        self._assert_has_keys( response_json, "slug", "title", "id" )
+
+    def test_index( self ):
+        create_response_json = self._create_valid_page_with_slug( "indexpage" )
+        assert self._users_index_has_page_with_id( create_response_json[ "id" ] )
+
+    def test_index_doesnt_show_unavailable_pages( self ):
+        create_response_json = self._create_valid_page_as( "others_page_index@bx.psu.edu", "otherspageindex" )
+        assert not self._users_index_has_page_with_id( create_response_json[ "id" ] )
+
+    def test_cannot_create_pages_with_same_slug( self ):
+        page_request = self._test_page_payload( slug="mypage1" )
+        page_response_1 = self._post( "pages", page_request )
+        self._assert_status_code_is( page_response_1, 200 )
+        page_response_2 = self._post( "pages", page_request )
+        self._assert_status_code_is( page_response_2, 400 )
+        self._assert_error_code_is( page_response_2, error_codes.USER_SLUG_DUPLICATE )
+
+    def test_page_requires_name( self ):
+        page_request = self._test_page_payload()
+        del page_request[ 'title' ]
+        page_response = self._post( "pages", page_request )
+        self._assert_status_code_is( page_response, 400 )
+        self._assert_error_code_is( page_response, error_codes.USER_OBJECT_ATTRIBUTE_MISSING )
+
+    def test_page_requires_slug( self ):
+        page_request = self._test_page_payload()
+        del page_request[ 'slug' ]
+        page_response = self._post( "pages", page_request )
+        self._assert_status_code_is( page_response, 400 )
+
+    def test_delete( self ):
+        response_json = self._create_valid_page_with_slug( "testdelete" )
+        delete_response = delete_request( self._api_url( "pages/%s" % response_json[ 'id' ], use_key=True ) )
+        self._assert_status_code_is( delete_response, 200 )
+
+    def test_404_on_delete_unknown_page( self ):
+        delete_response = delete_request( self._api_url( "pages/%s" % self._random_key(), use_key=True ) )
+        self._assert_status_code_is( delete_response, 404 )
+        self._assert_error_code_is( delete_response, error_codes.USER_OBJECT_NOT_FOUND )
+
+    def test_403_on_delete_unowned_page( self ):
+        page_response = self._create_valid_page_as( "others_page@bx.psu.edu", "otherspage" )
+        delete_response = delete_request( self._api_url( "pages/%s" % page_response[ "id" ], use_key=True ) )
+        self._assert_status_code_is( delete_response, 403 )
+        self._assert_error_code_is( delete_response, error_codes.USER_DOES_NOT_OWN_ITEM )
+
+    def test_show( self ):
+        response_json = self._create_valid_page_with_slug( "pagetoshow" )
+        show_response = self._get( "pages/%s" % response_json['id'] )
+        self._assert_status_code_is( show_response, 200 )
+        show_json = show_response.json()
+        self._assert_has_keys( show_json, "slug", "title", "id" )
+        self.assertEquals( show_json["slug"], "pagetoshow" )
+        self.assertEquals( show_json["title"], "MY PAGE" )
+        self.assertEquals( show_json["content"], "<p>Page!</p>" )
+
+    def test_403_on_unowner_show( self ):
+        response_json = self._create_valid_page_as( "others_page_show@bx.psu.edu", "otherspageshow" )
+        show_response = self._get( "pages/%s" % response_json['id'] )
+        self._assert_status_code_is( show_response, 403 )
+        self._assert_error_code_is( show_response, error_codes.USER_DOES_NOT_OWN_ITEM )
+
+    def _users_index_has_page_with_id( self, id ):
+        index_response = self._get( "pages" )
+        self._assert_status_code_is( index_response, 200 )
+        pages = index_response.json()
+        return id in map( itemgetter( "id" ), pages )