Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(torii/graphql): handle empty metadata and schema change #2731

Closed
wants to merge 1 commit into from
Closed

fix(torii/graphql): handle empty metadata and schema change #2731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions crates/torii/graphql/src/mapping.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ lazy_static! {
),
]);
pub static ref PAGE_INFO_TYPE_MAPPING: TypeMapping = TypeMapping::from([
(Name::new("hasPreviousPage"), TypeData::Simple(TypeRef::named(TypeRef::BOOLEAN))),
(Name::new("hasNextPage"), TypeData::Simple(TypeRef::named(TypeRef::BOOLEAN))),
(Name::new("hasPreviousPage"), TypeData::Simple(TypeRef::named_nn(TypeRef::BOOLEAN))),
(Name::new("hasNextPage"), TypeData::Simple(TypeRef::named_nn(TypeRef::BOOLEAN))),
(
Name::new("startCursor"),
TypeData::Simple(TypeRef::named(GraphqlType::Cursor.to_string())),
Expand Down Expand Up @@ -160,7 +160,7 @@ lazy_static! {
pub static ref ERC20_TOKEN_TYPE_MAPPING: TypeMapping = IndexMap::from([
(Name::new("name"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
(Name::new("symbol"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
(Name::new("decimals"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
(Name::new("decimals"), TypeData::Simple(TypeRef::named_nn(TypeRef::INT))),
(Name::new("contractAddress"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
(Name::new("amount"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
]);
Expand All @@ -171,9 +171,9 @@ lazy_static! {
(Name::new("tokenId"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
(Name::new("contractAddress"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
(Name::new("metadata"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
(Name::new("metadataName"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
(Name::new("metadataDescription"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
(Name::new("metadataAttributes"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
(Name::new("metadataName"), TypeData::Simple(TypeRef::named(TypeRef::STRING))),
(Name::new("metadataDescription"), TypeData::Simple(TypeRef::named(TypeRef::STRING))),
(Name::new("metadataAttributes"), TypeData::Simple(TypeRef::named(TypeRef::STRING))),
(Name::new("imagePath"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Ohayo! The imagePath should indeed be nullable, sensei!

Looking at the codebase, image_path is constructed using a simple format string: format!("{}/{}", token_id.join("/"), "image"). This is a static path construction that doesn't verify the actual existence of the image, while other metadata fields (name, description, attributes) are properly wrapped in Option. For consistency and correctness, it should be nullable.

  • In crates/torii/graphql/src/object/erc/erc_token.rs, image_path is defined as a non-optional String
  • In crates/torii/graphql/src/mapping.rs, it's mapped as non-nullable TypeRef::named_nn
  • The path construction in both token_transfer.rs and token_balance.rs doesn't validate image existence
-(Name::new("imagePath"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
+(Name::new("imagePath"), TypeData::Simple(TypeRef::named(TypeRef::STRING))),
🔗 Analysis chain

Consider making imagePath nullable for consistency

While the metadata fields are now properly nullable, imagePath remains non-nullable (TypeRef::named_nn). Should this also be nullable since it's part of the metadata that might not be available?

Let's check if imagePath is always guaranteed to be present:

Consider making imagePath nullable for consistency with other metadata fields:

-(Name::new("imagePath"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
+(Name::new("imagePath"), TypeData::Simple(TypeRef::named(TypeRef::STRING))),
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for imagePath handling to understand if it's always present
rg -A 5 "imagePath.*Some" --type rust
rg -A 5 "imagePath.*None" --type rust

Length of output: 79

Script:

#!/bin/bash
# Let's search for imagePath usage in the codebase more broadly
rg "imagePath" --type rust -B 2 -A 2

# Also check for any struct definitions or types that might contain imagePath
ast-grep --pattern 'struct $_ {
  $$$
  image_path: $_,
  $$$
}'

Length of output: 1080

Script:

#!/bin/bash
# Let's check the token struct definition to see how image_path is defined
rg "image_path" --type rust -B 5 -A 5

# Also check for any Option wrapping of image_path in struct definitions
ast-grep --pattern 'struct $_ {
  $$$
  image_path: Option<$_>,
  $$$
}'

Length of output: 6109

]);

Expand Down
12 changes: 9 additions & 3 deletions crates/torii/graphql/src/object/erc/token_balance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,14 @@
let token_id = row.token_id.split(':').collect::<Vec<&str>>();
assert!(token_id.len() == 2);

// skip the token if metadata is null
if row.metadata.is_none() {
continue;
}
Comment on lines +238 to +240
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Strengthen token_id parsing robustness, sensei

The current token_id parsing using split and assert could panic if the format is unexpected. Consider using a more robust approach.

-let token_id = row.token_id.split(':').collect::<Vec<&str>>();
-assert!(token_id.len() == 2);
+let token_id: Vec<&str> = row.token_id.split(':').collect();
+if token_id.len() != 2 {
+    warn!(
+        "Skipping token due to invalid token_id format: {}",
+        row.token_id
+    );
+    continue;
+}

Committable suggestion skipped: line range outside the PR's diff.

let metadata_str = row.metadata.as_ref().unwrap();

Check warning on line 242 in crates/torii/graphql/src/object/erc/token_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/token_balance.rs#L238-L242 

Added lines #L238 - L242 were not covered by tests
let metadata: serde_json::Value =
serde_json::from_str(&row.metadata).expect("metadata is always json");
serde_json::from_str(metadata_str).expect("metadata is always json");

Check warning on line 244 in crates/torii/graphql/src/object/erc/token_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/token_balance.rs#L244 

Added line #L244 was not covered by tests
let metadata_name =
metadata.get("name").map(|v| v.to_string().trim_matches('"').to_string());
let metadata_description = metadata
Expand All @@ -248,7 +254,7 @@

let token_metadata = Erc721Token {
name: row.name,
metadata: row.metadata,
metadata: metadata_str.to_owned(),

Check warning on line 257 in crates/torii/graphql/src/object/erc/token_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/token_balance.rs#L257 

Added line #L257 was not covered by tests
contract_address: row.contract_address,
symbol: row.symbol,
token_id: token_id[1].to_string(),
Expand Down Expand Up @@ -295,5 +301,5 @@
pub token_id: String,
pub balance: String,
pub contract_type: String,
pub metadata: String,
pub metadata: Option<String>,
}
12 changes: 9 additions & 3 deletions crates/torii/graphql/src/object/erc/token_transfer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,14 @@
let token_id = row.token_id.split(':').collect::<Vec<&str>>();
assert!(token_id.len() == 2);

// skip the token if metadata is null
if row.metadata.is_none() {
continue;
}

let metadata_str = row.metadata.as_ref().unwrap();

Check warning on line 271 in crates/torii/graphql/src/object/erc/token_transfer.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/token_transfer.rs#L267-L271 

Added lines #L267 - L271 were not covered by tests
let metadata: serde_json::Value =
serde_json::from_str(&row.metadata).expect("metadata is always json");
serde_json::from_str(metadata_str).expect("metadata is always json");

Check warning on line 273 in crates/torii/graphql/src/object/erc/token_transfer.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/token_transfer.rs#L273 

Added line #L273 was not covered by tests
Comment on lines +271 to +273
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance error handling for metadata JSON parsing, sensei!

The current implementation uses expect() which could panic if the metadata JSON is malformed. Consider using serde_json::from_str().map_err() to handle parsing errors gracefully.

-let metadata_str = row.metadata.as_ref().unwrap();
-let metadata: serde_json::Value =
-    serde_json::from_str(metadata_str).expect("metadata is always json");
+let metadata_str = row.metadata.as_ref().unwrap();
+let metadata: serde_json::Value = match serde_json::from_str(metadata_str) {
+    Ok(value) => value,
+    Err(e) => {
+        warn!("Failed to parse metadata JSON for token_id {}: {}", row.token_id, e);
+        continue;
+    }
+};
📝 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.

Suggested change
let metadata_str = row.metadata.as_ref().unwrap();
let metadata: serde_json::Value =
serde_json::from_str(&row.metadata).expect("metadata is always json");
serde_json::from_str(metadata_str).expect("metadata is always json");
let metadata_str = row.metadata.as_ref().unwrap();
let metadata: serde_json::Value = match serde_json::from_str(metadata_str) {
Ok(value) => value,
Err(e) => {
warn!("Failed to parse metadata JSON for token_id {}: {}", row.token_id, e);
continue;
}
};

let metadata_name =
metadata.get("name").map(|v| v.to_string().trim_matches('"').to_string());
let metadata_description = metadata
Expand All @@ -277,7 +283,7 @@

let token_metadata = ErcTokenType::Erc721(Erc721Token {
name: row.name,
metadata: row.metadata,
metadata: metadata_str.to_owned(),

Check warning on line 286 in crates/torii/graphql/src/object/erc/token_transfer.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/token_transfer.rs#L286 

Added line #L286 was not covered by tests
contract_address: row.contract_address,
symbol: row.symbol,
token_id: token_id[1].to_string(),
Expand Down Expand Up @@ -333,7 +339,7 @@
pub symbol: String,
pub decimals: u8,
pub contract_type: String,
pub metadata: String,
pub metadata: Option<String>,
}

#[derive(Debug, Clone)]
Expand Down
Loading