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

Rusty byte strings in RON, deprecate base64 (byte) strings #438

Merged
merged 25 commits into from
Sep 1, 2023

Conversation

juntyr
Copy link
Member

@juntyr juntyr commented Feb 6, 2023

Breaking: Rusty byte strings in RON, deprecate base64 (byte) strings

ron has for now encoded byte-slices and byte-bufs as base64-encoded strings. While this works perfectly fine when ron deserialises a document using type hints (i.e. when Deserializer::deserialize_byte_buf or Deserializer::deserialize_bytes are called), #436 has shown that it breaks down when ron is asked to deserialise bytes in a self-describing document. This issue occurs when Deserializer::deserialize_any is called, for instance, if the byte string is inside a #[serde(untagged)] enum.

This issue presents an opportunity to transition ron from using ambiguous base64 (byte) strings to using Rusty byte strings. For instance, the Rust value b"Hello ron!" is now encoded as the equivalent ron value b"Hello ron!" (instead of "SGVsbG8gcm9uIQ==" as before). ron's new Rusty byte strings follow the updated escape rules for Rust's byte strings from rust-lang/rfcs#3349 (assuming that the RFC will be accepted).

Request for Comment

As this is a breaking change in ron's format, that will affect user's of ron over a transition from v0.9 to v0.10 (see below), we would like to hear your thoughts on the transition and how to make it easier on you before we merge this PR.

What changes in v0.9?

Serialising

In the next breaking release of ron, v0.9, all calls to Serializer::serialize_bytes will output Rusty byte strings instead of base64-encoded strings.

Deserialising

In the next breaking release of ron, v0.9, all calls to Deserializer::deserialize_byte_buf and Deserializer::deserialize_bytes will accept both Rusty byte strings and base64-encoded (byte) strings. ron's Error::Base64Error variant will be deprecated, and will be removed in v0.10.

Value

ron's Value type gains a new Value::Bytes(Vec<u8>) variant to store an owned byte buffer.

What changes in v0.10?

One breaking release after the introduction of Rusty byte strings, base64-encoded (byte) strings will no longer be accepted during deserialising. The Error::Base64Error variant will be removed. However, ron may still produce non-generic error messages when deserialising a byte string and coming across a normal string that looks like it may be base64-encoded.

How can I continue using base64-encoded (byte) strings?

If you would like to retain the original behaviour and continue to use (now strongly typed) base64 strings, you will need to change the Serialize implementation of your byte value, e.g. using one of the following crates (non-exhaustive and without endorsement from ron):

With this PR, we have also provided a new example (examples/base64.rs) of continuing to use base64-encoded bytes with ron. In this example, two serde helpers are implemented which showcase

  1. how to serialise and deserialise base64-encoded (byte) strings in a strongly typed way that avoids RON incorrectly adds extra level of Base64 when roundtripping Bytes #436
  2. how to accept both Rusty byte strings and the legacy base64-encoded (byte) strings during deserialising

Final notes

Fixes #436

TODOs

  • Update the grammar
  • Document the extension
  • Add tests for the new Value behaviour
  • Add a test to document the v0.10 deprecation error while deserialising
  • Add tests to document the v0.9 optional base64 deserialising
  • Add a test to document a datatype-side workaround for only base64
  • Add a test to document a datatype-side workaround for base64 or Rusty byte strings
  • I've included my change in CHANGELOG.md
  • Add more code coverage tests for parse.rs, error.rs coverage can be handled separately
  • Change the example link to the main repo before merging

@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2023

Codecov Report

Patch coverage: 99.88% and project coverage change: +0.76% 🎉

Comparison is base (bc723e1) 95.33% compared to head (9e10d96) 96.10%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #438      +/-   ##
==========================================
+ Coverage   95.33%   96.10%   +0.76%     
==========================================
  Files          75       77       +2     
  Lines        9054     9868     +814     
==========================================
+ Hits         8632     9484     +852     
+ Misses        422      384      -38     
Files Changed Coverage Δ
src/parse.rs 98.21% <99.54%> (+1.44%) ⬆️
src/de/mod.rs 96.88% <100.00%> (+0.29%) ⬆️
src/de/tests.rs 100.00% <100.00%> (ø)
src/de/value.rs 97.75% <100.00%> (+3.84%) ⬆️
src/error.rs 46.09% <100.00%> (+3.86%) ⬆️
src/ser/mod.rs 96.73% <100.00%> (+0.12%) ⬆️
src/ser/tests.rs 100.00% <100.00%> (ø)
src/ser/value.rs 100.00% <100.00%> (+63.63%) ⬆️
src/value/mod.rs 94.96% <100.00%> (+1.12%) ⬆️
tests/407_raw_value.rs 100.00% <100.00%> (ø)
... and 7 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@juntyr

This comment was marked as outdated.

@juntyr juntyr mentioned this pull request Aug 18, 2023
1 task
@juntyr juntyr mentioned this pull request Aug 18, 2023
17 tasks
@juntyr juntyr changed the title [v0.9] Switch from base64 to rusty byte strings, deprecate base64 support Rusty byte strings in RON, deprecate base64 (byte) strings Aug 18, 2023
@juntyr juntyr marked this pull request as ready for review August 18, 2023 22:10
@juntyr juntyr self-assigned this Aug 18, 2023
@juntyr juntyr force-pushed the rusty-byte-str branch 4 times, most recently from 5a4b2c7 to bfcb50c Compare August 22, 2023 14:23
src/parse.rs Outdated Show resolved Hide resolved
@bestouff
Copy link

I don't think you should ever remove the base64 deserialization code. Some people probably have RON files stored somewhere that they can't easily modify, and they need them to work.

@juntyr
Copy link
Member Author

juntyr commented Aug 24, 2023

I don't think you should ever remove the base64 deserialization code. Some people probably have RON files stored somewhere that they can't easily modify, and they need them to work.

Thank you for your feedback @bestouff! Do you think the one of the above-described workarounds of moving the base64 encoding to the data type definition would work in your case? If not, I hope we can figure out a solution for your case.

@juntyr
Copy link
Member Author

juntyr commented Aug 31, 2023

Since no additional concerns have been voiced so far, I will merge this PR over the coming days.

@bestouff I would like to ensure that your concern about backwards compatibility is sufficiently resolved before base64 deserialisation is fully disabled (in v0.10 or later if necessary). Do you think that one of the above-described workarounds of moving the base64 encoding to the data type definition would work in your case?

@juntyr juntyr merged commit ba66c8e into ron-rs:master Sep 1, 2023
8 checks passed
@juntyr juntyr deleted the rusty-byte-str branch September 1, 2023 19:13
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.

RON incorrectly adds extra level of Base64 when roundtripping Bytes
3 participants