-
-
Notifications
You must be signed in to change notification settings - Fork 762
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
Bad operationId to error status code #278
Conversation
@@ -3,7 +3,7 @@ max-line-length=120 | |||
exclude=connexion/__init__.py | |||
|
|||
[tox] | |||
envlist=pypy,py27,py34,py35,py35-flask0.10.1,isort-check,flake8 | |||
envlist=py27,py34 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this back. Not sure if you committed that change by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, very much so! Sorry about that!
Rebase this PR please. |
Yep. Probably tomorrow. |
- Handle bad and missing operationIds with a well-defined error
…from the module with error case, but the result should be the same.
part of the PR earlier.
next up: the implementation.
complain about duplicate names.
resolver_error_handler is provided, try to add that as an operation with a randomized endpoint name. Otherwise, behave as before.
callback that's creating actual handler operations. That's more in line with the overall code structure.
ResolverError occurrences into a ResolverErrorHandler that returns the error code given.
and re-raises its exception information if exceptions relating to name resolution are raised at all. It still converts the meaningless ValueError and AttributeError into a ResolverError.
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 / "fakeapi/missing_op_id.yaml", "/api/v1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use this swagger file from the /fixtures
dir.
@jfinkhaeuser Would be nicer to have this as an option, it could be called |
Makes sense as an option to me, yes. Whether you change #284 or I change this one, I don't mind particularly :) |
It is preferably to this change to be in this PR since it is where this behavior is going to be merged. |
Alright, I'll look into it. |
{'title': 'OK'}) | ||
|
||
# Debug mode should ignore the error | ||
api = Api(TEST_FOLDER / "fakeapi/bad_specs.yaml", "/api/v1.0", |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
So now I've re-familiarized myself with everything, I've got to ask: nice to have as an option to what? Right now, it's an option to It's relatively hard adding this functionality to a So I've put it into Api instead, which makes a lot more sense to me. |
Yes, you are right! I looked through your code here after posting my comment and I see that it is already an option. I will do a final review before merging it. |
@@ -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): |
There was a problem hiding this comment.
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 the ability to turn bad or missing operationIds into run time errors returning a defined status code, e.g. 501 ("Not Implemented").
This change is largely useful for testing purposes.
When (black-box) testing implementations, it is not yet know whether a given implementation matches the given Swagger/OpenAPI specs. Especially during development, partial implementations might exist and test well, but some endpoints might still be missing from the implementation. This change lets connexion serve such partial implementations, and lets testers write expectations about missing functionality (i.e. results in a 501 error).
Note that this PR includes #274, #275, #276 and #277, and in fact depends on those changes. I would recommend merging the others first (or not, as it were) before merging this one.