From a2931e9f5592c7b9d62d470abbb4a3b7b389eae9 Mon Sep 17 00:00:00 2001
From: John Chilton <jmchilton@gmail.com>
Date: Fri, 10 Jan 2014 15:10:52 -0600
Subject: [PATCH 1/4] Pull extract_payload_from_request out of expose_api.
 Needs to be used in new version of API decorator and resulted in too much
 nesting in there the way it was.

---
 lib/galaxy/web/framework/__init__.py | 58 ++++++++++++++--------------
 1 file changed, 30 insertions(+), 28 deletions(-)

diff --git a/lib/galaxy/web/framework/__init__.py b/lib/galaxy/web/framework/__init__.py
index 4432d743924..ab1b48bc296 100644
--- a/lib/galaxy/web/framework/__init__.py
+++ b/lib/galaxy/web/framework/__init__.py
@@ -103,6 +103,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 +169,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'

From 8a6ed0ae8b9fc41444a182f88d08ee2daeffc6c3 Mon Sep 17 00:00:00 2001
From: John Chilton <jmchilton@gmail.com>
Date: Fri, 10 Jan 2014 15:10:52 -0600
Subject: [PATCH 2/4] Introduce new generation of API decorator... Add improved
 error handling. Introduce error code.

Use new decorator with tested methods in histories API.
---
 lib/galaxy/exceptions/__init__.py          |  47 ++++++-
 lib/galaxy/exceptions/error_codes.py       |  33 +++++
 lib/galaxy/web/__init__.py                 |   4 +
 lib/galaxy/web/framework/__init__.py       | 142 ++++++++++++++++++++-
 lib/galaxy/webapps/galaxy/api/histories.py |   6 +-
 5 files changed, 221 insertions(+), 11 deletions(-)
 create mode 100644 lib/galaxy/exceptions/error_codes.py

