Skip to content

Commit da33cde

Browse files
abonanderabonander
committed
feat: add MySqlTime, audit mysql::types for panics
Also clarifies the handling of `TIME` (we never realized it's used for both time-of-day and signed intervals) and adds appropriate impls for `std::time::Duration`, `time::Duration`, `chrono::TimeDelta`
1 parent 59d5ba6 commit da33cde

File tree

9 files changed

+1008
-86
lines changed

9 files changed

+1008
-86
lines changed

‎sqlx-mysql/src/protocol/statement/row.rs‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,20 @@ impl<'de> Decode<'de, &'de [MySqlColumn]> for BinaryRow {
4545
// NOTE: MySQL will never generate NULL types for non-NULL values
4646
let type_info = &column.type_info;
4747

48+
// Unlike Postgres, MySQL does not length-prefix every value in a binary row.
49+
// Values are *either* fixed-length or length-prefixed,
50+
// so we need to inspect the type code to be sure.
4851
let size: usize = match type_info.r#type {
52+
// All fixed-length types.
53+
ColumnType::LongLong => 8,
54+
ColumnType::Long | ColumnType::Int24 => 4,
55+
ColumnType::Short | ColumnType::Year => 2,
56+
ColumnType::Tiny => 1,
57+
ColumnType::Float => 4,
58+
ColumnType::Double => 8,
59+
60+
// Blobs and strings are prefixed with their length,
61+
// which is itself a length-encoded integer.
4962
ColumnType::String
5063
| ColumnType::VarChar
5164
| ColumnType::VarString
@@ -61,20 +74,15 @@ impl<'de> Decode<'de, &'de [MySqlColumn]> for BinaryRow {
6174
| ColumnType::Json
6275
| ColumnType::NewDecimal => buf.get_uint_lenenc() as usize,
6376

64-
ColumnType::LongLong => 8,
65-
ColumnType::Long | ColumnType::Int24 => 4,
66-
ColumnType::Short | ColumnType::Year => 2,
67-
ColumnType::Tiny => 1,
68-
ColumnType::Float => 4,
69-
ColumnType::Double => 8,
70-
77+
// Like strings and blobs, these values are variable-length.
78+
// Unlike strings and blobs, however, they exclusively use one byte for length.
7179
ColumnType::Time
7280
| ColumnType::Timestamp
7381
| ColumnType::Date
7482
| ColumnType::Datetime => {
75-
// The size of this type is important for decoding
83+
// Leave the length byte on the front of the value because decoding uses it.
7684
buf[0] as usize + 1
77-
}
85+
},
7886

7987
// NOTE: MySQL will never generate NULL types for non-NULL values
8088
ColumnType::Null => unreachable!(),

‎sqlx-mysql/src/types/chrono.rs‎

Lines changed: 73 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,13 @@
11
use bytes::Buf;
2-
use chrono::{
3-
DateTime, Datelike, Local, NaiveDate, NaiveDateTime, NaiveTime, TimeZone, Timelike, Utc,
4-
};
2+
use chrono::{DateTime, Datelike, Local, NaiveDate, NaiveDateTime, NaiveTime, TimeZone, Timelike, Utc};
3+
use sqlx_core::database::Database;
54

65
use crate::decode::Decode;
76
use crate::encode::{Encode, IsNull};
87
use crate::error::{BoxDynError, UnexpectedNullError};
98
use crate::protocol::text::ColumnType;
109
use crate::type_info::MySqlTypeInfo;
11-
use crate::types::Type;
10+
use crate::types::{MySqlTime, MySqlTimeSign, Type};
1211
use crate::{MySql, MySqlValueFormat, MySqlValueRef};
1312

1413
impl Type<MySql> for DateTime<Utc> {
@@ -63,7 +62,7 @@ impl<'r> Decode<'r, MySql> for DateTime<Local> {
6362

6463
impl Type<MySql> for NaiveTime {
6564
fn type_info() -> MySqlTypeInfo {
66-
MySqlTypeInfo::binary(ColumnType::Time)
65+
MySqlTime::type_info()
6766
}
6867
}
6968

@@ -75,7 +74,7 @@ impl Encode<'_, MySql> for NaiveTime {
7574
// NaiveTime is not negative
7675
buf.push(0);
7776

78-
// "date on 4 bytes little-endian format" (?)
77+
// Number of days in the interval; always 0 for time-of-day values.
7978
// https://mariadb.com/kb/en/resultset-row/#teimstamp-binary-encoding
8079
buf.extend_from_slice(&[0_u8; 4]);
8180

@@ -95,34 +94,18 @@ impl Encode<'_, MySql> for NaiveTime {
9594
}
9695
}
9796

