Implement Params for HashMap and serde_json::Value#615
Conversation
mlafeldt
left a comment
mlafeldt
left a comment
There was a problem hiding this comment.
The HashMap impl is solid. The serde_json impl has a fundamental type-safety problem: ToSql for serde_json::Value serializes everything to JSON strings. A fix would require Value variants to be converted to native Rust types before binding.
I suggest shipping the HashMap now and defer (delete) serde_json support since it's a convenience on top.
| .map(|i| { | ||
| let name = stmt.parameter_name(i)?; | ||
| let val = map.get(&name).ok_or(Error::InvalidParameterName(name))?; | ||
| Ok(val as &dyn ToSql) |
There was a problem hiding this comment.
One concern with the serde_json path: ToSql for serde_json::Value serializes everything via serde_json::to_string(), so e.g. json!(42) binds as the string "42" rather than an integer.
I think the test passes because DuckDB implicitly casts, but this will produce wrong results in cases like string-vs-numeric comparison.
| stmt.bind_parameters(¶ms) | ||
| } | ||
| Value::Array(ref array) => stmt.bind_parameters(&array[..]), | ||
| ref other => stmt.bind_parameters(&[other as &dyn ToSql]), |
There was a problem hiding this comment.
The non-Object Value fallthrough silently reinterprets named params as positional
Enhances and replaces #615. This adds named parameter support for two common use cases: - `named_params!` covers the ergonomic rusqlite-style API for mixed Rust value types. Useful when values are heterogeneous: ```rust named_params! { "name": name, "age": age, "active": true, } ``` - Generic `HashMap` support covers dynamic Python-dict-style binding. Useful when the parameter set is dynamic or already comes from application data: ```rust let params = HashMap::from([ ("min", 18), ("max", 65), ]); ``` Extra keys are rejected, matching DuckDB Python behavior.
|
I just merged #748, which brings your HashMap support + a nice new |
|
Thanks I got pulled into a different project so didn't have time to follow up but thanks for making the changes and getting it merged! |
2 participants
Thanks for implementing #609, that gave me enough to take a stab at implementing
Paramsforserde_json::ValueandHashMap. Let me know what you think!