Merge branch 'master' into fix/infinite-recursion-bug

.github/ISSUE_TEMPLATE/bug-report.md

@@ -0,0 +1,44 @@
+---
+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.
+- Please review the [guidelines for contributing](https://github.com/noirbizarre/flask-restplus/blob/master/CONTRIBUTING.rst)
+
+### **Code**
+
+```python
+from your_code import your_buggy_implementation
+```
+
+### **Repro Steps** (if applicable)
+1. ...
+2. ...
+3. Broken!
+
+### **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!

.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...

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)
 -------------------
@@ -17,6 +17,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)
 -------------------

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):

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__)

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):
         '''

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

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),

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)
 
@@ -480,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)

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', {

tests/test_swagger.py

@@ -2062,6 +2062,47 @@ def get(self):
 
         client.get_specs(status=200)
 
+    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,
@@ -2258,11 +2299,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'