Skip to content

WIP: json using reqwest/serde#10

Open
puzza007 wants to merge 1 commit into
dlesl:masterfrom
puzza007:json
Open

WIP: json using reqwest/serde#10
puzza007 wants to merge 1 commit into
dlesl:masterfrom
puzza007:json

Conversation

@puzza007
Copy link
Copy Markdown
Contributor

@puzza007 puzza007 commented Aug 8, 2021

This is the start of a little sketch of how json stuff might look if you were to use the Reqwest json support. Feel free to close if the direction seems bad!

There are some things I'm not sure about here:

  1. serde/rustler has a different idea about what constitutes valid json to, say, jsx. Could this be confusing?
  2. I'm not sure the best to decode the response body into an Erlang term (passing the env into do_req?) or if that's a good idea in general? Maybe it could be a configuration item? Perhaps it doesn't mesh well with the maps-based interface.

@puzza007 puzza007 force-pushed the json branch 2 times, most recently from 4165f27 to e50907a Compare August 9, 2021 00:03
@dlesl
Copy link
Copy Markdown
Owner

dlesl commented Aug 9, 2021

Interesting! A few questions and thoughts:

  • How does the encoding used by serde_rustler differ from that used by jsx?
  • In the test case you are passing in #{json => jsx:encode(Json)} but I guess the plan would be eventually to pass in a json-encodable erlang term here?
  • It could be frustrating to have a json encoder/decoder available that can only be used via the HTTP client (for example there is no way to see how your data is being encoded without sending a request).
  • Because of this, to me it seems like it would be better to have the json encoding/decoding in a separate library.
  • However if we have the json encoder in a separate library we lose the advantage of being able to do the json encoding/decoding on a tokio thread and pass the data directly to reqwest (unless there is a way of passing data between two separate rust NIFs?).
  • Continuing the above thought, is it a good idea to do the json encoding/decoding on a tokio thread? AFAICT it would mean copying the erlang term to an OwnedEnv which might be slow. OTOH doing it on the BEAM thread would probably need some sort of yielding (like jiffy does) or using a "dirty NIF" (not sure of the implications of that) for bigger payloads.

So what I'm thinking is that to justify adding json support in rust we would have to be looking at a compelling performance advantage over what can be achieved by using jiffy + erqwest for example.

@puzza007
Copy link
Copy Markdown
Contributor Author

puzza007 commented Aug 9, 2021

Interesting! A few questions and thoughts:

* How does the encoding used by serde_rustler differ from that used by jsx?

An example is the term {this,is,weird,json}. Serde will turn this into [<<"this">>,<<"is">>,<<"weird">>,<<"json">>] but jsx gives a badarg.

* In the test case you are passing in `#{json => jsx:encode(Json)}` but I guess the plan would be eventually to pass in a json-encodable erlang term here?

Ah. Vestigial test data (though still valid! 😂). I'll push an update.

* It could be frustrating to have a json encoder/decoder available that can only be used via the HTTP client (for example there is no way to see how your data is being encoded without sending a request).

Yes definitely. Although thinking about how I use e.g. Requests, I basically don't have any other json encoding/decoding in my apps aside from the requests calls themselves.

* Because of this, to me it seems like it would be better to have the json encoding/decoding in a separate library.

Gotcha.

* However if we have the json encoder in a separate library we lose the advantage of being able to do the json encoding/decoding on a tokio thread and pass the data directly to reqwest (unless there is a way of passing data between two separate rust NIFs?).

Hmm interesting idea!

* Continuing the above thought, is it a good idea to do the json encoding/decoding on a tokio thread? AFAICT it would mean copying the erlang term to an `OwnedEnv` which might be slow. OTOH doing it on the BEAM thread would probably need some sort of yielding (like jiffy does) or using a "dirty NIF" (not sure of the implications of that) for bigger payloads.

So what I'm thinking is that to justify adding json support in rust we would have to be looking at a compelling performance advantage over what can be achieved by using jiffy + erqwest for example.

Cool. I'll try to flesh this PR out and perhaps come up with some benchmark scenarios 👍

@dlesl
Copy link
Copy Markdown
Owner

dlesl commented Aug 11, 2021

An example is the term {this,is,weird,json}. Serde will turn this into [<<"this">>,<<"is">>,<<"weird">>,<<"json">>] but jsx gives a badarg.

IMHO that's not too bad. Taking a look at serde_rustler it looks like null becomes nil, though. I wonder if serde_rustler's maintainers would be open to making that configurable?

Yes definitely. Although thinking about how I use e.g. Requests, I basically don't have any other json encoding/decoding in my apps aside from the requests calls themselves.

Yea from that perspective it could be a practical feature to offer. I wonder if there's a way we could follow reqwest's approach and offer features behind flags? Not sure if rebar3 supports that though...

@dlesl
Copy link
Copy Markdown
Owner

dlesl commented Aug 12, 2021

Had a go at feature flags in #11

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.

2 participants