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

Proper Error types for the crate #386

Merged
merged 4 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
1,055 changes: 621 additions & 434 deletions Cargo.lock

Large diffs are not rendered by default.

10 changes: 5 additions & 5 deletions limitador-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ distributed_storage = ["limitador/distributed_storage"]
[dependencies]
limitador = { path = "../limitador", features = ['lenient_conditions'] }
tokio = { version = "1", features = ["full"] }
thiserror = "1"
thiserror = "2"
tonic = "0.12.3"
tonic-reflection = "0.12.3"
prost = "0.13.3"
Expand All @@ -38,17 +38,17 @@ opentelemetry-otlp = "0.15"
url = "2"
actix-web = "4.1"
actix-rt = "2"
paperclip = { version = "0.8.0", features = ["actix4"] }
paperclip = { version = "0.9", features = ["actix4"] }
serde = { version = "1", features = ["derive"] }
notify = "6.0.1"
notify = "7"
const_format = "0.2.31"
lazy_static = "1.4.0"
clap = "4.3"
sysinfo = "0.30.10"
sysinfo = "0.32"
openssl = { version = "0.10.66", features = ["vendored"] }
metrics = "0.22.3"
metrics-exporter-prometheus = "0.14.0"


[build-dependencies]
tonic-build = "0.11"
tonic-build = "0.12"
2 changes: 1 addition & 1 deletion limitador-server/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ fn generate_protobuf() -> Result<(), Box<dyn Error>> {
tonic_build::configure()
.build_server(true)
.file_descriptor_set_path(original_out_dir.join("rls.bin"))
.compile(
.compile_protos(
&["envoy/service/ratelimit/v3/rls.proto"],
&[
"vendor/protobufs/data-plane-api",
Expand Down
15 changes: 10 additions & 5 deletions limitador-server/src/envoy_rls/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,8 @@ mod tests {
60,
vec!["req.method == 'GET'"],
vec!["app_id"],
);
)
.expect("This must be a valid limit!");

let limiter = RateLimiter::new(10_000);
limiter.add_limit(limit);
Expand Down Expand Up @@ -394,8 +395,10 @@ mod tests {
let namespace = "test_namespace";

vec![
Limit::new(namespace, 10, 60, vec!["x == '1'"], vec!["z"]),
Limit::new(namespace, 0, 60, vec!["x == '1'", "y == '2'"], vec!["z"]),
Limit::new(namespace, 10, 60, vec!["x == '1'"], vec!["z"])
.expect("This must be a valid limit!"),
Limit::new(namespace, 0, 60, vec!["x == '1'", "y == '2'"], vec!["z"])
.expect("This must be a valid limit!"),
]
.into_iter()
.for_each(|limit| {
Expand Down Expand Up @@ -459,7 +462,8 @@ mod tests {
#[tokio::test]
async fn test_takes_into_account_the_hits_addend_param() {
let namespace = "test_namespace";
let limit = Limit::new(namespace, 10, 60, vec!["x == '1'"], vec!["y"]);
let limit = Limit::new(namespace, 10, 60, vec!["x == '1'"], vec!["y"])
.expect("This must be a valid limit!");

let limiter = RateLimiter::new(10_000);
limiter.add_limit(limit);
Expand Down Expand Up @@ -528,7 +532,8 @@ mod tests {
// "hits_addend" is optional according to the spec, and should default
// to 1, However, with the autogenerated structs it defaults to 0.
let namespace = "test_namespace";
let limit = Limit::new(namespace, 1, 60, vec!["x == '1'"], vec!["y"]);
let limit = Limit::new(namespace, 1, 60, vec!["x == '1'"], vec!["y"])
.expect("This must be a valid limit!");

let limiter = RateLimiter::new(10_000);
limiter.add_limit(limit);
Expand Down
14 changes: 8 additions & 6 deletions limitador-server/src/http_api/request_types.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use limitador::counter::Counter as LimitadorCounter;
use limitador::errors::LimitadorError;
use limitador::limit::Limit as LimitadorLimit;
use paperclip::actix::Apiv2Schema;
use serde::{Deserialize, Serialize};
use std::collections::{BTreeMap, HashMap};

// We need to define the Limit and Counter types. They're basically the same as
// defined in the lib but with some modifications to be able to derive
// Apiv2Schema (needed to generate the OpenAPI specs).
Expand Down Expand Up @@ -41,8 +41,10 @@ impl From<&LimitadorLimit> for Limit {
}
}

impl From<Limit> for LimitadorLimit {
fn from(limit: Limit) -> Self {
impl TryFrom<Limit> for LimitadorLimit {
type Error = LimitadorError;

fn try_from(limit: Limit) -> Result<Self, Self::Error> {
let mut limitador_limit = if let Some(id) = limit.id {
Self::with_id(
id,
Expand All @@ -51,22 +53,22 @@ impl From<Limit> for LimitadorLimit {
limit.seconds,
limit.conditions,
limit.variables,
)
)?
} else {
Self::new(
limit.namespace,
limit.max_value,
limit.seconds,
limit.conditions,
limit.variables,
)
)?
};

if let Some(name) = limit.name {
limitador_limit.set_name(name)
}

limitador_limit
Ok(limitador_limit)
}
}

Expand Down
3 changes: 2 additions & 1 deletion limitador-server/src/http_api/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,8 @@ mod tests {
60,
vec!["req.method == 'GET'"],
vec!["app_id"],
);
)
.expect("This must be a valid limit!");

match &limiter {
Limiter::Blocking(limiter) => limiter.add_limit(limit.clone()),
Expand Down
13 changes: 4 additions & 9 deletions limitador/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,14 @@ lenient_conditions = []

[dependencies]
moka = { version = "0.12", features = ["sync"] }
dashmap = "5.5.3"
getrandom = { version = "0.2", features = ["js"] }
dashmap = "6.1"
serde = { version = "1", features = ["derive", "rc"] }
postcard = { version = "1.0.4", features = ["use-std"] }
serde_json = "1"
rmp-serde = "1.1.0"
thiserror = "1"
futures = "0.3"
async-trait = "0.1"
cfg-if = "1"
tracing = "0.1.40"
metrics = "0.22.3"
metrics = "0.24"

# Optional dependencies
rocksdb = { version = "0.22", optional = true, features = ["multi-threaded-cf"] }
Expand All @@ -52,13 +48,12 @@ tokio = { version = "1", optional = true, features = [

base64 = { version = "0.22", optional = true }
tokio-stream = { version = "0.1", optional = true }
h2 = { version = "0.3", optional = true }
h2 = { version = "0.4", optional = true }
uuid = { version = "1.8.0", features = ["v4", "fast-rng"], optional = true }
tonic = { version = "0.12.3", optional = true }
tonic-reflection = { version = "0.12.3", optional = true }
prost = { version = "0.13.3", optional = true }
prost-types = { version = "0.13.3", optional = true }
time = "0.3.36"

[dev-dependencies]
serial_test = "3.0"
Expand All @@ -80,7 +75,7 @@ tokio = { version = "1", features = [
] }

[build-dependencies]
tonic-build = "0.11"
tonic-build = "0.12"

[[bench]]
name = "bench"
Expand Down
17 changes: 10 additions & 7 deletions limitador/benches/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -547,13 +547,16 @@ fn generate_test_limits(scenario: &TestScenario) -> (Vec<Limit>, Vec<TestCallPar
let namespace = idx_namespace.to_string();

for limit_idx in 0..scenario.n_limits_per_ns {
test_limits.push(Limit::new(
namespace.clone(),
u64::MAX,
((limit_idx * 60) + 10) as u64,
conditions.clone(),
variables.clone(),
))
test_limits.push(
Limit::new(
namespace.clone(),
u64::MAX,
((limit_idx * 60) + 10) as u64,
conditions.clone(),
variables.clone(),
)
.expect("This must be a valid limit!"),
)
}

call_params.push(TestCallParams {
Expand Down
2 changes: 1 addition & 1 deletion limitador/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ fn generate_protobuf() -> Result<(), Box<dyn Error>> {

tonic_build::configure()
.protoc_arg("--experimental_allow_proto3_optional")
.compile(&[proto_path], &[proto_dir])?;
.compile_protos(&[proto_path], &[proto_dir])?;
}

Ok(())
Expand Down
47 changes: 42 additions & 5 deletions limitador/src/errors.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,51 @@
use crate::limit::ConditionParsingError;
use crate::storage::StorageErr;
use thiserror::Error;
use std::convert::Infallible;
use std::error::Error;
use std::fmt::{Display, Formatter};

#[derive(Error, Debug, Eq, PartialEq)]
#[derive(Debug)]
pub enum LimitadorError {
#[error("error while accessing the limits storage: {0:?}")]
Storage(String),
StorageError(StorageErr),
InterpreterError(ConditionParsingError),
}

impl Display for LimitadorError {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
match self {
LimitadorError::StorageError(err) => {
write!(f, "error while accessing the limits storage: {err:?}")
}
LimitadorError::InterpreterError(err) => {
write!(f, "error parsing condition: {err:?}")
}
}
}
}

impl Error for LimitadorError {
fn source(&self) -> Option<&(dyn Error + 'static)> {
match self {
LimitadorError::StorageError(err) => Some(err),
LimitadorError::InterpreterError(err) => Some(err),
}
}
}

impl From<StorageErr> for LimitadorError {
fn from(e: StorageErr) -> Self {
Self::Storage(e.msg().to_owned())
Self::StorageError(e)
}
}

impl From<ConditionParsingError> for LimitadorError {
fn from(err: ConditionParsingError) -> Self {
LimitadorError::InterpreterError(err)
}
}

impl From<Infallible> for LimitadorError {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:
out of curiosity, why do we need to add this implementation?

When I apply this patch, it is not needed.

diff --git a/limitador/src/errors.rs b/limitador/src/errors.rs
index d590aaf..3de262e 100644
--- a/limitador/src/errors.rs
+++ b/limitador/src/errors.rs
@@ -1,6 +1,5 @@
 use crate::limit::ConditionParsingError;
 use crate::storage::StorageErr;
-use std::convert::Infallible;
 use std::error::Error;
 use std::fmt::{Display, Formatter};
 
@@ -43,9 +42,3 @@ impl From<ConditionParsingError> for LimitadorError {
         LimitadorError::InterpreterError(err)
     }
 }
-
-impl From<Infallible> for LimitadorError {
-    fn from(value: Infallible) -> Self {
-        unreachable!("unexpected infallible value: {:?}", value)
-    }
-}
diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs
index 94b95b1..511f083 100644
--- a/limitador/src/limit.rs
+++ b/limitador/src/limit.rs
@@ -1080,7 +1080,7 @@ mod tests {
             limit1.namespace.clone(),
             limit1.max_value + 10,
             limit1.seconds,
-            limit1.conditions.clone(),
+            vec!["req.method == 'GET'"],
             limit1.variables.clone(),
         )
         .expect("This must be a valid limit!");

Not arguing to change it, just curiosity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you pass an iterator of Condition which are TryFrom... but Infallible.
It'll go away, when I deal with the rest of the refactoring, but yeah... It is perfectly fine to pass Conditions right in the Limit that you know cannot fail.

fn from(value: Infallible) -> Self {
unreachable!("unexpected infallible value: {:?}", value)
}
}
Loading
Loading