Skip to content

Commit

Permalink
api: fix PaymentIndex not deserializing from query string correctly
Browse files Browse the repository at this point in the history
It turns out that the `PaymentIndex` in e.g. `get_new_payments` was
silently getting dropped due to a `#[serde(flatten)]` + query string
limitation.

For query strings, using `#[serde(flatten)]` on a field requires that
all inner fields must be string-ish types (&str, String, Cow<'_, str>,
etc...) OR use `SerializeDisplay` and `DeserializeFromStr`.

This issue is due to a limitation in `serde`. See:
<serde-rs/serde#1183>

The fix here is to remove `#[serde(flatten)]` for querystring
serializeable types (actually just `PaymentIndex`) and instead use
`SerializeDisplay` and `DeserializeFromStr`
  • Loading branch information
phlip9 committed Jul 31, 2023
1 parent fc0b7a6 commit 2a3a700
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 25 deletions.
3 changes: 0 additions & 3 deletions common/src/api/qs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ pub struct GetByScid {
pub struct GetPaymentByIndex {
/// The index of the payment to be fetched.
// We use index instead of id so the backend can query by primary key.
#[serde(flatten)]
pub index: PaymentIndex,
}

Expand All @@ -50,7 +49,6 @@ pub struct GetPaymentByIndex {
pub struct GetNewPayments {
/// Optional [`PaymentIndex`] at which the results should start, exclusive.
/// Payments with an index less than or equal to this will not be returned.
#[serde(flatten)]
pub start_index: Option<PaymentIndex>,
/// (Optional) the maximum number of results that can be returned.
pub limit: Option<u16>,
Expand All @@ -73,7 +71,6 @@ pub struct GetPaymentsByIds {
#[derive(Serialize, Deserialize)]
pub struct UpdatePaymentNote {
/// The index of the payment whose note should be updated.
#[serde(flatten)]
pub index: PaymentIndex,
/// The updated note.
pub note: Option<String>,
Expand Down
3 changes: 1 addition & 2 deletions common/src/ln/payments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ use crate::{
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
#[cfg_attr(all(any(test, feature = "test-utils")), derive(Arbitrary))]
pub struct BasicPayment {
#[serde(flatten)]
pub index: PaymentIndex,

pub kind: PaymentKind,
Expand Down Expand Up @@ -174,7 +173,7 @@ pub enum PaymentStatus {
/// 8776421933930532767-bc_ead3c01be0315dfd4e4c405aaca0f39076cff722a0f680c89c348e3bda9575f3
/// ```
#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd)]
#[derive(Serialize, Deserialize)]
#[derive(SerializeDisplay, DeserializeFromStr)]
#[cfg_attr(any(test, feature = "test-utils"), derive(Arbitrary))]
pub struct PaymentIndex {
pub created_at: TimestampMs,
Expand Down
33 changes: 13 additions & 20 deletions common/src/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@ use std::{
};

use anyhow::{anyhow, Context};
use serde::{de, Deserialize, Deserializer, Serialize};
use serde::{de, Serialize};

/// The number of milliseconds since the [`UNIX_EPOCH`].
///
/// - Internally represented by a non-negative [`i64`] to ease interoperability
/// with some platforms we use which don't support unsigned ints.
/// - Can represent any time from January 1st, 1970 00:00:00.000 UTC to roughly
/// 292 million years in the future.
#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd, Serialize)]
#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
#[derive(Serialize)]
pub struct TimestampMs(i64);

impl TimestampMs {
Expand Down Expand Up @@ -76,24 +77,6 @@ impl From<u32> for TimestampMs {
}
}

/// Enforces that the inner [`i64`] is non-negative.
impl<'de> Deserialize<'de> for TimestampMs {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
let value = i64::deserialize(deserializer)?;
if value >= 0 {
Ok(TimestampMs(value))
} else {
Err(de::Error::invalid_value(
de::Unexpected::Signed(value),
&"Unix timestamp must be non-negative",
))
}
}
}

impl FromStr for TimestampMs {
type Err = anyhow::Error;
fn from_str(s: &str) -> Result<Self, Self::Err> {
Expand All @@ -112,6 +95,16 @@ impl Display for TimestampMs {
}
}

impl<'de> de::Deserialize<'de> for TimestampMs {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: de::Deserializer<'de>,
{
i64::deserialize(deserializer)
.and_then(|x| Self::try_from(x).map_err(de::Error::custom))
}
}

#[cfg(any(test, feature = "test-utils"))]
mod arbitrary_impl {
use proptest::{
Expand Down

0 comments on commit 2a3a700

Please sign in to comment.