Skip to content

Commit f950ad3

Browse files
bordeuxclaude
andcommitted
refactor: eliminate code duplication in URL functions
Improvements: 1. Extracted common query string serialization logic into helper function - Created serialize_query_params() helper function - Used by both query_string_fn() and build_url_fn() - Eliminates ~30 lines of duplicate code 2. Removed inline tests from src/functions/url.rs - All tests already exist in tests/test_url_functions.rs - Cleaner separation of concerns Result: - src/functions/url.rs reduced from 317 to 228 lines (28% reduction) - No code duplication - All 34 tests still passing - Functionality unchanged 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 6c0ae42 commit f950ad3

1 file changed

Lines changed: 46 additions & 135 deletions

File tree

src/functions/url.rs

Lines changed: 46 additions & 135 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,48 @@ use minijinja::{Error, ErrorKind, Value};
1111
use std::collections::BTreeMap;
1212
use url::Url;
1313

14+
/// Convert a MiniJinja Value (object) to a URL-encoded query string
15+
///
16+
/// This is a helper function used by both `query_string_fn` and `build_url_fn`
17+
/// to avoid code duplication.
18+
fn serialize_query_params(params: &Value) -> Result<String, Error> {
19+
// Convert to serde_json::Value for easier iteration
20+
let json_value: serde_json::Value = serde_json::to_value(params).map_err(|e| {
21+
Error::new(
22+
ErrorKind::InvalidOperation,
23+
format!("Failed to convert params: {}", e),
24+
)
25+
})?;
26+
27+
if !json_value.is_object() {
28+
return Err(Error::new(
29+
ErrorKind::InvalidOperation,
30+
"query parameter must be an object",
31+
));
32+
}
33+
34+
let mut parts = Vec::new();
35+
36+
// Iterate over object fields
37+
if let Some(obj) = json_value.as_object() {
38+
for (key, value) in obj {
39+
let encoded_key = urlencoding::encode(key);
40+
// Convert value to string properly (without JSON quotes)
41+
let value_str = match value {
42+
serde_json::Value::String(s) => s.clone(),
43+
serde_json::Value::Number(n) => n.to_string(),
44+
serde_json::Value::Bool(b) => b.to_string(),
45+
serde_json::Value::Null => String::from("null"),
46+
_ => value.to_string(),
47+
};
48+
let encoded_value = urlencoding::encode(&value_str);
49+
parts.push(format!("{}={}", encoded_key, encoded_value));
50+
}
51+
}
52+
53+
Ok(parts.join("&"))
54+
}
55+
1456
/// Generate HTTP Basic Authentication header value
1557
///
1658
/// # Arguments
@@ -149,37 +191,8 @@ pub fn build_url_fn(kwargs: Kwargs) -> Result<Value, Error> {
149191
// Query is a string, use it directly
150192
s.to_string()
151193
} else {
152-
// Query is an object, serialize it using query_string logic
153-
let json_value: serde_json::Value = serde_json::to_value(&q).map_err(|e| {
154-
Error::new(
155-
ErrorKind::InvalidOperation,
156-
format!("Failed to convert query parameter: {}", e),
157-
)
158-
})?;
159-
160-
if !json_value.is_object() {
161-
return Err(Error::new(
162-
ErrorKind::InvalidOperation,
163-
"query parameter must be a string or object",
164-
));
165-
}
166-
167-
let mut parts = Vec::new();
168-
if let Some(obj) = json_value.as_object() {
169-
for (key, value) in obj {
170-
let encoded_key = urlencoding::encode(key);
171-
let value_str = match value {
172-
serde_json::Value::String(s) => s.clone(),
173-
serde_json::Value::Number(n) => n.to_string(),
174-
serde_json::Value::Bool(b) => b.to_string(),
175-
serde_json::Value::Null => String::from("null"),
176-
_ => value.to_string(),
177-
};
178-
let encoded_value = urlencoding::encode(&value_str);
179-
parts.push(format!("{}={}", encoded_key, encoded_value));
180-
}
181-
}
182-
parts.join("&")
194+
// Query is an object, serialize it
195+
serialize_query_params(&q)?
183196
};
184197

