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.

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

fix: do not panic when binding a large BigDecimal #3053

Merged
merged 4 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
3 changes: 3 additions & 0 deletions .github/workflows/sqlx.yml
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,9 @@ jobs:
matrix:
runtime: [async-std, tokio]
needs: check
env:
# Enable tests with SQLCipher
RUSTFLAGS: --cfg sqlite_test_sqlcipher
steps:
- uses: actions/checkout@v2

Expand Down
5 changes: 4 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,10 @@ hex = "0.4.3"
tempfile = "3.9.0"
criterion = { version = "0.5.1", features = ["async_tokio"] }

# Needed to test SQLCipher
# If this is an unconditional dev-dependency then Cargo will *always* try to build `libsqlite3-sys`,
# even when SQLite isn't the intended test target, and fail if the build environment is not set up for compiling C code.
[target.'cfg(sqlite_test_sqlcipher)'.dev-dependencies]
# Enable testing with SQLCipher if specifically requested.
libsqlite3-sys = { version = "0.27", features = ["bundled-sqlcipher"] }

#
Expand Down
20 changes: 20 additions & 0 deletions sqlx-postgres/src/types/bigdecimal-range.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#### Note: `BigDecimal` Has a Larger Range than `NUMERIC`
`BigDecimal` can represent values with a far, far greater range than the `NUMERIC` type in Postgres can.

`NUMERIC` is limited to 131,072 digits before the decimal point, and 16,384 digits after it.
See [Section 8.1, Numeric Types] of the Postgres manual for details.

Meanwhile, `BigDecimal` can theoretically represent a value with an arbitrary number of decimal digits, albeit
with a maximum of 2<sup>63</sup> significant figures.

Because encoding in the current API design _must_ be infallible,
when attempting to encode a `BigDecimal` that cannot fit in the wire representation of `NUMERIC`,
SQLx may instead encode a sentinel value that falls outside the allowed range but is still representable.

This will cause the query to return a `DatabaseError` with code `22P03` (`invalid_binary_representation`)
and the error message `invalid scale in external "numeric" value` (though this may be subject to change).

However, `BigDecimal` should be able to decode any `NUMERIC` value except `NaN`,
for which it has no representation.

[Section 8.1, Numeric Types]: https://www.postgresql.org/docs/current/datatype-numeric.html
32 changes: 25 additions & 7 deletions sqlx-postgres/src/types/bigdecimal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,23 +127,31 @@ impl TryFrom<&'_ BigDecimal> for PgNumeric {
}

Ok(PgNumeric::Number {
sign: match sign {
Sign::Plus | Sign::NoSign => PgNumericSign::Positive,
Sign::Minus => PgNumericSign::Negative,
},
sign: sign_to_pg(sign),
scale,
weight,
digits,
})
}
}

/// ### Panics
/// If this `BigDecimal` cannot be represented by `PgNumeric`.
#[doc=include_str!("bigdecimal-range.md")]
impl Encode<'_, Postgres> for BigDecimal {
fn encode_by_ref(&self, buf: &mut PgArgumentBuffer) -> IsNull {
// If the argument is too big, then we replace it with a less big argument.
// This less big argument is already outside the range of allowed PostgreSQL DECIMAL, which
// means that PostgreSQL will return the 22P03 error kind upon receiving it. This is the
// expected error, and the user should be ready to handle it anyway.
PgNumeric::try_from(self)
.expect("BigDecimal magnitude too great for Postgres NUMERIC type")
.unwrap_or_else(|_| {
PgNumeric::Number {
digits: vec![1],
// This is larger than the maximum allowed value, so Postgres should return an error.
scale: 0x4000,
weight: 0,
sign: sign_to_pg(self.sign()),
}
})
.encode(buf);

IsNull::No
Expand All @@ -156,6 +164,9 @@ impl Encode<'_, Postgres> for BigDecimal {
}
}

/// ### Note: `NaN`
/// `BigDecimal` has a greater range than `NUMERIC` (see the corresponding `Encode` impl for details)
/// but cannot represent `NaN`, so decoding may return an error.
impl Decode<'_, Postgres> for BigDecimal {
fn decode(value: PgValueRef<'_>) -> Result<Self, BoxDynError> {
match value.format() {
Expand All @@ -165,6 +176,13 @@ impl Decode<'_, Postgres> for BigDecimal {
}
}

fn sign_to_pg(sign: Sign) -> PgNumericSign {
match sign {
Sign::Plus | Sign::NoSign => PgNumericSign::Positive,
Sign::Minus => PgNumericSign::Negative,
}
}