97+
/// Decode from a `TIME` value.
98+
///
99+
/// ### Errors
100+
/// Returns an error if the `TIME` value is negative or exceeds `23:59:59.999999`.
98101
impl<'r> Decode<'r, MySql> for NaiveTime {
99102
fn decode(value: MySqlValueRef<'r>) -> Result<Self, BoxDynError> {
100103
match value.format() {
101104
MySqlValueFormat::Binary => {
102-
let mut buf = value.as_bytes()?;
103-
104-
// data length, expecting 8 or 12 (fractional seconds)
105-
let len = buf.get_u8();
106-
107-
// MySQL specifies that if all of hours, minutes, seconds, microseconds
108-
// are 0 then the length is 0 and no further data is send
109-
// https://dev.mysql.com/doc/internals/en/binary-protocol-value.html
110-
if len == 0 {
111-
return Ok(NaiveTime::from_hms_micro_opt(0, 0, 0, 0)
112-
.expect("expected NaiveTime to construct from all zeroes"));
113-
}
114-
115-
// is negative : int<1>
116-
let is_negative = buf.get_u8();
117-
debug_assert_eq!(is_negative, 0, "Negative dates/times are not supported");
118-
119-
// "date on 4 bytes little-endian format" (?)
120-
// https://mariadb.com/kb/en/resultset-row/#timestamp-binary-encoding
121-
buf.advance(4);
122-
123-
decode_time(len - 5, buf)
105+
// Covers most possible failure modes.
106+
MySqlTime::decode(value)?.try_into()
124107
}
125-
108+
// Retaining this parsing for now as it allows us to cross-check our impl.
126109
MySqlValueFormat::Text => {
127110
let s = value.as_str()?;
128111
NaiveTime::parse_from_str(s, "%H:%M:%S%.f").map_err(Into::into)
@@ -131,6 +114,55 @@ impl<'r> Decode<'r, MySql> for NaiveTime {
131114
}
132115
}
133116

117+
impl TryFrom<MySqlTime> for NaiveTime {
118+
type Error = BoxDynError;
119+
120+
fn try_from(time: MySqlTime) -> Result<Self, Self::Error> {
121+
NaiveTime::from_hms_micro_opt(
122+
time.hours(),
123+
time.minutes() as u32,
124+
time.seconds() as u32,
125+
time.microseconds(),
126+
)
127+
.ok_or_else(|| format!("Cannot convert `MySqlTime` value to `NaiveTime`: {time}").into())
128+
}
129+
}
130+
131+
impl From<MySqlTime> for chrono::TimeDelta {
132+
fn from(time: MySqlTime) -> Self {
133+
chrono::TimeDelta::new(time.whole_seconds_signed(), time.subsec_nanos())
134+
.expect("BUG: chrono::TimeDelta should have a greater range than MySqlTime")
135+
}
136+
}
137+
138+
impl TryFrom<chrono::TimeDelta> for MySqlTime {
139+
type Error = BoxDynError;
140+
141+
fn try_from(value: chrono::TimeDelta) -> Result<Self, Self::Error> {
142+
let sign = if value < chrono::TimeDelta::zero() { MySqlTimeSign::Negative } else {
143+
MySqlTimeSign::Positive
144+
};
145+
146+
Ok(
147+
// `std::time::Duration` has a greater positive range than `TimeDelta`
148+
// which makes it a great intermediate if you ignore the sign.
149+
MySqlTime::try_from(value.abs().to_std()?)?.with_sign(sign)
150+
)
151+
}
152+
}
153+
154+
impl Type<MySql> for chrono::TimeDelta {
155+
fn type_info() -> MySqlTypeInfo {
156+
MySqlTime::type_info()
157+
}
158+
}
159+
160+
impl<'r> Decode<'r, MySql> for chrono::TimeDelta {
161+
fn decode(value: <MySql as Database>::ValueRef<'r>) -> Result<Self, BoxDynError> {
162+
Ok(MySqlTime::decode(value)?.into())
163+
}
164+
}
165+
134166
impl Type<MySql> for NaiveDate {
135167
fn type_info() -> MySqlTypeInfo {
136168
MySqlTypeInfo::binary(ColumnType::Date)
@@ -155,7 +187,14 @@ impl<'r> Decode<'r, MySql> for NaiveDate {
155187
fn decode(value: MySqlValueRef<'r>) -> Result<Self, BoxDynError> {
156188
match value.format() {
157189
MySqlValueFormat::Binary => {
158-
decode_date(&value.as_bytes()?[1..])?.ok_or_else(|| UnexpectedNullError.into())
190+
let buf = value.as_bytes()?;
191+
192+
// Row decoding should have left the length prefix.
193+
if buf.is_empty() {
194+
return Err("empty buffer".into());
195+
}
196+
197+
decode_date(&buf[1..])?.ok_or_else(|| UnexpectedNullError.into())
159198
}
160199

161200
MySqlValueFormat::Text => {
@@ -214,6 +253,10 @@ impl<'r> Decode<'r, MySql> for NaiveDateTime {
214253
MySqlValueFormat::Binary => {
215254
let buf = value.as_bytes()?;
216255

256+
if buf.is_empty() {
257+
return Err("empty buffer".into());
258+
}
259+
217260
let len = buf[0];
218261
let date = decode_date(&buf[1..])?.ok_or(UnexpectedNullError)?;
219262

‎sqlx-mysql/src/types/float.rs‎

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ use crate::types::Type;
88
use crate::{MySql, MySqlTypeInfo, MySqlValueFormat, MySqlValueRef};
99

1010
fn real_compatible(ty: &MySqlTypeInfo) -> bool {
11+
// NOTE: `DECIMAL` is explicitly excluded because floating-point numbers have different semantics.
1112
matches!(ty.r#type, ColumnType::Float | ColumnType::Double)
1213
}
1314

@@ -53,12 +54,22 @@ impl Decode<'_, MySql> for f32 {
5354
MySqlValueFormat::Binary => {
5455
let buf = value.as_bytes()?;
5556

56-
if buf.len() == 8 {
57+
match buf.len() {
58+
// These functions panic if `buf` is not exactly the right size.
59+
4 => LittleEndian::read_f32(buf),
5760
// MySQL can return 8-byte DOUBLE values for a FLOAT
58-
// We take and truncate to f32 as that's the same behavior as *in* MySQL
59-
LittleEndian::read_f64(buf) as f32
60-
} else {
61-
LittleEndian::read_f32(buf)
61+
// We take and truncate to f32 as that's the same behavior as *in* MySQL,
62+
8 => LittleEndian::read_f64(buf) as f32,
63+
other => {
64+
// Users may try to decode a DECIMAL as floating point;
65+
// inform them why that's a bad idea.
66+
return Err(format!(
67+
"expected a FLOAT as 4 or 8 bytes, got {other} bytes; \
68+
note that decoding DECIMAL as `f32` is not supported \
69+
due to differing semantics"
70+
)
71+
.into());
72+
}
6273
}
6374
}
6475

@@ -70,7 +81,26 @@ impl Decode<'_, MySql> for f32 {
7081
impl Decode<'_, MySql> for f64 {
7182
fn decode(value: MySqlValueRef<'_>) -> Result<Self, BoxDynError> {
7283
Ok(match value.format() {
73-
MySqlValueFormat::Binary => LittleEndian::read_f64(value.as_bytes()?),
84+
MySqlValueFormat::Binary => {
85+
let buf = value.as_bytes()?;
86+
87+
// The `read_*` functions panic if `buf` is not exactly the right size.
88+
match buf.len() {
89+
// Allow implicit widening here
90+
4 => LittleEndian::read_f32(buf) as f64,
91+
8 => LittleEndian::read_f64(buf),
92+
other => {
93+
// Users may try to decode a DECIMAL as floating point;
94+
// inform them why that's a bad idea.
95+
return Err(format!(
96+
"expected a DOUBLE as 4 or 8 bytes, got {other} bytes; \
97+
note that decoding DECIMAL as `f64` is not supported \
98+
due to differing semantics"
99+
)
100+
.into());
101+
}
102+
}
103+
}
74104
MySqlValueFormat::Text => value.as_str()?.parse()?,
75105
})
76106
}

‎sqlx-mysql/src/types/int.rs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,20 @@ fn int_decode(value: MySqlValueRef<'_>) -> Result<i64, BoxDynError> {
9595
MySqlValueFormat::Text => value.as_str()?.parse()?,
9696
MySqlValueFormat::Binary => {
9797
let buf = value.as_bytes()?;
98+
99+
// Check conditions that could cause `read_int()` to panic.
100+
if buf.is_empty() {
101+
return Err("empty buffer".into());
102+
}
103+
104+
if buf.len() > 8 {
105+
return Err(format!(
106+
"expected no more than 8 bytes for integer value, got {}",
107+
buf.len()
108+
)
109+
.into());
110+
}
111+
98112
LittleEndian::read_int(buf, buf.len())
99113
}
100114
})

‎sqlx-mysql/src/types/mod.rs‎

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
//! | `IpAddr` | VARCHAR, TEXT |
2121
//! | `Ipv4Addr` | INET4 (MariaDB-only), VARCHAR, TEXT |
2222
//! | `Ipv6Addr` | INET6 (MariaDB-only), VARCHAR, TEXT |
23+
//! | [`MySqlTime`] | TIME (encode and decode full range) |
24+
//! | [`Duration`] | TIME (for decoding positive values only) |
2325
//!
2426
//! ##### Note: `BOOLEAN`/`BOOL` Type
2527
//! MySQL and MariaDB treat `BOOLEAN` as an alias of the `TINYINT` type:
@@ -38,6 +40,12 @@
3840
//! Thus, you must use the type override syntax in the query to tell the macros you are expecting
3941
//! a `bool` column. See the docs for `query!()` and `query_as!()` for details on this syntax.
4042
//!
43+
//! ### NOTE: MySQL's `TIME` type is signed
44+
//! MySQL's `TIME` type can be used as either a time-of-day value, or a signed interval.
45+
//! Thus, it may take on negative values.
46+
//!
47+
//! Decoding a [`std::time::Duration`] returns an error if the `TIME` value is negative.
48+
//!
4149
//! ### [`chrono`](https://crates.io/crates/chrono)
4250
//!
4351
//! Requires the `chrono` Cargo feature flag.
@@ -48,7 +56,20 @@
4856
//! | `chrono::DateTime<Local>` | TIMESTAMP |
4957
//! | `chrono::NaiveDateTime` | DATETIME |
5058
//! | `chrono::NaiveDate` | DATE |
51-
//! | `chrono::NaiveTime` | TIME |
59+
//! | `chrono::NaiveTime` | TIME (time-of-day only) |
60+
//! | `chrono::TimeDelta` | TIME (decodes full range; see note for encoding) |
61+
//!
62+
//! ### NOTE: MySQL's `TIME` type is dual-purpose
63+
//! MySQL's `TIME` type can be used as either a time-of-day value, or an interval.
64+
//! However, `chrono::NaiveTime` is designed only to represent a time-of-day.
65+
//!
66+
//! Decoding a `TIME` value as `chrono::NaiveTime` will return an error if the value is out of range.
67+
//!
68+
//! The [`MySqlTime`] type supports the full range and it also implements `TryInto<chrono::NaiveTime>`.
69+
//!
70+
//! Decoding a `chrono::TimeDelta` also supports the full range.
71+
//!
72+
//! To encode a `chrono::TimeDelta`, convert it to [`MySqlTime`] first using `TryFrom`/`TryInto`.
5273
//!
5374
//! ### [`time`](https://crates.io/crates/time)
5475
//!
@@ -59,7 +80,20 @@
5980
//! | `time::PrimitiveDateTime` | DATETIME |
6081
//! | `time::OffsetDateTime` | TIMESTAMP |
6182
//! | `time::Date` | DATE |
62-
//! | `time::Time` | TIME |
83+
//! | `time::Time` | TIME (time-of-day only) |
84+
//! | `time::Duration` | TIME (decodes full range; see note for encoding) |
85+
//!
86+
//! ### NOTE: MySQL's `TIME` type is dual-purpose
87+
//! MySQL's `TIME` type can be used as either a time-of-day value, or an interval.
88+
//! However, `time::Time` is designed only to represent a time-of-day.
89+
//!
90+
//! Decoding a `TIME` value as `time::Time` will return an error if the value is out of range.
91+
//!
92+
//! The [`MySqlTime`] type supports the full range, and it also implements `TryInto<time::Time>`.
93+
//!
94+
//! Decoding a `time::Duration` also supports the full range.
95+
//!
96+
//! To encode a `time::Duration`, convert it to [`MySqlTime`] first using `TryFrom`/`TryInto`.
6397
//!
6498
//! ### [`bigdecimal`](https://crates.io/crates/bigdecimal)
6599
//! Requires the `bigdecimal` Cargo feature flag.
@@ -102,11 +136,14 @@
102136
103137
pub(crate) use sqlx_core::types::*;
104138

139+
pub use mysql_time::{MySqlTime, MySqlTimeError, MySqlTimeSign};
140+
105141
mod bool;
106142
mod bytes;
107143
mod float;
108144
mod inet;
109145
mod int;
146+
mod mysql_time;
110147
mod str;
111148
mod text;
112149
mod uint;

0 commit comments

Comments
 (0)