edgedb / edgedb-rust

The official Rust binding for EdgeDB
https://edgedb.com
Apache License 2.0
214 stars 26 forks source link

Datetime.to_unix_micros returns negative #267

Closed hongquan closed 1 year ago

hongquan commented 1 year ago

Describe the bug

If we have a datetime at 2000-01-01, the edgedb_protocol::model::Datetime..to_unix_micros should return how many microseconds since 1970-01-01, it must be positive number. But currently, the function is returning negative.

Reproduction Include the code that is causing the error:

fn main() {
    println!("{}", edgedb_protocol::model::Datetime::MIN.to_unix_micros())
}

Expected behavior

Should get 64028966400000000.

Versions (please complete the following information):

Additional context Add any other context about the problem here.

scotttrinh commented 1 year ago

In your example, you're printing the microseconds since the unix epoch of the minimum date allowed, which is before 1970: https://github.com/edgedb/edgedb-rust/blob/72eec47813fdfd2dace98be5f959c15f45ca3f19/edgedb-protocol/src/model/time.rs#L601C65-L601C75

What number is it giving you for the date 2000-01-01 in your actual query?

hongquan commented 1 year ago

I changed the code to:

let now = SystemTime::now();
let dt = EDatetime::try_from(now).map_err(|_e| miette!("Error converting SystemTime to EDatetime"))?;
tracing::info!("Now to unix micros: {}", dt.to_unix_micros());

And got:

Now to unix micros: -204795426235311

(still negative number)

hongquan commented 1 year ago

Could you take a look @Dhghomon ?

Dhghomon commented 1 year ago

@hongquan Sure. Looks like the 30-year duration from 2000 to 1970 is being subtracted twice, once when creating the Datetime and again when calling the from_unix_micros() method. Here is where the subtraction is being done (here UNIX_EPOCH.micros is equal to -946684800000000):

fn _from_micros(micros: i64) -> Option<Datetime> {
    let micros = micros.checked_add(Self::UNIX_EPOCH.micros)?;
    if micros < Self::MIN.micros || micros > Self::MAX.micros {
        return None;
    }
    Some(Datetime { micros })
}

And then .to_unix_micros() adds this negative -946684800000000 again:

self.micros + Datetime::UNIX_EPOCH.micros

A quick test shows that:

[test]

fn test_if_doubled() { let micros = 0; let datetime = Datetime::from_unix_micros(micros); let doubled = datetime.to_micros() + datetime.to_micros(); assert_eq!(doubled, datetime.to_unix_micros()); }

So looks like simply changing + to - should do the trick. After that this test I've added passes:

    #[test]
    #[allow(deprecated)]
    fn to_and_from_unix_micros_roundtrip() {
        let zero_micros = 0;
        let datetime = Datetime::from_unix_micros(0);
        // Unix micros should equal 0
        assert_eq!(zero_micros, datetime.to_unix_micros());
        // Datetime (Postgres epoch-based) micros should be negative
        // Micros = negative micros to go from 2000 to 1970
        assert_eq!(datetime.micros, datetime.to_micros());
        assert_eq!(datetime.micros, Datetime::UNIX_EPOCH.micros);
        assert_eq!(datetime.micros, -946684800000000);
    }
Dhghomon commented 1 year ago

Will close as the fix for this has been merged now.