Skip to content

Commit 67bd38c

Browse files
committed
Reduce duplication and improve readability across codebase
- Extract test_route() helper in lib.rs to replace repeated Route struct literals - Extract create_client() helper in integration tests to replace 15 identical ClientBuilder chains - Extract single_thread_rt() helper in benchmarks to deduplicate tokio runtime creation - Simplify catch-all route warning from two-pass collect+iterate to single-pass O(n) - Replace magic `/ 2` jitter divisor with named JITTER_FRACTION_DENOM constant - Use consistent let-chain style for host matching, matching the method check pattern
1 parent 015efbc commit 67bd38c

6 files changed

Lines changed: 67 additions & 130 deletions

File tree

benches/throughput.rs

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,13 @@ use route_ratelimit::{RateLimitMiddleware, ThrottleBehavior};
44
use std::sync::Arc;
55
use std::time::Duration;
66

7+
fn single_thread_rt() -> tokio::runtime::Runtime {
8+
tokio::runtime::Builder::new_current_thread()
9+
.enable_time()
10+
.build()
11+
.unwrap()
12+
}
13+
714
/// Benchmark the full check_and_apply_limits path with varying route counts.
815
fn bench_check_and_apply_limits(c: &mut Criterion) {
916
let mut group = c.benchmark_group("check_and_apply_limits");
@@ -30,10 +37,7 @@ fn bench_check_and_apply_limits(c: &mut Criterion) {
3037
BenchmarkId::new("routes", route_count),
3138
&route_count,
3239
|b, _| {
33-
let rt = tokio::runtime::Builder::new_current_thread()
34-
.enable_time()
35-
.build()
36-
.unwrap();
40+
let rt = single_thread_rt();
3741
b.iter(|| {
3842
rt.block_on(async { black_box(middleware.check_and_apply_limits(&req).await) })
3943
})
@@ -120,10 +124,7 @@ fn bench_stacked_limits(c: &mut Criterion) {
120124
BenchmarkId::new("limits", limit_count),
121125
&limit_count,
122126
|b, _| {
123-
let rt = tokio::runtime::Builder::new_current_thread()
124-
.enable_time()
125-
.build()
126-
.unwrap();
127+
let rt = single_thread_rt();
127128
b.iter(|| {
128129
rt.block_on(async { black_box(middleware.check_and_apply_limits(&req).await) })
129130
})

src/builder.rs

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -142,35 +142,22 @@ impl RateLimitBuilder {
142142
/// Emit a warning if catch-all routes precede more specific routes.
143143
#[cfg(feature = "tracing")]
144144
fn warn_catch_all_route_order(&self) {
145-
// Collect indices of all catch-all routes
146-
let catch_all_indices: Vec<usize> = self
147-
.routes
148-
.iter()
149-
.enumerate()
150-
.filter(|(_, route)| route.is_catch_all())
151-
.map(|(i, _)| i)
152-
.collect();
153-
154-
// For each catch-all, warn about specific routes that follow it
155-
for &catch_all_index in &catch_all_indices {
156-
// Find the first specific route after this catch-all
157-
if let Some((specific_index, _)) = self
158-
.routes
159-
.iter()
160-
.enumerate()
161-
.skip(catch_all_index + 1)
162-
.find(|(_, route)| !route.is_catch_all())
163-
{
145+
let mut last_catch_all = None;
146+
for (i, route) in self.routes.iter().enumerate() {
147+
if route.is_catch_all() {
148+
last_catch_all = Some(i);
149+
} else if let Some(catch_all_index) = last_catch_all {
164150
tracing::warn!(
165151
catch_all_route_index = catch_all_index,
166-
specific_route_index = specific_index,
152+
specific_route_index = i,
167153
"Catch-all route (index {}) precedes more specific route (index {}). \
168154
All matching routes' limits are applied, so the catch-all will affect \
169155
requests intended for the specific route. Consider reordering routes \
170156
or using host-scoped builders.",
171157
catch_all_index,
172-
specific_index
158+
i
173159
);
160+
last_catch_all = None;
174161
}
175162
}
176163
}

src/lib.rs

Lines changed: 15 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -93,15 +93,19 @@ mod tests {
9393
use http::Method;
9494
use std::time::Duration;
9595

96-
#[test]
97-
fn test_route_matching_all() {
98-
let route = Route {
99-
host: None,
100-
method: None,
101-
path_prefix: String::new(),
96+
fn test_route(host: Option<&str>, method: Option<Method>, path: &str) -> Route {
97+
Route {
98+
host: host.map(String::from),
99+
method,
100+
path_prefix: path.to_string(),
102101
limits: vec![],
103102
on_limit: ThrottleBehavior::Delay,
104-
};
103+
}
104+
}
105+
106+
#[test]
107+
fn test_route_matching_all() {
108+
let route = test_route(None, None, "");
105109

106110
let req = reqwest::Client::new()
107111
.get("https://example.com/test")
@@ -113,13 +117,7 @@ mod tests {
113117

114118
#[test]
115119
fn test_route_matching_host() {
116-
let route = Route {
117-
host: Some("api.example.com".to_string()),
118-
method: None,
119-
path_prefix: String::new(),
120-
limits: vec![],
121-
on_limit: ThrottleBehavior::Delay,
122-
};
120+
let route = test_route(Some("api.example.com"), None, "");
123121

124122
let req_match = reqwest::Client::new()
125123
.get("https://api.example.com/test")
@@ -136,13 +134,7 @@ mod tests {
136134

137135
#[test]
138136
fn test_route_matching_method() {
139-
let route = Route {
140-
host: None,
141-
method: Some(Method::POST),
142-
path_prefix: String::new(),
143-
limits: vec![],
144-
on_limit: ThrottleBehavior::Delay,
145-
};
137+
let route = test_route(None, Some(Method::POST), "");
146138

147139
let req_match = reqwest::Client::new()
148140
.post("https://example.com/test")
@@ -159,13 +151,7 @@ mod tests {
159151

160152
#[test]
161153
fn test_route_matching_path_prefix() {
162-
let route = Route {
163-
host: None,
164-
method: None,
165-
path_prefix: "/api/v1".to_string(),
166-
limits: vec![],
167-
on_limit: ThrottleBehavior::Delay,
168-
};
154+
let route = test_route(None, None, "/api/v1");
169155

170156
let req_match = reqwest::Client::new()
171157
.get("https://example.com/api/v1/users")
@@ -182,13 +168,7 @@ mod tests {
182168

183169
#[test]
184170
fn test_route_matching_path_segment_boundary() {
185-
let route = Route {
186-
host: None,
187-
method: None,
188-
path_prefix: "/order".to_string(),
189-
limits: vec![],
190-
on_limit: ThrottleBehavior::Delay,
191-
};
171+
let route = test_route(None, None, "/order");
192172

193173
// Should match: exact, with trailing slash, with sub-path
194174
let req_exact = reqwest::Client::new()

src/middleware.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@ use crate::error::RateLimitError;
1414
use crate::gcra::GcraState;
1515
use crate::types::{Route, ThrottleBehavior};
1616

17+
/// Jitter adds up to 50% of the wait duration (denominator of the fraction).
18+
const JITTER_FRACTION_DENOM: u128 = 2;
19+
1720
/// The rate limiting middleware.
1821
///
1922
/// This middleware tracks rate limits and either delays or rejects requests
@@ -112,9 +115,10 @@ impl RateLimitMiddleware {
112115
) -> Result<(), RateLimitError> {
113116
match route.on_limit {
114117
ThrottleBehavior::Delay => {
115-
// Add jitter (0-50% of wait duration) to prevent thundering herd
118+
// Add jitter to prevent thundering herd
116119
let jitter_max_nanos =
117-
u64::try_from(wait_duration.as_nanos() / 2).unwrap_or(u64::MAX);
120+
u64::try_from(wait_duration.as_nanos() / JITTER_FRACTION_DENOM)
121+
.unwrap_or(u64::MAX);
118122
let jitter_nanos = if jitter_max_nanos > 0 {
119123
rand::rng().random_range(0..=jitter_max_nanos)
120124
} else {

src/types.rs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -158,11 +158,10 @@ impl Route {
158158
req_path: &str,
159159
) -> bool {
160160
// Check host
161-
if let Some(ref host) = self.host {
162-
match req_host {
163-
Some(h) if h == host => {}
164-
_ => return false,
165-
}
161+
if let Some(ref host) = self.host
162+
&& req_host != Some(host.as_str())
163+
{
164+
return false;
166165
}
167166

168167
// Check method

0 commit comments

Comments
 (0)