Repository: runwayml/model-sdk

Issue:
POSTing JSON without specifying content-type header throws 500
```bash
python model.py
Starting model server at http://0.0.0.0:8000...
```

```bash
curl -H "content-type: application/json" -d '{"some_option": 1}' http://0.0.0.0:8000 # 200 OK
curl -d '{"some_option": 1}' http://0.0.0.0:8000 # 500 server error
```

POSTing JSON without explicitly setting a `Content-Type: application/json` causes the request to bomb with the following stack trace.

```
127.0.0.1 - - [2019-04-08 14:27:14] "POST /setup HTTP/1.1" 200 165 0.001232
[2019-04-08 14:27:25,617] ERROR in app: Exception on /setup [POST]
Traceback (most recent call last):
  File "/Users/brannon/Documents/code/runway/rw_python_sdk/venv/lib/python3.7/site-packages/Flask-1.0.2-py3.7.egg/flask/app.py", line 2292, in wsgi_app
    response = self.full_dispatch_request()
  File "/Users/brannon/Documents/code/runway/rw_python_sdk/venv/lib/python3.7/site-packages/Flask-1.0.2-py3.7.egg/flask/app.py", line 1815, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/Users/brannon/Documents/code/runway/rw_python_sdk/venv/lib/python3.7/site-packages/Flask_Cors-3.0.7-py3.7.egg/flask_cors/extension.py", line 161, in wrapped_function
    return cors_after_request(app.make_response(f(*args, **kwargs)))
  File "/Users/brannon/Documents/code/runway/rw_python_sdk/venv/lib/python3.7/site-packages/Flask-1.0.2-py3.7.egg/flask/app.py", line 1718, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/Users/brannon/Documents/code/runway/rw_python_sdk/venv/lib/python3.7/site-packages/Flask-1.0.2-py3.7.egg/flask/_compat.py", line 35, in reraise
    raise value
  File "/Users/brannon/Documents/code/runway/rw_python_sdk/venv/lib/python3.7/site-packages/Flask-1.0.2-py3.7.egg/flask/app.py", line 1813, in full_dispatch_request
    rv = self.dispatch_request()
  File "/Users/brannon/Documents/code/runway/rw_python_sdk/venv/lib/python3.7/site-packages/Flask-1.0.2-py3.7.egg/flask/app.py", line 1799, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/Users/brannon/Documents/code/runway/rw_python_sdk/venv/lib/python3.7/site-packages/runway_python-0.0.55-py3.7.egg/runway/model.py", line 44, in setup_route
    self.setup_model(opts)
  File "/Users/brannon/Documents/code/runway/rw_python_sdk/venv/lib/python3.7/site-packages/runway_python-0.0.55-py3.7.egg/runway/model.py", line 152, in setup_model
    if name in opts:
TypeError: argument of type 'NoneType' is not iterable
```

