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

make_response does not work with pydantic #61

Closed
jo-gre opened this issue Jul 9, 2020 · 9 comments
Closed

make_response does not work with pydantic #61

jo-gre opened this issue Jul 9, 2020 · 9 comments

Comments

@jo-gre
Copy link

jo-gre commented Jul 9, 2020

Setup:

The API I'm working with uses Connexion and Pydantic as return wrapper for type safety.

Problem:

Connexion can handle pydantic classes and is able to parse them into a proper response. I.e. that a normal response could be a single pydantic dataclass object even without any status code. (https://pydantic-docs.helpmanual.io/usage/dataclasses/)
As soon as I use a decorator on such an endpoint the response is 500.

It would be really appreciated if this behavior could be deactivated somehow.

response = make_response(response)

@rycus86
Copy link
Owner

rycus86 commented Jul 9, 2020

Thanks for the report! I can try to have a look this weekend. Could you please provide a minimal failing test to verify any changes on?

@jo-gre
Copy link
Author

jo-gre commented Jul 9, 2020

Thanks for the quick answer.

Example.

main.py:

import connexion
from prometheus_flask_exporter import PrometheusMetrics

app = connexion.App(__name__)
metrics = PrometheusMetrics(app, export_defaults=None, excluded_paths=".")


if __name__ == '__main__':
    app.add_api('my_api.yaml')
    app.app.run(port=8080, use_reloader=False)

endpoints.py

from pydantic.dataclasses import dataclass
from foo.main import metrics


@dataclass
class Info:
    foo: str


@metrics.histogram("foo", "bar", labels={"foo": lambda: "bar"})
def test() -> Info:
    return Info('Test version')

my_api.yaml

openapi: 3.0.0
info:
  version: 1.0.0
  title: Test

paths:
  /test:
    get:
      operationId: endpoint.test
      responses:
        '200':
          description: Test

I'm Sorry for that not so minimal example. Since Connexion needs a yaml to work with and also reimports the file of the endpoints, the endpoints needs to be in their own file in order to initialize the histogram only once. Anyway you should be able to run this locally if you install Pydantic and Connexion.

Problem

The expected behavior of the /test endpoint would be: {"foo": "Test version"}, 200.
Calling it results in a 500:

[2020-07-09 17:14:54,055] ERROR in app: Exception on /test [GET]
Traceback (most recent call last):
  File "/home/jonasg/PycharmProjects/tmp/venv/lib/python3.8/site-packages/flask/app.py", line 2447, in wsgi_app
    response = self.full_dispatch_request()
  File "/home/jonasg/PycharmProjects/tmp/venv/lib/python3.8/site-packages/flask/app.py", line 1952, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/home/jonasg/PycharmProjects/tmp/venv/lib/python3.8/site-packages/flask/app.py", line 1821, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/home/jonasg/PycharmProjects/tmp/venv/lib/python3.8/site-packages/flask/_compat.py", line 39, in reraise
    raise value
  File "/home/jonasg/PycharmProjects/tmp/venv/lib/python3.8/site-packages/flask/app.py", line 1950, in full_dispatch_request
    rv = self.dispatch_request()
  File "/home/jonasg/PycharmProjects/tmp/venv/lib/python3.8/site-packages/flask/app.py", line 1936, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/jonasg/PycharmProjects/tmp/venv/lib/python3.8/site-packages/connexion/decorators/decorator.py", line 48, in wrapper
    response = function(request)
  File "/home/jonasg/PycharmProjects/tmp/venv/lib/python3.8/site-packages/connexion/decorators/uri_parsing.py", line 144, in wrapper
    response = function(request)
  File "/home/jonasg/PycharmProjects/tmp/venv/lib/python3.8/site-packages/connexion/decorators/parameter.py", line 121, in wrapper
    return function(**kwargs)
  File "/home/jonasg/PycharmProjects/tmp/venv/lib/python3.8/site-packages/prometheus_flask_exporter/__init__.py", line 629, in func
    response = make_response(response)
  File "/home/jonasg/PycharmProjects/tmp/venv/lib/python3.8/site-packages/flask/helpers.py", line 223, in make_response
    return current_app.make_response(args)
  File "/home/jonasg/PycharmProjects/tmp/venv/lib/python3.8/site-packages/flask/app.py", line 2127, in make_response
    raise TypeError(
TypeError: The view function did not return a valid response. The return type must be a string, dict, tuple, Response instance, or WSGI callable, but it was a Info.
127.0.0.1 - - [09/Jul/2020 17:14:54] "GET /test HTTP/1.1" 500 -

This should be caused by the "make_response" from base Flask since the method cannot handle Pydantic objects. My quick solution on API side would be to parse everything through jsonify in the return statement. But thats denies the Pydantic typing.

If you agree on this, I could fork the project over the weekend and try to add support for Connexion to the decorator.

@rycus86
Copy link
Owner

rycus86 commented Jul 9, 2020

I'd rather avoid the jsonifying, but curios to see how you'd implement it if you have time.

@rycus86
Copy link
Owner

rycus86 commented Jul 10, 2020

It looks like https://github.com/rycus86/prometheus_flask_exporter/blob/connexion-support/prometheus_flask_exporter/__init__.py#L754 should work, https://github.com/rycus86/prometheus_flask_exporter/blob/connexion-support/examples/connexion-pydantic/main.py has an example.

I'll sort out some CI issues then I'll cut a new release with it. 🎉

tl;dr:

import connexion
from prometheus_flask_exporter import ConnexionPrometheusMetrics

app = connexion.App(__name__)
metrics = ConnexionPrometheusMetrics(app, export_defaults=None)

Then everything should work as you'd expect. 🤞

@jo-gre
Copy link
Author

jo-gre commented Jul 10, 2020

That should solve the problem Indeed. Thank you for the quick help and keep up the good work 👍

@jo-gre
Copy link
Author

jo-gre commented Jul 10, 2020

But before we close this thread is there a to apply this to the UWsgiPrometheusMetrics ?

            if (
                "ENABLE_MULTIPROCESS_METRICS" in os.environ
                and os.environ["ENABLE_MULTIPROCESS_METRICS"]
            ):
                self.metrics = UWsgiPrometheusMetrics(app=app, export_defaults=False)
                with app.app_context():
                    self.metrics.register_endpoint("/metrics")
            else:
                self.metrics = PrometheusMetrics(app=app, export_defaults=False)

My original setup looks like this... And not sure right now if how I can make this work if the app initializes the UwsgiMetric.

@rycus86
Copy link
Owner

rycus86 commented Jul 10, 2020

Yep -- though a bit more cumbersome --, you can use UWsgiPrometheusMetrics(app=app, export_defaults=False, response_converter=FlaskApi.get_response) but let me know if that doesn't work once the new version is released. (just preparing it now)

@rycus86
Copy link
Owner

rycus86 commented Jul 22, 2020

I just realized this will still not work nicely with UWsgi and the recent Connexion response converter. 🤦‍♂️
Will try to think about how to be able to compose the two.

@rycus86 rycus86 reopened this Jul 22, 2020
@rycus86
Copy link
Owner

rycus86 commented Aug 26, 2020

OK, interestingly, Connexion works behind uWSGI without having to add any extra wrapping. 🤷‍♂️
I've put up an example here: https://github.com/rycus86/prometheus_flask_exporter/tree/master/examples/uwsgi-connexion

In the https://github.com/rycus86/prometheus_flask_exporter/blob/master/examples/uwsgi-connexion/Dockerfile you can see we run the Connexion app with uwsgi --http 0.0.0.0:4000 --module main:app --master --processes 4 --threads 2

And https://github.com/rycus86/prometheus_flask_exporter/blob/master/examples/uwsgi-connexion/main.py#L5 simply uses a ConnexionPrometheusMetrics for the metrics.

Let me know if this works for you!

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants