Skip to content

Commit

Permalink
Merge pull request #147 from Kuadrant/remove-unwrap
Browse files Browse the repository at this point in the history 
Replace all unwraps
  • Loading branch information
@adam-cattermole
adam-cattermole authored Nov 20, 2024
2 parents e88f73e + b3d4307 commit de863de
Show file tree
Hide file tree
Showing 10 changed files with 178 additions and 69 deletions.
23 changes: 13 additions & 10 deletions src/configuration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,7 @@ impl TryFrom<PluginConfiguration> for FilterConfig {
.expect("Predicates must not be compiled yet!");

for datum in &action.data {
let result = datum.item.compile();
if result.is_err() {
return Err(result.err().unwrap());
}
datum.item.compile()?;
}
}

Expand Down Expand Up @@ -204,7 +201,10 @@ impl<'de> Visitor<'de> for TimeoutVisitor {
E: Error,
{
match duration(Arc::new(string)) {
Ok(Value::Duration(duration)) => Ok(Timeout(duration.to_std().unwrap())),
Ok(Value::Duration(duration)) => duration
.to_std()
.map(Timeout)
.map_err(|e| E::custom(e.to_string())),
Err(e) => Err(E::custom(e)),
_ => Err(E::custom("Unsupported Duration Value")),
}
Expand Down Expand Up @@ -277,7 +277,7 @@ mod test {
}
assert!(res.is_ok());

let filter_config = res.unwrap();
let filter_config = res.expect("result is ok");
assert_eq!(filter_config.action_sets.len(), 1);

let services = &filter_config.services;
Expand Down Expand Up @@ -364,7 +364,7 @@ mod test {
}
assert!(res.is_ok());

let filter_config = res.unwrap();
let filter_config = res.expect("result is ok");
assert_eq!(filter_config.action_sets.len(), 0);
}

Expand Down Expand Up @@ -410,12 +410,15 @@ mod test {
}
assert!(res.is_ok());

let filter_config = res.unwrap();
let filter_config = res.expect("result is ok");
assert_eq!(filter_config.action_sets.len(), 1);

let services = &filter_config.services;
assert_eq!(
services.get("limitador").unwrap().timeout,
services
.get("limitador")
.expect("limitador service to be set")
.timeout,
Timeout(Duration::from_millis(20))
);

Expand Down Expand Up @@ -510,7 +513,7 @@ mod test {
}
assert!(res.is_ok());

let result = FilterConfig::try_from(res.unwrap());
let result = FilterConfig::try_from(res.expect("result is ok"));
let filter_config = result.expect("That didn't work");
let rlp_option = filter_config
.index
Expand Down
10 changes: 5 additions & 5 deletions src/configuration/action_set_index.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ mod tests {

let val = index.get_longest_match_action_sets("example.com");
assert!(val.is_some());
assert_eq!(val.unwrap()[0].name, "rlp1");
assert_eq!(val.expect("value must be some")[0].name, "rlp1");
}

#[test]
Expand All @@ -90,7 +90,7 @@ mod tests {
let val = index.get_longest_match_action_sets("test.example.com");

assert!(val.is_some());
assert_eq!(val.unwrap()[0].name, "rlp1");
assert_eq!(val.expect("value must be some")[0].name, "rlp1");
}

#[test]
Expand All @@ -103,11 +103,11 @@ mod tests {

let val = index.get_longest_match_action_sets("test.example.com");
assert!(val.is_some());
assert_eq!(val.unwrap()[0].name, "rlp2");
assert_eq!(val.expect("value must be some")[0].name, "rlp2");

let val = index.get_longest_match_action_sets("example.com");
assert!(val.is_some());
assert_eq!(val.unwrap()[0].name, "rlp1");
assert_eq!(val.expect("value must be some")[0].name, "rlp1");
}

#[test]
Expand All @@ -118,6 +118,6 @@ mod tests {

let val = index.get_longest_match_action_sets("test.example.com");
assert!(val.is_some());
assert_eq!(val.unwrap()[0].name, "rlp1");
assert_eq!(val.expect("value must be some")[0].name, "rlp1");
}
}
5 changes: 4 additions & 1 deletion src/data/attribute.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,10 @@ fn process_metadata(s: &Struct, prefix: String) -> Vec<(String, String)> {
let nested_struct = value.get_struct_value();
result.extend(process_metadata(nested_struct, current_prefix));
} else if let Some(v) = json {
result.push((current_prefix, serde_json::to_string(&v).unwrap()));
match serde_json::to_string(&v) {
Ok(ser) => result.push((current_prefix, ser)),
Err(e) => error!("failed to serialize json Value: {e:?}"),
}
}
}
result
Expand Down
86 changes: 58 additions & 28 deletions src/data/cel.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ use cel_parser::{parse, Expression as CelExpression, Member, ParseError};
use chrono::{DateTime, FixedOffset};
#[cfg(feature = "debug-host-behaviour")]
use log::debug;
use log::warn;
use proxy_wasm::types::{Bytes, Status};
use serde_json::Value as JsonValue;
use std::borrow::Cow;
use std::collections::hash_map::Entry;
use std::collections::HashMap;
use std::fmt::{Debug, Formatter};
Expand Down Expand Up @@ -87,7 +89,7 @@ impl Expression {

/// Decodes the query string and returns a Map where the key is the parameter's name and
/// the value is either a [`Value::String`] or a [`Value::List`] if the parameter's name is repeated
/// and the second arg is set not set to `false`.
/// and the second arg is not set to `false`.
/// see [`tests::decodes_query_string`]
fn decode_query_string(This(s): This<Arc<String>>, Arguments(args): Arguments) -> ResolveResult {
let allow_repeats = if args.len() == 2 {
Expand All @@ -102,8 +104,22 @@ fn decode_query_string(This(s): This<Arc<String>>, Arguments(args): Arguments) -
for part in s.split('&') {
let mut kv = part.split('=');
if let (Some(key), Some(value)) = (kv.next(), kv.next().or(Some(""))) {
let new_v: Value = decode(value).unwrap().into_owned().into();
match map.entry(decode(key).unwrap().into_owned().into()) {
let new_v: Value = decode(value)
.unwrap_or_else(|e| {
warn!("failed to decode query value, using default: {e:?}");
Cow::from(value)
})
.into_owned()
.into();
match map.entry(
decode(key)
.unwrap_or_else(|e| {
warn!("failed to decode query key, using default: {e:?}");
Cow::from(key)
})
.into_owned()
.into(),
) {
Entry::Occupied(mut e) => {
if allow_repeats {
if let Value::List(ref mut list) = e.get_mut() {
Expand All @@ -118,7 +134,15 @@ fn decode_query_string(This(s): This<Arc<String>>, Arguments(args): Arguments) -
}
}
Entry::Vacant(e) => {
e.insert(decode(value).unwrap().into_owned().into());
e.insert(
decode(value)
.unwrap_or_else(|e| {
warn!("failed to decode query value, using default: {e:?}");
Cow::from(value)
})
.into_owned()
.into(),
);
}
}
}
Expand Down Expand Up @@ -296,11 +320,11 @@ fn json_to_cel(json: &str) -> Value {
JsonValue::Bool(b) => b.into(),
JsonValue::Number(n) => {
if n.is_u64() {
n.as_u64().unwrap().into()
n.as_u64().expect("Unreachable: number must be u64").into()
} else if n.is_i64() {
n.as_i64().unwrap().into()
n.as_i64().expect("Unreachable: number must be i64").into()
} else {
n.as_f64().unwrap().into()
n.as_f64().expect("Unreachable: number must be f64").into()
}
}
JsonValue::String(str) => str.into(),
Expand Down Expand Up @@ -533,10 +557,14 @@ pub mod data {
fn it_works() {
let map = AttributeMap::new(
[
known_attribute_for(&"request.method".into()).unwrap(),
known_attribute_for(&"request.referer".into()).unwrap(),
known_attribute_for(&"source.address".into()).unwrap(),
known_attribute_for(&"destination.port".into()).unwrap(),
known_attribute_for(&"request.method".into())
.expect("request.method known attribute exists"),
known_attribute_for(&"request.referer".into())
.expect("request.referer known attribute exists"),
known_attribute_for(&"source.address".into())
.expect("source.address known attribute exists"),
known_attribute_for(&"destination.port".into())
.expect("destination.port known attribute exists"),
]
.into(),
);
Expand All @@ -548,38 +576,38 @@ pub mod data {
assert!(map.data.contains_key("destination"));
assert!(map.data.contains_key("request"));

match map.data.get("source").unwrap() {
match map.data.get("source").expect("source is some") {
Token::Node(map) => {
assert_eq!(map.len(), 1);
match map.get("address").unwrap() {
match map.get("address").expect("address is some") {
Token::Node(_) => panic!("Not supposed to get here!"),
Token::Value(v) => assert_eq!(v.path, "source.address".into()),
}
}
Token::Value(_) => panic!("Not supposed to get here!"),
}

match map.data.get("destination").unwrap() {
match map.data.get("destination").expect("destination is some") {
Token::Node(map) => {
assert_eq!(map.len(), 1);
match map.get("port").unwrap() {
match map.get("port").expect("port is some") {
Token::Node(_) => panic!("Not supposed to get here!"),
Token::Value(v) => assert_eq!(v.path, "destination.port".into()),
}
}
Token::Value(_) => panic!("Not supposed to get here!"),
}

match map.data.get("request").unwrap() {
match map.data.get("request").expect("request is some") {
Token::Node(map) => {
assert_eq!(map.len(), 2);
assert!(map.get("method").is_some());
match map.get("method").unwrap() {
match map.get("method").expect("method is some") {
Token::Node(_) => panic!("Not supposed to get here!"),
Token::Value(v) => assert_eq!(v.path, "request.method".into()),
}
assert!(map.get("referer").is_some());
match map.get("referer").unwrap() {
match map.get("referer").expect("referer is some") {
Token::Node(_) => panic!("Not supposed to get here!"),
Token::Value(v) => assert_eq!(v.path, "request.referer".into()),
}
Expand Down Expand Up @@ -611,7 +639,7 @@ mod tests {
let value = Expression::new(
"auth.identity.anonymous && auth.identity != null && auth.identity.foo > 3",
)
.unwrap();
.expect("This is valid CEL!");
assert_eq!(value.attributes.len(), 3);
assert_eq!(value.attributes[0].path, "auth.identity".into());
}
Expand All @@ -626,7 +654,7 @@ mod tests {
"true".bytes().collect(),
)));
let value = Expression::new("auth.identity.anonymous")
.unwrap()
.expect("This is valid CEL!")
.eval()
.expect("This must evaluate!");
assert_eq!(value, true.into());
Expand All @@ -636,7 +664,7 @@ mod tests {
"42".bytes().collect(),
)));
let value = Expression::new("auth.identity.age")
.unwrap()
.expect("This is valid CEL!")
.eval()
.expect("This must evaluate!");
assert_eq!(value, 42.into());
Expand All @@ -646,7 +674,7 @@ mod tests {
"42.3".bytes().collect(),
)));
let value = Expression::new("auth.identity.age")
.unwrap()
.expect("This is valid CEL!")
.eval()
.expect("This must evaluate!");
assert_eq!(value, 42.3.into());
Expand All @@ -656,7 +684,7 @@ mod tests {
"\"John\"".bytes().collect(),
)));
let value = Expression::new("auth.identity.age")
.unwrap()
.expect("This is valid CEL!")
.eval()
.expect("This must evaluate!");
assert_eq!(value, "John".into());
Expand All @@ -666,7 +694,7 @@ mod tests {
"-42".bytes().collect(),
)));
let value = Expression::new("auth.identity.name")
.unwrap()
.expect("This is valid CEL!")
.eval()
.expect("This must evaluate!");
assert_eq!(value, (-42).into());
Expand All @@ -677,7 +705,7 @@ mod tests {
"some random crap".bytes().collect(),
)));
let value = Expression::new("auth.identity.age")
.unwrap()
.expect("This is valid CEL!")
.eval()
.expect("This must evaluate!");
assert_eq!(value, "some random crap".into());
Expand Down Expand Up @@ -751,12 +779,14 @@ mod tests {
80_i64.to_le_bytes().into(),
)));
let value = known_attribute_for(&"destination.port".into())
.unwrap()
.expect("destination.port known attribute exists")
.get();
assert_eq!(value, 80.into());
property::test::TEST_PROPERTY_VALUE
.set(Some(("request.method".into(), "GET".bytes().collect())));
let value = known_attribute_for(&"request.method".into()).unwrap().get();
let value = known_attribute_for(&"request.method".into())
.expect("request.method known attribute exists")
.get();
assert_eq!(value, "GET".into());
}

Expand All @@ -778,7 +808,7 @@ mod tests {
b"\xCA\xFE".to_vec(),
)));
let value = Expression::new("getHostProperty(['foo', 'bar.baz'])")
.unwrap()
.expect("This is valid CEL!")
.eval()
.expect("This must evaluate!");
assert_eq!(value, Value::Bytes(Arc::new(b"\xCA\xFE".to_vec())));
Expand Down
3 changes: 2 additions & 1 deletion src/filter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ extern "C" fn start() {

proxy_wasm::set_log_level(LogLevel::Trace);
std::panic::set_hook(Box::new(|panic_info| {
proxy_wasm::hostcalls::log(LogLevel::Critical, &panic_info.to_string()).unwrap();
proxy_wasm::hostcalls::log(LogLevel::Critical, &panic_info.to_string())
.expect("failed to log panic_info");
}));
proxy_wasm::set_root_context(|context_id| -> Box<dyn RootContext> {
info!("#{} set_root_context", context_id);
Expand Down
4 changes: 3 additions & 1 deletion src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ mod tests {
.push(header("test", "some value"));
resp.response_headers_to_add
.push(header("other", "header value"));
let buffer = resp.write_to_bytes().unwrap();
let buffer = resp
.write_to_bytes()
.expect("must be able to write RateLimitResponse to bytes");
let expected: [u8; 45] = [
8, 1, 26, 18, 10, 4, 116, 101, 115, 116, 18, 10, 115, 111, 109, 101, 32, 118, 97, 108,
117, 101, 26, 21, 10, 5, 111, 116, 104, 101, 114, 18, 12, 104, 101, 97, 100, 101, 114,
Expand Down
Loading

0 comments on commit de863de

Please sign in to comment.