Skip to content

Commit

Permalink
update Nimbus SDK to provide more information on serde_json::Errors
Browse files Browse the repository at this point in the history
  • Loading branch information
jeddai committed Nov 8, 2024
1 parent 3088693 commit 775da21
Show file tree
Hide file tree
Showing 11 changed files with 114 additions and 31 deletions.
11 changes: 9 additions & 2 deletions components/nimbus/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ pub enum NimbusError {
RkvError(#[from] rkv::StoreError),
#[error("IO error: {0}")]
IOError(#[from] std::io::Error),
#[error("JSON Error: {0}")]
JSONError(#[from] serde_json::Error),
#[error("JSON Error: {0} — {1}")]
JSONError(String, String),
#[error("EvaluationError: {0}")]
EvaluationError(String),
#[error("Invalid Expression - didn't evaluate to a bool")]
Expand Down Expand Up @@ -103,6 +103,13 @@ pub enum CirrusClientError {
RequestMissingParameter(String),
}

#[cfg(test)]
impl From<serde_json::Error> for NimbusError {
fn from(error: serde_json::Error) -> Self {
NimbusError::JSONError("test".into(), error.to_string())
}
}

impl<'a> From<jexl_eval::error::EvaluationError<'a>> for NimbusError {
fn from(eval_error: jexl_eval::error::EvaluationError<'a>) -> Self {
NimbusError::EvaluationError(eval_error.to_string())
Expand Down
10 changes: 9 additions & 1 deletion components/nimbus/src/sampling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,15 @@ pub(crate) fn ratio_sample<T: serde::Serialize>(input: T, ratios: &[u32]) -> Res
/// that is larger than 6 bytes (Should never occur)
pub(crate) fn truncated_hash<T: serde::Serialize>(data: T) -> Result<[u8; 6]> {
let mut hasher = Sha256::new();
let data_str = serde_json::to_string(&data)?;
let data_str = match serde_json::to_string(&data) {
Ok(v) => v,
Err(e) => {
return Err(NimbusError::JSONError(
"data_str = nimbus::sampling::truncated_hash::serde_json::to_string".into(),
e.to_string(),
))
}
};
hasher.update(data_str.as_bytes());
Ok(hasher.finalize()[0..6].try_into()?)
}
Expand Down
10 changes: 9 additions & 1 deletion components/nimbus/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,15 @@ pub fn parse_experiments(payload: &str) -> Result<Vec<Experiment>> {
// We first encode the response into a `serde_json::Value`
// to allow us to deserialize each experiment individually,
// omitting any malformed experiments
let value: Value = serde_json::from_str(payload)?;
let value: Value = match serde_json::from_str(payload) {
Ok(v) => v,
Err(e) => {
return Err(NimbusError::JSONError(
"value = nimbus::schema::parse_experiments::serde_json::from_str".into(),
e.to_string(),
))
}
};
let data = value
.get("data")
.ok_or(NimbusError::InvalidExperimentFormat)?;
Expand Down
20 changes: 16 additions & 4 deletions components/nimbus/src/stateful/behavior.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,8 +357,14 @@ impl EventQueryType {
self
)));
}
let event = serde_json::from_value::<String>(args.first().unwrap().clone())?;
let interval = serde_json::from_value::<String>(args.get(1).unwrap().clone())?;
let event = match serde_json::from_value::<String>(args.first().unwrap().clone()) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("event = nimbus::stateful::behavior::EventQueryType::validate_counting_arguments::serde_json::from_value".into(), e.to_string()))
};
let interval = match serde_json::from_value::<String>(args.get(1).unwrap().clone()) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("interval = nimbus::stateful::behavior::EventQueryType::validate_counting_arguments::serde_json::from_value".into(), e.to_string()))
};
let interval = Interval::from_str(&interval)?;
let num_buckets = match args.get(2).unwrap().as_f64() {
Some(v) => v,
Expand Down Expand Up @@ -393,8 +399,14 @@ impl EventQueryType {
self
)));
}
let event = serde_json::from_value::<String>(args.first().unwrap().clone())?;
let interval = serde_json::from_value::<String>(args.get(1).unwrap().clone())?;
let event = match serde_json::from_value::<String>(args.first().unwrap().clone()) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("event = nimbus::stateful::behavior::EventQueryType::validate_last_seen_arguments::serde_json::from_value".into(), e.to_string()))
};
let interval = match serde_json::from_value::<String>(args.get(1).unwrap().clone()) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("interval = nimbus::stateful::behavior::EventQueryType::validate_last_seen_arguments::serde_json::from_value".into(), e.to_string()))
};
let interval = Interval::from_str(&interval)?;
let zero = &Value::from(0);
let starting_bucket = match args.get(2).unwrap_or(zero).as_f64() {
Expand Down
17 changes: 13 additions & 4 deletions components/nimbus/src/stateful/nimbus_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,12 @@ impl NimbusClient {
.read_from_db(db)?;

if let Some(recorded_context) = &self.recorded_context {
let targeting_helper = self.create_targeting_helper_with_context(serde_json::to_value(
let targeting_helper = self.create_targeting_helper_with_context(match serde_json::to_value(
&state.targeting_attributes,
)?);
) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("targeting_helper = nimbus::stateful::nimbus_client::NimbusClient::begin_initialize::serde_json::to_value".into(), e.to_string()))
});
recorded_context.execute_queries(targeting_helper.as_ref())?;
state
.targeting_attributes
Expand Down Expand Up @@ -512,7 +515,10 @@ impl NimbusClient {
let mut buf = String::new();
file.read_to_string(&mut buf)?;

let res = serde_json::from_str::<DateTime<Utc>>(&buf)?;
let res = match serde_json::from_str::<DateTime<Utc>>(&buf) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("res = nimbus::stateful::nimbus_client::get_creation_date_from_path::serde_json::from_str".into(), e.to_string()))
};
Ok(res)
}