#[cfg(test)]
mod bigdecimal_to_pgnumeric {
use super::{BigDecimal, PgNumeric, PgNumericSign};
Expand Down
4 changes: 4 additions & 0 deletions sqlx-postgres/src/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,17 @@
//! |---------------------------------------|------------------------------------------------------|
//! | `bigdecimal::BigDecimal` | NUMERIC |
//!
#![doc=include_str!("bigdecimal-range.md")]
//!
//! ### [`rust_decimal`](https://crates.io/crates/rust_decimal)
//! Requires the `rust_decimal` Cargo feature flag.
//!
//! | Rust type | Postgres type(s) |
//! |---------------------------------------|------------------------------------------------------|
//! | `rust_decimal::Decimal` | NUMERIC |
//!
#![doc=include_str!("rust_decimal-range.md")]
//!
//! ### [`chrono`](https://crates.io/crates/chrono)
//!
//! Requires the `chrono` Cargo feature flag.
Expand Down
10 changes: 10 additions & 0 deletions sqlx-postgres/src/types/rust_decimal-range.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#### Note: `rust_decimal::Decimal` Has a Smaller Range than `NUMERIC`
`NUMERIC` is can have up to 131,072 digits before the decimal point, and 16,384 digits after it.
See [Section 8.1, Numeric Types] of the Postgres manual for details.

However, `rust_decimal::Decimal` is limited to a maximum absolute magnitude of 2<sup>96</sup> - 1,
a number with 67 decimal digits, and a minimum absolute magnitude of 10<sup>-28</sup>, a number with, unsurprisingly,
28 decimal digits.

Thus, in contrast with `BigDecimal`, `NUMERIC` can actually represent every possible value of `rust_decimal::Decimal`,
but not the other way around. This means that encoding should never fail, but decoding can.
6 changes: 3 additions & 3 deletions sqlx-postgres/src/types/rust_decimal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ impl TryFrom<PgNumeric> for Decimal {
}
}

// This impl is effectively infallible because `NUMERIC` has a greater range than `Decimal`.
impl TryFrom<&'_ Decimal> for PgNumeric {
type Error = BoxDynError;

Expand Down Expand Up @@ -142,18 +143,17 @@ impl TryFrom<&'_ Decimal> for PgNumeric {
}
}

/// ### Panics
/// If this `Decimal` cannot be represented by `PgNumeric`.
impl Encode<'_, Postgres> for Decimal {
fn encode_by_ref(&self, buf: &mut PgArgumentBuffer) -> IsNull {
PgNumeric::try_from(self)
.expect("Decimal magnitude too great for Postgres NUMERIC type")
.expect("BUG: `Decimal` to `PgNumeric` conversion should be infallible")
.encode(buf);

IsNull::No
}
}

#[doc=include_str!("rust_decimal-range.md")]
impl Decode<'_, Postgres> for Decimal {
fn decode(value: PgValueRef<'_>) -> Result<Self, BoxDynError> {
match value.format() {
Expand Down
48 changes: 43 additions & 5 deletions tests/postgres/postgres.rs
Original file line number Diff line number Diff line change
Expand Up @@ -932,11 +932,7 @@ from (values (null)) vals(val)

#[sqlx_macros::test]
async fn test_listener_cleanup() -> anyhow::Result<()> {
#[cfg(feature = "_rt-tokio")]
use tokio::time::timeout;

#[cfg(feature = "_rt-async-std")]
use async_std::future::timeout;
use sqlx_core::rt::timeout;

use sqlx::pool::PoolOptions;
use sqlx::postgres::PgListener;
Expand Down Expand Up @@ -1837,3 +1833,45 @@ async fn test_error_handling_with_deferred_constraints() -> anyhow::Result<()> {

Ok(())
}

#[sqlx_macros::test]
#[cfg(feature = "bigdecimal")]
async fn test_issue_3052() {
use sqlx::types::BigDecimal;

// https://github.com/launchbadge/sqlx/issues/3052
// Previously, attempting to bind a `BigDecimal` would panic if the value was out of range.
// Now, we rewrite it to a sentinel value so that Postgres will return a range error.
let too_small: BigDecimal = "1E-65536".parse().unwrap();
let too_large: BigDecimal = "1E262144".parse().unwrap();

let mut conn = new::<Postgres>().await.unwrap();

let too_small_res = sqlx::query_scalar::<_, BigDecimal>("SELECT $1::numeric")
.bind(&too_small)
.fetch_one(&mut conn)
.await;

match too_small_res {
Err(sqlx::Error::Database(dbe)) => {
let dbe = dbe.downcast::<PgDatabaseError>();

assert_eq!(dbe.code(), "22P03");
}
other => panic!("expected Err(DatabaseError), got {other:?}"),
}

let too_large_res = sqlx::query_scalar::<_, BigDecimal>("SELECT $1::numeric")
.bind(&too_large)
.fetch_one(&mut conn)
.await;

match too_large_res {
Err(sqlx::Error::Database(dbe)) => {
let dbe = dbe.downcast::<PgDatabaseError>();

assert_eq!(dbe.code(), "22P03");
}
other => panic!("expected Err(DatabaseError), got {other:?}"),
}
}
1 change: 1 addition & 0 deletions tests/sqlite/sqlcipher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ async fn it_fails_if_password_is_incorrect() -> anyhow::Result<()> {
Ok(())
}

#[cfg(sqlite_test_sqlcipher)]
#[sqlx_macros::test]
async fn it_honors_order_of_encryption_pragmas() -> anyhow::Result<()> {
let (url, _dir) = new_db_url().await?;
Expand Down
Loading