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

Proper Error types for the crate #386

merged 4 commits into from
Nov 26, 2024

Conversation

alexsnaps
Copy link
Member

@alexsnaps alexsnaps commented Nov 15, 2024

Fixes #131

This is a first pass at this. LimitadorError::InterpreterError will "only" be used as a mean to cover for runtime errors, e.g. an expression that can't be resolved (see #385). Also, when doing that work, we should be able to return to a Limit::new -> Self over the current result and clean that API up a bit.

Pretty sure other cases will emerge for Limitador::Error as we move towards v2.0 of the server, but hopefully v1 for the crate 🤞

Signed-off-by: Alex Snaps <[email protected]>
@alexsnaps alexsnaps force-pushed the error_types branch 2 times, most recently from cffdc82 to 539d53c Compare November 25, 2024 19:58
@alexsnaps alexsnaps marked this pull request as ready for review November 25, 2024 19:59
@alexsnaps alexsnaps requested a review from a team November 25, 2024 20:04
Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

LGTM

}
}

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.

@alexsnaps alexsnaps merged commit d17acdb into main Nov 26, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Have proper error for the crate
2 participants