185198
if !query_str.is_empty() {
@@ -209,108 +222,6 @@ pub fn build_url_fn(kwargs: Kwargs) -> Result<Value, Error> {
209222
/// ```
210223
pub fn query_string_fn(kwargs: Kwargs) -> Result<Value, Error> {
211224
let params: Value = kwargs.get("params")?;
212-
213-
// Convert to serde_json::Value for easier iteration
214-
let json_value: serde_json::Value = serde_json::to_value(&params).map_err(|e| {
215-
Error::new(
216-
ErrorKind::InvalidOperation,
217-
format!("Failed to convert params: {}", e),
218-
)
219-
})?;
220-
221-
if !json_value.is_object() {
222-
return Err(Error::new(
223-
ErrorKind::InvalidOperation,
224-
"query_string() requires an object for 'params' parameter",
225-
));
226-
}
227-
228-
let mut parts = Vec::new();
229-
230-
// Iterate over object fields
231-
if let Some(obj) = json_value.as_object() {
232-
for (key, value) in obj {
233-
let encoded_key = urlencoding::encode(key);
234-
// Convert value to string properly (without JSON quotes)
235-
let value_str = match value {
236-
serde_json::Value::String(s) => s.clone(),
237-
serde_json::Value::Number(n) => n.to_string(),
238-
serde_json::Value::Bool(b) => b.to_string(),
239-
serde_json::Value::Null => String::from("null"),
240-
_ => value.to_string(),
241-
};
242-
let encoded_value = urlencoding::encode(&value_str);
243-
parts.push(format!("{}={}", encoded_key, encoded_value));
244-
}
245-
}
246-
247-
Ok(Value::from(parts.join("&")))
248-
}
249-
250-
#[cfg(test)]
251-
mod tests {
252-
use super::*;
253-
254-
#[test]
255-
fn test_basic_auth_simple() {
256-
let result = basic_auth_fn(Kwargs::from_iter(vec![
257-
("username", Value::from("admin")),
258-
("password", Value::from("secret")),
259-
]))
260-
.unwrap();
261-
262-
assert_eq!(result.to_string(), "Basic YWRtaW46c2VjcmV0");
263-
}
264-
265-
#[test]
266-
fn test_parse_url_simple() {
267-
let result = parse_url_fn(Kwargs::from_iter(vec![(
268-
"url",
269-
Value::from("https://example.com/path"),
270-
)]))
271-
.unwrap();
272-
273-
let obj = result.as_object().unwrap();
274-
assert_eq!(
275-
obj.get_value(&Value::from("scheme")).unwrap().as_str(),
276-
Some("https")
277-
);
278-
assert_eq!(
279-
obj.get_value(&Value::from("host")).unwrap().as_str(),
280-
Some("example.com")
281-
);
282-
assert_eq!(
283-
obj.get_value(&Value::from("path")).unwrap().as_str(),
284-
Some("/path")
285-
);
286-
}
287-
288-
#[test]
289-
fn test_build_url_simple() {
290-
let result = build_url_fn(Kwargs::from_iter(vec![
291-
("scheme", Value::from("https")),
292-
("host", Value::from("example.com")),
293-
("path", Value::from("/api")),
294-
]))
295-
.unwrap();
296-
297-
assert_eq!(result.to_string(), "https://example.com/api");
298-
}
299-
300-
#[test]
301-
fn test_query_string_simple() {
302-
let mut params = BTreeMap::new();
303-
params.insert("name".to_string(), Value::from("test"));
304-
params.insert("value".to_string(), Value::from(42));
305-
306-
let result = query_string_fn(Kwargs::from_iter(vec![(
307-
"params",
308-
Value::from_object(params),
309-
)]))
310-
.unwrap();
311-
312-
let output = result.to_string();
313-
assert!(output.contains("name=test"));
314-
assert!(output.contains("value=42"));
315-
}
225+
let query_str = serialize_query_params(&params)?;
226+
Ok(Value::from(query_str))
316227
}

0 commit comments

Comments
 (0)