Allow http.HTTPStatus enums as response status codes. #504

jkrukoff · 2017-08-08T21:41:16Z

Python 3.5 introduced a new enumeration "http.HTTPStatus" for
representing HTTP response status codes. The default response validation
introduced in connexion 1.1.12 highlighted the fact that connexion does
not natively support this type and was previously silently ignoring
non-integer status code representations.

This modifies the response validation code to extract the value when
given an enum instead of an int. Somewhat hacky test code is added to
check for enum support on python versions that include
"http.HTTPStatus".

Fixes #503

Changes proposed in this pull request:

Convert enums to ints in response validation matching.

Python 3.5 introduced a new enumeration "http.HTTPStatus" for representing HTTP response status codes. The default response validation introduced in connexion 1.1.12 highlighted the fact that connexion does not natively support this type and was previously silently ignoring non-integer status code representations. This modifies the response validation code to extract the value when given an enum instead of an int. Somewhat hacky test code is added to check for enum support on python versions that include "http.HTTPStatus".

coveralls · 2017-08-08T21:56:25Z

Coverage remained the same at 100.0% when pulling d7d6b4b on jkrukoff:master into d51e9c8 on zalando:master.

jmcs · 2017-08-15T06:08:06Z

I like it, but can you had a unit test for custom enum (this would cover the case where someone uses a backport of the enum).

jkrukoff · 2017-08-16T17:03:43Z

Do you have a suggestion on a good way to only run the test in python versions where Enum is available, or should I use the same ImportError based approach?

mvalkon · 2017-08-22T07:47:29Z

@jkrukoff there's a way to run the test with only specific python versions. First you need to indicate that the test should be skipped in python versions that do not support enums. You can use the @unittest.skipIf decorator:

@unittest.skipIf(sys.version_info < (3, 5), "Not supported in this version")
def test_your_test_name(...):

If you want to run your tests with multiple python versions, we're using tox for this. To verify that the above works you can run tox with -e command line parameter as follows:

$ tox -e py27,py34,py36

You can install tox with pip:

$ pip install tox

tox is currently not in https://github.com/zalando/connexion/blob/master/requirements-devel.txt but I'll make a PR to add it.

coveralls · 2017-09-15T18:55:12Z

Coverage remained the same at 100.0% when pulling 38ce0c8 on jkrukoff:master into f36c246 on zalando:master.

coveralls · 2017-09-15T18:55:12Z

Coverage remained the same at 100.0% when pulling 38ce0c8 on jkrukoff:master into f36c246 on zalando:master.

This reverts to exception based python version checking for both tests, due to the suggested unittest skipping alternative not being supported in all python versions. "unittest.case.SkipTest: Not supported in this version" is the error reported.

coveralls · 2017-09-15T19:30:41Z

Coverage remained the same at 100.0% when pulling 3e9da66 on jkrukoff:master into f36c246 on zalando:master.

jkrukoff · 2017-09-15T20:52:30Z

Added the test for arbitrary enums, which exposed an assumption inside flask that appears to only work for ints/actual HTTPStatus Enums. Moved the enum value extraction deeper into the stack to compensate.

Tried to use @unittest.skipif in the test suite, but that failed with an unsupported error from unittest for python versions <3.5. Reverted back to the previous approach to get tests passing again.

mvalkon · 2017-09-22T14:39:23Z

@jkrukoff unittest.skipIf has been introduced in 3.1. Out of interest, what sort of an exception did you get?

The change looks good to me.

jkrukoff · 2017-09-22T17:07:57Z

It was pretty close to "unittest.skipif is unsupported in this version". It it's been present since 3.1, would explain why the python 2.7 test suite was exploding with that error.

Though, honestly, that might have been my skip message being reflected back at me. Perhaps I was hitting some configuration in the test runner that labelled skips as errors?

Rebuilding f789831 will likely trip the error again.

jmcs · 2017-09-25T05:52:19Z

tests/fakeapi/hello.py

@@ -1,4 +1,6 @@
 #!/usr/bin/env python3
+import sys
+import unittest


This imports are not being used.

Ah, yeah, forgot to remove those from the attempt to use uniftest.skipIf.

jmcs · 2017-09-25T06:04:45Z

connexion/apis/flask_api.py

