Skip to content

Commit c192a4f

Browse files
aepfliclaude
aepfli
and
claude
authored
refactor: improve architecture and add edge case tests (#54)
- Extract generic evaluator dispatcher to eliminate type-specific duplication in evaluator.rs (~80 lines reduced via evaluate_with<F>() method) - Fix silent memory allocation failure by ensuring empty strings return non-null pointers, allowing callers to distinguish from allocation errors - Standardize error types with thiserror crate for consistent error handling - Use RefCell for WASM singleton instead of Mutex (semantically correct for single-threaded WASM, with Mutex fallback for native targets) - Add comprehensive edge case tests for operators and memory management: - Fractional operator: single bucket, unequal weights - Unicode: flag keys, context values, emoji in variants - Memory: large allocations, consecutive alloc/dealloc - Targeting: deeply nested rules, flag removal/re-addition - Semantic versioning edge cases 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Description <!-- Provide a brief description of your changes --> ## Related Issue <!-- Link to the related issue(s) --> Closes # ## Type of Change <!-- Mark the relevant option with an "x" --> - [ ] `feat`: New feature (minor version bump) - [ ] `fix`: Bug fix (patch version bump) - [ ] `docs`: Documentation only changes - [ ] `chore`: Maintenance tasks, dependency updates - [ ] `refactor`: Code refactoring without functional changes - [ ] `test`: Adding or updating tests - [ ] `ci`: CI/CD changes - [ ] `perf`: Performance improvements - [ ] `build`: Build system changes - [ ] `style`: Code style/formatting changes ## PR Title Format **IMPORTANT**: Since we use squash and merge, your PR title will become the commit message. Please ensure your PR title follows the [Conventional Commits](https://www.conventionalcommits.org/) format: ``` <type>(<optional-scope>): <description> ``` ### Examples: - `feat(operators): add new string comparison operator` - `fix(wasm): correct memory allocation bug` - `docs: update API examples in README` - `chore(deps): update rust dependencies` For breaking changes, use `!` after the type/scope or include `BREAKING CHANGE:` in the PR description: - `feat(api)!: redesign evaluation API` ## Testing <!-- Describe the testing you've performed --> - [ ] Unit tests added/updated - [ ] Integration tests added/updated - [ ] Manual testing performed - [ ] All tests pass (`cargo test`) - [ ] Code is formatted (`cargo fmt`) - [ ] Clippy checks pass (`cargo clippy -- -D warnings`) - [ ] WASM builds successfully (if applicable) ## Breaking Changes <!-- If this introduces breaking changes, describe them here --> - [ ] This PR includes breaking changes - [ ] Documentation has been updated to reflect breaking changes - [ ] Migration guide included (if needed) ## Additional Notes <!-- Any additional information, context, or screenshots --> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent f0fe238 commit c192a4f

6 files changed

Lines changed: 710 additions & 173 deletions

File tree

‎Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ serde = { version = "1.0", features = ["derive", "alloc"], default-features = fa
2222
serde_json = { version = "1.0", features = ["alloc"], default-features = false }
2323
boon = "0.6"
2424
murmurhash3 = "0.0.5"
25+
thiserror = "2.0"
2526
# Override ahash to avoid SIMD/AES-NI instructions that break Chicory WASM compatibility
2627
# ahash is pulled in by boon and uses AES-NI by default
2728
# We disable default features and use compile-time-rng to avoid runtime randomness

‎src/error.rs‎

Lines changed: 82 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
//! Error types for the flagd-evaluator library.
22
//!
3-
//! This module provides structured error types that serialize to JSON
4-
//! for consistent error handling across the WASM boundary.
3+
//! This module provides structured error types using thiserror for consistent
4+
//! error handling across the library and WASM boundary.
55
66
use serde::{Deserialize, Serialize};
7+
use thiserror::Error;
78

89
/// The type of error that occurred during evaluation.
9-
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
10+
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
1011
#[serde(rename_all = "snake_case")]
1112
pub enum ErrorType {
1213
/// Error parsing JSON input
@@ -17,10 +18,17 @@ pub enum ErrorType {
1718
MemoryError,
1819
/// Invalid input provided
1920
InvalidInput,
21+
/// Flag not found
22+
FlagNotFound,
23+
/// Type mismatch in flag value
24+
TypeMismatch,
25+
/// Configuration validation error
26+
ValidationError,
2027
}
2128

2229
/// Represents an error that occurred during evaluation.
23-
#[derive(Debug, Clone, Serialize, Deserialize)]
30+
#[derive(Debug, Clone, Error, Serialize, Deserialize)]
31+
#[error("{error_type:?}: {message}")]
2432
pub struct EvaluatorError {
2533
/// Human-readable error message
2634
pub message: String,
@@ -78,15 +86,51 @@ impl EvaluatorError {
7886
error_type: ErrorType::InvalidInput,
7987
}
8088
}
81-
}
8289

83-
impl std::fmt::Display for EvaluatorError {
84-
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
85-
write!(f, "{:?}: {}", self.error_type, self.message)
90+
/// Creates a new flag not found error.
91+
///
92+
/// # Arguments
93+
/// * `flag_key` - The key of the flag that was not found
94+
pub fn flag_not_found(flag_key: impl Into<String>) -> Self {
95+
Self {
96+
message: format!("Flag not found: {}", flag_key.into()),
97+
error_type: ErrorType::FlagNotFound,
98+
}
99+
}
100+
101+
/// Creates a new type mismatch error.
102+
///
103+
/// # Arguments
104+
/// * `message` - Description of the type mismatch
105+
pub fn type_mismatch(message: impl Into<String>) -> Self {
106+
Self {
107+
message: message.into(),
108+
error_type: ErrorType::TypeMismatch,
109+
}
110+
}
111+
112+
/// Creates a new validation error.
113+
///
114+
/// # Arguments
115+
/// * `message` - Description of the validation failure
116+
pub fn validation_error(message: impl Into<String>) -> Self {
117+
Self {
118+
message: message.into(),
119+
error_type: ErrorType::ValidationError,
120+
}
86121
}
87-
}
88122

89-
impl std::error::Error for EvaluatorError {}
123+
/// Converts the error to a JSON string.
124+
pub fn to_json_string(&self) -> String {
125+
serde_json::to_string(self).unwrap_or_else(|_| {
126+
format!(
127+
r#"{{"error_type":"{}","message":"{}"}}"#,
128+
serde_json::to_string(&self.error_type).unwrap_or_default(),
129+
self.message
130+
)
131+
})
132+
}
133+
}
90134

91135
#[cfg(test)]
92136
mod tests {
@@ -104,5 +148,33 @@ mod tests {
104148
let err = EvaluatorError::evaluation_error("eval failed");
105149
let display = format!("{}", err);
106150
assert!(display.contains("eval failed"));
151+
assert!(display.contains("EvaluationError"));
152+
}
153+
154+
#[test]
155+
fn test_flag_not_found_error() {
156+
let err = EvaluatorError::flag_not_found("my-flag");
157+
assert_eq!(err.error_type, ErrorType::FlagNotFound);
158+
assert!(err.message.contains("my-flag"));
159+
}
160+
161+
#[test]
162+
fn test_type_mismatch_error() {
163+
let err = EvaluatorError::type_mismatch("expected boolean, got string");
164+
assert_eq!(err.error_type, ErrorType::TypeMismatch);
165+
}
166+
167+
#[test]
168+
fn test_validation_error() {
169+
let err = EvaluatorError::validation_error("invalid config");
170+
assert_eq!(err.error_type, ErrorType::ValidationError);
171+
}
172+
173+
#[test]
174+
fn test_error_to_json() {
175+
let err = EvaluatorError::parse_error("test");
176+
let json = err.to_json_string();
177+
assert!(json.contains("parse_error"));
178+
assert!(json.contains("test"));
107179
}
108180
}

‎src/evaluator.rs‎

Lines changed: 75 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -150,136 +150,110 @@ impl FlagEvaluator {
150150
self.state = None;
151151
}
152152

153-
/// Evaluates a boolean flag.
154-
pub fn evaluate_bool(&self, flag_key: &str, context: &JsonValue) -> EvaluationResult {
153+
/// Generic evaluation dispatcher that eliminates duplication across type-specific methods.
154+
///
155+
/// # Arguments
156+
/// * `flag_key` - The key of the flag to evaluate
157+
/// * `context` - The evaluation context
158+
/// * `eval_fn` - The type-specific evaluation function to call
159+
/// * `default_value` - The default value to return if no state is loaded
160+
fn evaluate_with<F>(
161+
&self,
162+
flag_key: &str,
163+
context: &JsonValue,
164+
eval_fn: F,
165+
default_value: JsonValue,
166+
) -> EvaluationResult
167+
where
168+
F: FnOnce(
169+
&crate::model::FeatureFlag,
170+
&JsonValue,
171+
&std::collections::HashMap<String, JsonValue>,
172+
) -> EvaluationResult,
173+
{
155174
match &self.state {
156175
Some(state) => {
157176
let flag = state
158177
.flags
159178
.get(flag_key)
160179
.cloned()
161180
.unwrap_or_else(|| self.empty_flag(flag_key));
162-
crate::evaluation::evaluate_bool_flag(&flag, context, &state.flag_set_metadata)
181+
eval_fn(&flag, context, &state.flag_set_metadata)
163182
}
164-
None => EvaluationResult {
165-
value: JsonValue::Bool(false),
166-
variant: None,
167-
reason: crate::evaluation::ResolutionReason::Error,
168-
error_code: Some(crate::evaluation::ErrorCode::General),
169-
error_message: Some("No flag configuration loaded".to_string()),
170-
flag_metadata: None,
171-
},
183+
None => Self::no_state_error(default_value),
184+
}
185+
}
186+
187+
/// Creates a standard error response for when no state is loaded.
188+
fn no_state_error(default_value: JsonValue) -> EvaluationResult {
189+
EvaluationResult {
190+
value: default_value,
191+
variant: None,
192+
reason: crate::evaluation::ResolutionReason::Error,
193+
error_code: Some(crate::evaluation::ErrorCode::General),
194+
error_message: Some("No flag configuration loaded".to_string()),
195+
flag_metadata: None,
172196
}
173197
}
174198

199+
/// Evaluates a boolean flag.
200+
pub fn evaluate_bool(&self, flag_key: &str, context: &JsonValue) -> EvaluationResult {
201+
self.evaluate_with(
202+
flag_key,
203+
context,
204+
crate::evaluation::evaluate_bool_flag,
205+
JsonValue::Bool(false),
206+
)
207+
}
208+
175209
/// Evaluates a string flag.
176210
pub fn evaluate_string(&self, flag_key: &str, context: &JsonValue) -> EvaluationResult {
177-
match &self.state {
178-
Some(state) => {
179-
let flag = state
180-
.flags
181-
.get(flag_key)
182-
.cloned()
183-
.unwrap_or_else(|| self.empty_flag(flag_key));
184-
crate::evaluation::evaluate_string_flag(&flag, context, &state.flag_set_metadata)
185-
}
186-
None => EvaluationResult {
187-
value: JsonValue::String(String::new()),
188-
variant: None,
189-
reason: crate::evaluation::ResolutionReason::Error,
190-
error_code: Some(crate::evaluation::ErrorCode::General),
191-
error_message: Some("No flag configuration loaded".to_string()),
192-
flag_metadata: None,
193-
},
194-
}
211+
self.evaluate_with(
212+
flag_key,
213+
context,
214+
crate::evaluation::evaluate_string_flag,
215+
JsonValue::String(String::new()),
216+
)
195217
}
196218

197219
/// Evaluates an integer flag.
198220
pub fn evaluate_int(&self, flag_key: &str, context: &JsonValue) -> EvaluationResult {
199-
match &self.state {
200-
Some(state) => {
201-
let flag = state
202-
.flags
203-
.get(flag_key)
204-
.cloned()
205-
.unwrap_or_else(|| self.empty_flag(flag_key));
206-
crate::evaluation::evaluate_int_flag(&flag, context, &state.flag_set_metadata)
207-
}
208-
None => EvaluationResult {
209-
value: JsonValue::Number(0.into()),
210-
variant: None,
211-
reason: crate::evaluation::ResolutionReason::Error,
212-
error_code: Some(crate::evaluation::ErrorCode::General),
213-
error_message: Some("No flag configuration loaded".to_string()),
214-
flag_metadata: None,
215-
},
216-
}
221+
self.evaluate_with(
222+
flag_key,
223+
context,
224+
crate::evaluation::evaluate_int_flag,
225+
JsonValue::Number(0.into()),
226+
)
217227
}
218228

219229
/// Evaluates a float flag.
220230
pub fn evaluate_float(&self, flag_key: &str, context: &JsonValue) -> EvaluationResult {
221-
match &self.state {
222-
Some(state) => {
223-
let flag = state
224-
.flags
225-
.get(flag_key)
226-
.cloned()
227-
.unwrap_or_else(|| self.empty_flag(flag_key));
228-
crate::evaluation::evaluate_float_flag(&flag, context, &state.flag_set_metadata)
229-
}
230-
None => EvaluationResult {
231-
value: JsonValue::Number(serde_json::Number::from_f64(0.0).unwrap()),
232-
variant: None,
233-
reason: crate::evaluation::ResolutionReason::Error,
234-
error_code: Some(crate::evaluation::ErrorCode::General),
235-
error_message: Some("No flag configuration loaded".to_string()),
236-
flag_metadata: None,
237-
},
238-
}
231+
self.evaluate_with(
232+
flag_key,
233+
context,
234+
crate::evaluation::evaluate_float_flag,
235+
JsonValue::Number(serde_json::Number::from_f64(0.0).unwrap()),
236+
)
239237
}
240238

241239
/// Evaluates a generic flag (for objects/structs).
242240
pub fn evaluate_flag(&self, flag_key: &str, context: &JsonValue) -> EvaluationResult {
243-
match &self.state {
244-
Some(state) => {
245-
let flag = state
246-
.flags
247-
.get(flag_key)
248-
.cloned()
249-
.unwrap_or_else(|| self.empty_flag(flag_key));
250-
crate::evaluation::evaluate_flag(&flag, context, &state.flag_set_metadata)
251-
}
252-
None => EvaluationResult {
253-
value: JsonValue::Object(serde_json::Map::new()),
254-
variant: None,
255-
reason: crate::evaluation::ResolutionReason::Error,
256-
error_code: Some(crate::evaluation::ErrorCode::General),
257-
error_message: Some("No flag configuration loaded".to_string()),
258-
flag_metadata: None,
259-
},
260-
}
241+
self.evaluate_with(
242+
flag_key,
243+
context,
244+
crate::evaluation::evaluate_flag,
245+
JsonValue::Object(serde_json::Map::new()),
246+
)
261247
}
262248

263249
/// Evaluates an object flag with type checking.
264250
pub fn evaluate_object(&self, flag_key: &str, context: &JsonValue) -> EvaluationResult {
265-
match &self.state {
266-
Some(state) => {
267-
let flag = state
268-
.flags
269-
.get(flag_key)
270-
.cloned()
271-
.unwrap_or_else(|| self.empty_flag(flag_key));
272-
crate::evaluation::evaluate_object_flag(&flag, context, &state.flag_set_metadata)
273-
}
274-
None => EvaluationResult {
275-
value: JsonValue::Object(serde_json::Map::new()),
276-
variant: None,
277-
reason: crate::evaluation::ResolutionReason::Error,
278-
error_code: Some(crate::evaluation::ErrorCode::General),
279-
error_message: Some("No flag configuration loaded".to_string()),
280-
flag_metadata: None,
281-
},
282-
}
251+
self.evaluate_with(
252+
flag_key,
253+
context,
254+
crate::evaluation::evaluate_object_flag,
255+
JsonValue::Object(serde_json::Map::new()),
256+
)
283257
}
284258

285259
/// Helper to create an empty flag for missing flags.

0 commit comments

Comments
 (0)