RUST-1225 Add base64 string constructor to `Binary` #365

sanav33 · 2022-07-05T18:37:43Z

Added a Binary constructor that accepts a base64 string instead of the bytes themselves.

src/bson.rs

isabelatkinson · 2022-07-11T20:46:11Z

src/bson.rs

+    pub fn from_base64(
+        input: &str,
+        subtype: impl Into<Option<BinarySubtype>>,
+    ) -> Result<Self, DecodeError> {


We tend to avoid including types from other crates in our public API as they don't always have the same stability guarantees that we do. Under semantic versioning, any crate with a version that begins with 0 (e.g. 0.13.0, which is the version of the base64 crate) can make breaking changes at any time -- as an example, base64 could rename DecodeError to DecodingError, which would cause compilation failures for any code that relies upon the name being DecodeError. Because of this we'll want to avoid returning this type directly.

I looked over the various Error types we have defined in the BSON library and I'm not sure any of them are quite what we need for this situation. The closest would be the SerializationError under ser::Error, but that's more so used in the context of serializing to Bson. Alternatively, we could introduce a new Error type for this case.

Thoughts on this @patrickfreed @abr-egn @kmahar?

yeah, the error situation in the bson crate is really tough right now, since we don't have a single error type but rather have individual ones for each and every use case. The two closest precedents for a function like this are Uuid::parse_str and ObjectId::parse_str, and both use their respective type's error/result types, but it's unclear if we want to introduce yet another error type just for Binary values. It also doesn't seem like any of the existing ones match up very well here though.

We could consider introducing a general-purpose BsonError now, though it would only really be used for this purpose, and it might only add to the confusion. I kind of think we have to go with adding a new error case just for Binary values. In 3.0, I think we'll want to do a full redesign of the error hierarchy that eliminates a lot of the individual error types.

Agreed with Patrick on both the immediate thing to do and what we'll want to queue up for 3.0.

That sounds good to me. @sanav33 we can walk through what the new error type should look like, I think we'll want to add a binary module for it to live in to be consistent with our other error types. Also filed RUST-1406 to clean this up when we do a 3.0 release.

isabelatkinson

just a few small comments! looks like there are also some lint failures

isabelatkinson · 2022-08-16T18:51:25Z

src/binary/error.rs

+#[non_exhaustive]
+pub enum Error {
+    /// While trying to decode from base64, an error was returned.
+    DecodingError,


I think it would be helpful to include some additional information here about what went wrong during decoding, maybe a message field like InvalidUuidString here that stores the stringified version of the base64 error?

isabelatkinson · 2022-08-16T18:57:53Z

src/binary.rs

@@ -0,0 +1,81 @@
+pub mod error;


I think we should either move the stuff in this module directly into this file or make this pub(crate) and add a pub use error::{Error, Result} to be consistent with other types. For example, uuid's Error and Result types are contained in the uuid module rather than within a nested error module.

isabelatkinson

looks like there are still some lint errors

sanav33 · 2022-08-18T14:33:29Z

looks like there are still some lint errors

It looks like those failures are from an unrelated PR (#258). Should I go ahead and fix them anyway? @isabelatkinson

isabelatkinson · 2022-08-18T19:45:00Z

hmm you probably need to rebase this branch on main, it looks like #366 went in after this PR was created

sanav33 · 2022-08-19T00:24:26Z

I might be wrong but I think I rebased before the last push to this PR and it looks like all the changes from #366 are reflected in this PR.

isabelatkinson · 2022-08-19T16:04:23Z

ohh I see what the issue is, chrono must've just released a new version that deprecated some constants. I needed to run cargo update to see the lint errors locally. don't worry about those in this PR, I'll make a separate one to fix.

abr-egn

LGTM!

patrickfreed

A few minor comments, sorry for not getting them in before Sana left. Do you want to take this over @isabelatkinson?

patrickfreed · 2022-08-22T22:33:28Z

src/binary.rs

+    /// Creates a [`Binary`] from a base64 string and optional [`BinarySubtype`]. If the
+    /// `subtype` argument is `None`, the [`Binary`] constructed will default to
+    /// [`BinarySubtype::Generic`].
+    pub fn from_base64(input: &str, subtype: impl Into<Option<BinarySubtype>>) -> Result<Self> {


this &str can be an impl AsRef<str>

patrickfreed · 2022-08-22T22:33:46Z

src/binary.rs

+impl Binary {
+    /// Creates a [`Binary`] from a base64 string and optional [`BinarySubtype`]. If the
+    /// `subtype` argument is `None`, the [`Binary`] constructed will default to
+    /// [`BinarySubtype::Generic`].


I think it would be good to have a doc example here. I think the test we added for this would actually be pretty good.

isabelatkinson · 2022-11-29T17:15:50Z

there's an unrelated clippy failure -- going to fix in a separate PR

sanav33 requested a review from isabelatkinson July 5, 2022 18:37

isabelatkinson reviewed Jul 8, 2022

View reviewed changes

src/bson.rs Outdated Show resolved Hide resolved

src/bson.rs Outdated Show resolved Hide resolved

sanav33 requested a review from isabelatkinson July 11, 2022 18:55

isabelatkinson reviewed Jul 11, 2022

View reviewed changes

sanav33 force-pushed the RUST-1225/add-base64-to-binary-method branch from f896bf9 to ced93a7 Compare August 15, 2022 16:07

sanav33 requested a review from isabelatkinson August 15, 2022 16:07

isabelatkinson reviewed Aug 16, 2022

View reviewed changes

isabelatkinson reviewed Aug 18, 2022

View reviewed changes

isabelatkinson approved these changes Aug 19, 2022

View reviewed changes

isabelatkinson requested review from patrickfreed and abr-egn August 19, 2022 16:06

abr-egn approved these changes Aug 19, 2022

View reviewed changes

patrickfreed reviewed Aug 22, 2022

View reviewed changes

isabelatkinson changed the title ~~RUST-1225 Add base64 string to Binary constructor~~ RUST-1225 Add base64 string constructor to Binary Nov 29, 2022

sanav33 and others added 2 commits November 29, 2022 10:25

RUST-1225 Add base64 string constructor to Binary

9e48664

cleanup

88943fb

isabelatkinson force-pushed the RUST-1225/add-base64-to-binary-method branch from cecdc38 to 88943fb Compare November 29, 2022 17:02

isabelatkinson requested a review from patrickfreed November 29, 2022 17:15

patrickfreed approved these changes Nov 30, 2022

View reviewed changes

isabelatkinson merged commit 5e0bea6 into mongodb:main Nov 30, 2022

RUST-1225 Add base64 string constructor to Binary #365

RUST-1225 Add base64 string constructor to Binary #365

Uh oh!

Conversation

sanav33 commented Jul 5, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isabelatkinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isabelatkinson left a comment

Choose a reason for hiding this comment

Uh oh!

sanav33 commented Aug 18, 2022

Uh oh!

isabelatkinson commented Aug 18, 2022

Uh oh!

sanav33 commented Aug 19, 2022

Uh oh!

isabelatkinson commented Aug 19, 2022

Uh oh!

abr-egn left a comment

Choose a reason for hiding this comment

Uh oh!

patrickfreed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isabelatkinson commented Nov 29, 2022

Uh oh!

Uh oh!

RUST-1225 Add base64 string constructor to `Binary` #365

RUST-1225 Add base64 string constructor to `Binary` #365