Merge pull request #274 from jfinkhaeuser/fix-missing-operation-id

Fix operation ID handling

README.rst

@@ -110,7 +110,7 @@ path of your application (e.g ``swagger/``). Then run:
 See the `Connexion Pet Store Example Application`_ for a sample
 specification.
 
-Now you’re able run and use Connexion!
+Now you're able run and use Connexion!
 
 
 OAuth 2 Authentication and Authorization
@@ -171,7 +171,7 @@ Automatic Routing
 -----------------
 
 To customize this behavior, Connexion can use alternative
-``Resolvers``—for example, ``RestyResolver``. The ``RestyResolver``
+``Resolvers``--for example, ``RestyResolver``. The ``RestyResolver``
 will compose an ``operationId`` based on the path and HTTP method of
 the endpoints in your specification:
 
@@ -349,7 +349,7 @@ One way, `described by Flask`_, looks like this:
            debug=False/True, ssl_context=context)
 
 However, Connexion doesn't provide an ssl_context parameter. This is
-because Flask doesn't, either—but it uses `**kwargs` to send the
+because Flask doesn't, either--but it uses `**kwargs` to send the
 parameters to the underlying [werkzeug](http://werkzeug.pocoo.org/) server.
 
 The Swagger UI Console

connexion/api.py

@@ -24,6 +24,7 @@
 from swagger_spec_validator.validator20 import validate_spec
 
 from . import resolver, utils
+from .exceptions import ResolverError
 from .handlers import AuthErrorHandler
 from .operation import Operation
 
@@ -201,23 +202,30 @@ def add_paths(self, paths=None):
             # http://swagger.io/specification/#pathItemObject
             path_parameters = methods.get('parameters', [])
 
-            # TODO Error handling
             for method, endpoint in methods.items():
                 if method == 'parameters':
                     continue
                 try:
                     self.add_operation(method, path, endpoint, path_parameters)
-                except Exception:  # pylint: disable= W0703
-                    url = '{base_url}{path}'.format(base_url=self.base_url,
-                                                    path=path)
-                    error_msg = 'Failed to add operation for {method} {url}'.format(
-                        method=method.upper(),
-                        url=url)
-                    if self.debug:
-                        logger.exception(error_msg)
-                    else:
-                        logger.error(error_msg)
-                        six.reraise(*sys.exc_info())
+                except ResolverError as err:
+                    exc_info = err.exc_info
+                    if exc_info is None:
+                        exc_info = sys.exc_info()
+                    self._handle_add_operation_error(path, method, exc_info)
+                except Exception:
+                    # All other relevant exceptions should be handled as well.
+                    self._handle_add_operation_error(path, method, sys.exc_info())
+
+    def _handle_add_operation_error(self, path, method, exc_info):
+        url = '{base_url}{path}'.format(base_url=self.base_url, path=path)
+        error_msg = 'Failed to add operation for {method} {url}'.format(
+            method=method.upper(),
+            url=url)
+        if self.debug:
+            logger.exception(error_msg)
+        else:
+            logger.error(error_msg)
+            six.reraise(*exc_info)
 
     def add_auth_on_not_found(self):
         """

connexion/exceptions.py

@@ -16,6 +16,25 @@ class ConnexionException(Exception):
     pass
 
 
+class ResolverError(LookupError):
+    def __init__(self, reason='Unknown reason', exc_info=None):
+        """
+        :param reason: Reason why the resolver failed.
+        :type reason: str
+        :param exc_info: If specified, gives details of the original exception
+            as returned by sys.exc_info()
+        :type exc_info: tuple | None
+        """
+        self.reason = reason
+        self.exc_info = exc_info
+
+    def __str__(self):  # pragma: no cover
+        return '<ResolverError: {}>'.format(self.reason)
+
+    def __repr__(self):  # pragma: no cover
+        return '<ResolverError: {}>'.format(self.reason)
+
+
 class InvalidSpecification(ConnexionException):
     def __init__(self, reason='Unknown Reason'):
         """

connexion/resolver.py

@@ -15,6 +15,7 @@
 import re
 
 import connexion.utils as utils
+from connexion.exceptions import ResolverError
 
 logger = logging.getLogger('connexion.resolver')
 
@@ -61,15 +62,22 @@ def resolve_operation_id(self, operation):
         x_router_controller = spec.get('x-swagger-router-controller')
         if x_router_controller is None:
             return operation_id
-        return x_router_controller + '.' + operation_id
+        return '{}.{}'.format(x_router_controller, operation_id)
 
     def resolve_function_from_operation_id(self, operation_id):
         """
         Invokes the function_resolver
 
         :type operation_id: str
         """
-        return self.function_resolver(operation_id)
+        msg = 'Cannot resolve operationId "{}"!'.format(operation_id)
+        try:
+            return self.function_resolver(operation_id)
+        except ImportError:
+            import sys
+            raise ResolverError(msg, sys.exc_info())
+        except AttributeError:
+            raise ResolverError(msg)
 
 
 class RestyResolver(Resolver):
@@ -132,4 +140,4 @@ def get_function_name():
 
             return self.collection_endpoint_name if is_collection_endpoint else method.lower()
 
-        return get_controller_name() + '.' + get_function_name()
+        return '{}.{}'.format(get_controller_name(), get_function_name())

connexion/utils.py

@@ -120,8 +120,11 @@ def get_function_from_name(function_name):
             module = importlib.import_module(module_name)
         except ImportError as import_error:
             last_import_error = import_error
-            module_name, attr_path1 = module_name.rsplit('.', 1)
-            attr_path = '{0}.{1}'.format(attr_path1, attr_path)
+            if '.' in module_name:
+                module_name, attr_path1 = module_name.rsplit('.', 1)
+                attr_path = '{0}.{1}'.format(attr_path1, attr_path)
+            else:
+                raise
     try:
         function = deep_getattr(module, attr_path)
     except AttributeError:

tests/api/test_parameters.py

@@ -287,5 +287,3 @@ def test_args_kwargs(simple_app):
     resp = app_client.get('/v1.0/query-params-as-kwargs?foo=a&bar=b')
     assert resp.status_code == 200
     assert json.loads(resp.data.decode()) == {'foo': 'a'}
-
-

tests/fakeapi/hello.py

@@ -1,7 +1,5 @@
 #!/usr/bin/env python3
 
-import json
-
 from connexion import NoContent, problem, request
 from flask import redirect
 

tests/fakeapi/module_with_error.py

@@ -1,4 +1,6 @@
 # This is a test file, please do not delete.
 # It is used by the test:
 #  - `test_operation.py:test_invalid_operation_does_stop_application_to_setup`
+#  - `test_api.py:test_invalid_operation_does_stop_application_to_setup`
+#  - `test_api.py:test_invalid_operation_does_not_stop_application_in_debug_mode`
 from foo.bar import foobar  # noqa

tests/fakeapi/module_with_exception.py

@@ -0,0 +1,6 @@
+# This is a test file, please do not delete.
+# It is used by the test:
+#  - `test_operation.py:test_invalid_operation_does_stop_application_to_setup`
+#  - `test_api.py:test_invalid_operation_does_stop_application_to_setup`
+#  - `test_api.py:test_invalid_operation_does_not_stop_application_in_debug_mode`
+raise ValueError('Forced exception!')

tests/fixtures/missing_op_id/swagger.yaml

@@ -0,0 +1,17 @@
+swagger: "2.0"
+
+info:
+  title: "{{title}}"
+  version: "1.0"
+
+basePath: /v1.0
+
+paths:
+  /welcome:
+    put:
+      # operationId: XXX completely missing
+      responses:
+        200:
+          description: greeting response
+          schema:
+            type: object

tests/fixtures/module_not_implemented/swagger.yaml

@@ -0,0 +1,17 @@
+swagger: "2.0"
+
+info:
+  title: "{{title}}"
+  version: "1.0"
+
+basePath: /v1.0
+
+paths:
+  /welcome:
+    get:
+      operationId: no.module.or.function
+      responses:
+        200:
+          description: greeting response
+          schema:
+            type: object

tests/fixtures/user_module_loading_error/swagger.yaml

@@ -0,0 +1,17 @@
+swagger: "2.0"
+
+info:
+  title: "{{title}}"
+  version: "1.0"
+
+basePath: /v1.1
+
+paths:
+  /welcome:
+    get:
+      operationId: fakeapi.module_with_exception.something
+      responses:
+        200:
+          description: greeting response
+          schema:
+            type: object

tests/test_api.py

@@ -4,6 +4,7 @@
 import tempfile
 
 from connexion.api import Api
+from connexion.exceptions import ResolverError
 from swagger_spec_validator.common import SwaggerValidationError
 from yaml import YAMLError
 
@@ -33,12 +34,36 @@ def test_template():
 
 def test_invalid_operation_does_stop_application_to_setup():
     with pytest.raises(ImportError):
-        Api(TEST_FOLDER / "fakeapi/op_error_api.yaml", "/api/v1.0",
+        Api(TEST_FOLDER / "fixtures/op_error_api/swagger.yaml", "/api/v1.0",
+            {'title': 'OK'})
+
+    with pytest.raises(ResolverError):
+        Api(TEST_FOLDER / "fixtures/missing_op_id/swagger.yaml", "/api/v1.0",
+            {'title': 'OK'})
+
+    with pytest.raises(ImportError):
+        Api(TEST_FOLDER / "fixtures/module_not_implemented/swagger.yaml", "/api/v1.0",
+            {'title': 'OK'})
+
+    with pytest.raises(ValueError):
+        Api(TEST_FOLDER / "fixtures/user_module_loading_error/swagger.yaml", "/api/v1.0",
             {'title': 'OK'})
 
 
 def test_invalid_operation_does_not_stop_application_in_debug_mode():
-    api = Api(TEST_FOLDER / "fakeapi/op_error_api.yaml", "/api/v1.0",
+    api = Api(TEST_FOLDER / "fixtures/op_error_api/swagger.yaml", "/api/v1.0",
+              {'title': 'OK'}, debug=True)
+    assert api.specification['info']['title'] == 'OK'
+
+    api = Api(TEST_FOLDER / "fixtures/missing_op_id/swagger.yaml", "/api/v1.0",
+              {'title': 'OK'}, debug=True)
+    assert api.specification['info']['title'] == 'OK'
+
+    api = Api(TEST_FOLDER / "fixtures/user_module_loading_error/swagger.yaml", "/api/v1.0",
+              {'title': 'OK'}, debug=True)
+    assert api.specification['info']['title'] == 'OK'
+
+    api = Api(TEST_FOLDER / "fixtures/module_not_implemented/swagger.yaml", "/api/v1.0",
               {'title': 'OK'}, debug=True)
     assert api.specification['info']['title'] == 'OK'
 

tests/test_resolver.py

@@ -1,7 +1,10 @@
 import connexion.app
+from connexion.exceptions import ResolverError
 from connexion.operation import Operation
 from connexion.resolver import Resolver, RestyResolver
 
+import pytest
+
 PARAMETER_DEFINITIONS = {'myparam': {'in': 'path', 'type': 'integer'}}
 
 
@@ -15,6 +18,24 @@ def test_resty_get_function():
     assert function == connexion.app.App.common_error_handler
 
 
+def test_missing_operation_id():
+    # Missing operationIDs should result in a well-defined error that can
+    # be handled upstream.
+    with pytest.raises(ResolverError):
+        Resolver().resolve_function_from_operation_id(None)
+    with pytest.raises(ResolverError):
+        RestyResolver('connexion').resolve_function_from_operation_id(None)
+
+
+def test_bad_operation_id():
+    # Unresolvable operationIDs should result in a well-defined error that can
+    # be handled upstream.
+    with pytest.raises(ResolverError):
+        Resolver().resolve_function_from_operation_id('ohai.I.do.not.exist')
+    with pytest.raises(ResolverError):
+        RestyResolver('connexion').resolve_function_from_operation_id('ohai.I.do.not.exist')
+
+
 def test_standard_resolve_x_router_controller():
     operation = Operation(method='GET',
                           path='endpoint',