feat(torii-core): json value for ty for efficient json ser #2730
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2730 +/- ##
==========================================
- Coverage 56.24% 56.08% -0.17%
==========================================
Files 415 415
Lines 53241 53386 +145
==========================================
- Hits 29948 29943 -5
- Misses 23293 23443 +150 ☔ View full report in Codecov by Sentry. |
WalkthroughOhayo! This pull request introduces several changes, including the addition of the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
crates/torii/core/src/executor/mod.rs (1)
551-551: Ohayo! The JSON serialization change looks good, sensei!The switch to using
to_json_value()withserde_json::to_string()is a more idiomatic approach. However, consider adding more specific error handling for serialization failures.Consider wrapping the serialization in a more specific error type:
- let data = serde_json::to_string(&em_query.ty.to_json_value()?)?; + let data = serde_json::to_string(&em_query.ty.to_json_value()?) + .map_err(|e| anyhow::anyhow!("Failed to serialize event message type: {}", e))?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/dojo/types/Cargo.toml(1 hunks)crates/dojo/types/src/schema.rs(2 hunks)crates/torii/core/src/executor/mod.rs(1 hunks)
🔇 Additional comments (4)
crates/dojo/types/Cargo.toml (1)
23-23: Ohayo! The indexmap dependency addition looks good, sensei!
The workspace configuration is properly set up, maintaining consistency with other dependencies.
Let's verify the IndexMap usage in the codebase:
✅ Verification successful
Ohayo! The IndexMap dependency is well-justified and actively used, sensei!
The verification confirms that IndexMap is actively used in the codebase, particularly in crates/dojo/types/src/schema.rs as mentioned in the PR context, along with several other legitimate uses across the codebase. The workspace configuration ensures consistent versioning across all these usages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify IndexMap usage in the codebase
# Expected: Find usage in schema.rs and potentially other files
# Search for IndexMap type usage
rg "IndexMap|use indexmap" --type rust
# Search for specific JSON serialization related usage
ast-grep --pattern 'impl $_ for Ty {
$$$
IndexMap
$$$
}'
Length of output: 6775
crates/dojo/types/src/schema.rs (3)
2-2: Import FromStr Is Necessary
Ohayo sensei, the addition of use std::str::FromStr; is appropriate as it is required for parsing strings into Felt values later in the code.
5-6: Include crypto_bigint and indexmap Dependencies
The imports use crypto_bigint::{Encoding, U256}; and use indexmap::IndexMap; are necessary for handling big integers and maintaining insertion order in maps, respectively.
10-10: Import serde_json for JSON Handling
Ohayo sensei, the import use serde_json::{json, Value as JsonValue}; is essential for JSON serialization and deserialization in the newly added methods.
| Primitive::U8(v) => { | ||
| if let JsonValue::Number(n) = value { | ||
| *v = n.as_u64().map(|n| n as u8); | ||
| } | ||
| } | ||
| Primitive::U16(v) => { | ||
| if let JsonValue::Number(n) = value { | ||
| *v = n.as_u64().map(|n| n as u16); | ||
| } | ||
| } | ||
| Primitive::U32(v) => { | ||
| if let JsonValue::Number(n) = value { | ||
| *v = n.as_u64().map(|n| n as u32); | ||
| } | ||
| } | ||
| Primitive::U64(v) => { | ||
| if let JsonValue::String(s) = value { | ||
| *v = s.parse().ok(); | ||
| } | ||
| } | ||
| Primitive::U128(v) => { | ||
| if let JsonValue::String(s) = value { | ||
| *v = s.parse().ok(); | ||
| } | ||
| } | ||
| Primitive::USize(v) => { | ||
| if let JsonValue::Number(n) = value { | ||
| *v = n.as_u64().map(|n| n as u32); | ||
| } | ||
| } | ||
| Primitive::U256(v) => { | ||
| if let JsonValue::Object(obj) = value { | ||
| if let (Some(JsonValue::String(high)), Some(JsonValue::String(low))) = | ||
| (obj.get("high"), obj.get("low")) | ||
| { | ||
| if let (Ok(high), Ok(low)) = (high.parse::<u128>(), low.parse::<u128>()) | ||
| { | ||
| let mut bytes = [0u8; 32]; | ||
| bytes[..16].copy_from_slice(&high.to_be_bytes()); | ||
| bytes[16..].copy_from_slice(&low.to_be_bytes()); | ||
| *v = Some(U256::from_be_slice(&bytes)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Primitive::Felt252(v) => { | ||
| if let JsonValue::String(s) = value { | ||
| *v = Felt::from_str(&s).ok(); | ||
| } | ||
| } | ||
| Primitive::ClassHash(v) => { | ||
| if let JsonValue::String(s) = value { | ||
| *v = Felt::from_str(&s).ok(); | ||
| } | ||
| } | ||
| Primitive::ContractAddress(v) => { | ||
| if let JsonValue::String(s) = value { | ||
| *v = Felt::from_str(&s).ok(); | ||
| } | ||
| } | ||
| }, | ||
| (Ty::Struct(s), JsonValue::Object(obj)) => { | ||
| for member in &mut s.children { | ||
| if let Some(value) = obj.get(&member.name) { | ||
| member.ty.from_json_value(value.clone())?; | ||
| } | ||
| } | ||
| } | ||
| (Ty::Enum(e), JsonValue::Object(obj)) => { | ||
| if let Some((name, value)) = obj.into_iter().next() { | ||
| e.set_option(&name).map_err(|_| PrimitiveError::TypeMismatch)?; | ||
| if let Some(option) = e.option { | ||
| e.options[option as usize].ty.from_json_value(value)?; | ||
| } | ||
| } | ||
| } | ||
| (Ty::Array(items), JsonValue::Array(values)) => { | ||
| let template = items[0].clone(); | ||
| items.clear(); | ||
| for value in values { | ||
| let mut item = template.clone(); | ||
| item.from_json_value(value)?; | ||
| items.push(item); | ||
| } | ||
| } | ||
| (Ty::Tuple(items), JsonValue::Array(values)) => { | ||
| if items.len() != values.len() { | ||
| return Err(PrimitiveError::TypeMismatch); | ||
| } | ||
| for (item, value) in items.iter_mut().zip(values) { | ||
| item.from_json_value(value)?; | ||
| } | ||
| } | ||
| (Ty::ByteArray(bytes), JsonValue::String(s)) => { | ||
| *bytes = s; | ||
| } | ||
| _ => return Err(PrimitiveError::TypeMismatch), | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Improving Error Handling in from_json_value
The from_json_value method effectively deserializes JSON values into Ty instances. However, there are several areas where error handling can be enhanced to prevent silent failures.
Handle parsing errors for numeric primitives
Ohayo sensei, when parsing numeric values from JSON, assigning None on failure may lead to unexpected behavior. It's better to return an error if parsing fails.
Example modification for Primitive::I64 (lines 396-398):
if let JsonValue::String(s) = value {
- *v = s.parse().ok();
+ *v = Some(s.parse().map_err(|_| PrimitiveError::InvalidValue)?);
}Apply similar changes for other numeric primitives (I128, U64, U128), ensuring that parsing errors are properly propagated.
Ensure proper error handling for Primitive::U256 parsing
In lines 436-449, when parsing high and low values, failure to parse should result in an error rather than being silently ignored.
if let JsonValue::Object(obj) = value {
if let (Some(JsonValue::String(high)), Some(JsonValue::String(low))) =
(obj.get("high"), obj.get("low"))
{
- if let (Ok(high), Ok(low)) = (high.parse::<u128>(), low.parse::<u128>()) {
+ let high = high.parse::<u128>().map_err(|_| PrimitiveError::InvalidValue)?;
+ let low = low.parse::<u128>().map_err(|_| PrimitiveError::InvalidValue)?;
let mut bytes = [0u8; 32];
bytes[..16].copy_from_slice(&high.to_be_bytes());
bytes[16..].copy_from_slice(&low.to_be_bytes());
*v = Some(U256::from_be_slice(&bytes));
}
}Consistent error handling across all primitive types will enhance the robustness of the deserialization process.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Parse a JSON Value into a Ty | |
| pub fn from_json_value(&mut self, value: JsonValue) -> Result<(), PrimitiveError> { | |
| match (self, value) { | |
| (Ty::Primitive(primitive), value) => match primitive { | |
| Primitive::Bool(v) => { | |
| if let JsonValue::Bool(b) = value { | |
| *v = Some(b); | |
| } | |
| } | |
| Primitive::I8(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_i64().map(|n| n as i8); | |
| } | |
| } | |
| Primitive::I16(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_i64().map(|n| n as i16); | |
| } | |
| } | |
| Primitive::I32(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_i64().map(|n| n as i32); | |
| } | |
| } | |
| Primitive::I64(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = s.parse().ok(); | |
| } | |
| } | |
| Primitive::I128(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = s.parse().ok(); | |
| } | |
| } | |
| Primitive::U8(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_u64().map(|n| n as u8); | |
| } | |
| } | |
| Primitive::U16(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_u64().map(|n| n as u16); | |
| } | |
| } | |
| Primitive::U32(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_u64().map(|n| n as u32); | |
| } | |
| } | |
| Primitive::U64(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = s.parse().ok(); | |
| } | |
| } | |
| Primitive::U128(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = s.parse().ok(); | |
| } | |
| } | |
| Primitive::USize(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_u64().map(|n| n as u32); | |
| } | |
| } | |
| Primitive::U256(v) => { | |
| if let JsonValue::Object(obj) = value { | |
| if let (Some(JsonValue::String(high)), Some(JsonValue::String(low))) = | |
| (obj.get("high"), obj.get("low")) | |
| { | |
| if let (Ok(high), Ok(low)) = (high.parse::<u128>(), low.parse::<u128>()) | |
| { | |
| let mut bytes = [0u8; 32]; | |
| bytes[..16].copy_from_slice(&high.to_be_bytes()); | |
| bytes[16..].copy_from_slice(&low.to_be_bytes()); | |
| *v = Some(U256::from_be_slice(&bytes)); | |
| } | |
| } | |
| } | |
| } | |
| Primitive::Felt252(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = Felt::from_str(&s).ok(); | |
| } | |
| } | |
| Primitive::ClassHash(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = Felt::from_str(&s).ok(); | |
| } | |
| } | |
| Primitive::ContractAddress(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = Felt::from_str(&s).ok(); | |
| } | |
| } | |
| }, | |
| (Ty::Struct(s), JsonValue::Object(obj)) => { | |
| for member in &mut s.children { | |
| if let Some(value) = obj.get(&member.name) { | |
| member.ty.from_json_value(value.clone())?; | |
| } | |
| } | |
| } | |
| (Ty::Enum(e), JsonValue::Object(obj)) => { | |
| if let Some((name, value)) = obj.into_iter().next() { | |
| e.set_option(&name).map_err(|_| PrimitiveError::TypeMismatch)?; | |
| if let Some(option) = e.option { | |
| e.options[option as usize].ty.from_json_value(value)?; | |
| } | |
| } | |
| } | |
| (Ty::Array(items), JsonValue::Array(values)) => { | |
| let template = items[0].clone(); | |
| items.clear(); | |
| for value in values { | |
| let mut item = template.clone(); | |
| item.from_json_value(value)?; | |
| items.push(item); | |
| } | |
| } | |
| (Ty::Tuple(items), JsonValue::Array(values)) => { | |
| if items.len() != values.len() { | |
| return Err(PrimitiveError::TypeMismatch); | |
| } | |
| for (item, value) in items.iter_mut().zip(values) { | |
| item.from_json_value(value)?; | |
| } | |
| } | |
| (Ty::ByteArray(bytes), JsonValue::String(s)) => { | |
| *bytes = s; | |
| } | |
| _ => return Err(PrimitiveError::TypeMismatch), | |
| } | |
| Ok(()) | |
| } | |
| /// Parse a JSON Value into a Ty | |
| pub fn from_json_value(&mut self, value: JsonValue) -> Result<(), PrimitiveError> { | |
| match (self, value) { | |
| (Ty::Primitive(primitive), value) => match primitive { | |
| Primitive::Bool(v) => { | |
| if let JsonValue::Bool(b) = value { | |
| *v = Some(b); | |
| } | |
| } | |
| Primitive::I8(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_i64().map(|n| n as i8); | |
| } | |
| } | |
| Primitive::I16(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_i64().map(|n| n as i16); | |
| } | |
| } | |
| Primitive::I32(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_i64().map(|n| n as i32); | |
| } | |
| } | |
| Primitive::I64(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = Some(s.parse().map_err(|_| PrimitiveError::InvalidValue)?); | |
| } | |
| } | |
| Primitive::I128(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = Some(s.parse().map_err(|_| PrimitiveError::InvalidValue)?); | |
| } | |
| } | |
| Primitive::U8(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_u64().map(|n| n as u8); | |
| } | |
| } | |
| Primitive::U16(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_u64().map(|n| n as u16); | |
| } | |
| } | |
| Primitive::U32(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_u64().map(|n| n as u32); | |
| } | |
| } | |
| Primitive::U64(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = Some(s.parse().map_err(|_| PrimitiveError::InvalidValue)?); | |
| } | |
| } | |
| Primitive::U128(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = Some(s.parse().map_err(|_| PrimitiveError::InvalidValue)?); | |
| } | |
| } | |
| Primitive::USize(v) => { | |
| if let JsonValue::Number(n) = value { | |
| *v = n.as_u64().map(|n| n as u32); | |
| } | |
| } | |
| Primitive::U256(v) => { | |
| if let JsonValue::Object(obj) = value { | |
| if let (Some(JsonValue::String(high)), Some(JsonValue::String(low))) = | |
| (obj.get("high"), obj.get("low")) | |
| { | |
| let high = high.parse::<u128>().map_err(|_| PrimitiveError::InvalidValue)?; | |
| let low = low.parse::<u128>().map_err(|_| PrimitiveError::InvalidValue)?; | |
| let mut bytes = [0u8; 32]; | |
| bytes[..16].copy_from_slice(&high.to_be_bytes()); | |
| bytes[16..].copy_from_slice(&low.to_be_bytes()); | |
| *v = Some(U256::from_be_slice(&bytes)); | |
| } | |
| } | |
| } | |
| Primitive::Felt252(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = Felt::from_str(&s).ok(); | |
| } | |
| } | |
| Primitive::ClassHash(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = Felt::from_str(&s).ok(); | |
| } | |
| } | |
| Primitive::ContractAddress(v) => { | |
| if let JsonValue::String(s) = value { | |
| *v = Felt::from_str(&s).ok(); | |
| } | |
| } | |
| }, | |
| (Ty::Struct(s), JsonValue::Object(obj)) => { | |
| for member in &mut s.children { | |
| if let Some(value) = obj.get(&member.name) { | |
| member.ty.from_json_value(value.clone())?; | |
| } | |
| } | |
| } | |
| (Ty::Enum(e), JsonValue::Object(obj)) => { | |
| if let Some((name, value)) = obj.into_iter().next() { | |
| e.set_option(&name).map_err(|_| PrimitiveError::TypeMismatch)?; | |
| if let Some(option) = e.option { | |
| e.options[option as usize].ty.from_json_value(value)?; | |
| } | |
| } | |
| } | |
| (Ty::Array(items), JsonValue::Array(values)) => { | |
| let template = items[0].clone(); | |
| items.clear(); | |
| for value in values { | |
| let mut item = template.clone(); | |
| item.from_json_value(value)?; | |
| items.push(item); | |
| } | |
| } | |
| (Ty::Tuple(items), JsonValue::Array(values)) => { | |
| if items.len() != values.len() { | |
| return Err(PrimitiveError::TypeMismatch); | |
| } | |
| for (item, value) in items.iter_mut().zip(values) { | |
| item.from_json_value(value)?; | |
| } | |
| } | |
| (Ty::ByteArray(bytes), JsonValue::String(s)) => { | |
| *bytes = s; | |
| } | |
| _ => return Err(PrimitiveError::TypeMismatch), | |
| } | |
| Ok(()) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
Line range hint
366-383: Ohayo sensei! Consider optimizing the event message processingThe current implementation has some performance considerations:
- Multiple database queries for model cache within the loop
- Repeated string-to-felt conversions
- Building a HashMap in memory for all entities
Consider these optimizations:
- Batch fetch models from cache before the loop
- Pre-compute Felt values where possible
- Stream results instead of collecting all in memory
Here's a suggested approach:
async fn fetch_historical_event_messages( &self, query: &str, keys_pattern: Option<&str>, limit: Option<u32>, offset: Option<u32>, ) -> Result<Vec<proto::types::Entity>, Error> { + // Pre-fetch unique model IDs + let model_ids: HashSet<Felt> = db_entities + .iter() + .map(|(_, _, model_id, _)| Felt::from_str(model_id)) + .collect::<Result<_, _>>()?; + + // Batch fetch models + let model_cache: HashMap<Felt, _> = self.model_cache + .models(&model_ids.into_iter().collect::<Vec<_>>()) + .await? + .into_iter() + .map(|m| (m.id, m)) + .collect(); let mut entities = HashMap::new(); for (id, data, model_id, _) in db_entities { - let hashed_keys = Felt::from_str(&id).map_err(ParseError::FromStr)?.to_bytes_be().to_vec(); - let model = self - .model_cache - .model(&Felt::from_str(&model_id).map_err(ParseError::FromStr)?) - .await?; + let hashed_keys = cached_felts.get(&id).unwrap_or_else(|| { + let felt = Felt::from_str(&id).map_err(ParseError::FromStr)?; + cached_felts.insert(id.clone(), felt.to_bytes_be().to_vec()); + cached_felts.get(&id).unwrap() + }); + let model = model_cache.get(&Felt::from_str(&model_id)?).unwrap();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(1 hunks)
🔇 Additional comments (1)
crates/torii/grpc/src/server/mod.rs (1)
370-370:
Ohayo sensei! Avoid using unwrap(); handle potential JSON deserialization errors
Using unwrap() on serde_json::from_str(&data) can cause a panic if data is not valid JSON. It's better to handle the error properly and propagate it.
Apply this diff to handle the error properly:
-schema.from_json_value(serde_json::from_str(&data).unwrap())?;
+schema.from_json_value(serde_json::from_str(&data)?)?;
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/types/src/schema.rs(2 hunks)
🔇 Additional comments (4)
crates/dojo/types/src/schema.rs (4)
316-369: LGTM! Comprehensive JSON serialization implementation
The implementation handles all types correctly and provides proper error handling (except for the U256 case noted above).
334-335:
Ohayo sensei! Replace unwrap() with proper error handling
The use of unwrap() in the U256 conversion could cause panics. Let's handle potential errors gracefully.
Apply this diff:
- let high = u128::from_be_bytes(bytes[..16].try_into().unwrap());
- let low = u128::from_be_bytes(bytes[16..].try_into().unwrap());
+ let high = u128::from_be_bytes(bytes[..16].try_into().map_err(|_| PrimitiveError::TypeMismatch)?);
+ let low = u128::from_be_bytes(bytes[16..].try_into().map_err(|_| PrimitiveError::TypeMismatch)?);Likely invalid or redundant comment.
436-449: 🛠️ Refactor suggestion
Simplify U256 parsing and improve error handling
The nested if-let patterns make the code harder to read and error handling is inconsistent.
Consider this cleaner approach:
- if let JsonValue::Object(obj) = value {
- if let (Some(JsonValue::String(high)), Some(JsonValue::String(low))) =
- (obj.get("high"), obj.get("low"))
- {
- if let (Ok(high), Ok(low)) = (high.parse::<u128>(), low.parse::<u128>())
- {
- let mut bytes = [0u8; 32];
- bytes[..16].copy_from_slice(&high.to_be_bytes());
- bytes[16..].copy_from_slice(&low.to_be_bytes());
- *v = Some(U256::from_be_slice(&bytes));
- }
- }
- }
+ let obj = match value {
+ JsonValue::Object(obj) => obj,
+ _ => return Err(PrimitiveError::TypeMismatch),
+ };
+
+ let high = obj.get("high")
+ .and_then(|v| v.as_str())
+ .ok_or(PrimitiveError::TypeMismatch)?
+ .parse::<u128>()
+ .map_err(|_| PrimitiveError::InvalidValue)?;
+
+ let low = obj.get("low")
+ .and_then(|v| v.as_str())
+ .ok_or(PrimitiveError::TypeMismatch)?
+ .parse::<u128>()
+ .map_err(|_| PrimitiveError::InvalidValue)?;
+
+ let mut bytes = [0u8; 32];
+ bytes[..16].copy_from_slice(&high.to_be_bytes());
+ bytes[16..].copy_from_slice(&low.to_be_bytes());
+ *v = Some(U256::from_be_slice(&bytes));Likely invalid or redundant comment.
395-429:
Improve error handling for numeric string parsing
Ohayo sensei! Currently, parsing errors are silently ignored using .ok(). We should propagate these errors to handle invalid inputs properly.
Apply this pattern for all numeric string parsing:
- if let JsonValue::String(s) = value {
- *v = s.parse().ok();
- }
+ if let JsonValue::String(s) = value {
+ *v = Some(s.parse().map_err(|_| PrimitiveError::InvalidValue)?);
+ }Likely invalid or redundant comment.
| (Ty::Array(items), JsonValue::Array(values)) => { | ||
| let template = items[0].clone(); | ||
| items.clear(); | ||
| for value in values { | ||
| let mut item = template.clone(); | ||
| item.from_json_value(value)?; | ||
| items.push(item); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle empty arrays safely
The current implementation assumes arrays are non-empty and could panic when deserializing an empty array.
Apply this fix:
(Ty::Array(items), JsonValue::Array(values)) => {
+ if items.is_empty() {
+ return Err(PrimitiveError::TypeMismatch);
+ }
let template = items[0].clone();
items.clear();
for value in values {
let mut item = template.clone();
item.from_json_value(value)?;
items.push(item);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (Ty::Array(items), JsonValue::Array(values)) => { | |
| let template = items[0].clone(); | |
| items.clear(); | |
| for value in values { | |
| let mut item = template.clone(); | |
| item.from_json_value(value)?; | |
| items.push(item); | |
| } | |
| } | |
| (Ty::Array(items), JsonValue::Array(values)) => { | |
| if items.is_empty() { | |
| return Err(PrimitiveError::TypeMismatch); | |
| } | |
| let template = items[0].clone(); | |
| items.clear(); | |
| for value in values { | |
| let mut item = template.clone(); | |
| item.from_json_value(value)?; | |
| items.push(item); | |
| } | |
| } |
| Primitive::Felt252(Some(v)) => Ok(json!(format!("{:#x}", v))), | ||
| Primitive::ClassHash(Some(v)) => Ok(json!(format!("{:#x}", v))), | ||
| Primitive::ContractAddress(Some(v)) => Ok(json!(format!("{:#x}", v))), |
There was a problem hiding this comment.
We should in the future have this format being factorized. I always asking myself if it needs to be padded or not. x) Having this in functions will help for clearer context across torii/the stack.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/torii/grpc/src/server/mod.rs (2)
Line range hint
1012-1015: Consider more specific error handling for composite clauses, senseiThe error conversion in the composite clause builder uses a generic
Status::internalfor all error types. Consider mapping specific error cases to appropriate gRPC status codes for better error reporting.Example improvement:
- .map_err(|e| Status::internal(e.to_string()))?; + .map_err(|e| match e { + Error::Parse(_) => Status::invalid_argument(e.to_string()), + Error::Query(_) => Status::failed_precondition(e.to_string()), + _ => Status::internal(e.to_string()), + })?;
Line range hint
516-524: Optimize query building for better performance, senseiThe dynamic SQL query building with string concatenation could be optimized. Consider:
- Using query builders or prepared statements where possible
- Caching frequently used query templates
- Using bind parameters instead of string interpolation for better security and performance
Example improvement for the count query:
const COUNT_QUERY_TEMPLATE: &str = r#" SELECT count(*) FROM {table} WHERE {table}.keys REGEXP ? "#; // Cache and reuse the prepared statement let count_query = sqlx::query(COUNT_QUERY_TEMPLATE.replace("{table}", table)) .bind(&keys_pattern);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(1 hunks)
🔇 Additional comments (1)
crates/torii/grpc/src/server/mod.rs (1)
370-371: Ohayo! Nice error handling improvement, sensei!
The JSON deserialization error handling has been improved by properly mapping JSON parsing errors to ParseError::FromJsonStr instead of using unwrap().
Summary by CodeRabbit
New Features
Tyenum, allowing conversion to and from JSON values.to_json_valueandfrom_json_valuefor improved data handling.DojoWorldservice for better handling of complex queries.Chores
Bug Fixes