We should either give a 400 bad request error that describes the request must include this specific  content-type header, or we could sniff the payload and if it parses as JSON let it by.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/runwayml/model-sdk/pull/45?src=pr&el=h1) Report
> Merging [#45](https://codecov.io/gh/runwayml/model-sdk/pull/45?src=pr&el=desc) into [master](https://codecov.io/gh/runwayml/model-sdk/commit/97340ced834a040547e4fb72430224ef9bcf34f9?src=pr&el=desc) will **increase** coverage by `2.1%`.
> The diff coverage is `89.28%`.


```diff
@@            Coverage Diff            @@
##           master      #45     +/-   ##
=========================================
+ Coverage   91.98%   94.09%   +2.1%     
=========================================
  Files           5        5             
  Lines         424      440     +16     
=========================================
+ Hits          390      414     +24     
+ Misses         34       26      -8
```


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c3a0dae..443069c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,9 @@
 
 ## v0.0.70
 
+- Model server now wraps common server error codes in JSON responses (e.g. 401, 403, 404, 405, 500).
+- Model server now sniffs the body of `POST` for JSON even if `content-type: application/json` is not sent in the request header. [#1](https://github.com/runwayml/model-sdk/issues/1)
+- Model server now returns `content-type: application/json`. [#6](https://github.com/runwayml/model-sdk/issues/6)
 - Add `RW_NO_SERVE` environment variable and `no_serve` keyword argument to `runway.run()`. These settings prevent `runway.run()` from starting the Flask server so that mock HTTP requests can be made via `app.test_client()`. See [Testing Flask Applications](http://flask.pocoo.org/docs/1.0/testing/) for more details.
 - Add model tests in [`tests/test_model.py`](tests/test_model.py)
 - Minor change to `docs/` so that JavaScript HTTP -> HTTPS redirect only occurs when the protocol is actually `http:`.
diff --git a/runway/model.py b/runway/model.py
index 9eba787..ffe9d6e 100644
--- a/runway/model.py
+++ b/runway/model.py
@@ -5,13 +5,14 @@ import datetime
 import traceback
 import json
 from six import reraise
-from flask import Flask, request
+from flask import Flask, request, jsonify
 from flask_cors import CORS
 from gevent.pywsgi import WSGIServer
 from .exceptions import RunwayError, MissingInputError, MissingOptionError, \
     InferenceError, UnknownCommandError, SetupError
 from .data_types import *
-from .utils import gzipped, serialize_command, cast_to_obj
+from .utils import gzipped, serialize_command, cast_to_obj, \
+    validate_post_request_body_is_json, get_json_or_none_if_invalid
 
 class RunwayModel(object):
     """A Runway Model server. A singleton instance of this class is created automatically
@@ -27,12 +28,44 @@ class RunwayModel(object):
         self.running_status = 'STARTING'
         self.app = Flask(__name__)
         CORS(self.app)
+        self.define_error_handlers()
         self.define_routes()
 
+    def define_error_handlers(self):
+
+        # not yet implemented, but if and when it is lets make sure its returned
+        # as JSON
+        @self.app.errorhandler(401)
+        def unauthorized(e):
+            msg = 'Unauthorized (well... '
+            msg += 'really unauthenticated but hey I didn\'t write the spec).'
+            return jsonify(dict(error=msg)), 401
+
+        # not yet implemented, but if and when it is lets make sure its returned
+        # as JSON
+        @self.app.errorhandler(403)
+        def forbidden(e):
+            return jsonify(dict(error='Forbidden.')), 403
+
+        @self.app.errorhandler(404)
+        def page_not_found(e):
+            return jsonify(dict(error='Not found.')), 404
+
+        @self.app.errorhandler(405)
+        def method_not_allowed(e):
+            return jsonify(dict(error='Method not allowed.')), 405
+
+        # we shouldn't have any of these as we are wrapping errors in
+        # RunwayError objects and returning stacktraces, but it can't hurt
+        # to be safe.
+        @self.app.errorhandler(500)
+        def internal_server_error(e):
+            return jsonify(dict(error='Internal server error.')), 500
+
     def define_routes(self):
         @self.app.route('/')
         def manifest():
-            return json.dumps(dict(
+            return jsonify(dict(
                 options=[opt.to_dict() for opt in self.options],
                 commands=[serialize_command(cmd) for cmd in self.commands.values()]
             ))
@@ -42,20 +75,22 @@ class RunwayModel(object):
             return self.running_status
 
         @self.app.route('/setup', methods=['POST'])
+        @validate_post_request_body_is_json
         def setup_route():
-            opts = request.json
+            opts = get_json_or_none_if_invalid(request)
             try:
                 self.setup_model(opts)
-                return json.dumps(dict(success=True))
+                return jsonify(dict(success=True))
             except RunwayError as err:
                 err.print_exception()
-                return json.dumps(err.to_response()), err.code
+                return jsonify(err.to_response()), err.code
 
         @self.app.route('/setup', methods=['GET'])
         def setup_options_route():
-            return json.dumps(self.options)
+            return jsonify(self.options)
 
         @self.app.route('/<command_name>', methods=['POST'])
+        @validate_post_request_body_is_json
         def command_route(command_name):
             try:
                 try:
@@ -64,7 +99,7 @@ class RunwayModel(object):
                     raise UnknownCommandError(command_name)
                 inputs = self.commands[command_name]['inputs']
                 outputs = self.commands[command_name]['outputs']
-                input_dict = request.json
+                input_dict = get_json_or_none_if_invalid(request)
                 deserialized_inputs = {}
                 for inp in inputs:
                     name = inp.name
@@ -86,11 +121,11 @@ class RunwayModel(object):
                 for out in outputs:
                     name = out.to_dict()['name']
                     serialized_outputs[name] = out.serialize(results[name])
-                return json.dumps(serialized_outputs).encode('utf8')
+                return jsonify(serialized_outputs)
 
             except RunwayError as err:
                 err.print_exception()
-                return json.dumps(err.to_response()), err.code
+                return jsonify(err.to_response()), err.code
 
         @self.app.route('/<command_name>', methods=['GET'])
         def usage_route(command_name):
@@ -99,10 +134,10 @@ class RunwayModel(object):
                     command = self.commands[command_name]
                 except KeyError:
                     raise UnknownCommandError(command_name)
-                return json.dumps(serialize_command(command))
+                return jsonify(serialize_command(command))
             except RunwayError as err:
                 err.print_exception()
-                return json.dumps(err.to_response())
+                return jsonify(err.to_response()), err.code
 
     def setup(self, decorated_fn=None, options=None):
         """This decorator is used to wrap your own ``setup()`` (or equivalent)
diff --git a/runway/utils.py b/runway/utils.py
index 486f197..d088e2e 100644
--- a/runway/utils.py
+++ b/runway/utils.py
@@ -12,7 +12,7 @@ if sys.version_info[0] < 3:
 else:
     from io import BytesIO as IO
 import numpy as np
-from flask import after_this_request, request
+from flask import after_this_request, request, jsonify
 
 
 URL_REGEX = re.compile(
@@ -23,6 +23,19 @@ URL_REGEX = re.compile(
         r'(?::\d+)?' # optional port
         r'(?:/?|[/?]\S+)$', re.IGNORECASE)
 
+def validate_post_request_body_is_json(f):
+    @functools.wraps(f)
+    def wrapped(*args, **kwargs):
+        json = get_json_or_none_if_invalid(request)
+        if json is not None:
+            return f(*args, **kwargs)
+        else:
+            err_msg = 'The body of all POST requests must contain JSON'
+            return jsonify(dict(error=err_msg)), 400
+    return wrapped
+
+def get_json_or_none_if_invalid(request):
+    return request.get_json(force=True, silent=True)
 
 def serialize_command(cmd):
     ret = {}


Test patch:
diff --git a/tests/test_model.py b/tests/test_model.py
index b3b2a60..e440f1a 100644
--- a/tests/test_model.py
+++ b/tests/test_model.py
@@ -12,6 +12,7 @@ from runway.data_types import category, text, number, array, image, vector, file
 from runway.exceptions import *
 from utils import get_test_client
 from deepdiff import DeepDiff
+from flask import abort
 
 os.environ['RW_NO_SERVE'] = '1'
 
@@ -68,11 +69,15 @@ def test_model_setup_and_command():
 
     # check the manifest via a GET /
     response = client.get('/')
+    assert response.is_json
+
     manifest = json.loads(response.data)
     assert manifest == expected_manifest
 
     # check the input/output manifest for GET /test_command
     response = client.get('/test_command')
+    assert response.is_json
+
     command_manifest = json.loads(response.data)
     assert command_manifest == expected_manifest['commands'][0]
 
@@ -80,6 +85,7 @@ def test_model_setup_and_command():
         'input': 'test input'
     }
     response = client.post('/test_command', json=post_data)
+    assert response.is_json
     assert json.loads(response.data) == { 'output' : 100 }
 
     assert closure['command_ran'] == True
@@ -263,6 +269,213 @@ def test_meta(capsys):
 
     os.environ['RW_META'] = '0'
 
+def test_post_setup_json_no_mime_type():
+
+    rw = RunwayModel()
+
+    @rw.setup(options={'input': text})
+    def setup(opts):
+        pass
+
+    rw.run(debug=True)
+
+    client = get_test_client(rw)
+    response = client.post('/setup', data='{"input": "test input"}')
+    assert response.is_json
+    assert json.loads(response.data) == { 'success': True }
+
+def test_post_setup_invalid_json_no_mime_type():
+
+    rw = RunwayModel()
+
+    @rw.setup(options={'input': text})
+    def setup(opts):
+        pass
+
+    rw.run(debug=True)
+
+    client = get_test_client(rw)
+    response = client.post('/setup', data='{"input": test input"}')
+
+    assert response.is_json
+    assert response.status_code == 400
+
+    expect = { 'error': 'The body of all POST requests must contain JSON' }
+    assert json.loads(response.data) == expect
+
+
+def test_post_setup_json_mime_type():
+
+    rw = RunwayModel()
+
+    @rw.setup(options={'input': text})
+    def setup(opts):
+        pass
+
+    rw.run(debug=True)
+
+    client = get_test_client(rw)
+    response = client.post('/setup', json={ 'input': 'test input' })
+    assert response.is_json
+    assert json.loads(response.data) == { 'success': True }
+
+def test_post_setup_form_encoding():
+
+    rw = RunwayModel()
+
+    @rw.setup(options={'input': text})
+    def setup(opts):
+        pass
+
+    rw.run(debug=True)
+
+    client = get_test_client(rw)
+
+    content_type='application/x-www-form-urlencoded'
+    response = client.post('/setup', data='input=test', content_type=content_type)
+
+    assert response.is_json
+    assert response.status_code == 400
+
+    expect = { 'error': 'The body of all POST requests must contain JSON' }
+    assert json.loads(response.data) == expect
+
+def test_post_command_json_no_mime_type():
+
+    rw = RunwayModel()
+
+    @rw.command('times_two', inputs={ 'input': number }, outputs={ 'output': number })
+    def times_two(model, args):
+        return args['input'] * 2
+
+    rw.run(debug=True)
+
+    client = get_test_client(rw)
+    response = client.post('/times_two', data='{ "input": 5 }')
+    assert response.is_json
+    assert json.loads(response.data) == { 'output': 10 }
+
+def test_post_command_json_mime_type():
+
+    rw = RunwayModel()
+
+    @rw.command('times_two', inputs={ 'input': number }, outputs={ 'output': number })
+    def times_two(model, args):
+        return args['input'] * 2
+
+    rw.run(debug=True)
+
+    client = get_test_client(rw)
+    response = client.post('/times_two', json={ 'input': 5 })
+    assert response.is_json
+    assert json.loads(response.data) == { 'output': 10 }
+
+def test_post_command_form_encoding():
+
+    rw = RunwayModel()
+
+    @rw.command('times_two', inputs={ 'input': number }, outputs={ 'output': number })
+    def times_two(model, args):
+        return args['input'] * 2
+
+    rw.run(debug=True)
+
+    client = get_test_client(rw)
+
+    content_type='application/x-www-form-urlencoded'
+    response = client.post('/times_two', data='input=5', content_type=content_type)
+    assert response.is_json
+    assert response.status_code == 400
+
+    expect = { 'error': 'The body of all POST requests must contain JSON' }
+    assert json.loads(response.data) == expect
+
+def test_405_method_not_allowed():
+
+    rw = RunwayModel()
+
+    @rw.setup(options={'input': text})
+    def setup(opts):
+        pass
+
+    rw.run(debug=True)
+
+    client = get_test_client(rw)
+    response = client.put('/setup', json= { 'input': 'test input'})
+
+    assert response.is_json
+    assert response.status_code == 405
+    assert response.json == { 'error': 'Method not allowed.' }
+
+def test_404_not_found():
+
+    rw = RunwayModel()
+
+    rw.run(debug=True)
+
+    client = get_test_client(rw)
+    response = client.get('/asfd')
+
+    assert response.is_json
+    assert response.status_code == 404
+
+def test_401_unauthorized():
+
+    rw = RunwayModel()
+
+    @rw.app.route('/test/unauthorized')
+    def unauthorized():
+        abort(401)
+
+    rw.run(debug=True)
+
+    client = get_test_client(rw)
+    response = client.get('/test/unauthorized')
+
+    assert response.is_json
+    assert response.status_code == 401
+
+    expect = { 'error': 'Unauthorized (well... really unauthenticated but hey I didn\'t write the spec).' }
+    assert response.json == expect
+
+def test_403_forbidden():
+
+    rw = RunwayModel()
+
+    @rw.app.route('/test/forbidden')
+    def unauthorized():
+        abort(403)
+
+    rw.run(debug=True)
+
+    client = get_test_client(rw)
+    response = client.get('/test/forbidden')
+
+    assert response.is_json
+    assert response.status_code == 403
+
+    expect = { 'error': 'Forbidden.' }
+    assert response.json == expect
+
+def test_500_internal_server_error():
+
+    rw = RunwayModel()
+
+    @rw.app.route('/test/internal_server_error')
+    def unauthorized():
+        abort(500)
+
+    rw.run(debug=True)
+
+    client = get_test_client(rw)
+    response = client.get('/test/internal_server_error')
+
+    assert response.is_json
+    assert response.status_code == 500
+
+    expect = { 'error': 'Internal server error.' }
+    assert response.json == expect
+
 def test_setup_error_setup_no_args():
 
     rw = RunwayModel()
@@ -300,4 +513,5 @@ def test_inference_error():
     rw.run(debug=True)
 
     response = client.post('test_command', json={ 'input': 5 })
+    assert response.is_json
     assert 'InferenceError' in str(response.data)