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 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
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
69 changes: 69 additions & 0 deletions doc/quickstart.rst
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,75 @@ 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
silently fallback to the
default ``json.dumps``
*serializer* if it cannot
manage to import the one
you configured.


.. 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
36 changes: 31 additions & 5 deletions flask_restplus/representations.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,44 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals, absolute_import

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

from json import dumps

from flask import make_response, current_app

DEFAULT_SERIALIZER = 'dumps'
serializer = None


def _importer(mod_name, func_name=DEFAULT_SERIALIZER, default=dumps):
imported = importlib.import_module(mod_name)
return getattr(imported, func_name, default)


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

global serializer

settings = current_app.config.get('RESTPLUS_JSON', {})
custom_serializer = current_app.config.get('RESTPLUS_JSON_SERIALIZER', None)

# If the user wants to use a custom serializer, let it be
if serializer is None and custom_serializer:
try:
serializer = _importer(custom_serializer)
except ImportError:
if '.' in custom_serializer:
mod, func = custom_serializer.rsplit('.', 1)
try:
serializer = _importer(mod, func)
except ImportError:
pass

# fallback, no serializer found so far, use the default one
if serializer is None:
serializer = dumps

# 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 +48,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
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
41 changes: 41 additions & 0 deletions tests/test_representations.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import flask_restplus.representations as rep

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):
r = rep.output_json(payload, 200)
assert loads(r.get_data(True)) == loads(dumps(payload))


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

@SteadBytes SteadBytes May 11, 2019

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

@SteadBytes SteadBytes May 15, 2019

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 rep.serializer == udumps


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


def test_config_custom_serializer_fallback(app):
# test fallback
rep.serializer = None
app.config['RESTPLUS_JSON_SERIALIZER'] = 'ujson.lol.dumps'
rep.output_json(payload, 200)
assert rep.serializer == dumps