Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad operationId to error status code #278

Merged
merged 29 commits into from
Sep 13, 2016
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a0071d6
- Add test cases for bad and missing operationIds
jfinkhaeuser Sep 7, 2016
ad0a908
Add test cases for missing and bad operationIds - slightly different …
jfinkhaeuser Sep 7, 2016
1648dd5
YAML file for the previous test case
jfinkhaeuser Sep 7, 2016
1914e56
ImportError should also be translated into ResolverError
jfinkhaeuser Sep 7, 2016
875efbf
Obviously, ImportError no longer falls through. That should have been
jfinkhaeuser Sep 7, 2016
3cd04f8
Add test cases for the resolver_error flag. That's what it should do,
jfinkhaeuser Sep 7, 2016
d27264d
Add ability to add random string to endpoint names so that flask doesn't
jfinkhaeuser Sep 7, 2016
7864a9d
Mhh, that change should not have been there.
jfinkhaeuser Sep 7, 2016
2cae3b1
Fix flake8 complaints
jfinkhaeuser Sep 7, 2016
44d4140
Add ability for Operation to randomize endpoint name (off by default)
jfinkhaeuser Sep 7, 2016
92f51ea
Improve Api test cases
jfinkhaeuser Sep 7, 2016
8c55654
Move ResolverError over to exceptions where it seems to be long.
jfinkhaeuser Sep 7, 2016
4d706d5
Give the same string representation as other exception classes
jfinkhaeuser Sep 7, 2016
1e786d9
If a ResolverError is raised during the addition of operations, and a
jfinkhaeuser Sep 7, 2016
eacfe47
Instead of making resolver_error_handler a magic dict, make it a
jfinkhaeuser Sep 7, 2016
34ee0d1
Add ResolverErrorHandler, analogous to AuthErrorHandler
jfinkhaeuser Sep 7, 2016
4203f4c
When adding an API, allow specifying an error code for turning
jfinkhaeuser Sep 7, 2016
f50da76
Add YAML for bad specs test
jfinkhaeuser Sep 7, 2016
9196b38
Report more about the ResolverError details.
jfinkhaeuser Sep 7, 2016
37154d3
Makes sure #250 is still working. This explicitly handles ImportError
jfinkhaeuser Sep 7, 2016
70c5bb9
Merge branch 'reuse-endpoint-names' into bad-operation-id-to-5xx
jfinkhaeuser Sep 9, 2016
328aa51
Fix isort errors. Some of these look like they're not from my branch to
jfinkhaeuser Sep 12, 2016
9d4a40e
Merge branch 'fix-missing-operation-id' into bad-operation-id-to-5xx
jfinkhaeuser Sep 13, 2016
0bc04c5
Merge branch 'master' into bad-operation-id-to-5xx
jfinkhaeuser Sep 13, 2016
ecd7d3a
Fix tests after merge
jfinkhaeuser Sep 13, 2016
6476c03
Fix isort errors
jfinkhaeuser Sep 13, 2016
7b7b11d
This file shouldn't have been added.
jfinkhaeuser Sep 13, 2016
74cec24
Fixes after running & fixing `TOXENV=isort-check tox`
jfinkhaeuser Sep 13, 2016
fa328f7
Move bad_specs fixtures to its own fixtures folder
jfinkhaeuser Sep 13, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 39 additions & 5 deletions connexion/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
SWAGGER_UI_PATH = MODULE_PATH / 'vendor' / 'swagger-ui'
SWAGGER_UI_URL = 'ui'

RESOLVER_ERROR_ENDPOINT_RANDOM_DIGITS = 6

logger = logging.getLogger('connexion.api')


