fix: ensure metadata loading errors are correctly logged #2149
Conversation
WalkthroughThe recent changes primarily involve adding error handling mechanisms across various files in the codebase, specifically utilizing the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- bin/sozo/src/commands/auth.rs (1 hunks)
- bin/sozo/src/commands/build.rs (2 hunks)
- bin/sozo/src/commands/call.rs (1 hunks)
- bin/sozo/src/commands/execute.rs (1 hunks)
- bin/sozo/src/commands/model.rs (1 hunks)
- bin/sozo/tests/register_test.rs (1 hunks)
- crates/dojo-lang/src/compiler.rs (1 hunks)
- crates/dojo-world/src/metadata.rs (3 hunks)
- crates/sozo/ops/src/migration/mod.rs (1 hunks)
- crates/sozo/ops/src/tests/migration.rs (3 hunks)
- crates/sozo/ops/src/tests/setup.rs (1 hunks)
- crates/torii/core/src/sql_test.rs (2 hunks)
- crates/torii/graphql/src/tests/mod.rs (1 hunks)
- crates/torii/grpc/src/server/tests/entities_test.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- bin/sozo/tests/register_test.rs
Additional comments not posted (15)
bin/sozo/src/commands/model.rs (1)
116-116: Ensure proper error handling when retrieving the default namespace.The addition of the
?operator ensures that errors are propagated correctly. Make sure to handle these errors appropriately in the calling context.crates/sozo/ops/src/migration/mod.rs (1)
81-81: Ensure proper error handling when retrieving the default namespace.The addition of the
?operator ensures that errors are propagated correctly. Make sure to handle these errors appropriately in the calling context.bin/sozo/src/commands/build.rs (2)
63-63: Ensure proper error handling when retrieving Dojo metadata.The addition of the
?operator ensures that errors are propagated correctly. Make sure to handle these errors appropriately in the calling context.
146-149: Ensure proper error handling in async context.The use of
unwrapandexpectin the async context can lead to panics if errors occur. Consider handling these errors more gracefully.Do you want me to suggest a more graceful error handling approach for this async context?
crates/torii/graphql/src/tests/mod.rs (1)
294-294: Ensure proper error handling in test scenarios.The use of
unwrapensures that errors are not silently ignored in test scenarios. Make sure that the test setup is robust enough to handle potential errors.bin/sozo/src/commands/auth.rs (1)
67-67: Ensure proper error handling with?operator.The addition of the
?operator to theget_default_namespace_from_ws(&ws)function call ensures that any errors are propagated correctly. This is a good practice for handling errors in Rust.crates/torii/core/src/sql_test.rs (2)
80-80: Ensure proper error handling with.unwrap()in test scenarios.The addition of the
.unwrap()method to theget_default_namespace_from_ws(&ws)function call ensures that any errors are handled in test scenarios. While this is acceptable in test code, ensure that the tests are designed to handle potential errors gracefully.
225-225: Ensure proper error handling with.unwrap()in test scenarios.The addition of the
.unwrap()method to theget_default_namespace_from_ws(&ws)function call ensures that any errors are handled in test scenarios. While this is acceptable in test code, ensure that the tests are designed to handle potential errors gracefully.crates/dojo-world/src/metadata.rs (3)
56-58: Ensure proper error handling withResultreturn type.The
get_default_namespace_from_wsfunction now returns aResult<String>, which enhances error handling by allowing the function to propagate errors. This is a good practice for handling errors in Rust.
101-112: Ensure proper error context messages.The improved error context messages in the
dojo_metadata_from_workspacefunction provide more descriptive error messages, making it easier to diagnose issues. This is a good practice for improving error handling.
445-452: Ensure proper error context messages inMetadataExtimplementation.The improved error context messages in the
MetadataExtimplementation provide more descriptive error messages, making it easier to diagnose issues. This is a good practice for improving error handling.crates/sozo/ops/src/tests/migration.rs (3)
208-208: Ensure proper error handling with.unwrap()in test scenarios.The addition of the
.unwrap()method to theget_default_namespace_from_ws(&ws)function call ensures that any errors are handled in test scenarios. While this is acceptable in test code, ensure that the tests are designed to handle potential errors gracefully.
378-378: Ensure proper error handling with.unwrap()in test scenarios.The addition of the
.unwrap()method to theget_default_namespace_from_ws(&ws)function call ensures that any errors are handled in test scenarios. While this is acceptable in test code, ensure that the tests are designed to handle potential errors gracefully.
411-411: Ensure proper error handling with.unwrap()in test scenarios.The addition of the
.unwrap()method to theget_default_namespace_from_ws(&ws)function call ensures that any errors are handled in test scenarios. While this is acceptable in test code, ensure that the tests are designed to handle potential errors gracefully.crates/dojo-lang/src/compiler.rs (1)
90-90: LGTM! But verify the function usage in the codebase.The code changes are approved. Propagating errors using the
?operator ensures that errors are not silently ignored.However, ensure that all function calls to
compilehandle the propagated errors correctly.Verification successful
Verify error handling for
compilefunction calls indojo-langcrate.We need to ensure that all calls to the
compilefunction incrates/dojo-lang/src/compiler.rshandle the propagated errors correctly. We'll refine our search to focus on the specific function and its usage within thedojo-langcrate.
Re-verify the function usage and error handling for
compilefunction calls.We need to re-verify the function calls to
compileand ensure proper error handling. This time, we'll search across all crates to ensure comprehensive coverage.
Re-verify the function usage and error handling for
compilefunction calls usingrg.We need to re-verify the function calls to
compileand ensure proper error handling usingrgfor a broader search scope.
Review the function usage and error handling for
compilefunction calls.The search results indicate multiple
compilefunction definitions and calls. Specifically, we need to ensure that thecompilefunction incrates/dojo-lang/src/compiler.rsis handled correctly.
Function Definition:
crates/dojo-lang/src/compiler.rs: fn compile(&self, unit: CairoCompilationUnit, db: &mut RootDatabase, ws: &Workspace<'_>) -> Result<()> {Function Calls:
crates/dojo-lang/src/scarb_internal/mod.rs: if let Err(err) = ws.config().compilers().compile(unit.clone(), &mut (db), &ws) {The function call in
crates/dojo-lang/src/scarb_internal/mod.rsuses error handling withif let Err(err), which is correct.Conclusion: The function calls to
compilein thedojo-langcrate handle errors correctly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `compile` handle the propagated errors correctly. # Test: Search for the function usage. Expect: Proper error handling. rg --type rust $'compile'Length of output: 69084
Script:
#!/bin/bash # Description: Verify all function calls to `compile` in `crates/dojo-lang/src/compiler.rs` handle the propagated errors correctly. # Search for the `compile` function definition and calls within the `dojo-lang` crate. ast-grep --lang rust --pattern $'fn compile($_, $_, $_) -> $_ { $$$ }' > compile_definition.txt # Extract the function name and its usage in the crate. grep -E 'fn compile' compile_definition.txt # Search for the function calls to `compile` and ensure proper error handling. ast-grep --lang rust --pattern $'compile($_, $_, $_)?' crates/dojo-lang/ ast-grep --lang rust --pattern $'compile($_, $_, $_).unwrap()' crates/dojo-lang/ ast-grep --lang rust --pattern $'compile($_, $_, $_).expect($$)' crates/dojo-lang/Length of output: 359
Script:
#!/bin/bash # Description: Re-verify all function calls to `compile` in the codebase and ensure proper error handling. # Search for the `compile` function definition within the `dojo-lang` crate. ast-grep --lang rust --pattern $'fn compile($_, $_, $_) -> $_ { $$$ }' > compile_definition.txt # Extract the function name and its usage in the crate. grep -E 'fn compile' compile_definition.txt # Search for the function calls to `compile` and ensure proper error handling across all crates. ast-grep --lang rust --pattern $'compile($_, $_, $_)?' > compile_calls_handled.txt ast-grep --lang rust --pattern $'compile($_, $_, $_).unwrap()' > compile_calls_unwrap.txt ast-grep --lang rust --pattern $'compile($_, $_, $_).expect($$)' > compile_calls_expect.txt # Output the results for review. cat compile_calls_handled.txt cat compile_calls_unwrap.txt cat compile_calls_expect.txtLength of output: 399
Script:
#!/bin/bash # Description: Re-verify all function calls to `compile` in the codebase and ensure proper error handling using `rg`. # Search for the `compile` function definition within the `dojo-lang` crate. rg --type rust --pcre2 --context 5 'fn compile\(' > compile_definition.txt # Search for the function calls to `compile` across all crates and ensure proper error handling. rg --type rust --pcre2 'compile\(' > compile_calls.txt # Output the results for review. cat compile_definition.txt cat compile_calls.txtLength of output: 2308
| @@ -47,7 +47,7 @@ impl CallArgs { | |||
| self.tag_or_address | |||
| } else { | |||
| let ws = scarb::ops::read_workspace(config.manifest_path(), config)?; | |||
| let default_namespace = get_default_namespace_from_ws(&ws); | |||
| let default_namespace = get_default_namespace_from_ws(&ws)?; | |||
There was a problem hiding this comment.
LGTM! Consider adding error context for better debugging.
The addition of the ? operator ensures that errors are propagated correctly. However, adding context to the error can help with debugging.
let default_namespace = get_default_namespace_from_ws(&ws).context("Failed to get default namespace from workspace")?;| @@ -56,7 +56,7 @@ impl ExecuteArgs { | |||
| self.tag_or_address | |||
| } else { | |||
| let ws = scarb::ops::read_workspace(config.manifest_path(), config)?; | |||
| let default_namespace = get_default_namespace_from_ws(&ws); | |||
| let default_namespace = get_default_namespace_from_ws(&ws)?; | |||
There was a problem hiding this comment.
LGTM! Consider adding error context for better debugging.
The addition of the ? operator ensures that errors are propagated correctly. However, adding context to the error can help with debugging.
let default_namespace = get_default_namespace_from_ws(&ws).context("Failed to get default namespace from workspace")?;| @@ -61,7 +61,7 @@ pub fn setup_migration(config: &Config) -> Result<MigrationStrategy> { | |||
| let base_dir = manifest_path.parent().unwrap(); | |||
| let target_dir = format!("{}/target/dev", base_dir); | |||
|
|
|||
| let default_namespace = get_default_namespace_from_ws(&ws); | |||
| let default_namespace = get_default_namespace_from_ws(&ws).unwrap(); | |||
There was a problem hiding this comment.
Consider adding a meaningful error message.
Using .unwrap() in test setups is generally acceptable, but providing a meaningful error message can make debugging easier.
let default_namespace = get_default_namespace_from_ws(&ws).expect("Failed to get default namespace from workspace");| @@ -50,7 +50,7 @@ async fn test_entities_queries() { | |||
|
|
|||
| let target_path = ws.target_dir().path_existent().unwrap().join(config.profile().to_string()); | |||
|
|
|||
| let default_namespace = get_default_namespace_from_ws(&ws); | |||
| let default_namespace = get_default_namespace_from_ws(&ws).unwrap(); | |||
There was a problem hiding this comment.
Consider adding a meaningful error message.
Using .unwrap() in tests is generally acceptable, but providing a meaningful error message can make debugging easier.
let default_namespace = get_default_namespace_from_ws(&ws).expect("Failed to get default namespace from workspace");
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2149 +/- ##
==========================================
- Coverage 67.89% 67.88% -0.02%
==========================================
Files 336 336
Lines 43086 43094 +8
==========================================
+ Hits 29255 29256 +1
- Misses 13831 13838 +7 ☔ View full report in Codecov by Sentry. |
Description
To ensure smooth transition to
1.0.0, this PR ensures that the errors from incorrect configuration are guiding the user to make the according changes.Summary by CodeRabbit
Refactor
?operator for more robust error propagation.Tests
.unwrap()where necessary to ensure valid function call results.Documentation