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

stop relying on ujson (fix #589, #587, #507) #637

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ Changelog
Current
-------

- **Behavior**:
* ``ujson`` is not the default json serializer anymore. A new configuration option is available instead: ``RESTPLUS_JSON_SERIALIZER`` (:issue:`507`, :issue:`587`, :issue:`589`, :pr:`637`)
- Add new `Wildcard` fields (:pr:`255`)
- Fix ABC deprecation warnings (:pr:`580`)
- Fix `@api.expect(..., validate=False)` decorators for an :class:`Api` where `validate=True` is set on the constructor (:issue:`609`, :pr:`610`)
Expand Down
71 changes: 71 additions & 0 deletions doc/quickstart.rst
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,77 @@ parameter to some classes or function to force order preservation:
- globally on :class:`Namespace`: ``ns = Namespace(ordered=True)``
- locally on :func:`marshal`: ``return marshal(data, fields, ordered=True)``

Configuration
-------------

The following configuration options exist for Flask-RESTPlus:

============================ =============== ==================================
OPTION DEFAULT VALUE DESCRIPTION
============================ =============== ==================================
``BUNDLE_ERRORS`` ``False`` Bundle all the validation errors
instead of returning only the
first one encountered.
See the `Error Handling
<parsing.html#error-handling>`__
section of the documentation for
details.
``RESTPLUS_VALIDATE`` ``False`` Whether to enforce payload
validation by default when using
the ``@api.expect()`` decorator.
See the `@api.expect()
<swagger.html#the-api-expect-decorator>`__
documentation for details.
``RESTPLUS_MASK_HEADER`` ``X-Fields`` Choose the name of the *Header*
that will contain the masks to
apply to your answer.
See the `Fields masks <mask.html>`__
documentation for details.
``RESTPLUS_MASK_SWAGGER`` ``True`` Whether to enable the mask
documentation in your swagger or
not.
See the `mask usage
<mask.html#usage>`__ documentation
for details.
``RESTPLUS_JSON`` ``{}`` Dictionary of options to pass to
the json *serializer* (by default
``json.dumps``).
``RESTPLUS_JSON_SERIALIZER`` ``None`` Here you can choose your
own/preferred json *serializer*.
You can either specify the name
of the module (example: ``ujson``)
or you can give the full name of
your *serializer* (example:
``ujson.dumps``).

.. note::
If you only specify the module
name the default Flask-RESTPlus
behavior is to import its
``dumps`` method.


.. note::
Flask-RESTPlus will always
fallback to the default
``json.dumps`` *serializer*
if it cannot manage to import
the one you configured.
In such case, a
``UserWarning`` will be
raised.


.. warning::
We only officially support
python's builtin
``json.dumps``.
Please keep in mind some
serializers may behave
differently depending on the
input types (floats, dates,
etc.).
============================ =============== ==================================

Full example
------------
Expand Down
3 changes: 2 additions & 1 deletion flask_restplus/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from .postman import PostmanCollectionV1
from .resource import Resource
from .swagger import Swagger
from .utils import default_id, camel_to_dash, unpack
from .utils import default_id, camel_to_dash, preload_serializer, unpack
from .representations import output_json
from ._http import HTTPStatus

Expand Down Expand Up @@ -209,6 +209,7 @@ def _init_app(self, app):
self._validate = self._validate if self._validate is not None else app.config.get('RESTPLUS_VALIDATE', False)
app.config.setdefault('RESTPLUS_MASK_HEADER', 'X-Fields')
app.config.setdefault('RESTPLUS_MASK_SWAGGER', True)
preload_serializer(app)

def __getattr__(self, name):
try:
Expand Down
14 changes: 7 additions & 7 deletions flask_restplus/representations.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals, absolute_import

try:
from ujson import dumps
except ImportError:
from json import dumps

from flask import make_response, current_app

from .utils import preload_serializer


def output_json(data, code, headers=None):
'''Makes a Flask response with a JSON encoded body'''

settings = current_app.config.get('RESTPLUS_JSON', {})
serializer = current_app.config.get('RESTPLUS_CACHED_SERIALIZER')
if serializer is None:
preload_serializer(current_app)
serializer = current_app.config.get('RESTPLUS_CACHED_SERIALIZER')

# If we're in debug mode, and the indent is not set, we set it to a
# reasonable value here. Note that this won't override any existing value
Expand All @@ -22,7 +22,7 @@ def output_json(data, code, headers=None):

# always end the json dumps with a new line
# see https://github.com/mitsuhiko/flask/pull/1262
dumped = dumps(data, **settings) + "\n"
dumped = serializer(data, **settings) + "\n"

resp = make_response(dumped, code)
resp.headers.extend(headers or {})
Expand Down
49 changes: 48 additions & 1 deletion flask_restplus/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@
from __future__ import unicode_literals

import re
import importlib
import warnings

from collections import OrderedDict
from copy import deepcopy
from json import dumps
from six import iteritems

from ._http import HTTPStatus
Expand All @@ -14,7 +17,51 @@
ALL_CAP_RE = re.compile('([a-z0-9])([A-Z])')


__all__ = ('merge', 'camel_to_dash', 'default_id', 'not_none', 'not_none_sorted', 'unpack')
__all__ = ('preload_serializer', 'importer', 'merge', 'camel_to_dash', 'default_id',
'not_none', 'not_none_sorted', 'unpack')


def preload_serializer(app):
'''
Preload the json serializer for the given ``app``.

:param flask.Flask app: The flask application object
'''
custom_serializer = app.config.get('RESTPLUS_JSON_SERIALIZER', None)
serializer = None

# If the user wants to use a custom serializer, let it be
if custom_serializer:
try:
serializer = importer(custom_serializer, 'dumps')
except ImportError:
if '.' in custom_serializer:
mod, func = custom_serializer.rsplit('.', 1)
try:
serializer = importer(mod, func)
except ImportError:
warnings.warn("Unable to load custom serializer '{}', falling back to "
"'json.dumps'".format(custom_serializer),
UserWarning)

# fallback, no serializer found so far, use the default one
if serializer is None:
serializer = dumps
app.config['RESTPLUS_CACHED_SERIALIZER'] = serializer


def importer(mod_name, obj_name, default=None):
'''
Import the given ``obj_name`` from the given ``mod_name``.

:param str mod_name: Module from which to import the ``obj_name``
:param str obj_name: Object to import from ``mod_name``
:param object default: Default object to return

:return: Imported object
'''
imported = importlib.import_module(mod_name)
return getattr(imported, obj_name, default)


def merge(first, second):
Expand Down
1 change: 1 addition & 0 deletions requirements/test.pip
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ pytest-mock==1.6.3
pytest-profiling==1.2.11
pytest-sugar==0.9.0
tzlocal
ujson
46 changes: 46 additions & 0 deletions tests/test_representations.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import pytest

import flask_restplus.representations as rep
from flask_restplus.utils import preload_serializer

from json import dumps, loads
from ujson import dumps as udumps, loads as uloads

payload = {
'id': 1,
'name': 'toto',
'address': 'test',
}


def test_representations_serialization_output_correct(app):
print(app.config)
r = rep.output_json(payload, 200)
assert loads(r.get_data(True)) == loads(dumps(payload))


def test_config_custom_serializer_is_module(app, api):
# enforce a custom serializer
app.config['RESTPLUS_JSON_SERIALIZER'] = 'ujson'
# now reset serializer
preload_serializer(app)
r2 = rep.output_json(payload, 200)
assert uloads(r2.get_data(True)) == uloads(udumps(payload))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is testing several different things within a single test case - how do you feel about splitting it up into several test cases so that it's clearer to see the individual assertions? Something like:

def test_representations_serialization_output_correct():
    # original test comparing get_data and dumps outputs

def test_config_custom_serializer_is_module():
    # current_app.config['RESTPLUS_JSON_SERIALIZER'] = 'ujson'
    # test .dumps method is used 

def test_config_custom_serializer_is_function():
    # current_app.config['RESTPLUS_JSON_SERIALIZER'] = 'my_custom_json.module.dump_method'
    # test correct function imported

def test_config_custom_serializer_fallback():
    # test fallback to json.dumps

This makes the intended behaviour clearer and is easier to see from a test failure where the issue is 😄

I think it's a good idea to at least log a message when a fallback to json.dumps takes place. It can fallback without breaking, but the user's should be aware that it's happening (adding it to the docs is still good though so thank you for that). Perhaps something like:

import warnings

# after failing to import user's custom serializer
warnings.warn(
    "unable to import RESTPLUS_JSON_SERIALIZER={}, falling back to json.dumps".format(
        current_app.config["RESTPLUS_JSON_SERIALIZER"]
    )
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I have spit the test.
I'm not quite sure about the warnings.warn yet though. I think we should define a communication with upper apps policy first so people know how to deal with those.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thank you! Sorry, I'm not sure what you mean by

communication with upper apps policy

Could you please clarify? Did you mean define a way in which Flask-RESTPlus should interact with other applications?

Using warnings/warn is a common practice for Flask extensions that wish to communicate misconfiguration/possible error to the user. For example:

Or using logger.warning is also a common approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please clarify? Did you mean define a way in which Flask-RESTPlus should interact with other applications?

I mean we should document what kind of communication people using FLask-RESTPlus should expect.
I know warnings.warn is a common practice, but we should document this.
The reason is the default filter might hide some messages hence this doesn't provide any help to the users:

import warnings

warnings.warn('You should not use this', DeprecationWarning)
warnings.warn('Hey, something went wrong', UserWarning)

Result in:

$ python /tmp/warn.py
/tmp/warn.py:4: UserWarning: Hey, something went wrong
  warnings.warn('Hey, something went wrong', UserWarning)

So by default, unless people know they should expect a DeprecationWarning they won't notice it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes I understand - thank you for clarifying 😄 Yes I agree that this should definitely be documented - it's good to be clear to the users so thank you for pointing that out 👍 .

How about changing this:
https://github.com/noirbizarre/flask-restplus/pull/637/files#diff-dce4048f760a2254db20f3a78d7fcfa1R362 to say something like:

Flask-RESTPlus will always fallback to the default json.dumps serializer if it cannot manage to import the one you configured. This is indicated by a warning during initialisation.

And then include an example of the output when starting the application, for example using logging:

* Serving Flask app "t" (lazy loading)
 * Environment: production
   WARNING: Do not use the development server in a production environment.
   Use a production WSGI server instead.
 * Debug mode: on
 * Running on http://127.0.0.1:5000/ (Press CTRL+C to quit)
 * Restarting with stat
unable to import RESTPLUS_JSON_SERIALIZER=broken_seralizer.dumps, falling back to json.dumps
 * Debugger is active!
 * Debugger PIN: 627-628-298

Or using warnings:

 * Serving Flask app "t" (lazy loading)
 * Environment: production
   WARNING: Do not use the development server in a production environment.
   Use a production WSGI server instead.
 * Debug mode: on
 * Running on http://127.0.0.1:5000/ (Press CTRL+C to quit)
 * Restarting with stat
/home/ben/Documents/flask-restplus/flask_restplus/api.py:213: UserWarning: unable to import RESTPLUS_JSON_SERIALIZER=broken_seralizer.dumps, falling back to json.dumps
  warnings.warn("unable to import RESTPLUS_JSON_SERIALIZER=broken_seralizer.dumps, falling back to json.dumps")
 * Debugger is active!
 * Debugger PIN: 627-628-298

I'm just very hesitant to fallback completely silently (even if it is documented) as the user will not be able to tell when it has happened.

What do you think? 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only problem I see with this is the code path in which this warning will be raised is not deterministic (it will happen only after some route returns and if it needs marshalling).

Maybe the solution is to load the serializer within the Api constructor.

This way, people know when/where to catch the warnings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the warnings.warn as suggested.
This should happen early in the initialization of the Api unless you pass it a Blueprint instead of a Flask app (in which case the warning will be raised when calling a route that needs serialization.
The loaded serializer is cached within the app.config. I don't think this is the best place to do so, but I didn't find any other...

assert app.config.get('RESTPLUS_CACHED_SERIALIZER') == udumps


def test_config_custom_serializer_is_function(app, api):
# test other config syntax
app.config['RESTPLUS_JSON_SERIALIZER'] = 'ujson.dumps'
preload_serializer(app)
rep.output_json(payload, 200)
assert app.config.get('RESTPLUS_CACHED_SERIALIZER') == udumps


def test_config_custom_serializer_fallback(app, api):
# test fallback
app.config['RESTPLUS_JSON_SERIALIZER'] = 'ujson.lol.dumps'
with pytest.warns(UserWarning):
preload_serializer(app)
rep.output_json(payload, 200)
assert app.config.get('RESTPLUS_CACHED_SERIALIZER') == dumps