Expand Down Expand Up @@ -630,7 +636,10 @@ impl NimbusClient {

fn merge_additional_context(&self, context: Option<JsonObject>) -> Result<Value> {
let context = context.map(Value::Object);
let targeting = serde_json::to_value(self.get_targeting_attributes())?;
let targeting = match serde_json::to_value(self.get_targeting_attributes()) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("targeting = nimbus::stateful::nimbus_client::NimbusClient::merge_additional_context::serde_json::to_value".into(), e.to_string()))
};
let context = match context {
Some(v) => v.defaults(&targeting)?,
None => targeting,
Expand Down
25 changes: 20 additions & 5 deletions components/nimbus/src/stateful/persistence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,10 @@ impl SingleStore {
key: &str,
persisted_data: &T,
) -> Result<()> {
let persisted_json = serde_json::to_string(persisted_data)?;
let persisted_json = match serde_json::to_string(persisted_data) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("persisted_json = nimbus::stateful::persistence::SingleStore::put::serde_json::to_string".into(), e.to_string()))
};
self.store
.put(writer, key, &rkv::Value::Json(&persisted_json))?;
Ok(())
Expand Down Expand Up @@ -187,7 +190,10 @@ impl SingleStore {
match persisted_data {
Some(data) => {
if let rkv::Value::Json(data) = data {
Ok(Some(serde_json::from_str::<T>(data)?))
Ok(Some(match serde_json::from_str::<T>(data) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("match persisted_data nimbus::stateful::persistence::SingleStore::get::serde_json::from_str".into(), e.to_string()))
}))
} else {
Err(NimbusError::InvalidPersistedData)
}
Expand Down Expand Up @@ -239,7 +245,10 @@ impl SingleStore {
let mut iter = self.store.iter_start(reader)?;
while let Some(Ok((_, data))) = iter.next() {
if let rkv::Value::Json(data) = data {
result.push(serde_json::from_str::<T>(data)?);
result.push(match serde_json::from_str::<T>(data) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("rkv::Value::Json(data) nimbus::stateful::persistence::SingleStore::collect_all::serde_json::from_str".into(), e.to_string()))
});
}
}
Ok(result)
Expand Down Expand Up @@ -510,7 +519,10 @@ impl Database {
match persisted_data {
Some(data) => {
if let rkv::Value::Json(data) = data {
Ok(Some(serde_json::from_str::<T>(data)?))
Ok(Some(match serde_json::from_str::<T>(data) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("rkv::Value::Json(data) nimbus::stateful::persistence::Database::get::serde_json::from_str".into(), e.to_string()))
}))
} else {
Err(NimbusError::InvalidPersistedData)
}
Expand All @@ -533,7 +545,10 @@ impl Database {
let mut iter = self.get_store(store_id).store.iter_start(&reader)?;
while let Some(Ok((_, data))) = iter.next() {
if let rkv::Value::Json(data) = data {
result.push(serde_json::from_str::<T>(data)?);
result.push(match serde_json::from_str::<T>(data) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("rkv::Value::Json(data) nimbus::stateful::persistence::Database::collect_all::serde_json::from_str".into(), e.to_string()))
});
}
}
Ok(result)
Expand Down
17 changes: 13 additions & 4 deletions components/nimbus/src/stateless/cirrus_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,10 @@ impl CirrusClient {
metrics_handler: Box<dyn MetricsHandler>,
coenrolling_feature_ids: Vec<String>,
) -> Result<Self> {
let app_context: AppContext = serde_json::from_str(&app_context)?;
let app_context: AppContext = match serde_json::from_str(&app_context) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("app_context = nimbus::stateless::cirrus_client::CirrusClient::new::serde_json::from_str".into(), e.to_string()))
};
Ok(Self {
app_context,
coenrolling_feature_ids,
Expand All @@ -112,7 +115,10 @@ impl CirrusClient {
request_context,
is_user_participating,
prev_enrollments,
} = serde_json::from_str(request.as_str())?;
} = match serde_json::from_str(request.as_str()) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("EnrollmentRequest { .. } = nimbus::stateless::cirrus_client::CirrusClient::handle_enrollment::serde_json::from_str".into(), e.to_string()))
};
let client_id = if let Some(client_id) = client_id {
client_id
} else {
Expand All @@ -121,12 +127,15 @@ impl CirrusClient {
));
};

