Skip to content

allow send_json to send any serde::Serialize value #446

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

Merged
merged 2 commits into from
Dec 17, 2021

Conversation

soruh
Copy link
Contributor

@soruh soruh commented Dec 11, 2021

I am currently writing a project using ureq with the json feature enabled and noticed the following which confused me while trying to understand the API:

  • send_json does not allow users to send any serde::Serialize-able data and instead requires converting data into a serde_json::Value before sending. This is inefficient, since data that could be serialize immediately needs to be converted into an intermediate in-memory JSON format instead.
  • the API uses renamed versions of the serde_json types it uses

This PR:

  • changes the signature of send_json to allow for any type implementing serde::Serialize (including serde_json::Value) to be sent. ¹
  • replaces all uses of the renamed serde_json types in the API with the proper names from serde_json
  • deprecates the renamed types from serde_json
  • re-exports the used versions of serde_json and serde

¹ this is achieved by serializing the data into a Vec<u8> in send_json and passing that through Payload::Bytes.
This has the effect that the crate internal Debug implementation now prints the bytes of the json data instead. It would be possible to use Payload::Text with the "utf8" encoding instead, but the risk of encoding errors seems to outweight the minor benefit in debugging to me.

as they are just wrapper around serde_json::{Value, Map, to_value}.
Instead expose the serde_json and serde crates used.
@algesten
Copy link
Owner

Hi @soruh! Welcome to ureq!

While I agree that impl serde::Serialize is more elegant, changing this would be a breaking change, and thus require a major version bump.

Right now there is no major bump planned, but I would like to save this PR until such time that we have a bunch of breaking changes.

Thanks!

@soruh
Copy link
Contributor Author

soruh commented Dec 11, 2021

I'm not 100% sure why this would be a breaking change since this will still work with serde_json::Values (since they implement serde::Serialize).

If it really is a breaking change I agree that it's not worth a major version bump on it's own.

@soruh
Copy link
Contributor Author

soruh commented Dec 11, 2021

Ah, I guess if someone is passing Request::send_json into a function taking a fn(Request, serde_json::Value) -> Response it would break.

@algesten
Copy link
Owner

Ah. True.serde::Value obviously implements serde::Serialize hm hm, didn't think about that.

That makes it less a breaking change.

@jsha thoughts?

@jsha
Copy link
Collaborator

jsha commented Dec 17, 2021

I'm definitely in favor of accepting the more general serde::Serialize. I think given that serde_json::Value impls Serialize, it's reasonably non-breaking, although @soruh makes a good point that it is conceivably possible we could break some (unlikely) code. I think I'm okay with not bumping the major version number for that.

One way to sidestep and not worry about even that minor risk of breakage would be to make a new method send_serializable with the wider bound. And then in some future major version update we could make send_json an alias of that.

Interesting blog post about breakage budgets here: https://alexgaynor.net/2021/oct/07/whats-in-a-version-number/. Also, the Rust for Rustaceans book is released and has some nice advice about what's a breaking change in Rust.

@algesten
Copy link
Owner

I agree with all this. Let's take a punt on this not upsetting any users.

@algesten algesten merged commit 61e2402 into algesten:main Dec 17, 2021
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.

None yet

3 participants