RUST-1129 Make the dependency on `chrono` optional and provide equivalent `time` interop #352

abr-egn · 2022-04-05T20:13:51Z

This PR swaps out the internal operations that used chrono for equivalent ones using time, makes the chrono dependency optional, and adds optional interop methods for time structs.

abr-egn · 2022-04-05T20:37:50Z

src/datetime.rs

+        use std::result::Result;
+        match self.to_time().format(&Rfc3339) {
+            Result::Ok(s) => s,
+            Result::Err(e) => e.to_string(),


I'm not thrilled about handling the error this way but I couldn't think of a better option that wouldn't be a breaking change.

given that the chrono equivalent never errored, any idea what can lead this method to fail / how likely we are to encounter this?

From the source, the time method fails if the year is outside [0,9999]; the chrono equivalent doesn't have that check.

The time method has some other potential failure cases as well since it's a more general method, but for the specific purpose of formatting an OffsetDateTime I don't think they can happen.

hmmm, should we consider deprecating this method in favor of a new version that propagates the error correctly? seems like it might help cut down on the chances anyone hits this weirdness

The time crate actually has a large-dates flag which expands this to +-99,999, which we may want to consider enabling here, as MongoDB technically supports any date that can be represented as an i64 from the Unix epoch. Should we enable that flag, we'll want to let the maintainer know of our use case here so that the feature flag stays around in the future.

Also, rather than erroring, it may be more appropriate to panic here. Could we test some inputs that error in time and see if they panic in chrono? If they do it might be okay for us to panic here as well. Another option would be to cap the date to time's maximum value and use that for the date here. I think we do that in a few places already with chrono's maximum date. If none of those work, then I agree we probably should deprecate this method and introduce a new one, since I think including error information as a String could be pretty cumbersome or dangerous.

that solution SGTM. since we use the flag already it probably seems worth chiming in on that discussion linked above to let them know why

And as a last check, are we okay with the risk of breaking user code with this? It's technically possible that a user has a super large date in their DB somewhere that used to format okay but will panic now via this method or fail to serialize via the serde helper.

I think the risk is okay here - we're being loud about this change with the deprecation warning, and I think we should also call it out in the patch notes. It's also worth considering that the RFC itself is explicit about only applying to years in the 0000-9999 range and that the year field is fixed at four digits, so the chrono implementation is producing output that's probably not going to be accepted elsewhere.

That said, I'd be interested to hear if @kmahar or @isabelatkinson have other opinions.

Oh nice, I didn't know that about the RFC, that definitely makes this change seem fine to me then.

agree it seems fine 👍

abr-egn · 2022-04-05T20:39:48Z

src/extjson/models.rs

-                        })?
-                        .into();
-                Ok(crate::DateTime::from_chrono(datetime))
+                let datetime = crate::DateTime::parse_rfc3339_str(date.as_str()).map_err(|_| {


Here and elsewhere I changed it from the "parse into external datetime, convert to bson datetime" pattern to just directly using the methods bson::DateTime provides.

abr-egn · 2022-04-06T15:32:32Z

src/bson.rs

@@ -430,7 +437,7 @@ impl Bson {
                })
            }
            Bson::ObjectId(v) => json!({"$oid": v.to_hex()}),
-            Bson::DateTime(v) if v.timestamp_millis() >= 0 && v.to_chrono().year() <= 99999 => {
+            Bson::DateTime(v) if v.timestamp_millis() >= 0 && v.to_time().year() <= 99999 => {


I'm not sure if the year constraint here is correct - it's different than the one below.

from my reading of the extJSON spec we should drop a 9 here to match below... see datetime entries in this table.

I was wondering how we would pass the corresponding corpus test, but it looks like that test case does not have a relaxed extJSON version specified so we likely make no assertions on it -- maybe it's omitted because it's the same as the canonical, but IMO it seems like test runners should probably confirm their relaxed output in that case matches canonical. perhaps we should add an assertion to our corpus runner so that for a test case with no expected relaxed output, we check that the relaxed output is equal to the expected canonical output?

if that just works, I'd propose we file a DRIVERS ticket that suggests amending the corpus spec to require this assertion.

thoughts?

It looks like that doesn't just work, unfortunately. I filed RUST-1261 to follow up on that.

kmahar

looking good! just a few questions/thoughts

.evergreen/config.yml

kmahar · 2022-04-06T19:30:32Z

src/bson.rs

@@ -430,7 +437,7 @@ impl Bson {
                })
            }
            Bson::ObjectId(v) => json!({"$oid": v.to_hex()}),
