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.

Sign up for GitHub

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

Jump to bottom

fix(timestamp conversion): use timestamp_nanos_opt to avoid panics #979

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

fix(timestamp conversion): use timestamp_nanos_opt to avoid panics #979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ab9b810
fix(timestamp conversion): use timestamp_nanos_opt to avoid panics wh…
pront Aug 6, 2024
f253ce8
changelog
pront Aug 6, 2024
e71bd86
cleanup
pront Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/979.fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Replaced all usages of `timestamp_nanos` with `timestamp_nanos_opt` to avoid panics when the timestamp is out of range.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Replaced all usages of `timestamp_nanos` with `timestamp_nanos_opt` to avoid panics when the timestamp is out of range.
`to_unix_timestamp`, `to_float`, and `uuid_v7` can now return an error if the supplied timestamp is unrepresentable as a nanosecond timestamp. Previously the function calls would panic.

The changelogs should have the user in mind as the audience. I also think this is a breaking change, no? We'll want to document how to migrate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, marking this PR as a draft for now. I will try to find some propose a migration plan. I need to refresh my memory on the deprecation process as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We provide linters for such changes. In this way we have eliminated the deprecation of to_timestamp.
I think there is no problem in this case, because the vector during validation will report that to_unix_timestamp returns an error, and this is not handled

8 changes: 6 additions & 2 deletions src/compiler/value/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::Kind;
use crate::compiler::ExpressionError;
use crate::diagnostic::DiagnosticMessage;

use super::Kind;
use crate::prelude::ValueError::OutOfRange;

#[derive(thiserror::Error, Debug, PartialEq, Eq)]
pub enum ValueError {
Expand Down Expand Up @@ -55,6 +55,9 @@ pub enum ValueError {

#[error("can't merge type {1} into {0}")]
Merge(Kind, Kind),

#[error("can't convert out of range {0}")]
OutOfRange(Kind),
}

impl DiagnosticMessage for ValueError {
Expand All @@ -81,6 +84,7 @@ impl DiagnosticMessage for ValueError {
Lt(..) => 313,
Le(..) => 314,
Merge(..) => 315,
OutOfRange(..) => 316,
}
}
}
Expand Down
10 changes: 7 additions & 3 deletions src/stdlib/to_float.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,13 @@ fn to_float(value: Value) -> Resolved {
Integer(v) => Ok(Value::from_f64_or_zero(v as f64)),
Boolean(v) => Ok(NotNan::new(if v { 1.0 } else { 0.0 }).unwrap().into()),
Null => Ok(NotNan::new(0.0).unwrap().into()),
Timestamp(v) => Ok(Value::from_f64_or_zero(
v.timestamp_nanos() as f64 / 1_000_000_000_f64,
)),
Timestamp(v) => {
let nanoseconds = match v.timestamp_nanos_opt() {
Some(nanos) => nanos as f64,
None => return Err(ValueError::OutOfRange(Kind::timestamp()).into()),
};
Ok(Value::from_f64_or_zero(nanoseconds / 1_000_000_000_f64))
}
Bytes(v) => Conversion::Float
.convert(v)
.map_err(|e| e.to_string().into()),
Expand Down
12 changes: 11 additions & 1 deletion src/stdlib/to_unix_timestamp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ fn to_unix_timestamp(value: Value, unit: Unit) -> Resolved {
Unit::Seconds => ts.timestamp(),
Unit::Milliseconds => ts.timestamp_millis(),
Unit::Microseconds => ts.timestamp_micros(),
Unit::Nanoseconds => ts.timestamp_nanos(),
Unit::Nanoseconds => match ts.timestamp_nanos_opt() {
None => return Err(ValueError::OutOfRange(Kind::timestamp()).into()),
Some(nanos) => nanos,
},
};
Ok(time.into())
}
Expand Down Expand Up @@ -186,5 +189,12 @@ mod test {
want: Ok(1_609_459_200_000_000_000_i64),
tdef: TypeDef::integer().infallible(),
}
out_of_range {
args: func_args![value: chrono::Utc.ymd(0, 1, 1).and_hms_milli(0, 0, 0, 0),
unit: "nanoseconds"
],
want: Err("can't convert out of range timestamp"),
tdef: TypeDef::integer().infallible(),
}
];
}
17 changes: 10 additions & 7 deletions src/stdlib/uuid_v7.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,25 @@ use bytes::Bytes;
use chrono::{DateTime, Utc};
use uuid::{timestamp::Timestamp, NoContext};

fn uuid_v7(timestamp: Option<Value>) -> Value {
let timestamp: DateTime<Utc> = if let Some(timestamp) = timestamp {
timestamp.try_timestamp().unwrap()
fn uuid_v7(timestamp: Option<Value>) -> Resolved {
let utc_timestamp: DateTime<Utc> = if let Some(timestamp) = timestamp {
timestamp.try_timestamp()?
} else {
Utc::now()
};

let seconds = timestamp.timestamp() as u64;
let nanoseconds = timestamp.timestamp_nanos() as u32;
let seconds = utc_timestamp.timestamp() as u64;
let nanoseconds = match utc_timestamp.timestamp_nanos_opt() {
Some(nanos) => nanos as u32,
None => return Err(ValueError::OutOfRange(Kind::timestamp()).into()),
};
let timestamp = Timestamp::from_unix(NoContext, seconds, nanoseconds);

let mut buffer = [0; 36];
let uuid = uuid::Uuid::new_v7(timestamp)
.hyphenated()
.encode_lower(&mut buffer);
Bytes::copy_from_slice(uuid.as_bytes()).into()
Ok(Bytes::copy_from_slice(uuid.as_bytes()).into())
}

#[derive(Clone, Copy, Debug)]
Expand Down Expand Up @@ -77,7 +80,7 @@ impl FunctionExpression for UuidV7Fn {
.map(|m| m.resolve(ctx))
.transpose()?;

Ok(uuid_v7(timestamp))
uuid_v7(timestamp)
}

fn type_def(&self, _: &TypeState) -> TypeDef {
Expand Down
Loading