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

overridable json lib #1344

Closed
wants to merge 17 commits into from
Closed

overridable json lib #1344

wants to merge 17 commits into from

Conversation

dmig
Copy link

@dmig dmig commented Oct 5, 2020

Imlement #717

  • add vscode artifacts to gitignore
  • new public module
  • use json functions from jsonlib module
  • add .json() doccomment
  • a compatibility condition for json.dumps returning bytes (e.g. orjson)
  • fix test
  • add documentation section
  • fix override not working
  • json encoder override test
  • json decoder override tests

@jcugat
Copy link
Member

jcugat commented Oct 5, 2020

Not sure I like this approach. This could cause problems if a third party lib (for example a github client built on top httpx) decides to use orjson, and the end user (the one that's building something on top the github client) wants ujson instead. Depending on the import order httpx would use one or the other, but noticing that would be quite hard.

Why not allow a new param in httpx.Client() similar to aiohttp's json_serialize?

@dmig
Copy link
Author

dmig commented Oct 5, 2020

This is a quick-n-dirty solution as mentioned here #717 (comment).

Currently, I'm working on better implementation.

@dmig
Copy link
Author

dmig commented Oct 6, 2020

Updated implementation: encoder/decoder are now passed as BaseClient parameters.
Any comments?

@tomchristie
Copy link
Member

So, I really don't particularly want us pushing support for this into the client or making a formal public API out of this.

I'm just about okay with us allowing for a workaround to override the json import, but I don't think we should push users towards it, because I generally think it's a poor decision to switch away from the stdlib json implementation.

Third party libs all tend to have different behaviours from stdlib in obscure corner cases, and the "let's switch to X because it's faster" is almost always made as a pre-mature optimisation that's not evidence led. The network is generally going to be the bottleneck - so trading off improving JSON deserialization speeds vs. differences in behaviour vs. stdlib isn't usually a great option, or something that we should be promoting.

As I say, I'm just about okay with providing a get-out that allows switching the json import out, but let's keep it simple, and not even necessarily push folks towards it from the docs.

requests have managed to do with just that for the last 9 years - I think we'd be okay too. 😃

@dmig-alarstudios
Copy link

@tomchristie should I revert last commits?
I like the current implementation more than a module with overridable functions.

...or making a formal public API out of this.

Ok, what do you think of BaseClient json_encoder/json_decoder public properties instead of constructor arguments?

The network is generally going to be the bottleneck...

Think about microservices: a lot of small high-performance applications, running within one network or even one server. That's my case and it is probably popular.
This is when performance matters.

@eseglem
Copy link

eseglem commented Oct 8, 2020

I totally agree that there can be obscure corner cases, and users should understand the implications before using non standard libraries. Httpx definitely should not just pick a library for itself. Using stdlib, unless explicitly set not to, makes sense.

However, I don't really see allowing it to be overridden as promoting their use. Just letting it happen, if someone makes the decision to do so. And, I think there is many legitimate reasons for not wanting to use stdlib, even though a lot of the uses are pre-mature optimization.

What about how Pydantic does it: https://pydantic-docs.helpmanual.io/usage/exporting_models/#custom-json-deserialisation

It defaults the config to json.loads and json.dumps but allows people to override the class and swap them out if they want. At their own risk.

I could see something like this maybe:

class MyResponse(https.Response):
    json_loads = orjson.loads
    json_dumps = orjson.dumps

class MyAsycnClient(httpx.AsyncClient):
    response_class = MyResponse

Then the you can just call MyAsyncClient everywhere instead of httpx.AsyncClient for convenience.

@tomchristie
Copy link
Member

Ok, what do you think of BaseClient json_encoder/json_decoder public properties instead of constructor arguments?

So, I'm not convinced we'd necessarily want to accept API additions to support this, but if we do we want them to be absolutely as narrow as possible. With that in mind, I think the two options we could consider accepting pull requests for would be...

Option 1. Globally overridable library substitution.

httpx.json = orjson  # Or perhaps httpx.jsonlib = ...

Option 2. Client level library substitution.

httpx.Client(jsonlib=...)

I'm not super keen on option 2, because it'd also end up requiring to add public API for jsonlib=... to the Request and Response constructors.

@tomchristie
Copy link
Member

So, a more narrow alternative for consideration here... #1352

@dmig-alarstudios
Copy link

The way from #1352 is much less flexible, it won't allow to overload a single encoder/decoder function.

@florimondmanca
Copy link
Member

@dmig-alarstudios Yes, it would…

import orjson
import ujson


class CustomJSON:
    def loads(self, text):
        return orjson.loads(text)

    def dumps(self, value):
        return ujson.dumps(value)


httpx.json = CustomJSON()

(Although I'm not sure what the use case for the JSON encoder and decoder to be different would be, but as you can see it's possible using the API from #1352.)

@florimondmanca
Copy link
Member

florimondmanca commented Oct 10, 2020

Going to go ahead and close this one for now for housekeeping, since it seems we agree that #1352 would be a more promising candidate to solve the problem at hand.

@dmig Thank you so much for the time you put into this!

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

Successfully merging this pull request may close these issues.

6 participants