-            Bson::DateTime(v) if v.timestamp_millis() >= 0 && v.to_chrono().year() <= 99999 => {
+            Bson::DateTime(v) if v.timestamp_millis() >= 0 && v.to_time().year() <= 99999 => {


from my reading of the extJSON spec we should drop a 9 here to match below... see datetime entries in this table.

I was wondering how we would pass the corresponding corpus test, but it looks like that test case does not have a relaxed extJSON version specified so we likely make no assertions on it -- maybe it's omitted because it's the same as the canonical, but IMO it seems like test runners should probably confirm their relaxed output in that case matches canonical. perhaps we should add an assertion to our corpus runner so that for a test case with no expected relaxed output, we check that the relaxed output is equal to the expected canonical output?

if that just works, I'd propose we file a DRIVERS ticket that suggests amending the corpus spec to require this assertion.

thoughts?

kmahar · 2022-04-07T17:02:29Z

src/datetime.rs

+        use std::result::Result;
+        match self.to_time().format(&Rfc3339) {
+            Result::Ok(s) => s,
+            Result::Err(e) => e.to_string(),


given that the chrono equivalent never errored, any idea what can lead this method to fail / how likely we are to encounter this?

patrickfreed · 2022-04-11T20:37:45Z

src/datetime.rs

+        use std::result::Result;
+        match self.to_time().format(&Rfc3339) {
+            Result::Ok(s) => s,
+            Result::Err(e) => e.to_string(),


The time crate actually has a large-dates flag which expands this to +-99,999, which we may want to consider enabling here, as MongoDB technically supports any date that can be represented as an i64 from the Unix epoch. Should we enable that flag, we'll want to let the maintainer know of our use case here so that the feature flag stays around in the future.

Also, rather than erroring, it may be more appropriate to panic here. Could we test some inputs that error in time and see if they panic in chrono? If they do it might be okay for us to panic here as well. Another option would be to cap the date to time's maximum value and use that for the date here. I think we do that in a few places already with chrono's maximum date. If none of those work, then I agree we probably should deprecate this method and introduce a new one, since I think including error information as a String could be pretty cumbersome or dangerous.

src/serde_helpers.rs

src/datetime.rs

patrickfreed · 2022-04-13T14:50:09Z

src/datetime.rs

+        use std::result::Result;
+        match self.to_time().format(&Rfc3339) {
+            Result::Ok(s) => s,
+            Result::Err(e) => e.to_string(),


And as a last check, are we okay with the risk of breaking user code with this? It's technically possible that a user has a super large date in their DB somewhere that used to format okay but will panic now via this method or fail to serialize via the serde helper.

isabelatkinson

lgtm mod patrick's renaming comment!

abr-egn added 8 commits April 5, 2022 10:34

wip

c8d9c92

code complete but test failing

37ff7a7

fix y10k handling

661df20

fix some tests

9fcefb0

fmt

f5d9e97

fix the other year check

eb2c905

clippy

c30968a

fmt again

3bbf48d

abr-egn commented Apr 5, 2022

View reviewed changes

abr-egn added 5 commits April 6, 2022 09:55

more robust y10k handling

df035a6

bump msrv to 1.51

49c4b68

rustfmt

42f4169

include time-0_3 and serde_with in tests

35771ee

use large-dates

b7febee

abr-egn commented Apr 6, 2022

View reviewed changes

msrv 1.53

d0e25f0

abr-egn marked this pull request as ready for review April 6, 2022 17:45

abr-egn requested review from patrickfreed, isabelatkinson and kmahar April 6, 2022 17:45

kmahar reviewed Apr 7, 2022

View reviewed changes

patrickfreed reviewed Apr 11, 2022

View reviewed changes

abr-egn added 2 commits April 12, 2022 13:18

include version in time helper module name

ea5cec2

deprecate to_rfc_3339_string in favor of a try_ variant

57b6cc5

kmahar approved these changes Apr 12, 2022

View reviewed changes

patrickfreed reviewed Apr 13, 2022

View reviewed changes

isabelatkinson approved these changes Apr 14, 2022

View reviewed changes

abr-egn added 2 commits April 14, 2022 15:48

qualify time methods

e797618

rustfmt

bee6ae5

patrickfreed approved these changes Apr 14, 2022

View reviewed changes

abr-egn merged commit b62338a into mongodb:main Apr 15, 2022

RUST-1129 Make the dependency on chrono optional and provide equivalent time interop #352

RUST-1129 Make the dependency on chrono optional and provide equivalent time interop #352

Uh oh!

Conversation

abr-egn commented Apr 5, 2022

Uh oh!

abr-egn Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

abr-egn Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

kmahar left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isabelatkinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RUST-1129 Make the dependency on `chrono` optional and provide equivalent `time` interop #352

RUST-1129 Make the dependency on `chrono` optional and provide equivalent `time` interop #352

abr-egn Apr 5, 2022 •

edited

Loading

abr-egn Apr 13, 2022 •

edited

Loading