Page MenuHomePhabricator
Differential D11594

[lib] errors utils to distinguish between session creation issues
ClosedPublic
Actions

Authored by kamil on Apr 9 2024, 3:26 AM.
Tags
None
Referenced Files
F6240444: D11594.diff
Thu, Apr 24, 10:49 PM
Unknown Object (File)
Sat, Apr 19, 5:22 PM
Unknown Object (File)
Thu, Apr 17, 11:18 AM
Unknown Object (File)
Thu, Apr 17, 7:44 AM
Unknown Object (File)
Thu, Apr 17, 4:33 AM
Unknown Object (File)
Thu, Apr 17, 3:25 AM
Unknown Object (File)
Wed, Apr 16, 3:31 AM
Unknown Object (File)
Thu, Mar 27, 6:45 PM
View All 83 Files
Subscribers
ashoat

Details

Reviewers
bartek
marcin
tomek
Commits
rCOMMf4b392ccb485: [lib] errors utils to distinguish between session creation issues
Summary

Constants with errors introduced earlier in the stack.

Depends on D11591

Test Plan

Tested all error cases on both web and native

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil created this revision.Apr 9 2024, 3:26 AM
kamil held this revision as a draft.
Herald added a subscriber: ashoat. · View Herald TranscriptApr 9 2024, 3:27 AM
Harbormaster completed remote builds in B28076: Diff 38926.Apr 9 2024, 3:42 AM
kamil added a child revision: D11595: [web/native] make `overwrite` param for inbound session mandatory and unify web & native.Apr 9 2024, 4:44 AM
kamil published this revision for review.Apr 9 2024, 6:03 AM
kamil added inline comments.
lib/handlers/peer-to-peer-message-handler.js
45 ↗(On Diff #38926)

handled in D11596

lib/utils/errors.js
49 ↗(On Diff #38926)

added by IDE, I'll remove it before landing

ashoat added inline comments.Apr 10 2024, 7:51 AM
lib/utils/errors.js
74–81 ↗(On Diff #38926)
  1. Should this have been introduced in D11595, and used there in worker-crypto.js?
  2. I think it's weird to add these Olm-specific errors to a file that otherwise is fairly generic. Can you find a different place for them?
ashoat added inline comments.Apr 10 2024, 7:53 AM
lib/utils/errors.js
74–81 ↗(On Diff #38926)

Ignore point 1, I didn't realize this is earlier in the stack. Will follow up about using the constants on the linked diff

Point 2 still stands

marcin accepted this revision.Apr 11 2024, 12:49 AM

Please address @ashoat suggestion before landing.

This revision is now accepted and ready to land.Apr 11 2024, 12:49 AM
kamil updated this revision to Diff 39036.Apr 11 2024, 3:59 AM

move olmSessionErrors from errors.js to olm-utils.js

Harbormaster completed remote builds in B28162: Diff 39036.Apr 11 2024, 4:24 AM
Closed by commit rCOMMf4b392ccb485: [lib] errors utils to distinguish between session creation issues (authored by kamil). · Explain WhyApr 11 2024, 4:47 AM
This revision was automatically updated to reflect the committed changes.
kamil added a commit: rCOMMf4b392ccb485: [lib] errors utils to distinguish between session creation issues.

Revision Contents
Changeset List

PathSize
lib/
handlers/
35 lines
utils/
10 lines

Diff 39036

View Options

lib/handlers/peer-to-peer-message-handler.js

// TODO: get rid of this // TODO: get rid of this
#![allow(dead_code)] #![allow(dead_code)]
use std::collections::HashMap; use std::collections::HashMap;
use aws_sdk_dynamodb::model::AttributeValue; use aws_sdk_dynamodb::model::AttributeValue;
use chrono::{DateTime, Utc}; use chrono::{DateTime, Utc};
use tracing::warn;
use crate::{ use crate::{
constants::devices_table::*, constants::devices_table::*,
database::parse_string_attribute, database::parse_string_attribute,
error::{DBItemAttributeError, DBItemError, FromAttributeValue}, error::{DBItemAttributeError, DBItemError, FromAttributeValue},
grpc_services::protos::unauth::DeviceType, grpc_services::protos::unauth::DeviceType,
}; };
Show All 34 Lines
struct DeviceListKeyAttribute(DateTime<Utc>); struct DeviceListKeyAttribute(DateTime<Utc>);
impl From<DeviceIDAttribute> for AttributeValue { impl From<DeviceIDAttribute> for AttributeValue {
fn from(value: DeviceIDAttribute) -> Self { fn from(value: DeviceIDAttribute) -> Self {
AttributeValue::S(format!("{DEVICE_ITEM_KEY_PREFIX}{}", value.0)) AttributeValue::S(format!("{DEVICE_ITEM_KEY_PREFIX}{}", value.0))
} }
} }
impl From<DeviceListKeyAttribute> for AttributeValue {
fn from(value: DeviceListKeyAttribute) -> Self {
AttributeValue::S(format!(
"{DEVICE_LIST_KEY_PREFIX}{}",
value.0.to_rfc3339()
))
}
}
impl TryFrom<Option<AttributeValue>> for DeviceIDAttribute { impl TryFrom<Option<AttributeValue>> for DeviceIDAttribute {
type Error = DBItemError; type Error = DBItemError;
fn try_from(value: Option<AttributeValue>) -> Result<Self, Self::Error> { fn try_from(value: Option<AttributeValue>) -> Result<Self, Self::Error> {
let item_id = parse_string_attribute(ATTR_ITEM_ID, value)?; let item_id = parse_string_attribute(ATTR_ITEM_ID, value)?;
// remove the device- prefix // remove the device- prefix
let device_id = item_id let device_id = item_id
.strip_prefix(DEVICE_ITEM_KEY_PREFIX) .strip_prefix(DEVICE_ITEM_KEY_PREFIX)
.ok_or_else(|| DBItemError { .ok_or_else(|| DBItemError {
attribute_name: ATTR_ITEM_ID.to_string(), attribute_name: ATTR_ITEM_ID.to_string(),
attribute_value: Some(AttributeValue::S(item_id.clone())), attribute_value: Some(AttributeValue::S(item_id.clone())),
attribute_error: DBItemAttributeError::InvalidValue, attribute_error: DBItemAttributeError::InvalidValue,
})? })?
.to_string(); .to_string();
Ok(Self(device_id)) Ok(Self(device_id))
} }
} }
impl TryFrom<Option<AttributeValue>> for DeviceListKeyAttribute {
type Error = DBItemError;
fn try_from(value: Option<AttributeValue>) -> Result<Self, Self::Error> {
let item_id = parse_string_attribute(ATTR_ITEM_ID, value)?;
// remove the device-list- prefix, then parse the timestamp
let timestamp: DateTime<Utc> = item_id
.strip_prefix(DEVICE_LIST_KEY_PREFIX)
.ok_or_else(|| DBItemError {
attribute_name: ATTR_ITEM_ID.to_string(),
attribute_value: Some(AttributeValue::S(item_id.clone())),
attribute_error: DBItemAttributeError::InvalidValue,
})
.and_then(|s| {
s.parse().map_err(|e| {
DBItemError::new(
ATTR_ITEM_ID.to_string(),
Some(AttributeValue::S(item_id.clone())),
DBItemAttributeError::InvalidTimestamp(e),
)
})
})?;
Ok(Self(timestamp))
}
}
impl TryFrom<RawAttributes> for DeviceRow { impl TryFrom<RawAttributes> for DeviceRow {
type Error = DBItemError; type Error = DBItemError;
fn try_from(mut attrs: RawAttributes) -> Result<Self, Self::Error> { fn try_from(mut attrs: RawAttributes) -> Result<Self, Self::Error> {
let user_id = let user_id =
parse_string_attribute(ATTR_USER_ID, attrs.remove(ATTR_USER_ID))?; parse_string_attribute(ATTR_USER_ID, attrs.remove(ATTR_USER_ID))?;
let DeviceIDAttribute(device_id) = attrs.remove(ATTR_ITEM_ID).try_into()?; let DeviceIDAttribute(device_id) = attrs.remove(ATTR_ITEM_ID).try_into()?;
▲ Show 20 Lines • Show All 95 Lines • ▼ Show 20 Lines if let Some(social_proof) = value.social_proof {
ATTR_SOCIAL_PROOF.to_string(), ATTR_SOCIAL_PROOF.to_string(),
AttributeValue::S(social_proof), AttributeValue::S(social_proof),
); );
} }
attrs attrs
} }
} }
impl TryFrom<RawAttributes> for DeviceListRow {
type Error = DBItemError;
fn try_from(mut attrs: RawAttributes) -> Result<Self, Self::Error> {
let user_id =
parse_string_attribute(ATTR_USER_ID, attrs.remove(ATTR_USER_ID))?;
let DeviceListKeyAttribute(timestamp) =
attrs.remove(ATTR_ITEM_ID).try_into()?;
// validate timestamps are in sync
let timestamps_match = attrs
.remove(ATTR_TIMESTAMP)
.and_then(|attr| attr.as_n().ok().cloned())
.and_then(|val| val.parse::<i64>().ok())
.filter(|val| *val == timestamp.timestamp_millis())
.is_some();
if !timestamps_match {
warn!(
"DeviceList timestamp mismatch for (userID={}, itemID={})",
&user_id,
timestamp.to_rfc3339()
);
}
// this should be a list of strings
let device_ids = attrs
.remove(ATTR_DEVICE_IDS)
.ok_or_else(|| {
DBItemError::new(
ATTR_DEVICE_IDS.to_string(),
None,
DBItemAttributeError::Missing,
)
})?
.to_vec(ATTR_DEVICE_IDS)?
.iter()
.map(|v| v.to_string("device_ids[?]").cloned())
.collect::<Result<Vec<String>, DBItemError>>()?;
Ok(Self {
user_id,
timestamp,
device_ids,
})
}
}
impl From<DeviceListRow> for RawAttributes {
fn from(device_list: DeviceListRow) -> Self {
let mut attrs = HashMap::new();
attrs.insert(
ATTR_USER_ID.to_string(),
AttributeValue::S(device_list.user_id.clone()),
);
attrs.insert(
ATTR_ITEM_ID.to_string(),
DeviceListKeyAttribute(device_list.timestamp).into(),
);
attrs.insert(
ATTR_TIMESTAMP.to_string(),
AttributeValue::N(device_list.timestamp.timestamp_millis().to_string()),
);
bartekAuthorUnsubmitted
Done
Inline Actions

Note that we use RFC 3339 string for ItemID while the ATTR_TIMESTAMP uses milliseconds number. Generally I use the former everywhere (simpler parsing without NaiveDateTime conversions) but I thought the latter is easier for DDB to sort by (it's used only as index sorting key and not read anywhere else)

bartek: Note that we use RFC 3339 string for `ItemID` while the `ATTR_TIMESTAMP` uses milliseconds…
attrs.insert(
ATTR_DEVICE_IDS.to_string(),
AttributeValue::L(
device_list
.device_ids
.into_iter()
.map(AttributeValue::S)
.collect(),
),
);
attrs
}
}
View Options

lib/utils/olm-utils.js

// TODO: get rid of this // TODO: get rid of this
#![allow(dead_code)] #![allow(dead_code)]
use std::collections::HashMap; use std::collections::HashMap;
use aws_sdk_dynamodb::model::AttributeValue; use aws_sdk_dynamodb::model::AttributeValue;
use chrono::{DateTime, Utc}; use chrono::{DateTime, Utc};
use tracing::warn;
use crate::{ use crate::{
constants::devices_table::*, constants::devices_table::*,
database::parse_string_attribute, database::parse_string_attribute,
error::{DBItemAttributeError, DBItemError, FromAttributeValue}, error::{DBItemAttributeError, DBItemError, FromAttributeValue},
grpc_services::protos::unauth::DeviceType, grpc_services::protos::unauth::DeviceType,
}; };
Show All 34 Lines
struct DeviceListKeyAttribute(DateTime<Utc>); struct DeviceListKeyAttribute(DateTime<Utc>);
impl From<DeviceIDAttribute> for AttributeValue { impl From<DeviceIDAttribute> for AttributeValue {
fn from(value: DeviceIDAttribute) -> Self { fn from(value: DeviceIDAttribute) -> Self {
AttributeValue::S(format!("{DEVICE_ITEM_KEY_PREFIX}{}", value.0)) AttributeValue::S(format!("{DEVICE_ITEM_KEY_PREFIX}{}", value.0))
} }
} }
impl From<DeviceListKeyAttribute> for AttributeValue {
fn from(value: DeviceListKeyAttribute) -> Self {
AttributeValue::S(format!(
"{DEVICE_LIST_KEY_PREFIX}{}",
value.0.to_rfc3339()
))
}
}
impl TryFrom<Option<AttributeValue>> for DeviceIDAttribute { impl TryFrom<Option<AttributeValue>> for DeviceIDAttribute {
type Error = DBItemError; type Error = DBItemError;
fn try_from(value: Option<AttributeValue>) -> Result<Self, Self::Error> { fn try_from(value: Option<AttributeValue>) -> Result<Self, Self::Error> {
let item_id = parse_string_attribute(ATTR_ITEM_ID, value)?; let item_id = parse_string_attribute(ATTR_ITEM_ID, value)?;
// remove the device- prefix // remove the device- prefix
let device_id = item_id let device_id = item_id
.strip_prefix(DEVICE_ITEM_KEY_PREFIX) .strip_prefix(DEVICE_ITEM_KEY_PREFIX)
.ok_or_else(|| DBItemError { .ok_or_else(|| DBItemError {
attribute_name: ATTR_ITEM_ID.to_string(), attribute_name: ATTR_ITEM_ID.to_string(),
attribute_value: Some(AttributeValue::S(item_id.clone())), attribute_value: Some(AttributeValue::S(item_id.clone())),
attribute_error: DBItemAttributeError::InvalidValue, attribute_error: DBItemAttributeError::InvalidValue,
})? })?
.to_string(); .to_string();
Ok(Self(device_id)) Ok(Self(device_id))
} }
} }
impl TryFrom<Option<AttributeValue>> for DeviceListKeyAttribute {
type Error = DBItemError;
fn try_from(value: Option<AttributeValue>) -> Result<Self, Self::Error> {
let item_id = parse_string_attribute(ATTR_ITEM_ID, value)?;
// remove the device-list- prefix, then parse the timestamp
let timestamp: DateTime<Utc> = item_id
.strip_prefix(DEVICE_LIST_KEY_PREFIX)
.ok_or_else(|| DBItemError {
attribute_name: ATTR_ITEM_ID.to_string(),
attribute_value: Some(AttributeValue::S(item_id.clone())),
attribute_error: DBItemAttributeError::InvalidValue,
})
.and_then(|s| {
s.parse().map_err(|e| {
DBItemError::new(
ATTR_ITEM_ID.to_string(),
Some(AttributeValue::S(item_id.clone())),
DBItemAttributeError::InvalidTimestamp(e),
)
})
})?;
Ok(Self(timestamp))
}
}
impl TryFrom<RawAttributes> for DeviceRow { impl TryFrom<RawAttributes> for DeviceRow {
type Error = DBItemError; type Error = DBItemError;
fn try_from(mut attrs: RawAttributes) -> Result<Self, Self::Error> { fn try_from(mut attrs: RawAttributes) -> Result<Self, Self::Error> {
let user_id = let user_id =
parse_string_attribute(ATTR_USER_ID, attrs.remove(ATTR_USER_ID))?; parse_string_attribute(ATTR_USER_ID, attrs.remove(ATTR_USER_ID))?;
let DeviceIDAttribute(device_id) = attrs.remove(ATTR_ITEM_ID).try_into()?; let DeviceIDAttribute(device_id) = attrs.remove(ATTR_ITEM_ID).try_into()?;
▲ Show 20 Lines • Show All 95 Lines • ▼ Show 20 Lines if let Some(social_proof) = value.social_proof {
ATTR_SOCIAL_PROOF.to_string(), ATTR_SOCIAL_PROOF.to_string(),
AttributeValue::S(social_proof), AttributeValue::S(social_proof),
); );
} }
attrs attrs
} }
} }
impl TryFrom<RawAttributes> for DeviceListRow {
type Error = DBItemError;
fn try_from(mut attrs: RawAttributes) -> Result<Self, Self::Error> {
let user_id =
parse_string_attribute(ATTR_USER_ID, attrs.remove(ATTR_USER_ID))?;
let DeviceListKeyAttribute(timestamp) =
attrs.remove(ATTR_ITEM_ID).try_into()?;
// validate timestamps are in sync
let timestamps_match = attrs
.remove(ATTR_TIMESTAMP)
.and_then(|attr| attr.as_n().ok().cloned())
.and_then(|val| val.parse::<i64>().ok())
.filter(|val| *val == timestamp.timestamp_millis())
.is_some();
if !timestamps_match {
warn!(
"DeviceList timestamp mismatch for (userID={}, itemID={})",
&user_id,
timestamp.to_rfc3339()
);
}
// this should be a list of strings
let device_ids = attrs
.remove(ATTR_DEVICE_IDS)
.ok_or_else(|| {
DBItemError::new(
ATTR_DEVICE_IDS.to_string(),
None,
DBItemAttributeError::Missing,
)
})?
.to_vec(ATTR_DEVICE_IDS)?
.iter()
.map(|v| v.to_string("device_ids[?]").cloned())
.collect::<Result<Vec<String>, DBItemError>>()?;
Ok(Self {
user_id,
timestamp,
device_ids,
})
}
}
impl From<DeviceListRow> for RawAttributes {
fn from(device_list: DeviceListRow) -> Self {
let mut attrs = HashMap::new();
attrs.insert(
ATTR_USER_ID.to_string(),
AttributeValue::S(device_list.user_id.clone()),
);
attrs.insert(
ATTR_ITEM_ID.to_string(),
DeviceListKeyAttribute(device_list.timestamp).into(),
);
attrs.insert(
ATTR_TIMESTAMP.to_string(),
AttributeValue::N(device_list.timestamp.timestamp_millis().to_string()),
);
bartekAuthorUnsubmitted
Done
Inline Actions

Note that we use RFC 3339 string for ItemID while the ATTR_TIMESTAMP uses milliseconds number. Generally I use the former everywhere (simpler parsing without NaiveDateTime conversions) but I thought the latter is easier for DDB to sort by (it's used only as index sorting key and not read anywhere else)

bartek: Note that we use RFC 3339 string for `ItemID` while the `ATTR_TIMESTAMP` uses milliseconds…
attrs.insert(
ATTR_DEVICE_IDS.to_string(),
AttributeValue::L(
device_list
.device_ids
.into_iter()
.map(AttributeValue::S)
.collect(),
),
);
attrs
}
}