diff --git a/lib/galaxy/exceptions/__init__.py b/lib/galaxy/exceptions/__init__.py
index c3bd4bde319..75c9aba18bd 100644
--- a/lib/galaxy/exceptions/__init__.py
+++ b/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 """
diff --git a/lib/galaxy/exceptions/error_codes.py b/lib/galaxy/exceptions/error_codes.py
new file mode 100644
index 00000000000..4fe658e6754
--- /dev/null
+++ b/lib/galaxy/exceptions/error_codes.py
@@ -0,0 +1,33 @@
+# 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 )
+
+# TODO: Guidelines for error message langauge?
+UNKNOWN = ErrorCode(0, UNKNOWN_ERROR_MESSAGE)
+
+USER_CANNOT_RUN_AS = ErrorCode(400001, "User does not have permissions to run jobs as another user.")
+USER_INVALID_RUN_AS = ErrorCode(400002, "Invalid run_as request - run_as user does not exist.")
+USER_INVALID_JSON = ErrorCode(400003, "Your request did not appear to be valid JSON, please consult the API documentation.")
+USER_OBJECT_ATTRIBUTE_INVALID = ErrorCode(400004, "Attempted to create or update object with invalid attribute value.")
+USER_OBJECT_ATTRIBUTE_MISSING = ErrorCode(400005, "Attempted to create object without required attribute.")
+USER_SLUG_DUPLICATE = ErrorCode(400006, "Slug must be unique per user.")
+
+USER_NO_API_KEY = ErrorCode(403001, "API Authentication Required for this request")
+USER_CANNOT_ACCESS_ITEM = ErrorCode(403002, "User cannot access specified item.")
+USER_DOES_NOT_OWN_ITEM = ErrorCode(403003, "User does not own specified item.")
+
+USER_OBJECT_NOT_FOUND = ErrorCode(404001, "No such object not found.")
diff --git a/lib/galaxy/web/__init__.py b/lib/galaxy/web/__init__.py
index 107c5002d23..70b016c53e4 100644
--- a/lib/galaxy/web/__init__.py
+++ b/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
diff --git a/lib/galaxy/web/framework/__init__.py b/lib/galaxy/web/framework/__init__.py
index ab1b48bc296..eecdfb9f83c 100644
--- a/lib/galaxy/web/framework/__init__.py
+++ b/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
@@ -212,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 ):
diff --git a/lib/galaxy/webapps/galaxy/api/histories.py b/lib/galaxy/webapps/galaxy/api/histories.py
index 6f58b702f62..254dab8c693 100644
--- a/lib/galaxy/webapps/galaxy/api/histories.py
+++ b/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 )

From e66768ea742e6132194d1e61c885f634d5753246 Mon Sep 17 00:00:00 2001
From: John Chilton <jmchilton@gmail.com>
Date: Fri, 10 Jan 2014 15:10:52 -0600
Subject: [PATCH 3/4] Add functional tests for pages API. Touch up pages API in
 response to test cases. Update pages API to use newer API decorator.

---
 .../webapps/galaxy/api/page_revisions.py      |  78 +++++++------
 lib/galaxy/webapps/galaxy/api/pages.py        |  95 ++++++++--------
 test/base/api.py                              |   6 +
 test/functional/api/test_page_revisions.py    |  35 ++++++
 test/functional/api/test_pages.py             | 105 ++++++++++++++++++
 5 files changed, 237 insertions(+), 82 deletions(-)
 create mode 100644 test/functional/api/test_page_revisions.py
 create mode 100644 test/functional/api/test_pages.py

diff --git a/lib/galaxy/webapps/galaxy/api/page_revisions.py b/lib/galaxy/webapps/galaxy/api/page_revisions.py
index 25cbbc3f1ad..56e3bded8dd 100644
--- a/lib/galaxy/webapps/galaxy/api/page_revisions.py
+++ b/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()
diff --git a/lib/galaxy/webapps/galaxy/api/pages.py b/lib/galaxy/webapps/galaxy/api/pages.py
index 92e01fcbebb..aa27c6afc09 100644
--- a/lib/galaxy/webapps/galaxy/api/pages.py
+++ b/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
diff --git a/test/base/api.py b/test/base/api.py
index d1d9a1f7583..aea87bfa811 100644
--- a/test/base/api.py
+++ b/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"
 
diff --git a/test/functional/api/test_page_revisions.py b/test/functional/api/test_page_revisions.py
new file mode 100644
index 00000000000..2fa91b0992a
--- /dev/null
+++ b/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 )
diff --git a/test/functional/api/test_pages.py b/test/functional/api/test_pages.py
new file mode 100644
index 00000000000..8b46d8b071d
--- /dev/null
+++ b/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 )

From a71ea3eceba4907295b4af097ffe9c9f68d9996c Mon Sep 17 00:00:00 2001
From: John Chilton <jmchilton@gmail.com>
Date: Fri, 10 Jan 2014 15:10:52 -0600
Subject: [PATCH 4/4] Parse error codes/messages from new JSON file. Same
 python interface to this data, but the data can now be reused by clients in
 other languages (e.g. potentially automating the creation of typed Java
 exceptions).

---
 lib/galaxy/exceptions/error_codes.json | 57 ++++++++++++++++++++++++++
 lib/galaxy/exceptions/error_codes.py   | 27 ++++++------
 2 files changed, 70 insertions(+), 14 deletions(-)
 create mode 100644 lib/galaxy/exceptions/error_codes.json

diff --git a/lib/galaxy/exceptions/error_codes.json b/lib/galaxy/exceptions/error_codes.json
new file mode 100644
index 00000000000..f896d87d261
--- /dev/null
+++ b/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."
+   }
+]
diff --git a/lib/galaxy/exceptions/error_codes.py b/lib/galaxy/exceptions/error_codes.py
index 4fe658e6754..4afe67bea80 100644
--- a/lib/galaxy/exceptions/error_codes.py
+++ b/lib/galaxy/exceptions/error_codes.py
@@ -1,3 +1,6 @@
+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.
@@ -16,18 +19,14 @@ class ErrorCode( object ):
     def __int__( self ):
         return int( self.code )
 
-# TODO: Guidelines for error message langauge?
-UNKNOWN = ErrorCode(0, UNKNOWN_ERROR_MESSAGE)
+    @staticmethod
+    def from_dict( entry ):
+        name = entry.get("name")
+        code = entry.get("code")
+        message = entry.get("message")
+        return ( name, ErrorCode( code, message ) )
 
-USER_CANNOT_RUN_AS = ErrorCode(400001, "User does not have permissions to run jobs as another user.")
-USER_INVALID_RUN_AS = ErrorCode(400002, "Invalid run_as request - run_as user does not exist.")
-USER_INVALID_JSON = ErrorCode(400003, "Your request did not appear to be valid JSON, please consult the API documentation.")
-USER_OBJECT_ATTRIBUTE_INVALID = ErrorCode(400004, "Attempted to create or update object with invalid attribute value.")
-USER_OBJECT_ATTRIBUTE_MISSING = ErrorCode(400005, "Attempted to create object without required attribute.")
-USER_SLUG_DUPLICATE = ErrorCode(400006, "Slug must be unique per user.")
-
-USER_NO_API_KEY = ErrorCode(403001, "API Authentication Required for this request")
-USER_CANNOT_ACCESS_ITEM = ErrorCode(403002, "User cannot access specified item.")
-USER_DOES_NOT_OWN_ITEM = ErrorCode(403003, "User does not own specified item.")
-
-USER_OBJECT_NOT_FOUND = ErrorCode(404001, "No such object not found.")
+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