Expand Down Expand Up @@ -64,7 +66,7 @@ class Api(object):
def __init__(self, swagger_yaml_path, base_url=None, arguments=None,
swagger_json=None, swagger_ui=None, swagger_path=None, swagger_url=None,
validate_responses=False, strict_validation=False, resolver=resolver.Resolver(),
auth_all_paths=False, debug=False):
auth_all_paths=False, debug=False, resolver_error_handler=None):
"""
:type swagger_yaml_path: pathlib.Path
:type base_url: str | None
Expand All @@ -78,8 +80,12 @@ def __init__(self, swagger_yaml_path, base_url=None, arguments=None,
:type auth_all_paths: bool
:type debug: bool
:param resolver: Callable that maps operationID to a function
:param resolver_error_handler: If given, a callable that generates an
Operation used for handling ResolveErrors
:type resolver_error_handler: callable | None
"""
self.debug = debug
self.resolver_error_handler = resolver_error_handler
self.swagger_yaml_path = pathlib.Path(swagger_yaml_path)
logger.debug('Loading specification: %s', swagger_yaml_path,
extra={'swagger_yaml': swagger_yaml_path,
Expand Down Expand Up @@ -181,6 +187,28 @@ def add_operation(self, method, path, swagger_operation, path_parameters):
validate_responses=self.validate_responses,
strict_validation=self.strict_validation,
resolver=self.resolver)
self._add_operation_internal(method, path, operation)

def _add_resolver_error_handler(self, method, path, err):
"""
Adds a handler for ResolverError for the given method and path.
"""
operation = self.resolver_error_handler(err,
method=method,
path=path,
app_produces=self.produces,
app_security=self.security,
security_definitions=self.security_definitions,
definitions=self.definitions,
parameter_definitions=self.parameter_definitions,
response_definitions=self.response_definitions,
validate_responses=self.validate_responses,
strict_validation=self.strict_validation,
resolver=self.resolver,
randomize_endpoint=RESOLVER_ERROR_ENDPOINT_RANDOM_DIGITS)
self._add_operation_internal(method, path, operation)

def _add_operation_internal(self, method, path, operation):
operation_id = operation.operation_id
logger.debug('... Adding %s -> %s', method.upper(), operation_id,
extra=vars(operation))
Expand Down Expand Up @@ -208,10 +236,16 @@ def add_paths(self, paths=None):
try:
self.add_operation(method, path, endpoint, path_parameters)
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)
# If we have an error handler for resolver errors, add it
# as an operation (but randomize the flask endpoint name).
# Otherwise treat it as any other error.
if self.resolver_error_handler is not None:
self._add_resolver_error_handler(method, path, err)
else:
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())
Expand Down
19 changes: 18 additions & 1 deletion connexion/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ def common_error_handler(exception):

def add_api(self, swagger_file, base_path=None, arguments=None, auth_all_paths=None, swagger_json=None,
swagger_ui=None, swagger_path=None, swagger_url=None, validate_responses=False,
strict_validation=False, resolver=Resolver()):
strict_validation=False, resolver=Resolver(), resolver_error=None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not very clear that resolver_error is expected to be a HTTP status code. Would be nice to have a more friendly name here.

"""
Adds an API to the application based on a swagger file

Expand All @@ -122,8 +122,17 @@ def add_api(self, swagger_file, base_path=None, arguments=None, auth_all_paths=N
:type strict_validation: bool
:param resolver: Operation resolver.
:type resolver: Resolver | types.FunctionType
:param resolver_error: If specified, turns ResolverError into error
responses with the given status code.
:type resolver_error: int | None
:rtype: Api
"""
# Turn the resolver_error code into a handler object
self.resolver_error = resolver_error
resolver_error_handler = None
if resolver_error is not None:
resolver_error_handler = self._resolver_error_handler

resolver = Resolver(resolver) if hasattr(resolver, '__call__') else resolver

swagger_json = swagger_json if swagger_json is not None else self.swagger_json
Expand All @@ -143,13 +152,21 @@ def add_api(self, swagger_file, base_path=None, arguments=None, auth_all_paths=N
swagger_path=swagger_path,
swagger_url=swagger_url,
resolver=resolver,
resolver_error_handler=resolver_error_handler,
validate_responses=validate_responses,
strict_validation=strict_validation,
auth_all_paths=auth_all_paths,
debug=self.debug)
self.app.register_blueprint(api.blueprint)
return api

def _resolver_error_handler(self, *args, **kwargs):
from connexion.handlers import ResolverErrorHandler
kwargs['operation'] = {
'operationId': 'connexion.handlers.ResolverErrorHandler',
}
return ResolverErrorHandler(self.resolver_error, *args, **kwargs)

def add_error_handler(self, error_code, function):
"""

Expand Down
20 changes: 19 additions & 1 deletion connexion/handlers.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

import logging

from .operation import SecureOperation
from .operation import Operation, SecureOperation
from .problem import problem

logger = logging.getLogger('connexion.handlers')
Expand Down Expand Up @@ -42,3 +42,21 @@ def handle(self, *args, **kwargs):
Actual handler for the execution after authentication.
"""
return problem(title=self.exception.name, detail=self.exception.description, status=self.exception.code)


class ResolverErrorHandler(Operation):
"""
Handler for responding to ResolverError.
"""

def __init__(self, status_code, exception, *args, **kwargs):
self.status_code = status_code
self.exception = exception
Operation.__init__(self, *args, **kwargs)

@property
def function(self):
return self.handle