@@ -154,6 +154,12 @@ def _build_flask_response(cls, mimetype=None, content_type=None,
        flask_response = flask.current_app.response_class(**kwargs)  # type: flask.Response

        if status_code is not None:
+            try:


Please use hasattr here since the most common use case is status_code being an integer.

hasattr is faster than try... except AtributeError for cases where the attribute doesn't exist.

Yes, and you get to repeat yourself while doing the check. It'd be even faster if just didn't support enums at all!

Will update.

coveralls · 2017-10-04T22:47:50Z

Coverage remained the same at 100.0% when pulling 3e8596e on jkrukoff:master into f36c246 on zalando:master.

coveralls · 2017-10-04T22:47:50Z

Coverage remained the same at 100.0% when pulling 3e8596e on jkrukoff:master into f36c246 on zalando:master.

hjacobs · 2017-10-05T06:59:59Z

👍

A small bug was introduced in spec-first#500 when sanitizing the request query parameters. Instead of fetching the arguments from `request.query`, the parameters were sanitized from the `request.form`. This causes an error log to be printed, as the parameters are not expected for the query (for example in the case of a POST request). Fixes spec-first#504

* FIX a typo causing an ERROR log A small bug was introduced in #500 when sanitizing the request query parameters. Instead of fetching the arguments from `request.query`, the parameters were sanitized from the `request.form`. This causes an error log to be printed, as the parameters are not expected for the query (for example in the case of a POST request). Fixes #504 * adds a fixture for testing query param sanitazion Adds a simple fixture to test form and query parameter sanitazion. This is mostly related to #522, in which the `formData` parameters were treated as query parameters. * add a test to validate form data params * introduce testfixtures library

jkrukoff added 2 commits September 15, 2017 12:10

Merge branch 'master' of github.com:zalando/connexion

6ada3f0

[master]: Restructure tests from PR comments.

f789831

jkrukoff force-pushed the master branch from 07d5191 to 38ce0c8 Compare September 15, 2017 18:44

jkrukoff force-pushed the master branch from 38ce0c8 to 4bcbb46 Compare September 15, 2017 18:57

[master]: Move enum handling deeper into the stack.

3e9da66

spec-first deleted a comment Sep 15, 2017

jmcs reviewed Sep 25, 2017

View reviewed changes

[master]: Respond to yet more PR comments.

3e8596e

hjacobs merged commit 94f64b8 into spec-first:master Oct 5, 2017

mvalkon mentioned this pull request Oct 5, 2017

FIX a typo causing an ERROR log #523

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow http.HTTPStatus enums as response status codes. #504

Allow http.HTTPStatus enums as response status codes. #504

jkrukoff commented Aug 8, 2017

coveralls commented Aug 8, 2017 •

edited

Loading

jmcs commented Aug 15, 2017

jkrukoff commented Aug 16, 2017

mvalkon commented Aug 22, 2017 •

edited

Loading

coveralls commented Sep 15, 2017

coveralls commented Sep 15, 2017

coveralls commented Sep 15, 2017 •

edited

Loading

jkrukoff commented Sep 15, 2017

mvalkon commented Sep 22, 2017

jkrukoff commented Sep 22, 2017 •

edited

Loading

jmcs Sep 25, 2017

jkrukoff Oct 4, 2017

jmcs Sep 25, 2017

jkrukoff Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017 •

edited

Loading

hjacobs commented Oct 5, 2017

Allow http.HTTPStatus enums as response status codes. #504

Allow http.HTTPStatus enums as response status codes. #504

Conversation

jkrukoff commented Aug 8, 2017

coveralls commented Aug 8, 2017 • edited Loading

jmcs commented Aug 15, 2017

jkrukoff commented Aug 16, 2017

mvalkon commented Aug 22, 2017 • edited Loading

coveralls commented Sep 15, 2017

coveralls commented Sep 15, 2017

coveralls commented Sep 15, 2017 • edited Loading

jkrukoff commented Sep 15, 2017

mvalkon commented Sep 22, 2017

jkrukoff commented Sep 22, 2017 • edited Loading

jmcs Sep 25, 2017

Choose a reason for hiding this comment

jkrukoff Oct 4, 2017

Choose a reason for hiding this comment

jmcs Sep 25, 2017

Choose a reason for hiding this comment

jkrukoff Oct 4, 2017

Choose a reason for hiding this comment

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017 • edited Loading

hjacobs commented Oct 5, 2017

coveralls commented Aug 8, 2017 •

edited

Loading

mvalkon commented Aug 22, 2017 •

edited

Loading

coveralls commented Sep 15, 2017 •

edited

Loading

jkrukoff commented Sep 22, 2017 •

edited

Loading

coveralls commented Oct 4, 2017 •

edited

Loading