Ok(serde_json::to_string(&self.enroll(
Ok(match serde_json::to_string(&self.enroll(
client_id,
request_context,
is_user_participating,
&prev_enrollments,
)?)?)
)?) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("return nimbus::stateless::cirrus_client::CirrusClient::handle_enrollment::serde_json::to_string".into(), e.to_string()))
})
}

pub(crate) fn enroll(
Expand Down
10 changes: 9 additions & 1 deletion components/nimbus/src/strings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,15 @@ use serde_json::{value::Value, Map};

#[allow(dead_code)]
pub fn fmt<T: serde::Serialize>(template: &str, context: &T) -> Result<String> {
let obj: Value = serde_json::to_value(context)?;
let obj: Value = match serde_json::to_value(context) {
Ok(v) => v,
Err(e) => {
return Err(NimbusError::JSONError(
"obj = nimbus::strings::fmt::serde_json::to_value".into(),
e.to_string(),
))
}
};

fmt_with_value(template, &obj)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ fn test_malformed_payload() {
"#;

let result = parse_experiments(payload).unwrap_err();
assert!(matches!(result, NimbusError::JSONError(_)));
assert!(matches!(result, NimbusError::JSONError(_, _)));
}

// This response body includes a matching schema version, a non-matching schema version,
Expand Down
8 changes: 4 additions & 4 deletions components/nimbus/src/tests/stateful/test_evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ fn test_event_transform_sum_cnz_avg_avgnz_parameters() {
&th,
),
Some(EnrollmentStatus::Error {
reason: "EvaluationError: Custom error: JSON Error: invalid type: floating point `1.0`, expected a string"
reason: "EvaluationError: Custom error: JSON Error: event = nimbus::stateful::behavior::EventQueryType::validate_counting_arguments::serde_json::from_value — invalid type: floating point `1.0`, expected a string"
.to_string()
})
);
Expand All @@ -168,7 +168,7 @@ fn test_event_transform_sum_cnz_avg_avgnz_parameters() {
&th,
),
Some(EnrollmentStatus::Error {
reason: "EvaluationError: Custom error: JSON Error: invalid type: floating point `1.0`, expected a string"
reason: "EvaluationError: Custom error: JSON Error: interval = nimbus::stateful::behavior::EventQueryType::validate_counting_arguments::serde_json::from_value — invalid type: floating point `1.0`, expected a string"
.to_string()
})
);
Expand Down Expand Up @@ -234,7 +234,7 @@ fn test_event_transform_last_seen_parameters() {
&th,
),
Some(EnrollmentStatus::Error {
reason: "EvaluationError: Custom error: JSON Error: invalid type: floating point `1.0`, expected a string"
reason: "EvaluationError: Custom error: JSON Error: event = nimbus::stateful::behavior::EventQueryType::validate_last_seen_arguments::serde_json::from_value — invalid type: floating point `1.0`, expected a string"
.to_string()
})
);
Expand All @@ -244,7 +244,7 @@ fn test_event_transform_last_seen_parameters() {
&th,
),
Some(EnrollmentStatus::Error {
reason: "EvaluationError: Custom error: JSON Error: invalid type: floating point `1.0`, expected a string"
reason: "EvaluationError: Custom error: JSON Error: interval = nimbus::stateful::behavior::EventQueryType::validate_last_seen_arguments::serde_json::from_value — invalid type: floating point `1.0`, expected a string"
.to_string()
})
);
Expand Down
15 changes: 11 additions & 4 deletions components/nimbus/tests/test_restart.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ mod test {
use super::common::new_test_client_with_db;
#[cfg(feature = "rkv-safe-mode")]
use nimbus::error::Result;
use nimbus::NimbusError;
use serde_json::json;

fn experiment_target_false() -> serde_json::Value {
Expand Down Expand Up @@ -132,12 +133,15 @@ mod test {
let temp_dir = tempfile::tempdir()?;
let client = new_test_client_with_db(&temp_dir)?;
client.initialize()?;
let experiment_json = serde_json::to_string(&json!({
let experiment_json = match serde_json::to_string(&json!({
"data": [
experiment_target_false(),
experiment_zero_buckets(),
]
}))?;
})) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("test".into(), e.to_string())),
};
client.set_experiments_locally(experiment_json.clone())?;
client.apply_pending_experiments()?;
// the experiment_target_false experiment has a 'targeting' of "false", we test to ensure that
Expand Down Expand Up @@ -180,13 +184,16 @@ mod test {
let ta = client.get_targeting_attributes();
assert_eq!(ta.active_experiments, expected);

let experiment_json = serde_json::to_string(&json!({
let experiment_json = match serde_json::to_string(&json!({
"data": [
experiment_target_false(),
experiment_zero_buckets(),
experiment_always_enroll(),
]
}))?;
})) {
Ok(v) => v,
Err(e) => return Err(NimbusError::JSONError("test".into(), e.to_string())),
};
client.set_experiments_locally(experiment_json)?;
client.apply_pending_experiments()?;

Expand Down

0 comments on commit 775da21

Please sign in to comment.