def handle(self, *args, **kwargs):
return problem(title='Not Implemented', detail=self.exception.reason, status=self.status_code)
5 changes: 3 additions & 2 deletions connexion/operation.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ class Operation(SecureOperation):
def __init__(self, method, path, operation, resolver, app_produces,
path_parameters=None, app_security=None, security_definitions=None,
definitions=None, parameter_definitions=None, response_definitions=None,
validate_responses=False, strict_validation=False):
validate_responses=False, strict_validation=False, randomize_endpoint=None):
"""
This class uses the OperationID identify the module and function that will handle the operation

Expand Down Expand Up @@ -162,6 +162,7 @@ def __init__(self, method, path, operation, resolver, app_produces,
self.validate_responses = validate_responses
self.strict_validation = strict_validation
self.operation = operation
self.randomize_endpoint = randomize_endpoint

# todo support definition references
# todo support references to application level parameters
Expand All @@ -174,7 +175,7 @@ def __init__(self, method, path, operation, resolver, app_produces,

resolution = resolver.resolve(self)
self.operation_id = resolution.operation_id
self.endpoint_name = flaskify_endpoint(self.operation_id)
self.endpoint_name = flaskify_endpoint(self.operation_id, self.randomize_endpoint)
self.__undecorated_function = resolution.function

self.validate_defaults()
Expand Down
1 change: 0 additions & 1 deletion examples/sqlalchemy/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import connexion
from connexion import NoContent

import orm

db_session = None
Expand Down
14 changes: 14 additions & 0 deletions tests/api/test_responses.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,3 +152,17 @@ def test_maybe_blob_or_json(simple_app):
text, number = unpack('!4sh', resp.data)
assert text == b'cool'
assert number == 8


def test_bad_operations(bad_operations_app):
# Bad operationIds in bad_operations_app should result in 501
app_client = bad_operations_app.app.test_client()

resp = app_client.get('/v1.0/welcome')
assert resp.status_code == 501

resp = app_client.put('/v1.0/welcome')
assert resp.status_code == 501

resp = app_client.post('/v1.0/welcome')
assert resp.status_code == 501
5 changes: 5 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,8 @@ def secure_api_app():
@pytest.fixture(scope="session")
def unordered_definition_app():
return build_app_from_fixture('unordered_definition')


@pytest.fixture(scope="session")
def bad_operations_app():
return build_app_from_fixture('bad_operations', resolver_error=501)
23 changes: 23 additions & 0 deletions tests/fakeapi/bad_specs.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
swagger: "2.0"

info:
title: "{{title}}"
version: "1.0"

basePath: /v1.0

paths:
/welcome:
get:
operationId: fakeapi.foo_bar.search
parameters:
# The default below validates, but is obviously the wrong type
- name: foo
in: query
type: integer
default: somestring
responses:
200:
description: search
schema:
type: object
31 changes: 31 additions & 0 deletions tests/fixtures/bad_operations/swagger.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
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
put:
# operationId: XXX completely missing
responses:
200:
description: greeting response
schema:
type: object
post:
operationId: fakeapi.module_with_error.something
responses:
200:
description: greeting response
schema:
type: object
25 changes: 23 additions & 2 deletions tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import tempfile

from connexion.api import Api
from connexion.exceptions import ResolverError
from connexion.exceptions import InvalidSpecification, ResolverError
from swagger_spec_validator.common import SwaggerValidationError
from yaml import YAMLError

Expand Down Expand Up @@ -49,6 +49,10 @@ def test_invalid_operation_does_stop_application_to_setup():
Api(TEST_FOLDER / "fixtures/user_module_loading_error/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'})


def test_invalid_operation_does_not_stop_application_in_debug_mode():
api = Api(TEST_FOLDER / "fixtures/op_error_api/swagger.yaml", "/api/v1.0",
Expand All @@ -59,11 +63,28 @@ def test_invalid_operation_does_not_stop_application_in_debug_mode():
{'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'

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",
api = Api(TEST_FOLDER / "fixtures/missing_op_id/swagger.yaml", "/api/v1.0",
{'title': 'OK'}, debug=True)
assert api.specification['info']['title'] == 'OK'


def test_other_errors_stop_application_to_setup():
# The previous tests were just about operationId not being resolvable.
# Other errors should still result exceptions!
with pytest.raises(InvalidSpecification):
Api(TEST_FOLDER / "fakeapi/bad_specs.yaml", "/api/v1.0",
{'title': 'OK'})

# Debug mode should ignore the error
api = Api(TEST_FOLDER / "fakeapi/bad_specs.yaml", "/api/v1.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please move those specs to a proper directory within /fixtures?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, the bad specs I missed. One moment.

{'title': 'OK'}, debug=True)
assert api.specification['info']['title'] == 'OK'

Expand Down
1 change: 1 addition & 0 deletions tests/test_operation.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import pytest


TEST_FOLDER = pathlib.Path(__file__).parent

DEFINITIONS = {'new_stack': {'required': ['image_version', 'keep_stacks', 'new_traffic', 'senza_yaml'],
Expand Down