stop relying on ujson (fix #589, #587, #507) #637
Conversation
|
Coverall seems drunk. |
|
As discussed with @noirbizarre we give people the ability to choose their own json serializer through a new dedicated This way if they are not affected by a |
There was a problem hiding this comment.
Future work -- this solution does the import on each run of output_json. That might slow things down. Suggest splitting this into a separate module that's instantiate on app instantiation, and passed around...or some other option that keeps the choice around for the length of the app and reuses
|
@j5awry actually the import only occurs once (during the first call) then the serializer is cached within the I just ran some tests to confirm the caching is working as it should and it turns out it does =) |
Sweet! i'd still like it as a separate module for clarity, but that's a personal thing. If it sticks around cached in the global, then we're good! Again a personal thing with an aversion to global :) |
|
Does using the default Python |
|
I don't know whether this is representative or not but here are the benchmark results. Without ujson: With |
|
Looks like for the average case (mean) the two are fairly similar, however Is it worth adding test to ensure that importing the configured On a separate note, the addition of the configuration options documentation is nice 👍 The I'll create a PR to this PR to add that and the serializer import tests so we can get this merged in 😄 EDIT: @ziirish whilst adding some tests for the serializer import, I have found quite a significant issue - take a look at my comment https://github.com/noirbizarre/flask-restplus/pull/637/files#r282764702 and let me know your thoughts 😄 |
ziirish
force-pushed
the
fix-drop-ujson
branch
2 times, most recently
from
7e885a3
to
9325cc6
Compare
| # then enforce a custom serializer | ||
| current_app.config['RESTPLUS_JSON_SERIALIZER'] = 'ujson' | ||
| r2 = rep.output_json(payload, 200) | ||
| assert uloads(r2.get_data(True)) == uloads(udumps(payload)) |
There was a problem hiding this comment.
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.dumpsThis 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"]
)
)There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.dumpsserializer if it cannot manage to import the one you configured. This is indicated by awarningduring 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? 😄
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
…N_SERIALIZER config option
…h a warnings.warn(UserWarning)
ujson does not seem to be maintained anymore. Some bugs reported months (or years ago) start to affect our users.
Since we don't ship with ujson in our requirements and this was just a "optional performance improvement" I think it is safe to remove it.
Note: this reverts #318