From 10b8a65974fe9e59e1fc230cccb37710f0d0d2ee Mon Sep 17 00:00:00 2001 From: Matt Flaherty Date: Thu, 4 Jul 2019 17:44:48 +0100 Subject: [PATCH 1/9] Fix for issue #661 --- CHANGELOG.rst | 1 + flask_restplus/namespace.py | 4 ++-- flask_restplus/swagger.py | 3 ++- tests/conftest.py | 14 ++++++++++++- tests/test_swagger.py | 41 +++++++++++++++++++++++++++++++++++++ 5 files changed, 59 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 262b1dda..b675916f 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -12,6 +12,7 @@ Current - Ensure `basePath` is always a path - Hide Namespaces with all hidden Resources from Swagger documentation - Per route Swagger documentation for multiple routes on a ``Resource`` +- Fix Swagger `duplicate mapping key` problem from conflicts between response codes given as string or integer (:issue`661`) 0.12.1 (2018-09-28) ------------------- diff --git a/flask_restplus/namespace.py b/flask_restplus/namespace.py index 1328ef99..d23a66ff 100644 --- a/flask_restplus/namespace.py +++ b/flask_restplus/namespace.py @@ -238,7 +238,7 @@ def marshal_with(self, fields, as_list=False, code=HTTPStatus.OK, description=No def wrapper(func): doc = { 'responses': { - code: (description, [fields]) if as_list else (description, fields) + str(code): (description, [fields]) if as_list else (description, fields) }, '__mask__': kwargs.get('mask', True), # Mask values can't be determined outside app context } @@ -289,7 +289,7 @@ def response(self, code, description, model=None, **kwargs): :param ModelBase model: an optional response model ''' - return self.doc(responses={code: (description, model, kwargs)}) + return self.doc(responses={str(code): (description, model, kwargs)}) def header(self, name, description=None, **kwargs): ''' diff --git a/flask_restplus/swagger.py b/flask_restplus/swagger.py index 681b08d2..77c364bf 100644 --- a/flask_restplus/swagger.py +++ b/flask_restplus/swagger.py @@ -485,6 +485,7 @@ def responses_for(self, doc, method): for d in doc, doc[method]: if 'responses' in d: for code, response in iteritems(d['responses']): + code = str(code) if isinstance(response, string_types): description = response model = None @@ -514,7 +515,7 @@ def responses_for(self, doc, method): for name, description in iteritems(d['docstring']['raises']): for exception, handler in iteritems(self.api.error_handlers): error_responses = getattr(handler, '__apidoc__', {}).get('responses', {}) - code = list(error_responses.keys())[0] if error_responses else None + code = str(list(error_responses.keys())[0]) if error_responses else None if code and exception.__name__ == name: responses[code] = {'$ref': '#/responses/{0}'.format(name)} break diff --git a/tests/conftest.py b/tests/conftest.py index 753801ce..27b1d914 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -11,11 +11,23 @@ class TestClient(FlaskClient): + # Borrowed from https://pythonadventures.wordpress.com/2016/03/06/detect-duplicate-keys-in-a-json-file/ + # Thank you to Wordpress author @ubuntuincident, aka Jabba Laci. + def dict_raise_on_duplicates(self, ordered_pairs): + """Reject duplicate keys.""" + d = {} + for k, v in ordered_pairs: + if k in d: + raise ValueError("duplicate key: %r" % (k,)) + else: + d[k] = v + return d + def get_json(self, url, status=200, **kwargs): response = self.get(url, **kwargs) assert response.status_code == status assert response.content_type == 'application/json' - return json.loads(response.data.decode('utf8')) + return json.loads(response.data.decode('utf8'), object_pairs_hook=self.dict_raise_on_duplicates) def post_json(self, url, data, status=200, **kwargs): response = self.post(url, data=json.dumps(data), diff --git a/tests/test_swagger.py b/tests/test_swagger.py index 859994ae..91bccfa5 100644 --- a/tests/test_swagger.py +++ b/tests/test_swagger.py @@ -2042,6 +2042,47 @@ def get(self): client.get_specs(status=500) + def test_specs_no_duplicate_response_keys(self, api, client): + ''' + This tests that the swagger.json document will not be written with duplicate object keys + due to the coercion of dict keys to string. The last @api.response should win. + ''' + # Note the use of a strings '404' and '200' in class decorators as opposed to ints in method decorators. + @api.response('404', 'Not Found') + class BaseResource(restplus.Resource): + def get(self): + pass + + model = api.model('SomeModel', { + 'message': restplus.fields.String, + }) + + @api.route('/test/') + @api.response('200', 'Success') + class TestResource(BaseResource): + # @api.marshal_with also yields a response + @api.marshal_with(model, code=200, description='Success on method') + @api.response(404, 'Not Found on method') + def get(self): + {} + + data = client.get_specs('') + paths = data['paths'] + + op = paths['/test/']['get'] + print(op['responses']) + assert op['responses'] == { + '200': { + 'description': 'Success on method', + 'schema': { + '$ref': '#/definitions/SomeModel' + } + }, + '404': { + 'description': 'Not Found on method', + } + } + def test_clone(self, api, client): parent = api.model('Person', { 'name': restplus.fields.String, From 9268600e5c9b2a84753d2d73172d89e22580077e Mon Sep 17 00:00:00 2001 From: Aaron Luna Date: Wed, 21 Aug 2019 22:11:59 -0700 Subject: [PATCH 2/9] Issue template for bugs/defects --- .github/ISSUE_TEMPLATE/bug-report.md | 38 ++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 .github/ISSUE_TEMPLATE/bug-report.md diff --git a/.github/ISSUE_TEMPLATE/bug-report.md b/.github/ISSUE_TEMPLATE/bug-report.md new file mode 100644 index 00000000..dc209dff --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug-report.md @@ -0,0 +1,38 @@ +--- +name: Bug Report +about: Tell us how Flask-RESTPlus is broken +title: '' +labels: bug +assignees: '' + +--- + +### ***** **BEFORE LOGGING AN ISSUE** ***** + +- Is this something you can **debug and fix**? Send a pull request! Bug fixes and documentation fixes are welcome. +- Please check if a similar issue already exists or has been closed before. Seriously, nobody here is getting paid. Help us out and take five minutes to make sure you aren't submitting a duplicate. + +### **Code** + +```python +from your_code import your_buggy_implementation +``` + +### **Expected behavior** +A description of what you expected to happen. + +### **Actual Behavior** +A description of the unexpected, buggy behavior. + +### **Error Messages/Stack Trace** +If applicable, add the stack trace produced by the error + +### **Environment** +- Python version +- Flask version +- Flask-RESTPlus version +- Other installed Flask extensions + +### **Additional Context** + +This is your last chance to provide any pertinent details, don't let this opportunity pass you by! From 30bda89e95ba0a0ad280568282cd44738d2e8eda Mon Sep 17 00:00:00 2001 From: Aaron Luna Date: Thu, 22 Aug 2019 15:48:08 -0700 Subject: [PATCH 3/9] inconsistent capitalization, added repro steps --- .github/ISSUE_TEMPLATE/bug-report.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/bug-report.md b/.github/ISSUE_TEMPLATE/bug-report.md index dc209dff..2255671e 100644 --- a/.github/ISSUE_TEMPLATE/bug-report.md +++ b/.github/ISSUE_TEMPLATE/bug-report.md @@ -18,7 +18,12 @@ assignees: '' from your_code import your_buggy_implementation ``` -### **Expected behavior** +### **Repro Steps** (if applicable) +1. ... +2. ... +3. Broken! + +### **Expected Behavior** A description of what you expected to happen. ### **Actual Behavior** From 2a12fc62af1264e1a32ca6c642ea17ad21269dbf Mon Sep 17 00:00:00 2001 From: Aaron Luna Date: Thu, 22 Aug 2019 16:04:37 -0700 Subject: [PATCH 4/9] added link to CONTRIBUTING.rst --- .github/ISSUE_TEMPLATE/bug-report.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/ISSUE_TEMPLATE/bug-report.md b/.github/ISSUE_TEMPLATE/bug-report.md index 2255671e..bd343344 100644 --- a/.github/ISSUE_TEMPLATE/bug-report.md +++ b/.github/ISSUE_TEMPLATE/bug-report.md @@ -11,6 +11,7 @@ assignees: '' - Is this something you can **debug and fix**? Send a pull request! Bug fixes and documentation fixes are welcome. - Please check if a similar issue already exists or has been closed before. Seriously, nobody here is getting paid. Help us out and take five minutes to make sure you aren't submitting a duplicate. +- Please review the [guidelines for contributing](https://github.com/noirbizarre/flask-restplus/blob/master/CONTRIBUTING.rst) ### **Code** From 47a0eb71c826dfdb8946cb969d47ac235940f856 Mon Sep 17 00:00:00 2001 From: Aaron Luna Date: Thu, 22 Aug 2019 16:26:02 -0700 Subject: [PATCH 5/9] Pull request template --- .../PULL_REQUEST_TEMPLATE.md | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .github/PULL_REQUEST_TEMPLATE/PULL_REQUEST_TEMPLATE.md diff --git a/.github/PULL_REQUEST_TEMPLATE/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 00000000..54e75570 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,25 @@ +## Proposed changes + +At a high level, describe your reasoning for making these changes. If you are fixing a bug or resolving a feature request, **please include a link to the issue**. + +## Types of changes + +What types of changes does your code introduce? +_Put an `x` in the boxes that apply_ + +- [ ] Bugfix (non-breaking change which fixes an issue) +- [ ] New feature (non-breaking change which adds functionality) +- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) + +## Checklist + +_Put an `x` in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code._ + +- [ ] I have read the [guidelines for contributing](https://github.com/noirbizarre/flask-restplus/blob/master/CONTRIBUTING.rst) +- [ ] All unit tests pass on my local version with my changes +- [ ] I have added tests that prove my fix is effective or that my feature works +- [ ] I have added necessary documentation (if appropriate) + +## Further comments + +If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc... From a48acaa8e72796cb847fab89d787cc13073b104b Mon Sep 17 00:00:00 2001 From: Robert Smallshire Date: Tue, 27 Aug 2019 19:12:43 +0200 Subject: [PATCH 6/9] Modifies Api.error_router so that exceptions raised in error handlers are not suppressed in the the logs. Prior to this change, exceptions raises in error handlers registered to an Api were suppressed, and the original exception which caused the error handler to be invoked was reported in logs. This made it very challenging to diagnose programming errors in error handlers, or even identify that an error handler implementation was faulty in the first place. With this change, programming mistakes and other exceptions raised in error handlers are properly reported, and a 500 Internal Server Error returned. The exception which was being handled is chained to this subsequent error and also reported. --- flask_restplus/api.py | 4 ++-- tests/test_errors.py | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/flask_restplus/api.py b/flask_restplus/api.py index 37276503..438da580 100644 --- a/flask_restplus/api.py +++ b/flask_restplus/api.py @@ -579,8 +579,8 @@ def error_router(self, original_handler, e): if self._has_fr_route(): try: return self.handle_error(e) - except Exception: - pass # Fall through to original handler + except Exception as f: + return original_handler(f) return original_handler(e) def handle_error(self, e): diff --git a/tests/test_errors.py b/tests/test_errors.py index 64032a42..19a958b8 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -2,6 +2,8 @@ from __future__ import unicode_literals import json +import logging + import pytest from flask import Blueprint, abort @@ -162,6 +164,31 @@ def handle_custom_exception(error): 'test': 'value', } + def test_blunder_in_errorhandler_is_not_suppressed_in_logs(self, app, client, caplog): + + api = restplus.Api(app) + + class CustomException(RuntimeError): + pass + + class ProgrammingBlunder(Exception): + pass + + @api.route('/test/', endpoint="test") + class TestResource(restplus.Resource): + def get(self): + raise CustomException('error') + + @api.errorhandler(CustomException) + def handle_custom_exception(error): + raise ProgrammingBlunder("This exception needs to be logged, not suppressed, then cause 500") + + with caplog.at_level(logging.ERROR): + response = client.get('/test/') + exc_type, value, traceback = caplog.records[0].exc_info + assert exc_type is ProgrammingBlunder + assert response.status_code == 500 + def test_errorhandler_for_custom_exception_with_headers(self, app, client): api = restplus.Api(app) From c6f87b699aa7a6e8dab09c8d755419a4fb64d950 Mon Sep 17 00:00:00 2001 From: Robert Smallshire Date: Tue, 27 Aug 2019 19:37:37 +0200 Subject: [PATCH 7/9] Make test_error_router_falls_back_to_original that exceptions in errorhandlers registers on Api are not suppressed. Prior to this change, this test did indeed check that the error router fell back to the original handler, but it also (incorrectly, in my view) asserted that the exception passed to that handler was the error that caused the handler to be invoked. This suppressed any errors in error handler implementation, making them hard to get right. Now we assert that any programming (or other) exceptions raises in the error handler are reported. The original hander-invoking error is chained to this error, so there is no loss of information. --- tests/test_errors.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/test_errors.py b/tests/test_errors.py index 19a958b8..ae4dbf6f 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -507,15 +507,23 @@ def test_handle_not_include_error_message(self, app): assert 'message' not in json.loads(response.data.decode()) def test_error_router_falls_back_to_original(self, app, mocker): + class ProgrammingBlunder(Exception): + pass + + blunder = ProgrammingBlunder("This exception needs to be detectable") + + def raise_blunder(arg): + raise blunder + api = restplus.Api(app) app.handle_exception = mocker.Mock() - api.handle_error = mocker.Mock(side_effect=Exception()) + api.handle_error = mocker.Mock(side_effect=raise_blunder) api._has_fr_route = mocker.Mock(return_value=True) exception = mocker.Mock(spec=HTTPException) api.error_router(app.handle_exception, exception) - app.handle_exception.assert_called_with(exception) + app.handle_exception.assert_called_with(blunder) def test_fr_405(self, app, client): api = restplus.Api(app) From f1ecb4a6a1bfc673c054f36b90d68da525446411 Mon Sep 17 00:00:00 2001 From: Robert Smallshire Date: Tue, 27 Aug 2019 21:39:16 +0200 Subject: [PATCH 8/9] Adds an entry to the change log, as per the contributing guide. --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 69b6190d..1555fba6 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -6,7 +6,7 @@ Changelog Current ------- -- Nothing yet +- Ensure that exceptions raised in error handler, including programming errors, are logged (:issue:`705`, :pr:`706`) 0.13.0 (2019-08-12) ------------------- From a7e363a8352efc70c8d160ef9526dc4572733a1e Mon Sep 17 00:00:00 2001 From: idchlife Date: Thu, 12 Sep 2019 18:05:11 +0300 Subject: [PATCH 9/9] Fix ambiguity error with Polymorphism candidates (isinstance() problem) (#691) * Fix for better Polymorphism support Right now Polymorphism fails when using something like this (models in sqlalchemy): models: ```python class Product(BaseModel): discr = Column(String) __mapper_args__ = { 'polymorphic_on': discr, 'polymorphic_identity': "product" } class VegetableProduct(Product): name = Column(String) __mapper_args__ = { 'polymorphic_identity': "vegetable" } class DairyProduct(VegetableProduct): __mapper_args__ = { 'polymorphic_identity': "dairy" } ``` mapper: ```python resource_product = api.model("Product", { 'discr': fields.String }) product_mapper = { VegetableProduct: api.inherit("Vegetable", resource_product, { 'name': fields.String }), DairyProduct: api.inherit("Dairy", resource_product, { 'name': fields.String }), } resource_result = api.model("ResourceResult", { 'products': fields.List(fields.Polymorphism(product_mapper)) }) ``` This sparkles error here: https://github.com/noirbizarre/flask-restplus/blob/master/flask_restplus/fields.py#L682 Because candidates will be: VegetableProduct and DairyProduct, since isinstance() will return True for both classes (I personally surprised it works like this) Checking by __class__.__name__ removes this error and it works as intended. I hope my quick fix did not break anything, I'm eager to update it if I don't know, more elegant solution is needed. * adjusted ambiguity test for new feature * stricter check for type * changed to simple class check * type() check for Polymorphism, adjusted tests * type() check for Polymorphism, adjusted tests * removed local dev files * updated test name, also PR in docblock --- flask_restplus/fields.py | 2 +- tests/test_fields.py | 8 +++++--- tests/test_swagger.py | 4 ++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/flask_restplus/fields.py b/flask_restplus/fields.py index 7e75700a..ad2e00fe 100644 --- a/flask_restplus/fields.py +++ b/flask_restplus/fields.py @@ -674,7 +674,7 @@ def output(self, key, obj, ordered=False, **kwargs): if not hasattr(value, '__class__'): raise ValueError('Polymorph field only accept class instances') - candidates = [fields for cls, fields in iteritems(self.mapping) if isinstance(value, cls)] + candidates = [fields for cls, fields in iteritems(self.mapping) if type(value) == cls] if len(candidates) <= 0: raise ValueError('Unknown class: ' + value.__class__.__name__) diff --git a/tests/test_fields.py b/tests/test_fields.py index d0117353..fbc156f0 100644 --- a/tests/test_fields.py +++ b/tests/test_fields.py @@ -1177,7 +1177,10 @@ class Child2(object): with pytest.raises(ValueError): api.marshal({'owner': object()}, thing) - def test_polymorph_field_ambiguous_mapping(self, api): + def test_polymorph_field_does_not_have_ambiguous_mappings(self, api): + """ + Regression test for https://github.com/noirbizarre/flask-restplus/pull/691 + """ parent = api.model('Parent', { 'name': fields.String, }) @@ -1201,8 +1204,7 @@ class Child(Parent): 'owner': fields.Polymorph(mapping), }) - with pytest.raises(ValueError): - api.marshal({'owner': Child()}, thing) + api.marshal({'owner': Child()}, thing) def test_polymorph_field_required_default(self, api): parent = api.model('Person', { diff --git a/tests/test_swagger.py b/tests/test_swagger.py index 91bccfa5..9e583f55 100644 --- a/tests/test_swagger.py +++ b/tests/test_swagger.py @@ -2279,11 +2279,11 @@ def get(self): assert path['get']['responses']['200']['schema']['$ref'] == '#/definitions/Output' def test_polymorph_inherit_list(self, api, client): - class Child1: + class Child1(object): name = 'Child1' extra1 = 'extra1' - class Child2: + class Child2(object): name = 'Child2' extra2 = 'extra2'